audit: align mocks to Rust for claims 2-6 + parity tests by amiecorso · Pull Request #89 · base/base-std

amiecorso · 2026-05-28T02:47:37Z

Summary

Response to the recent B20 audit pass. For each of the 6 specific discrepancies the auditor flagged, this PR either (a) brings the Solidity mock into selector-level parity with the Rust precompile and adds an explicit test that would catch divergence, or (b) adds the missing parity test where the behavior was already correct.

Verified against current main on both repos (base-std@4e6f474, f9d9d2804). All 506 unit tests pass mock-only and 504/0/2 in fork mode against the Rust precompile (the 2 skips are pre-existing unrelated slot-helper tests).

The auditor's two discussion points (check ordering, storage cleanup) are intentionally out of scope here. Check ordering is a separate effort (differential test harness across precondition pairs, planned as a follow-up PR). Storage cleanup was based on an incorrect premise — mapping[k] = 0 and delete mapping[k] are byte-identical at the EVM trie level.

Per-claim breakdown

#	Claim	Source change	Test added
2	Empty stablecoin currency	Add `if (cb.length == 0) revert InvalidCurrency(...)` to `MockB20Factory.createB20` STABLECOIN branch. Fix `_isValidFiatCode("")` returning vacuously true.	`test_createB20_revert_emptyCurrency`
3	`getB20Address` invalid variant	None — ABI decoder rejects before any function body runs	`test_getB20Address_revert_outOfRangeVariant`
4	`batchBurn` zero amount	Add `if (amounts[i] == 0) revert InvalidAmount();` per-element in `MockB20Security.batchBurn` loop	`test_batchBurn_revert_zeroAmountInBatch`
5	`security_redeem_burn` zero amount	Add `if (amount == 0) revert InvalidAmount();` to `MockB20Security._redeemBurn` before shares math	`test_redeem_revert_zeroAmount`
6	`permit` malformed signature	None — already behavior-equivalent	`test_permit_revert_malformedSignature`

Claim 1 (single vs three createToken) isn't a real divergence; both sides have a single parameterized dispatcher.

Test results

Mock-only suite: 506 passed (added 7 new tests)
Fork suite vs Rust f9d9d2804: 504/0/2 (the 2 skips are pre-existing slot-helper tests, not introduced by this PR)

Notes worth flagging

Claim 2 has a fixup! commit on top. The Rust precompile reverts InvalidCurrency("") on empty currency, not MissingRequiredField() as I initially guessed. The fork test surfaced it immediately. The fixup brings Solidity to the correct selector and renames the test from _missingCurrency to _emptyCurrency. Squash with the base commit if you prefer a single clean commit per claim.
Claim 3 has no source change. Solidity's ABI decoder rejects out-of-range enum bytes with Panic(0x21) before any function body runs; Rust rejects with InvalidVariant(). Both reject — different selectors. No way to make Solidity match without breaking the public ABI. New test asserts vm.expectRevert() (any revert) to pin the parity-of-rejection.
Claim 6 has no source change. Behavior was already equivalent — alloy returns Err on malformed sigs and maps to InvalidSigner(0, owner), same as Solidity's defensive zero check. New test exercises the malformed v=0 path as regression coverage.
One side-effect bug fixed along the way: _isValidFiatCode("") was returning vacuously true in the test helper, which is what made vm.assume(!_isValidFiatCode(code)) silently exclude empty from every currency fuzz test. Fixed in claim 2's commit. Future fuzz tests of the same shape won't have this blind spot.
Bound sweep completed for the audit-relevant surface. Four success-test bounds relaxed from [1, max] to [0, max] on amount/spendAmount parameters in allowance.t.sol, transferFrom.t.sol (2 tests), and transferFromWithMemo.t.sol. The Rust precompile has no zero-amount guard on regular mint/burn/transfer/transferFrom (only batchBurn and security_redeem have them, both covered by claims 4 and 5), so the broader fuzz domain reproduces no new divergences against Rust — fork suite stays at 504/0/2. The other [1, max] sites in the suite are intentional: revert tests (bound is part of the setup), share-ratio tests (ratio = 0 is mathematically degenerate), chainId tests (chainId = 0 doesn't exist in the wild), and the redeem/batchBurn success tests where [1, max] is now the correct precondition after claims 4 and 5.
Audit of remaining zero-edge fuzz exclusions. Swept the rest of the suite for the same shape of bug. Two adjacent areas (empty value on updateSecurityIdentifier, stored zero on share-ratio reads) turned out to already be covered by existing tests at the base-function level (emptyValueRemoves, defaultIsWad, zeroRestoresWadFallback). Added two derived-function-level tests for the ratio fallback (test_toShares_success_explicitZeroRatioFallsBackToWad, test_sharesOf_success_explicitZeroRatioFallsBackToWad) so a refactor that reads the storage slot directly instead of going through _sharesToTokensRatio() would fail loudly. The remaining bound(x, 1, max) sites in the suite are intentional: revert tests, redeem/batchBurn success tests where [1, max] is the correct precondition after claims 4 and 5, and chainId >= 1 in permit tests (chainId=0 doesn't exist in the wild).

Out of scope (follow-ups)

Differential check-ordering test harness (auditor's D1). Real divergence on mint (and likely burnBlocked). Design captured in cortex doc; planned as a separate PR.

Commits

66f4afe claim 2: reject empty stablecoin currency with MissingRequiredField
eaeb5fd claim 3: add raw-bytes test for getB20Address out-of-range variant
3661d00 claim 4: reject zero amount per-element in batchBurn
84c4a84 claim 5: reject zero amount in security redeem with InvalidAmount
4106630 claim 6: parity test for malformed-signature permit recovery
9056a14 fixup! claim 2: align empty-currency selector to InvalidCurrency
2e334cd fuzz: extend success-test amount bounds to cover zero on non-rejecting paths
70a6df1 gap 2: cover stored-zero share-ratio fallback at toShares + sharesOf level

Mirrors the Rust precompile's empty-currency rejection. The format-check loop was vacuously safe on empty input (no bytes to inspect). Adds an explicit length check matching the SECURITY-variant isin guard. Also fixes the test helper _isValidFiatCode so empty returns false (it was returning vacuously true, causing every fuzz test that filtered with vm.assume(!_isValidFiatCode) to silently exclude empty).

Mirrors test_createB20_revert_outOfRangeVariant. No source change: Solidity's ABI decoder rejects out-of-range enum bytes with Panic(0x21) before any function body runs, so an explicit InvalidVariant guard at the body level would be unreachable. The Rust precompile rejects the same input with the typed InvalidVariant() revert. Observable behavior matches (both backends revert); the specific selectors differ.

Mirrors the Rust precompile's per-element zero-amount guard. Fires inside the loop before the balance check, so a zero in any slot reverts the whole batch (all-or-nothing atomicity). New test test_batchBurn_revert_zeroAmountInBatch exercises the canonical case (zero in the second slot) and asserts both balances are unchanged.

Mirrors the Rust precompile's explicit amount.is_zero() guard. Fires AFTER pause/policy and BEFORE the shares math, so zero-amount redeems revert with IB20.InvalidAmount() (matching Rust) rather than being absorbed by the downstream shares == 0 check (which would surface as BelowMinimumRedeemable). The distinction matters for integrators that distinguish 'amount must be non-zero' from 'amount too small to clear the floor'.

Pins the cross-backend equivalence on the recovered == address(0) path. EVM's ecrecover returns address(0) for malformed (v, r, s); alloy in the Rust precompile returns Err from recover_address_from_prehash and maps it to InvalidSigner(address(0), owner). Same observable result, different code paths. No source change: the behavior is already correct on both sides; the new test prevents silent drift.

Fork test confirmed the Rust precompile reverts InvalidCurrency("") on empty currency, not MissingRequiredField(). Switch Solidity guard to match. Test renamed test_createB20_revert_missingCurrency -> test_createB20_revert_emptyCurrency to reflect the actual selector. Squash this into the claim 2 commit when reviewing.

…g paths The Rust precompile does not reject zero amount on regular mint/burn/transfer/transferFrom (only batchBurn and security_redeem have explicit zero guards, both covered by claims 4 and 5). Success tests for non-rejecting paths should therefore fuzz the full valid input domain rather than defensively excluding zero. Relaxes four success-test bounds: allowance decreasesAfterTransferFrom (spendAmount), transferFrom decreasesAllowance (spendAmount), transferFrom infiniteAllowanceUnchanged (amount), transferFromWithMemo movesBalanceAndDecreasesAllowance (amount). All assertions hold at zero (x - 0 == x, 0 == 0). Full fork suite passes 502/0/2 against the Rust precompile with the broader fuzz domain.

…level The documented behavior is that a stored sharesToTokensRatio of zero resolves as WAD_PRECISION on the read surface. The base-level fallback is already covered by test_sharesToTokensRatio_success_zeroRestoresWadFallback. These two tests pin the same property at the derived-function level so a refactor that bypasses _sharesToTokensRatio() and reads the storage slot directly would fail. Both backends behave identically (verified via fork suite).

amiecorso · 2026-05-28T03:25:49Z

+            // Per-element zero-amount guard, matching the Rust precompile. Fires inside
+            // the loop before the balance check, so a zero in any slot reverts the whole
+            // batch (all-or-nothing atomicity).
+            if (amounts[i] == 0) revert InvalidAmount();


in B20 default, we allow 0 amount for operations (transfer requires 0 to be allowed per ERC20 spec, and we allow the same for burn and mint as they're forms of transfer and the consistency is nice / we should let people decide for themselves whether they allow 0 amounts which can be relevant to batch ergonomics etc.)

Do we want to allow 0 here after all? (same question below)

Defer to @stevieraykatz on how securities should function

"Allow 0" as a rule more closely follows ERC20 "treat 0-amount transfers as valid"

amiecorso · 2026-05-28T03:26:23Z

+            // no bytes to inspect on empty input and would vacuously succeed otherwise.
+            // Reverts InvalidCurrency("") to match the Rust precompile's selector.
            bytes memory cb = bytes(p.currency);
+            if (cb.length == 0) revert InvalidCurrency(p.currency);


I think the only question here is whether we want this to revert with "InvalidCurrency" which is what the rust code does, or "MissingRequiredField"

Recommendation: stick with InvalidCurrency and match rust. more informative error.

Collapse aligned trailing comments and wrap a long .call(...) expression per forge fmt. Fixes the CI format check on PR #89. Test behavior is unchanged.

stevieraykatz · 2026-05-28T19:39:33Z

+        // and BEFORE the shares math, so zero-amount surfaces as InvalidAmount() rather than
+        // being absorbed by the shares == 0 path below (which is reserved for nonzero amounts
+        // that round to zero shares under low ratios).
+        if (amount == 0) revert InvalidAmount();


same logic as above. we should allow 0-burn

amiecorso added 8 commits May 27, 2026 19:30

amiecorso commented May 28, 2026

View reviewed changes

This was referenced May 28, 2026

test(check order): differential revert-order harness for B20 multi-precondition ops #90

Draft

fix(roles): reject revokeRole on sole admin (BOP-196) #91

Merged

Merge branch 'main' into ac/audit-fixes

1a86b1a

amiecorso requested a review from stevieraykatz May 28, 2026 18:34

style: forge fmt

1d8a44c

Collapse aligned trailing comments and wrap a long .call(...) expression per forge fmt. Fixes the CI format check on PR #89. Test behavior is unchanged.

stevieraykatz reviewed May 28, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

audit: align mocks to Rust for claims 2-6 + parity tests#89

audit: align mocks to Rust for claims 2-6 + parity tests#89
amiecorso wants to merge 10 commits into
mainfrom
ac/audit-fixes

amiecorso commented May 28, 2026 •

edited

Loading

Uh oh!

amiecorso May 28, 2026

Uh oh!

amiecorso May 28, 2026

Uh oh!

stevieraykatz May 28, 2026 •

edited

Loading

Uh oh!

amiecorso May 28, 2026

Uh oh!

amiecorso May 28, 2026

Uh oh!

stevieraykatz May 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

amiecorso commented May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Per-claim breakdown

Test results

Notes worth flagging

Out of scope (follow-ups)

Commits

Uh oh!

amiecorso May 28, 2026

Choose a reason for hiding this comment

Uh oh!

amiecorso May 28, 2026

Choose a reason for hiding this comment

Uh oh!

stevieraykatz May 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amiecorso May 28, 2026

Choose a reason for hiding this comment

Uh oh!

amiecorso May 28, 2026

Choose a reason for hiding this comment

Uh oh!

stevieraykatz May 28, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

amiecorso commented May 28, 2026 •

edited

Loading

stevieraykatz May 28, 2026 •

edited

Loading