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` |
 
@@ -68,7 +69,9 @@ only one side of a branch").
 The VM still enforces the same rules at execution time, via
 `runstate.HasOpenMark()` and the `errSendWhileMarkOpen` /
 `errSetAssetWhileMarkOpen` / `errSaveWhileMarkOpen` sentinels in `vm.go`:
-`Exec` does not require a verified program.
+`Exec` does not require a verified program. The same goes for a truncated
+two-word instruction (`errTruncatedInstruction`) and an aliased pull destination
+(`errPullDestAliasesInput`).
 
 **`internal/funds` must not be relaxed along with it.** The tree-walking
 interpreter shares `RunState.MarkEnd` (see `interpreter.go`), and it is not
@@ -82,6 +85,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: rejected by the verifier, and traps</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,9 @@
 package vm
 
-var ErrSendWhileMarkOpen = errSendWhileMarkOpen
+var (
+	ErrSendWhileMarkOpen       = errSendWhileMarkOpen
+	ErrTruncatedInstruction    = errTruncatedInstruction
+	ErrPullDestAliasesInput    = errPullDestAliasesInput
+	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,8 @@ 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,
+	// and an InternalError at execution time
 	Op_PullAccount Opcode = 0x50
 
 	// account?, cap?, scope?

internal/vm/ir_test.go

@@ -1369,3 +1369,68 @@ 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. The verifier rejects it; unverified, Exec does too.
+func TestIRPullDestAliasingItsBoundIsRejected(t *testing.T) {
+	store := balances(map[string]int64{"src": 30})
+
+	t.Run("cap", func(t *testing.T) {
+		execErr := runUnverifiedIRExpectingError(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)
+`, "aliases its cap or overdraft", store, nil)
+		require.Equal(t, vm.InternalError{Err: vm.ErrPullDestAliasesInput}, execErr)
+	})
+
+	t.Run("overdraft", func(t *testing.T) {
+		execErr := runUnverifiedIRExpectingError(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)
+`, "aliases its cap or overdraft", store, nil)
+		require.Equal(t, vm.InternalError{Err: vm.ErrPullDestAliasesInput}, execErr)
+	})
+}
+
+// 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,16 @@ var (
 	errSaveWhileMarkOpen     = errors.New("save while a mark is open")
 )
 
+// Malformed pull/send sequences the verifier does not fully cover, rejected by
+// the arms below. The queue checks are not statically decidable: whether a send
+// drains the queue depends on the balances.
+var (
+	errTruncatedInstruction    = errors.New("truncated instruction: missing ext word")
+	errPullDestAliasesInput    = errors.New("pull destination aliases its cap or overdraft")
+	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,12 +166,19 @@ 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.
+			if pc == instructionsLen {
+				return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction}
+			}
 			instrExt := instrs[pc]
 			pc++
 
+			// funds.Pull writes the destination before it reads the bounds on the
+			// bounded path, so an aliased cap is ignored and an aliased overdraft
+			// doubles the balance
+			if instr.A == instr.C || instr.A == instrExt.A {
+				return funds.ExecutionResult{}, InternalError{Err: errPullDestAliasesInput}
+			}
+
 			account := stringsRegs[instr.B]
 
 			var cap *big.Int
@@ -251,6 +268,9 @@ func Exec[S Store](
 				return funds.ExecutionResult{}, InternalError{Err: errSaveWhileMarkOpen}
 			}
 
+			if pc == instructionsLen {
+				return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction}
+			}
 			instrExt := instrs[pc]
 			pc++
 
@@ -292,6 +312,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)
 
@@ -356,6 +380,9 @@ func Exec[S Store](
 			txMeta[stringsRegs[instr.A]] = stringsRegs[instr.B]
 
 		case Op_SetAccountMeta:
+			if pc == instructionsLen {
+				return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction}
+			}
 			instrExt := instrs[pc]
 			pc++
 
@@ -374,6 +401,9 @@ func Exec[S Store](
 			accountsMeta[key] = stringsRegs[instr.C]
 
 		case Op_MetaStr:
+			if pc == instructionsLen {
+				return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction}
+			}
 			instrExt := instrs[pc]
 			pc++
 
@@ -388,6 +418,9 @@ func Exec[S Store](
 			stringsRegs[instr.A] = v
 
 		case Op_MetaInt:
+			if pc == instructionsLen {
+				return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction}
+			}
 			instrExt := instrs[pc]
 			pc++
 
@@ -407,6 +440,9 @@ func Exec[S Store](
 			intsRegs[instr.A].Set(n)
 
 		case Op_MetaPortion:
+			if pc == instructionsLen {
+				return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction}
+			}
 			instrExt := instrs[pc]
 			pc++
 
@@ -426,8 +462,9 @@ 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.
+			if pc == instructionsLen {
+				return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction}
+			}
 			instrExt := instrs[pc]
 			pc++
 
@@ -553,6 +590,9 @@ func Exec[S Store](
 			portionsRegs[instr.A].SetFrac(num, den)
 
 		case Op_Balance:
+			if pc == instructionsLen {
+				return funds.ExecutionResult{}, InternalError{Err: errTruncatedInstruction}
+			}
 			instrExt := instrs[pc]
 			pc++
 
@@ -635,6 +675,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) {