Skip to content

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

Merged
ascandone merged 2 commits into
mainfrom
fix/vm-verify-pull-aliasing
Oct 22, 2026
Merged

ascandone merged 2 commits into
mainfrom
fix/vm-verify-pull-aliasing

Conversation

@ascandone

Copy link
Copy Markdown
Contributor

Problem

A pull_account whose destination register is also its cap or overdraft passes vm.Verify and produces wrong postings with no error. funds.Pull writes the destination before it reads the bounds on the bounded path, so an aliased cap stops bounding the pull and an aliased overdraft doubles the balance. Balance 30, cap 100, overdraft 0 pulls 60, leaving the source at -30 with a zero overdraft.

Compiled numscript never emits this: the allocator never aliases a destination with an operand of the same instruction. It matters for bytecode that did not come out of this process's compiler, which is what the ledger replays.

Fix

Verify rejects the shape (two byte compares in decodeInstr). Nothing changes in Exec: verify-first stays the contract.

bytecode-verifier.md, instruction-encoding.md and the opcode comment document the rule.

Tests

  • TestVerifyPullDestAliasesCapOrOverdraft: both aliasings rejected; cap and overdraft may share a register.
  • TestIRPullDestAliasingItsBoundIsRejected: the same shapes written as IR.
  • A FuzzExec seed for the aliased shape.
  • go test ./..., -race on internal/vm and internal/compiler, TestCompiledCorpusPassesVerify and 15s of FuzzExec pass.

Split out of #217, which keeps the queued-funds work.

funds.Pull writes the destination before it reads the bounds, so an aliased
cap no longer bounds the pull and an aliased overdraft doubles the balance.
The verifier now rejects that shape.
@ascandone
ascandone requested a review from Azorlogh October 21, 2026 15:03
@NumaryBot NumaryBot added risk: medium NumaryBot classified this pull request as medium risk. bot-reviewed NumaryBot completed its review workflow for the current head. review-approved The NumaryBot review gate is satisfied for the current head. labels Oct 21, 2026

@NumaryBot NumaryBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The required automated review completed with no remaining findings.

@NumaryBot NumaryBot removed the review-approved The NumaryBot review gate is satisfied for the current head. label Oct 22, 2026
@ascandone
ascandone enabled auto-merge (squash) October 22, 2026 08:47
@NumaryBot NumaryBot added the review-approved The NumaryBot review gate is satisfied for the current head. label Oct 22, 2026

@NumaryBot NumaryBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The required automated review completed with no remaining findings.

@ascandone
ascandone merged commit aac2b88 into main Oct 22, 2026
6 checks passed
@ascandone
ascandone deleted the fix/vm-verify-pull-aliasing branch October 22, 2026 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bot-reviewed NumaryBot completed its review workflow for the current head. review-approved The NumaryBot review gate is satisfied for the current head. risk: medium NumaryBot classified this pull request as medium risk.

Development

Successfully merging this pull request may close these issues.

3 participants