test(check order): differential revert-order harness for B20 multi-precondition ops#90
Open
amiecorso wants to merge 8 commits into
Open
test(check order): differential revert-order harness for B20 multi-precondition ops#90amiecorso wants to merge 8 commits into
amiecorso wants to merge 8 commits into
Conversation
First contract in the deferred follow-up from PR #89. Enumerates all C(5, 2) = 10 pairs of preconditions that mint enforces (ROLE, ZERO-RECEIVER, PAUSE, POLICY, SUPPLY-CAP) and asserts the canonical first-firing revert selector for each. Adopts the Solidity reference's defensive-depth order as canonical (Option A from the audit-response report): ROLE -> ZERO -> PAUSE -> POLICY -> CAP. Mock-mode: 10/10 pass (Solidity already follows canonical). Fork-mode against current Rust precompile: 8/10 pass, 2 fail. The 2 failures surface real Rust divergences: (1) Rust checks zero-receiver before the role modifier, so role_beats_zeroRecipient fails with InvalidReceiver instead of AccessControlUnauthorizedAccount; (2) Rust checks policy before pause, so pause_beats_policy fails with PolicyForbids instead of ContractPaused. Both are exactly the divergences the audit-response report predicted.
…ndition ops Adds differential check-order tests for: burn, burnBlocked, transfer, transferFrom, approve, permit, batchBurn, batchMint, and security redeem. Canonical order in every case is the Solidity reference's order (Option A from the audit-response report). Tests pass in mock mode (Solidity already follows canonical) and pass in fork mode for every pair that agrees with Rust. Five fork-mode failures surface the actual Rust divergences from canonical: (1) mint: Rust checks zero-receiver before the role modifier; (2) mint: Rust checks receiver-policy before the pause guard; (3) transfer: Rust checks zero-sender before zero-receiver (only matters when both addresses are zero); (4) burnBlocked: Rust checks the blocked-membership guard before the pause guard; (5) transferFrom: Rust checks the executor-policy before consuming allowance. No Rust changes in this PR — the harness exists to highlight divergences and prevent regressions going forward. Mock-only suite: 553/0/0. Fork suite filtered to revertOrder tests: 49/5/0 (5 expected fails).
…createB20 Adds differential check-order tests for the remaining multi-precondition functions on the B20 surface: renounceRole, renounceLastAdmin, updatePolicy, updateSupplyCap, updateSecurityIdentifier, announce, and createB20 (STABLECOIN + SECURITY arms). All 13 new tests pass in mock and fork mode — no new Rust divergences surfaced. The 5 known divergences from the earlier commit are unchanged. revokeRole is the one multi-precondition function intentionally not covered here: its only interesting pair is ROLE vs LAST-ADMIN, but the LAST-ADMIN guard is being added in PR #91 (BOP-196) and the test will land alongside that fix to avoid a stranded mock-mode failure. Coverage now: 67 tests across 17 files. Fork: 62 pass / 5 fail (same 5 divergences as before).
Reflow vm.expectRevert(abi.encodeWithSelector(...)) blocks per forge fmt's line-width rules. Fixes the CI format check on PR #90. Test behavior is unchanged.
…ac/check-order-tests
Now that PR #91 (BOP-196) has merged into main and through to this branch via the merge commit, the revokeRole LAST-ADMIN guard exists in MockB20. Adds the deferred ROLE-vs-LAST-ADMIN pair test. Both mock and fork pass: in both backends the role-admin check fires before the body's last-admin check, so the canonical ordering holds. Coverage now: 68 tests across 18 files. Fork: 63 pass / 5 fail (same 5 long-known divergences).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow-up from PR #89 (audit response). Adds a differential check-order test harness covering every multi-precondition function in the B20 surface. The harness pins the canonical first-firing revert selector for each pair of preconditions a function enforces, so that any future drift between the Solidity reference and the Rust precompile surfaces as a precise selector diff in fork mode.
Canonical order = Solidity reference (Option A from the audit-response report). Tests pass in mock mode iff Solidity follows canonical (it does, by construction), and they pass in fork mode iff Rust agrees with canonical. The five fork-mode failures in this PR are intentional and surface real Rust↔Solidity divergences — no Rust changes here; this PR exists to highlight the divergences and lock them in as regression coverage.
Coverage
Exhaustive across the B20 surface — covers every multi-precondition function. PR #91 (BOP-196) merging into this branch unblocked the final
revokeRolepair.test/unit/B20/supply/mint_revertOrder.t.soltest/unit/B20/supply/burn_revertOrder.t.soltest/unit/B20/supply/burnBlocked_revertOrder.t.soltest/unit/B20/supply/updateSupplyCap_revertOrder.t.soltest/unit/B20/erc20/transfer_revertOrder.t.soltest/unit/B20/erc20/transferFrom_revertOrder.t.soltest/unit/B20/erc20/approve_revertOrder.t.soltest/unit/B20/permit/permit_revertOrder.t.soltest/unit/B20/policy/updatePolicy_revertOrder.t.soltest/unit/B20/roles/revokeRole_revertOrder.t.soltest/unit/B20/roles/renounceRole_revertOrder.t.soltest/unit/B20/roles/renounceLastAdmin_revertOrder.t.soltest/unit/B20Security/batch/batchBurn_revertOrder.t.soltest/unit/B20Security/batch/batchMint_revertOrder.t.soltest/unit/B20Security/redeem/redeem_revertOrder.t.soltest/unit/B20Security/identifiers/updateSecurityIdentifier_revertOrder.t.soltest/unit/B20Security/announcement/announce_revertOrder.t.soltest/unit/B20Factory/createB20_revertOrder.t.solFull mock-only suite passes with no regressions to existing tests.
Rust↔Solidity divergences surfaced
The five fork-mode failures are the actual check-order divergences between the Rust precompile (
base@f9d9d2804) and the Solidity reference. Each is a precise selector diff at a specific multi-violation input:mintrolevszeroRecipientAccessControlUnauthorizedAccountInvalidReceiver(0)mintpausevspolicyContractPaused(MINT)PolicyForbids(MINT_RECEIVER, ...)transferzeroReceivervszeroSenderInvalidReceiver(0)InvalidSender(0)burnBlockedpausevsblockedContractPaused(BURN)AccountNotBlocked(...)transferFromallowancevsexecutorPolicyInsufficientAllowancePolicyForbids(EXECUTOR, ...)These five are now structural facts of the codebase. The PR's value is making them visible and pinning them so:
Each divergence has a tracking ticket under the Internal Audit Fixes milestone (see the thread on #proj-b20).
Design notes
transferFromonly adds tests for its two new preconditions (ALLOWANCE, EXECUTOR-POLICY); the underlying_transferbody pairs are already pinned bytransfer_revertOrder.t.sol. SimilarlybatchMintonly pins LENGTH/EMPTY against the first per-element check (ROLE), since the rest of_mint's order is pinned bymint_revertOrder.t.sol.vm.prankpattern. Tests that need a role-resolution view call (e.g.security().BURN_FROM_ROLE()) resolve it to a local variable BEFORE thevm.prank, to avoid the prank being consumed by the view call. Same pattern as_setRedeemPolicyinB20SecurityTest.Out of scope
Commits
68d763dtest(check-order): differential mint revert-ordering harnesse337e92test(check-order): extend revert-order harness across all multi-precondition ops5a827f1test(check-order): extend harness to roles, admin ops, announce, and createB2003cefcdstyle: forge fmtd7e49catest(check-order): revokeRole pair test (unblocked by BOP-196 merge)