fix(vm): fail closed on malformed pull/send bytecode

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` |
 
@@ -82,6 +83,18 @@ verified, so `ErrNoOpenMark` and the INVARIANT documented on `MarkEnd` stay.
 it is a legitimate user-facing error (`InvalidUncappedSource`, "unbounded source
 is not allowed here"), not a malformed program. Deliberately left to run time.
 
+### Funds left in the queue
+
+A pull debits its source at once; the posting only exists once a send drains the
+queue. Funds still queued when `Op_SetCurrentAsset` runs would be posted under
+the new asset, and funds still queued when the run ends are a debit that never
+became a posting. Whether a capped send empties the queue depends on the
+balances, so neither is decidable from the instruction stream: `Exec` checks
+both at run time (`errSetAssetWithQueuedFunds`, `errEndWithQueuedFunds`, via
+`runstate.HasQueuedFunds()`). Compiled numscript never trips them: every send
+statement either pulls exactly what it sends (`Op_CheckEnoughFunds`) or sends
+everything it pulled.
+
 ### Anything semantic
 
 A verified program can still produce nonsense: allotment portions that don't sum

instruction-encoding.md

@@ -27,7 +27,7 @@ Instructions are **4 bytes** wide: `[Opcode: 8] [A: 8] [B: 8] [C: 8]`.
     <tr>
       <td>0</td><td><code>0x00</code></td><td><strong>SET_CURRENT_ASSET</strong></td>
       <td>asset</td><td>-</td><td>-</td>
-      <td>Sets the current asset (used by <code>PULL_ACCOUNT</code> / <code>SEND_TO_ACCOUNT</code>) from <code>str_regs[A]</code></td>
+      <td>Sets the current asset (used by <code>PULL_ACCOUNT</code> / <code>SEND_TO_ACCOUNT</code>) from <code>str_regs[A]</code>. Traps while a mark is open, or while the source queue still holds funds pulled under the previous asset</td>
     </tr>
     <tr>
       <td>1</td><td><code>0x01</code></td><td><strong>ASSERT_SAME_ASSET</strong></td>
@@ -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/funds/funds.go

@@ -244,6 +244,18 @@ func (s *RunState) GetAccountBalance(account, scope, asset, color string) (*big.
 	return new(big.Int).Set(bal), nil
 }
 
+// HasQueuedFunds reports whether a queued source still holds a positive amount.
+// Zero-amount entries do not count: a Pull of a zero cap queues one, and a Send
+// of a zero cap does not consume it.
+func (s *RunState) HasQueuedFunds() bool {
+	for i := range s.sources {
+		if s.sources[i].amount.Sign() > 0 {
+			return true
+		}
+	}
+	return false
+}
+
 // Pull debits up to cap from src's (currentAsset, color) balance, queues the
 // pulled amount as a funding source tagged with color, and writes the amount made
 // available into out:

internal/vm/export_test.go

@@ -1,3 +1,7 @@
 package vm
 
-var ErrSendWhileMarkOpen = errSendWhileMarkOpen
+var (
+	ErrSendWhileMarkOpen       = errSendWhileMarkOpen
+	ErrSetAssetWithQueuedFunds = errSetAssetWithQueuedFunds
+	ErrEndWithQueuedFunds      = errEndWithQueuedFunds
+)

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

@@ -36,6 +36,8 @@ type Opcode byte
 // a category without renumbering. See instruction-encoding.md.
 const (
 	// --- state & assertions (0x00) ---
+	// InternalError while a mark is open or while the source queue still holds
+	// funds pulled under the previous asset
 	Op_SetCurrentAsset Opcode = 0x00
 
 	Op_AssertSameAsset Opcode = 0x01
@@ -168,6 +170,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,66 @@ 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")
+	})
+}
+
+// Funds pulled under one asset and still queued when the asset changes would be
+// posted under the new asset. Not statically decidable (a capped send drains the
+// queue or not depending on the balances), so this is a run-time check only.
+func TestIRSetCurrentAssetWithQueuedFundsIsAnInternalError(t *testing.T) {
+	execErr := runIRExpectingError(t, `
+  $usd = "USD/2"
+  set_current_asset($usd)
+  $src = "src"
+  $dest = "dest"
+  $overdraft = 0
+  $cap = 100
+  $pulled = pull_account(account: $src, cap: $cap, overdraft: $overdraft)
+  $eur = "EUR/2"
+  set_current_asset($eur)
+  send_to_account(account: $dest)
+`, balances(map[string]int64{"src": 100}), nil)
+	require.Equal(t, vm.InternalError{Err: vm.ErrSetAssetWithQueuedFunds}, execErr)
+}
+
+func TestIREndWithQueuedFundsIsAnInternalError(t *testing.T) {
+	execErr := runIRExpectingError(t, `
+  $asset = "USD/2"
+  set_current_asset($asset)
+  $src = "src"
+  $overdraft = 0
+  $cap = 100
+  $pulled = pull_account(account: $src, cap: $cap, overdraft: $overdraft)
+`, balances(map[string]int64{"src": 100}), nil)
+	require.Equal(t, vm.InternalError{Err: vm.ErrEndWithQueuedFunds}, execErr)
+}

internal/vm/verify.go

@@ -580,6 +580,10 @@ func decodeInstr(instr, ext Instruction) (decoded, error) {
 
 	// --- funds & postings
 	case Op_PullAccount:
+		// Exec's arm rejects this too; see errPullDestAliasesInput
+		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))
+}

internal/vm/vm.go

@@ -30,6 +30,13 @@ var (
 	errSaveWhileMarkOpen     = errors.New("save while a mark is open")
 )
 
+// Not statically decidable: whether a send drains the queue depends on the
+// balances.
+var (
+	errSetAssetWithQueuedFunds = errors.New("set_current_asset with funds still queued")
+	errEndWithQueuedFunds      = errors.New("run ended with funds still queued")
+)
+
 type Vm struct {
 	Program  Program
 	runstate *funds.RunState
@@ -156,9 +163,6 @@ func Exec[S Store](
 		switch Opcode(instr.Opcode) {
 		// --- Domain-specific ops
 		case Op_PullAccount:
-			// TODO crashes if this is the last instruction (the ext word is
-			// missing): instrs[pc] reads past the end. e.g. a program ending in a
-			// lone Op_PullAccount word.
 			instrExt := instrs[pc]
 			pc++
 
@@ -292,6 +296,10 @@ func Exec[S Store](
 			if runstate.HasOpenMark() {
 				return funds.ExecutionResult{}, InternalError{Err: errSetAssetWhileMarkOpen}
 			}
+			// funds queued under the previous asset would be posted under the new one
+			if runstate.HasQueuedFunds() {
+				return funds.ExecutionResult{}, InternalError{Err: errSetAssetWithQueuedFunds}
+			}
 			currentAsset = stringsRegs[instr.A]
 			runstate.SetCurrentAsset(currentAsset)
 
@@ -426,8 +434,6 @@ func Exec[S Store](
 			portionsRegs[instr.A].Set(r)
 
 		case Op_MetaMonetary:
-			// TODO crashes if this is the last instruction (the ext word carrying the
-			// amount destination is missing), same as Op_PullAccount.
 			instrExt := instrs[pc]
 			pc++
 
@@ -635,6 +641,11 @@ func Exec[S Store](
 		})
 	}
 
+	// a debit that never became a posting
+	if runstate.HasQueuedFunds() {
+		return funds.ExecutionResult{}, InternalError{Err: errEndWithQueuedFunds}
+	}
+
 	postings := runstate.GetPostings()
 	for _, p := range postings {
 		if !funds.ValidatePosting(p) {

internal/vm/vm_test.go

@@ -537,3 +537,81 @@ func TestExecutionErrorsUnwrap(t *testing.T) {
 	require.ErrorIs(t, InternalError{Err: cause}, cause)
 	require.ErrorIs(t, StoreError{Wrapped: cause}, cause)
 }
+
+// --- malformed pull/send sequences -----------------------------------------
+
+func TestSetCurrentAssetWithQueuedFundsIsAnInternalError(t *testing.T) {
+	// pull 100 USD/2 from @src, switch to EUR/2, send: the posting would be in
+	// EUR/2 while the debit was in USD/2
+	prog := Program{
+		Instructions: []Instruction{
+			bc(Op_LoadStr, 0, 0),
+			bc(Op_LoadStr, 1, 1),
+			bc(Op_LoadStr, 2, 2),
+			bc(Op_LoadStr, 3, 3),
+			bc(Op_LoadInt, 1, 0),
+			bc(Op_LoadInt, 2, 1),
+			abc(Op_SetCurrentAsset, 0, nilReg, nilReg),
+			abc(Op_PullAccount, 0, 1, 1),
+			abc(0, 2, nilReg, nilReg), // ext
+			abc(Op_SetCurrentAsset, 3, nilReg, nilReg),
+			abc(Op_SendToAccount, 2, nilReg, nilReg),
+		},
+		StringsPool: []string{"USD/2", "src", "dest", "EUR/2"},
+		IntsPool:    []big.Int{*big.NewInt(100), *big.NewInt(0)},
+	}
+	store := mockStore{bal: map[funds.PairKey]int64{{Account: "src", Asset: "USD/2"}: 100}}
+
+	require.NoError(t, Verify(fullBanks(prog)))
+	_, err := Exec(context.Background(), newTestVm(prog), nil, store)
+	require.Equal(t, InternalError{Err: errSetAssetWithQueuedFunds}, err)
+}
+
+func TestEndWithQueuedFundsIsAnInternalError(t *testing.T) {
+	prog := Program{
+		Instructions: []Instruction{
+			bc(Op_LoadStr, 0, 0),
+			bc(Op_LoadStr, 1, 1),
+			bc(Op_LoadInt, 1, 0),
+			bc(Op_LoadInt, 2, 1),
+			abc(Op_SetCurrentAsset, 0, nilReg, nilReg),
+			abc(Op_PullAccount, 0, 1, 1),
+			abc(0, 2, nilReg, nilReg), // ext
+		},
+		StringsPool: []string{"USD/2", "src"},
+		IntsPool:    []big.Int{*big.NewInt(100), *big.NewInt(0)},
+	}
+	store := mockStore{bal: map[funds.PairKey]int64{{Account: "src", Asset: "USD/2"}: 100}}
+
+	require.NoError(t, Verify(fullBanks(prog)))
+	_, err := Exec(context.Background(), newTestVm(prog), nil, store)
+	require.Equal(t, InternalError{Err: errEndWithQueuedFunds}, err)
+}
+
+// `send [USD/2 0]` lowers to a zero-cap pull (which queues a zero-amount source)
+// followed by a zero-cap send (which does not consume it): the queued zero must
+// not count as funds, at the next asset switch or at the end of the run.
+func TestQueuedZeroAmountIsNotQueuedFunds(t *testing.T) {
+	prog := Program{
+		Instructions: []Instruction{
+			bc(Op_LoadStr, 0, 0),
+			bc(Op_LoadStr, 1, 1),
+			bc(Op_LoadStr, 2, 2),
+			bc(Op_LoadInt, 0, 0), // 0
+			abc(Op_SetCurrentAsset, 0, nilReg, nilReg),
+			abc(Op_PullAccount, 1, 1, 0),
+			abc(0, nilReg, nilReg, nilReg), // ext: unbounded overdraft
+			abc(Op_SendToAccount, 2, 0, nilReg),
+			abc(Op_SetCurrentAsset, 0, nilReg, nilReg),
+			abc(Op_PullAccount, 1, 1, 0),
+			abc(0, nilReg, nilReg, nilReg), // ext: unbounded overdraft
+		},
+		StringsPool: []string{"USD/2", "world", "dest"},
+		IntsPool:    []big.Int{*big.NewInt(0)},
+	}
+
+	require.NoError(t, Verify(fullBanks(prog)))
+	res, err := Exec(context.Background(), newTestVm(prog), nil, mockStore{})
+	require.Nil(t, err)
+	require.Empty(t, res.Postings)
+}