fix(vm): reject a pull whose destination aliases its cap or overdraft

bytecode-verifier.md

@@ -37,6 +37,7 @@ that passes vars should use the second form.
 | Flag operands are 0 or 1 | `Op_MarkEnd` tests `A == 1`, so a 2 silently commits a region that meant to rewind |
 | Definite assignment | Reading a register not written on every path reaching the instruction |
 | The current asset is set before it is used | A `send` or `pull` before any `set_current_asset` |
+| A pull's destination does not alias its cap or overdraft | `funds.Pull` writes the destination before it reads the bounds on the bounded path: aliased, the cap no longer bounds the pull and the overdraft doubles the balance |
 | Mark discipline (see below) | An `Op_MarkEnd` with no open mark, a send/save/asset change inside a region, a run ending with a region open |
 | (`VerifyWithVars`) Var-pool indices are in range | `Op_LoadVar*` indexing past the pool, or dereferencing a nil `*Vars` |
 

instruction-encoding.md

@@ -355,7 +355,7 @@ Instructions are **4 bytes** wide: `[Opcode: 8] [A: 8] [B: 8] [C: 8]`.
     <tr>
       <td>&#8203;</td><td>&#8203;</td><td><strong>&#8627; cont.</strong></td>
       <td>overdraft</td><td>color</td><td>scope</td>
-      <td>Overdraft bound <code>int_regs[A]</code> (<code>0xFF</code> = unbounded), color <code>str_regs[B]</code> and scope <code>str_regs[C]</code> (<code>0xFF</code> = none). Cap and overdraft both <code>0xFF</code> traps (unbounded source)</td>
+      <td>Overdraft bound <code>int_regs[A]</code> (<code>0xFF</code> = unbounded), color <code>str_regs[B]</code> and scope <code>str_regs[C]</code> (<code>0xFF</code> = none). Cap and overdraft both <code>0xFF</code> traps (unbounded source). The destination register may not be the cap or the overdraft register (verifier-rejected)</td>
     </tr>
     <tr>
       <td>81</td><td><code>0x51</code></td><td><strong>SEND_TO_ACCOUNT</strong></td>

internal/vm/fuzz_test.go

@@ -23,6 +23,7 @@ func FuzzExec(f *testing.F) {
 	f.Add([]byte{byte(Op_LoadStr), 0, 0, 0, byte(Op_Jmp), 0, 1, 0}) // jump past the end
 	f.Add([]byte{byte(Op_ConstTrue), 0, 0, 0, byte(Op_JmpIfTrue), 0, 0, 0})
 	f.Add([]byte{byte(Op_MarkPush), 0, 0, 0, byte(Op_MarkEnd), 1, 0, 0})
+	f.Add([]byte{byte(Op_PullAccount), 1, 0, 1, 0xFF, nilReg, nilReg, nilReg}) // dest aliases cap
 
 	stringsPool := []string{"world", "dest", "USD/2"}
 	intsPool := []big.Int{*big.NewInt(0), *big.NewInt(7)}

internal/vm/instruction.go

@@ -168,6 +168,7 @@ const (
 	// The most general form: account,cap,overdraft,color,scope
 	// The 0xFF special register means NULL for cap,overdraft,color and scope
 	// ext.A = overdraft (int reg), ext.B = color (str reg), ext.C = scope (str reg)
+	// A (dest) may not be the same register as C or ext.A: verifier-rejected
 	Op_PullAccount Opcode = 0x50
 
 	// account?, cap?, scope?

internal/vm/ir_test.go

@@ -1369,3 +1369,35 @@ func TestIRErrorsDoNotAliasRegisters(t *testing.T) {
 		require.Equal(t, vm.NegativeAmountError{Amount: *big.NewInt(-20)}, second)
 	})
 }
+
+// A pull whose destination is also its cap or overdraft reads the bound after
+// having overwritten it.
+func TestIRPullDestAliasingItsBoundIsRejected(t *testing.T) {
+	t.Run("cap", func(t *testing.T) {
+		program := assembleUnverifiedIR(t, `
+  $asset = "USD/2"
+  set_current_asset($asset)
+  $src = "src"
+  $dest = "dest"
+  $overdraft = 0
+  $cap = 100
+  $cap = pull_account(account: $src, cap: $cap, overdraft: $overdraft)
+  send_to_account(account: $dest)
+`)
+		require.ErrorContains(t, vm.Verify(program), "aliases its cap or overdraft")
+	})
+
+	t.Run("overdraft", func(t *testing.T) {
+		program := assembleUnverifiedIR(t, `
+  $asset = "USD/2"
+  set_current_asset($asset)
+  $src = "src"
+  $dest = "dest"
+  $overdraft = 0
+  $cap = 100
+  $overdraft = pull_account(account: $src, cap: $cap, overdraft: $overdraft)
+  send_to_account(account: $dest)
+`)
+		require.ErrorContains(t, vm.Verify(program), "aliases its cap or overdraft")
+	})
+}

internal/vm/verify.go

@@ -580,6 +580,10 @@ func decodeInstr(instr, ext Instruction) (decoded, error) {
 
 	// --- funds & postings
 	case Op_PullAccount:
+		// funds.Pull writes the destination before it reads the bounds
+		if instr.A == instr.C || instr.A == ext.A {
+			err = fmt.Errorf("pull destination int register %d aliases its cap or overdraft", instr.A)
+		}
 		read(bankStr, instr.B)    // account
 		readOpt(bankInt, instr.C) // cap
 		readOpt(bankInt, ext.A)   // overdraft

internal/vm/verify_test.go

@@ -258,3 +258,25 @@ func TestVerifyWithVarsAllowsNilWhenNoneAreRead(t *testing.T) {
 func TestVerifyEmptyProgram(t *testing.T) {
 	mustAccept(t, Program{})
 }
+
+func TestVerifyPullDestAliasesCapOrOverdraft(t *testing.T) {
+	pull := func(dest, cap, overdraft byte) Program {
+		return Program{
+			Instructions: []Instruction{
+				bc(Op_LoadStr, 0, 0),
+				bc(Op_LoadInt, 0, 0),
+				bc(Op_LoadInt, 1, 0),
+				abc(Op_SetCurrentAsset, 0, nilReg, nilReg),
+				abc(Op_PullAccount, dest, 0, cap),
+				abc(0, overdraft, nilReg, nilReg), // ext
+			},
+			StringsPool: []string{"acc"},
+			IntsPool:    []big.Int{*big.NewInt(1)},
+		}
+	}
+	requireRejectedWith(t, pull(0, 0, 1), "aliases its cap or overdraft")
+	requireRejectedWith(t, pull(1, 0, 1), "aliases its cap or overdraft")
+	mustAccept(t, pull(2, 0, 1))
+	// the two bounds are read-only, so they may share a register
+	mustAccept(t, pull(2, 0, 0))
+}