Skip to content

Executable review follow-up: realistic fixtures, vector replay, report re-audit, CI - #21

Merged
phroi merged 13 commits into
masterfrom
audit-follow-up
Sep 26, 2026
Merged

Executable review follow-up: realistic fixtures, vector replay, report re-audit, CI#21
phroi merged 13 commits into
masterfrom
audit-follow-up

Conversation

@phroi

@phroi phroi commented Sep 26, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

This PR revisits our executable review of the deployed scripts. No contract code or release binary changes: only the tests, the golden-vector generator, the report and a CI workflow.

Tests

  • Every verification now checks that each cell holds its occupied capacity. For transactions the scripts accept, it also applies the node's capacity balance and DAO lock-size rules, including the lock-size rule's exemption for deposits before block 10,000,000. Many fixtures were building cells or transactions a node would reject; with realistic cells and funded transactions a few verdicts changed to what real cells show.
  • The golden vectors behind the stack's testkit now live here, in scripts/vector-gen. The suite replays every constructible vector against the release binaries.
  • New regressions back report claims that had no executed test. Each commit lists them.
  • SHANNONS is renamed CKB, so 1_000 * CKB reads as the amount it is.
  • Shared helpers (cell, input, fail, create_deposit, create_udt, create_receipt) replace the repeated builders, taking the suite from 16,412 to 12,160 lines. A before/after log of every verified transaction shows the refactor builds the same transactions with the same results.

Report

  • Re-audited claim by claim. The main corrections:
    • LO-01 names its cause: limit_order runs only when a transaction holds a master or spends an order, so order cells created alone go unchecked.
    • Receiptless deposits are unbounded in size, and value no minted iCKB matches is effectively burned.
    • Deposits return their occupied capacity to the withdrawer, so those with the smallest iCKB value pay the most per iCKB.
    • The aggregate-deposit spread path costs its user.
  • The dead CKB links now pin release v0.210.0. Whitepaper links point at master, since both repositories evolve.
  • Renamed to ICKB-Audit-Report.md with a review period, since it keeps evolving.

CI

.github/workflows/tests.yml runs on pull requests and pushes to master. It runs cargo test --locked -p tests with the pinned Rust from scripts/rust-toolchain, then builds the vector generator, which the suite does not compile. The checkout action is pinned by commit and the job has read-only permissions.

From scripts/, cargo test -p tests passes all 231 tests, and every commit passes on its own.

Merging renames the report, so the whitepaper's link to 20260501-ICKB-Audit-Report.md needs its prepared update right after.

@phroi
test: name the shannons-per-CKB constant CKB
37ad50f 
The suite wrote CKB amounts as `1_000 * SHANNONS`, which reads as 1,000
shannons although it means 1,000 CKB in shannons. The constant is now CKB,
so the same amount reads `1_000 * CKB`. No value or behavior changes.
@phroi
test: track the golden-vector generator
e5e09a5 
The stack's testkit checks its TypeScript contract oracle against golden
vectors computed by the contracts' own Rust logic. The generator lived only as
untracked files in a local checkout, so the stack's fixture could not be
regenerated from any commit.

vector-gen compiles c256.rs and ickb_logic/constants.rs straight from the
contract sources and holds byte-exact copies of the private deposit_to_ickb
and validate, which drift_checks asserts are still substrings of their
sources. The cases and expected values live in src/vectors.rs so the contract
tests can replay them; src/main.rs writes them as JSON. Its output parses to
the same data as the stack's committed fixture.

The crate stays outside the contracts workspace. Its lockfile uses format 3,
since the pinned Rust 1.74 cannot read format 4.
@phroi
test: fixtures hold consensus-valid cells, checked on every verify
35bf3ad 
ckb-testtool runs scripts only, so fixtures could build cells or withdrawals a
node would reject and still pass. Many did: order cells were sized for 73
bytes of order data instead of 89, xUDT cells sat 26 CKB below their occupied
capacity, owner, master and funding cells held a few hundred shannons, and
named locks carried args longer than any real lock. What those tests showed
depended on the fixture rather than on the scripts.

Tests now verify through VerifyRealistic::verify. It asserts every input and
output holds at least its occupied capacity, and, for transactions the scripts
accept, the node's DaoScriptSizeVerifier rule that a deposit and its
withdrawal request use locks of the same size. Self-tests show both checks
fire. The fixtures follow:

- typed cells take occupied_capacity(lock, type, data length);
- CKB amounts written in shannons become CKB;
- named locks take a secp lock's 20-byte args, so mainnet replay capacities
  fit;
- DAO transactions whose outputs exceed their inputs gain a funding input,
  appended so input and output indices still pair;
- foreign deposits wrapped by Owned Owner use a lock of its size.

Existing tests now state what real cells show:

- a terminal CKB->UDT fill passes at exact occupied capacity;
- a fulfilled order continuation fails as InvalidMatch;
- a lock-only order returns the typed MissingUdtType, where the undersized
  fixtures had trapped with -1;
- two cloned live orders permuted in a match strand neither master;
- the real-order stranding uses a foreign-token fake order;
- the spread-path test follows one verified trajectory from receipt to
  claim.
@phroi
test: replay the golden vectors against the release binaries
39f1947 
The golden vectors had only ever been computed by copies of contract
functions, never checked against the compiled scripts. The contract tests now
include vector-gen/src/vectors.rs and replay its rows as real transactions:

- each deposit vector mints exactly its expected iCKB at phase 2, while one
  shannon more or less fails with AmountMismatch;
- each match vector gets the copied validate's verdict from the binary, which
  also parses the order the copies skip;
- drift_checks runs with the suite, so a copy that stops matching its source
  fails here too.

Two match rows shrink a fulfilled CKB->UDT order below its occupied capacity,
which consensus forbids. They are skipped by name, so a new unconstructible
row breaks the test instead of vanishing; this also records that
AttemptToChangeFulfilled cannot be reached on chain.

vector-gen's README records how to regenerate the stack's fixture, including
the Prettier step that makes it byte-identical.
@phroi
test: iCKB Logic regressions for claims the review had not executed
ff7c5d0 
Each test backs a report conclusion that had no executable evidence:

- receiptless deposits of 500 CKB and 0 CKB unoccupied are created without
  the deposit bounds, which iCKB Logic checks only when a receipt makes it
  run, and are withdrawn at exactly their iCKB value;
- withdrawing a 1,000 CKB deposit returns more CKB per iCKB than a 100k CKB
  one, because the claim also returns the occupied capacity the valuation
  leaves out;
- under a non-genesis AR, ten shannons past the shifted soft-cap edge mint
  cap+9, which only normalization before the haircut yields;
- a withdrawal request locked by iCKB Logic fails with ScriptMisuse, with a
  user-locked control;
- withdrawing and re-depositing in one transaction verifies with the request
  at the deposit's index and fails with DAO error -20 when the two swap;
- a deposit and receipt created in the same block and spent together mint
  nothing, while minting 1 fails with AmountMismatch;
- a phase 2 conversion with 512 unrelated outputs stays within the cycle
  budget.
@phroi
test: Owned Owner regressions for pairing and a signed sparse claim
b622548 
- Two owned withdrawal requests with a single owner cell fail with Mismatch,
  the reverse of the existing two-owners-for-one-request case.
- Phase 1 consumes two noncontiguous inputs under the same secp lock, then
  phase 2 spends the signed owner cell and completes the DAO claim; a signing
  test shows the noncontiguous group signature verifies and a tampered
  witness does not. The signing helper is generalized from a contiguous input
  range to any input indices for it.
@phroi
test: a mint pairs each order with exactly one master
35e667d 
A master cell carries limit_order as its type, so the script runs at mint and
pairs every order with exactly one master. One order with its master mints;
two orders on one master fail with SameMaster; a master without an order and
an order pointing at a cell that is no master fail with InvalidConfiguration.
This bounds the Limit Order confusion attack: it needs order cells created in
a transaction with no master, where the script does not run.
@phroi
docs: the report states what the executable review now shows
7ac6838 
The report is updated to the tests added before it, and corrected where a
re-audit found it wrong or unsupported. Contract-source citations stay pinned
to the reviewed commit 454cfa9, whose code is deployed and immutable.

Findings and conclusions:
- LO-01 now names its cause: limit_order runs only when a transaction creates
  or spends a master or spends an order, so order cells created alone go
  unchecked. Phantom and fake orders come from there, and a fake order can
  strand a real one. Permuting identical orders and a minter pairing its own
  orders and masters are listed as harmless.
- Script Grouping states that each iCKB script validates the whole
  transaction whenever it runs, so its only gaps come from transactions where
  it does not run.
- Deposit bounds apply only to receipted deposits. The pool can hold
  deposits of any size, and value no minted iCKB matches is effectively
  burned.
- The aggregate-deposit spread path costs its user, not the reverse.
- Small deposits return their occupied capacity to the withdrawer, so
  smallest-first is optimal within the bounds and below the soft cap.
- Scenarios 4F and 6G, iCKB-locked requests, one order per master and the
  unreachable AttemptToChangeFulfilled and DuplicatedMaster now cite tests or
  code.
- Foreign DAO wrapping is limited by the node's lock-size rule for deposits
  since block 10,000,000.
- Four narrowed claims: double execution, the node (not the VM) enforcing
  occupied capacity, the xUDT witness tests, and the verifier's scope.

Citations:
- The dead links to nervosnetwork/ckb@6730f80, a commit that does not exist,
  now pin v0.210.0 with re-derived lines. The DAO lock-size rule is equality.
- The deployed NervosDAO source is identified through ckb-system-scripts
  v0.5.4.
- Whitepaper links point at master, since both repositories evolve, and one
  quote follows the whitepaper's current wording.
- The stack's connector filter, which exists only on its unmerged branch, is
  described without a link.
- The 180-epoch cycle cites RFC 0023.
- The executed test count is 230.

The report's Markdown tables are realigned.
@phroi
docs: rename the report and give it a review period
6bdfc78 
The report is an evolving review, revised since its first completion on
2026-05-01, so a single date in its name and header misdescribes it. It is
now ICKB-Audit-Report.md with a review period of 2026-05-01 to 2026-09-26;
the repository history records each revision. Both README links follow the
rename.
@phroi
phroi force-pushed the audit-follow-up branch 3 times, most recently from 9962f64 to 7f4ea4c Compare September 26, 2026 13:45
@phroi phroi changed the title Executable review follow-up: realistic fixtures, vector replay, report re-audit Executable review follow-up: realistic fixtures, vector replay, report re-audit, CI Sep 26, 2026
@phroi
test: shared helpers for cells, inputs and failing verifications
42a0009 
The suite spelled out cells with multi-line CellOutput builders (932),
inputs with CellInput builders (540), and each expected failure as verify,
unwrap_err and assert_script_error (120). Deposit, UDT and receipt cells
repeated the same create_cell shape about 140 times.

cell(capacity, lock, type) replaces 930 of the cell builders; the two left
measure occupied capacity or omit a capacity on purpose. input(out_point)
replaces the 481 inputs without a since. fail(context, tx, code) replaces the
120 failure checks. create_deposit, create_udt and create_receipt build the
recurring live cells at the sizes the fixtures already used. Needless double
borrows are removed. The suite drops from 16,412 to 12,160 lines.

No test changes what it checks. A temporary log of every verification, run
before and after, recorded each transaction's inputs, outputs, data, since
values, header deps and witness lengths, with random out points masked,
together with its result. Of the 230 tests, 206 verify transactions: the 184
whose transactions are deterministic built identical ones, and the 22 that
use random keys or hash-ordered headers returned the same results. As a
control, changing one funding cell by 1 CKB makes that log differ.
@phroi
test: accepted transactions also pass the node's capacity checks
5a731c4 
The previous check stopped short of two node rules, so some passing
fixtures described transactions a node would reject, and one rule was
applied more strictly than the node does.

- CapacityVerifier: outputs may not hold more than inputs, unless a DAO
  input is present and the DAO script checks compensation instead. Thirty
  tests minted, melted, matched or replayed vectors while creating CKB; each
  such transaction now gains a funding input, appended so existing input
  indices keep their meaning. The vector replay funds exactly the CKB each
  order gains. A signed phase 2 test now signs only its secp input group.
- DaoScriptSizeVerifier exempts deposits committed before block 10,000,000,
  which the check ignored. It now honors that exemption. The two
  foreign-wrapping tests keep their original user locks, valid again for
  their legacy deposits. The self-test rejects a post-10,000,000 withdrawal
  and a control accepts a legacy one.

Every assertion is unchanged. Two crosswire tests already
accept either of two error codes, which vary between runs with the random
out points. The report's scope statement and the tests
README describe both rules, and the test count becomes 231.
@phroi
docs: the withdrawal bonus follows a deposit's iCKB value, not its size
b9e15f9 
Scenario 3B said withdrawing the smallest deposits first is optimal. Below
the soft cap, the claim per iCKB burned is AR_n / AR_0 plus 82 CKB divided by
the deposit's iCKB value, and that value depends on the deposit's
accumulated rate as well as its unoccupied capacity. At a withdrawal AR of
1.3e16, a 1,050 CKB deposit made at 1.2e16 pays more per iCKB than a 1,000 CKB
deposit made at 1.1e16. The report now says to withdraw the deposits with
the smallest iCKB value first.
@phroi
ci: run the contract tests on pull requests and master pushes
be28246 
Nothing ran the suite outside a local checkout, so a change could break the
tests or the vector generator unnoticed. The workflow runs the test crate,
which executes the committed release binaries, checks their deployment
hashes and replays the golden vectors. It also builds the vector-gen writer,
which the suite does not compile. rustup takes the pinned Rust from
scripts/rust-toolchain; the checkout action is pinned by commit, and the job
has read-only permissions.
@phroi

phroi commented Sep 26, 2026

Copy link
Copy Markdown
Member Author

LGTM!!

Phroi %304

@phroi
phroi merged commit c951927 into master Sep 26, 2026
1 check passed
@phroi phroi mentioned this pull request Sep 26, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@phroi