From 733cd92368e2880c68f8a7cb23ae4e95ffa6036a Mon Sep 17 00:00:00 2001 From: Amie Corso Date: Fri, 29 May 2026 10:43:18 -0700 Subject: [PATCH 1/5] =?UTF-8?q?refactor:=20order=20B20=20mock=20checks=20p?= =?UTF-8?q?ause=20=E2=86=92=20role=20=E2=86=92=20input=20via=20modifiers?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hoist cross-cutting gates from shared internal helpers to external entrypoints as modifiers, so every mutating op evaluates its preconditions in the canonical order: pause → role → input → allowance → policy → invariants → effects Modifier list order equals evaluation order (Solidity runs modifiers left-to-right, body at `_`), so each function's modifier set reads top-to-bottom as its revert precedence. MockB20 - New modifiers: `whenNotPaused` (bypass-aware), `validReceiver`, `validSender`, `validApprover`, `validSpender`, `nonEmptyFeatures`. - Internal helpers reduced to pure mechanics: `_transfer` (policy + balance + effects), `_mint` (policy + supply-cap + effects), `_burnRaw` (balance + effects). `_burnSelf` folded away. MockB20Security - New `whenNotPausedStrict` modifier mirrors `onlyRoleStrict`: no factory-bootstrap bypass, used by the holder-initiated `redeem` / `redeemWithMemo` path. - `batchMint` declares `whenNotPaused(MINT) + onlyRole(MINT_ROLE)` itself (was inherited per-element via `_mint`); per-element `validReceiver` inlined in the loop since modifier params evaluate once at entry. - `batchBurn` swaps inline pause check for the `whenNotPaused(BURN)` modifier; role-strict semantics preserved. - `_redeemBurn` body strips its inline pause check. Interface natspec - `IB20` / `IB20Security` revert-order lists rewritten as explicit numbered lists in the new canonical order for `transfer`, `transferFrom`, `approve`, `mint`, `burn`, `burnBlocked`, `pause`, `unpause`, `redeem`, `batchMint`, `batchBurn`. Tests - Revert-order tests under `test/unit/B20/{erc20,supply}/*_revertOrder.t.sol` and `test/unit/B20Security/batch/*_revertOrder.t.sol` updated to assert the new precedence (canonical-order docstrings rewritten; pair tests renamed and flipped where precedence reversed). - Two happy-path tests in `test/unit/B20Security/batch/batchMint.t.sol` now grant MINT_ROLE before triggering EmptyBatch / LengthMismatch since the role gate now fires before body checks. - forge build clean. forge test: 592 / 592 passing. Out of band: the production Rust precompile (separate repo) must be reordered identically and mock ↔ Rust parity verified on multi-failure inputs, especially `transferFrom`. Coordinate with the Rust owner. Refs: BOP-215 --- src/interfaces/IB20.sol | 125 ++++++++----- src/interfaces/IB20Security.sol | 104 ++++++---- test/lib/mocks/MockB20.sol | 177 ++++++++++++++---- test/lib/mocks/MockB20Security.sol | 51 ++++- .../B20/erc20/transferFrom_revertOrder.t.sol | 175 +++++++++++++++-- .../unit/B20/erc20/transfer_revertOrder.t.sol | 52 ++--- .../B20/supply/burnBlocked_revertOrder.t.sol | 27 ++- test/unit/B20/supply/burn_revertOrder.t.sol | 26 +-- test/unit/B20/supply/mint_revertOrder.t.sol | 135 +++++++------ .../batch/batchBurn_revertOrder.t.sol | 106 +++++------ test/unit/B20Security/batch/batchMint.t.sol | 23 ++- .../batch/batchMint_revertOrder.t.sol | 97 ++++++++-- 12 files changed, 748 insertions(+), 350 deletions(-) diff --git a/src/interfaces/IB20.sol b/src/interfaces/IB20.sol index 6901ecca..9de40a05 100644 --- a/src/interfaces/IB20.sol +++ b/src/interfaces/IB20.sol @@ -469,15 +469,18 @@ interface IB20 { /// @notice Allowance granted by `owner` to `spender`. function allowance(address owner, address spender) external view returns (uint256); - /// @notice Transfers `amount` from `msg.sender` to `to`. Reverts with: - /// - `ContractPaused(TRANSFER)` if `TRANSFER` is paused. - /// - `PolicyForbids(TRANSFER_SENDER_POLICY, policyId)` if `msg.sender` - /// is not authorized under the active `TRANSFER_SENDER_POLICY` policy. - /// - `PolicyForbids(TRANSFER_RECEIVER_POLICY, policyId)` if `to` is not - /// authorized under the active `TRANSFER_RECEIVER_POLICY` policy. - /// - `InsufficientBalance(msg.sender, balance, amount)` if the - /// caller does not have enough balance. - /// - `InvalidReceiver(to)` if `to == address(0)`. + /// @notice Transfers `amount` from `msg.sender` to `to`. Preconditions + /// evaluate in the order listed; when multiple are violated, + /// the earliest-listed revert is the one that surfaces: + /// 1. `ContractPaused(TRANSFER)` if `TRANSFER` is paused. + /// 2. `InvalidReceiver(to)` if `to == address(0)`. + /// 3. `InvalidSender(msg.sender)` if `msg.sender == address(0)`. + /// 4. `PolicyForbids(TRANSFER_SENDER_POLICY, policyId)` if `msg.sender` + /// is not authorized under the active `TRANSFER_SENDER_POLICY`. + /// 5. `PolicyForbids(TRANSFER_RECEIVER_POLICY, policyId)` if `to` + /// is not authorized under the active `TRANSFER_RECEIVER_POLICY`. + /// 6. `InsufficientBalance(msg.sender, balance, amount)` if the + /// caller does not have enough balance. /// @dev Does NOT consult the `TRANSFER_EXECUTOR_POLICY` policy: on direct /// `transfer` the executor IS the sender, and the sender /// check already covers that address. When the token is @@ -486,13 +489,23 @@ interface IB20 { function transfer(address to, uint256 amount) external returns (bool); /// @notice Transfers `amount` from `from` to `to` using `msg.sender`'s - /// allowance. Reverts as `transfer` does, plus: - /// - `InsufficientAllowance(msg.sender, allowance, amount)` - /// if the caller does not have enough allowance from `from`. - /// - `InvalidSender(from)` if `from == address(0)`. - /// - `PolicyForbids(TRANSFER_EXECUTOR_POLICY, policyId)` if - /// `msg.sender != from` and `msg.sender` is not authorized - /// under the active `TRANSFER_EXECUTOR_POLICY` policy. + /// allowance. Preconditions evaluate in the order listed; + /// when multiple are violated, the earliest-listed revert is + /// the one that surfaces: + /// 1. `ContractPaused(TRANSFER)` if `TRANSFER` is paused. + /// 2. `InvalidReceiver(to)` if `to == address(0)`. + /// 3. `InvalidSender(from)` if `from == address(0)`. + /// 4. `InsufficientAllowance(msg.sender, allowance, amount)` + /// if the caller does not have enough allowance from `from`. + /// 5. `PolicyForbids(TRANSFER_EXECUTOR_POLICY, policyId)` if + /// `msg.sender != from` and `msg.sender` is not authorized + /// under the active `TRANSFER_EXECUTOR_POLICY`. + /// 6. `PolicyForbids(TRANSFER_SENDER_POLICY, policyId)` if `from` + /// is not authorized under the active `TRANSFER_SENDER_POLICY`. + /// 7. `PolicyForbids(TRANSFER_RECEIVER_POLICY, policyId)` if `to` + /// is not authorized under the active `TRANSFER_RECEIVER_POLICY`. + /// 8. `InsufficientBalance(from, balance, amount)` if `from` + /// does not have enough balance. /// @dev The sender-side check is performed against `from` (the /// party whose balance moves), the receiver check against /// `to`, and the executor check against `msg.sender` only @@ -502,11 +515,12 @@ interface IB20 { /// @notice Sets `spender`'s allowance to `amount`. NOT gated by any /// policy or by pause; only the act of MOVING balance is gated. - /// @dev Reverts with `InvalidApprover(msg.sender)` if the - /// caller is `address(0)` (theoretically unreachable for - /// normal callers but enforced for parity with OZ ERC20), - /// and `InvalidSpender(spender)` if - /// `spender == address(0)`. + /// @dev Reverts in the following canonical order (first violated + /// check wins): + /// 1. `InvalidApprover(msg.sender)` if the caller is + /// `address(0)` (theoretically unreachable for normal + /// callers but enforced for parity with OZ ERC20). + /// 2. `InvalidSpender(spender)` if `spender == address(0)`. function approve(address spender, uint256 amount) external returns (bool); /*////////////////////////////////////////////////////////////// @@ -558,12 +572,17 @@ interface IB20 { MINT / BURN //////////////////////////////////////////////////////////////*/ - /// @notice Mints `amount` to `to`. Requires `MINT_ROLE`. Subject to: - /// 1. `totalSupply + amount <= supplyCap` (else - /// `SupplyCapExceeded`). - /// 2. `MINT` is not paused (else `ContractPaused(MINT)`). - /// 3. `to` is authorized under the active `MINT_RECEIVER_POLICY` - /// policy (else `PolicyForbids(MINT_RECEIVER_POLICY, policyId)`). + /// @notice Mints `amount` to `to`. Preconditions evaluate in the + /// order listed; when multiple are violated, the + /// earliest-listed revert is the one that surfaces: + /// 1. `ContractPaused(MINT)` if `MINT` is paused. + /// 2. `AccessControlUnauthorizedAccount(msg.sender, MINT_ROLE)` + /// if the caller does not hold `MINT_ROLE`. + /// 3. `InvalidReceiver(to)` if `to == address(0)`. + /// 4. `PolicyForbids(MINT_RECEIVER_POLICY, policyId)` if `to` + /// is not authorized under the active `MINT_RECEIVER_POLICY`. + /// 5. `SupplyCapExceeded(cap, totalSupply + amount)` if the + /// mint would push `totalSupply` past `supplyCap`. /// @dev Per-minter rate limiting is NOT enshrined at any level /// (Default or variant). Minter quotas live in EVM /// periphery contracts: a controller / wrapper that holds @@ -578,12 +597,16 @@ interface IB20 { /// after the standard `Transfer` event. function mintWithMemo(address to, uint256 amount, bytes32 memo) external; - /// @notice Burns `amount` from the caller's own balance. Requires - /// `BURN_ROLE`. Subject to `BURN` not being paused (else - /// `ContractPaused(BURN)`). NOT subject to any policy: - /// burn destroys the caller's own supply with no recipient. - /// Reverts with `InsufficientBalance(caller, balance, amount)` - /// if the caller does not have enough balance. + /// @notice Burns `amount` from the caller's own balance. Preconditions + /// evaluate in the order listed; when multiple are violated, + /// the earliest-listed revert is the one that surfaces: + /// 1. `ContractPaused(BURN)` if `BURN` is paused. + /// 2. `AccessControlUnauthorizedAccount(msg.sender, BURN_ROLE)` + /// if the caller does not hold `BURN_ROLE`. + /// 3. `InsufficientBalance(caller, balance, amount)` if the + /// caller does not have enough balance. + /// NOT subject to any policy: burn destroys the caller's own + /// supply with no recipient. /// @dev To destroy balance held by a third party (compliance /// seizure from a policy-blocked address), use `burnBlocked`. /// Emits `Transfer(caller, address(0), amount)`. @@ -593,16 +616,18 @@ interface IB20 { /// after the standard `Transfer` event. function burnWithMemo(uint256 amount, bytes32 memo) external; - /// @notice Destroys `amount` of `from`'s balance. Requires - /// `BURN_BLOCKED_ROLE`. Subject to: - /// 1. `BURN` is not paused (else `ContractPaused(BURN)`). - /// 2. `from` is NOT authorized under the active - /// `TRANSFER_SENDER_POLICY` policy (else `AccountNotBlocked(from)`). - /// `burnBlocked` exists for seizure of policy-blocked - /// balance; calling it against an authorized address is - /// rejected by design. - /// 3. `amount <= balanceOf(from)` (else - /// `InsufficientBalance(from, balance, amount)`). + /// @notice Destroys `amount` of `from`'s balance. Preconditions + /// evaluate in the order listed; when multiple are violated, + /// the earliest-listed revert is the one that surfaces: + /// 1. `ContractPaused(BURN)` if `BURN` is paused. + /// 2. `AccessControlUnauthorizedAccount(msg.sender, BURN_BLOCKED_ROLE)` + /// if the caller does not hold `BURN_BLOCKED_ROLE`. + /// 3. `AccountNotBlocked(from)` if `from` IS authorized under + /// the active `TRANSFER_SENDER_POLICY` policy. `burnBlocked` + /// exists for seizure of policy-blocked balance; calling + /// it against a non-blocked address is rejected by design. + /// 4. `InsufficientBalance(from, balance, amount)` if + /// `amount > balanceOf(from)`. /// @dev Designed for sanctions-seizure flows where compliance /// requires destruction of balance held by a blocked /// address. Tokens that follow a "freeze, never seize" @@ -705,16 +730,22 @@ interface IB20 { /// @notice Pauses the `features` operations. Additive: features /// already paused remain paused, and the listed features /// become paused (duplicates within the call are idempotent). - /// Requires `PAUSE_ROLE`. Reverts with `EmptyFeatureSet` if - /// `features.length == 0`. + /// Reverts in the following canonical order (first violated + /// check wins): + /// 1. `AccessControlUnauthorizedAccount(msg.sender, PAUSE_ROLE)` + /// if the caller does not hold `PAUSE_ROLE`. + /// 2. `EmptyFeatureSet()` if `features.length == 0`. function pause(PausableFeature[] calldata features) external; /// @notice Unpauses the `features` operations. Listed features /// become unpaused; features not listed are unaffected /// (duplicates are idempotent; unpausing a feature that is /// not currently paused is a no-op for that feature). - /// Requires `UNPAUSE_ROLE`. Reverts with `EmptyFeatureSet` - /// if `features.length == 0`. + /// Reverts in the following canonical order (first violated + /// check wins): + /// 1. `AccessControlUnauthorizedAccount(msg.sender, UNPAUSE_ROLE)` + /// if the caller does not hold `UNPAUSE_ROLE`. + /// 2. `EmptyFeatureSet()` if `features.length == 0`. function unpause(PausableFeature[] calldata features) external; /*////////////////////////////////////////////////////////////// diff --git a/src/interfaces/IB20Security.sol b/src/interfaces/IB20Security.sol index 0d4bb8fb..6f7b4e89 100644 --- a/src/interfaces/IB20Security.sol +++ b/src/interfaces/IB20Security.sol @@ -374,18 +374,25 @@ interface IB20Security is IB20 { /// allocations, secondary issuances, etc.) that need to /// land many recipients in one transaction. /// - /// @dev Requires `MINT_ROLE`. Subject to the `MINT_RECEIVER_POLICY` - /// policy per recipient and to the `MINT` pause vector. - /// Reverts with `LengthMismatch(recipients.length, - /// amounts.length)` if the parallel arrays disagree, and - /// with `EmptyBatch()` if either array is empty. - /// All-or-nothing: if any element reverts (e.g. - /// `SupplyCapExceeded` after a partial accumulation, or - /// `PolicyForbids(MINT_RECEIVER_POLICY, ...)` for a - /// policy-blocked recipient), the entire transaction - /// reverts and no partial state is committed. Emits - /// `Transfer(address(0), recipients[i], amounts[i])` per - /// element. Standard usage is to invoke this through + /// @dev Reverts in the following canonical order (first violated + /// check wins): + /// 1. `ContractPaused(MINT)` if `MINT` is paused. + /// 2. `AccessControlUnauthorizedAccount(msg.sender, MINT_ROLE)` + /// if the caller does not hold `MINT_ROLE`. + /// 3. `LengthMismatch(recipients.length, amounts.length)` if + /// the parallel arrays disagree. + /// 4. `EmptyBatch()` if either array is empty. + /// 5..N. Per-element checks inside the loop: `InvalidReceiver`, + /// `PolicyForbids(MINT_RECEIVER_POLICY, ...)`, + /// `SupplyCapExceeded` — see `mint`'s natspec for the + /// per-element precedence. + /// The pause and role gates are evaluated ONCE for the + /// whole batch (entrypoint modifiers); per-element gates + /// fire per recipient inside the loop. + /// All-or-nothing: if any element reverts, the entire + /// transaction reverts and no partial state is committed. + /// Emits `Transfer(address(0), recipients[i], amounts[i])` + /// per element. Standard usage is to invoke this through /// `announce(...)`'s `internalCalls`, which brackets the /// issuance with a matching disclosure atomically (see /// the contract-level "Announcement pairing" notes). @@ -410,26 +417,32 @@ interface IB20Security is IB20 { /// debits in one transaction without first arranging for /// each account to be policy-blocked. /// - /// @dev Requires `BURN_FROM_ROLE`. NOT gated by any policy: - /// the corporate-actions desk is trusted to pick the right - /// set of accounts off-chain, and the role grant is the - /// on-chain authorization. Subject to the `BURN` pause - /// vector. Reverts with `LengthMismatch(accounts.length, - /// amounts.length)` if the parallel arrays disagree, and - /// with `EmptyBatch()` if either array is empty. - /// All-or-nothing: if any element reverts (e.g. - /// `InsufficientBalance(accounts[k], balance, amounts[k])`), - /// the entire transaction reverts and no partial state is - /// committed. Emits `Transfer(accounts[i], address(0), - /// amounts[i])` per element; does NOT emit `BurnedBlocked` - /// (that event is reserved for `burnBlocked`'s sanctions - /// semantics). Standard usage is to invoke this through - /// `announce(...)`'s `internalCalls`, which brackets the - /// clawback with a matching disclosure atomically (see the - /// contract-level "Announcement pairing" notes). Direct - /// invocation by a role holder remains permitted for - /// emergency override but produces no `Announcement` / - /// `EndAnnouncement` bracket. + /// @dev `BURN_FROM_ROLE` is enforced WITHOUT the factory-bootstrap + /// bypass — clawback against existing balances has no + /// init-time use case, so the role check is unconditional. + /// NOT gated by any policy: the corporate-actions desk is + /// trusted to pick the right set of accounts off-chain, and + /// the role grant is the on-chain authorization. Reverts in + /// the following canonical order (first violated check wins): + /// 1. `ContractPaused(BURN)` if `BURN` is paused. + /// 2. `AccessControlUnauthorizedAccount(msg.sender, BURN_FROM_ROLE)` + /// if the caller does not hold `BURN_FROM_ROLE`. + /// 3. `LengthMismatch(accounts.length, amounts.length)` if + /// the parallel arrays disagree. + /// 4. `EmptyBatch()` if either array is empty. + /// 5. Per-element `InsufficientBalance(accounts[k], balance, + /// amounts[k])` from `_burnRaw`. + /// All-or-nothing: if any element reverts, the entire + /// transaction reverts and no partial state is committed. + /// Emits `Transfer(accounts[i], address(0), amounts[i])` + /// per element; does NOT emit `BurnedBlocked` (that event + /// is reserved for `burnBlocked`'s sanctions semantics). + /// Standard usage is to invoke this through `announce(...)`'s + /// `internalCalls`, which brackets the clawback with a + /// matching disclosure atomically (see the contract-level + /// "Announcement pairing" notes). Direct invocation by a + /// role holder remains permitted for emergency override but + /// produces no `Announcement` / `EndAnnouncement` bracket. /// /// @param accounts Accounts whose balances will be debited. /// @param amounts Per-account amounts, parallel to `accounts`. @@ -442,15 +455,24 @@ interface IB20Security is IB20 { /// @notice Burns `amount` tokens from the caller, recording intent /// to settle off-chain. /// - /// @dev Subject to the `REDEEM_SENDER_POLICY` policy and to the - /// `REDEEM` pause vector. Reverts with - /// `BelowMinimumRedeemable(shares, minimumRedeemable)` if - /// the corresponding share amount (`amount * - /// sharesToTokensRatio / WAD_PRECISION`) is zero OR is - /// strictly less than `minimumRedeemable`. Zero-share - /// redemptions are always rejected, regardless of - /// `minimumRedeemable`'s configured value, so a holder - /// cannot burn token dust that resolves to no shares. + /// @dev Reverts in the following canonical order (first violated + /// check wins): + /// 1. `ContractPaused(REDEEM)` if `REDEEM` is paused. + /// Enforced WITHOUT the factory-bootstrap bypass — redeem + /// is a holder-initiated path with no legitimate init-time + /// use case. + /// 2. `PolicyForbids(REDEEM_SENDER_POLICY, policyId)` if + /// `msg.sender` is not authorized under the active + /// `REDEEM_SENDER_POLICY` policy. + /// 3. `BelowMinimumRedeemable(shares, minimumRedeemable)` if + /// the corresponding share amount (`amount * + /// sharesToTokensRatio / WAD_PRECISION`) is zero OR is + /// strictly less than `minimumRedeemable`. Zero-share + /// redemptions are always rejected, regardless of + /// `minimumRedeemable`'s configured value, so a holder + /// cannot burn token dust that resolves to no shares. + /// 4. `InsufficientBalance(caller, balance, amount)` if the + /// caller does not have enough balance. /// Emits `Transfer(caller, address(0), amount)` followed by /// `Redeemed(caller, amount, sharesToTokensRatio)`. /// diff --git a/test/lib/mocks/MockB20.sol b/test/lib/mocks/MockB20.sol index 8a149de4..5bf22529 100644 --- a/test/lib/mocks/MockB20.sol +++ b/test/lib/mocks/MockB20.sol @@ -95,6 +95,22 @@ contract MockB20 is IB20 { // MODIFIERS // ============================================================ + /// @dev Reverts `ContractPaused(feature)` if `feature` is paused, + /// unless the call is from the factory in the bootstrap window + /// (`_isPrivileged()`). This is the canonical pause gate and + /// is always listed FIRST in a function's modifier set so + /// pause-vs-anything precedence reads off the modifier list + /// directly (modifiers run left-to-right, body at `_`). Token + /// invariants (balance, supply cap) are never bypassed and are + /// enforced regardless. Variants that reject the bootstrap + /// bypass (currently `MockB20Security`'s `redeem` path) declare + /// their own `whenNotPausedStrict` modifier rather than + /// overriding this one. + modifier whenNotPaused(PausableFeature feature) { + if (!_isPrivileged() && _isPaused(feature)) revert ContractPaused(feature); + _; + } + /// @dev Gates a function on `msg.sender` holding `role`. Reverts /// `AccessControlUnauthorizedAccount` if not. The factory /// bootstrap window (`_isPrivileged()`) bypasses the check; @@ -115,6 +131,45 @@ contract MockB20 is IB20 { _; } + /// @dev Input-validation modifier: reverts `InvalidReceiver(to)` + /// if `to == address(0)`. ALWAYS enforced; never bypassed by + /// the factory bootstrap window. Occupies the third slot in + /// the canonical `pause → role → input` modifier order. + modifier validReceiver(address to) { + if (to == address(0)) revert InvalidReceiver(to); + _; + } + + /// @dev Input-validation modifier: reverts `InvalidSender(from)` + /// if `from == address(0)`. ALWAYS enforced; never bypassed. + modifier validSender(address from) { + if (from == address(0)) revert InvalidSender(from); + _; + } + + /// @dev Input-validation modifier: reverts `InvalidApprover(owner)` + /// if `owner == address(0)`. ALWAYS enforced; never bypassed. + modifier validApprover(address owner) { + if (owner == address(0)) revert InvalidApprover(owner); + _; + } + + /// @dev Input-validation modifier: reverts `InvalidSpender(spender)` + /// if `spender == address(0)`. ALWAYS enforced; never bypassed. + modifier validSpender(address spender) { + if (spender == address(0)) revert InvalidSpender(spender); + _; + } + + /// @dev Input-validation modifier: reverts `EmptyFeatureSet()` if + /// `features.length == 0`. ALWAYS enforced; never bypassed. + modifier nonEmptyFeatures(PausableFeature[] calldata features) { + if (features.length == 0) revert EmptyFeatureSet(); + _; + } + + // ============================================================ + // ERC-20: VIEWS // ============================================================ // ERC-20: VIEWS // ============================================================ @@ -148,12 +203,24 @@ contract MockB20 is IB20 { // ERC-20: MUTATIONS // ============================================================ - function transfer(address to, uint256 amount) external returns (bool) { + function transfer(address to, uint256 amount) + external + whenNotPaused(PausableFeature.TRANSFER) + validReceiver(to) + validSender(msg.sender) + returns (bool) + { _transfer(msg.sender, to, amount); return true; } - function transferFrom(address from, address to, uint256 amount) external returns (bool) { + function transferFrom(address from, address to, uint256 amount) + external + whenNotPaused(PausableFeature.TRANSFER) + validReceiver(to) + validSender(from) + returns (bool) + { if (!_isPrivileged()) { // Allowance is consumed unconditionally outside the factory // bootstrap window. Matches OZ ERC20 and the Rust precompile, @@ -174,9 +241,12 @@ contract MockB20 is IB20 { return true; } - function approve(address spender, uint256 amount) external returns (bool) { - if (msg.sender == address(0)) revert InvalidApprover(msg.sender); - if (spender == address(0)) revert InvalidSpender(spender); + function approve(address spender, uint256 amount) + external + validApprover(msg.sender) + validSpender(spender) + returns (bool) + { MockB20Storage.layout().allowances[msg.sender][spender] = amount; emit Approval(msg.sender, spender, amount); return true; @@ -186,13 +256,25 @@ contract MockB20 is IB20 { // MEMO TRANSFER VARIANTS // ============================================================ - function transferWithMemo(address to, uint256 amount, bytes32 memo) external returns (bool) { + function transferWithMemo(address to, uint256 amount, bytes32 memo) + external + whenNotPaused(PausableFeature.TRANSFER) + validReceiver(to) + validSender(msg.sender) + returns (bool) + { _transfer(msg.sender, to, amount); emit Memo(msg.sender, memo); return true; } - function transferFromWithMemo(address from, address to, uint256 amount, bytes32 memo) external returns (bool) { + function transferFromWithMemo(address from, address to, uint256 amount, bytes32 memo) + external + whenNotPaused(PausableFeature.TRANSFER) + validReceiver(to) + validSender(from) + returns (bool) + { if (!_isPrivileged()) { _consumeAllowance(from, msg.sender, amount); if (msg.sender != from) { @@ -226,27 +308,44 @@ contract MockB20 is IB20 { // MINT / BURN // ============================================================ - function mint(address to, uint256 amount) external { + function mint(address to, uint256 amount) + external + whenNotPaused(PausableFeature.MINT) + onlyRole(MINT_ROLE) + validReceiver(to) + { _mint(to, amount); } - function mintWithMemo(address to, uint256 amount, bytes32 memo) external { + function mintWithMemo(address to, uint256 amount, bytes32 memo) + external + whenNotPaused(PausableFeature.MINT) + onlyRole(MINT_ROLE) + validReceiver(to) + { _mint(to, amount); emit Memo(msg.sender, memo); } - function burn(uint256 amount) external { - _burnSelf(msg.sender, amount); + function burn(uint256 amount) external whenNotPaused(PausableFeature.BURN) onlyRole(BURN_ROLE) { + _burnRaw(msg.sender, amount); } - function burnWithMemo(uint256 amount, bytes32 memo) external { - _burnSelf(msg.sender, amount); + function burnWithMemo(uint256 amount, bytes32 memo) + external + whenNotPaused(PausableFeature.BURN) + onlyRole(BURN_ROLE) + { + _burnRaw(msg.sender, amount); emit Memo(msg.sender, memo); } - function burnBlocked(address from, uint256 amount) external onlyRole(BURN_BLOCKED_ROLE) { + function burnBlocked(address from, uint256 amount) + external + whenNotPaused(PausableFeature.BURN) + onlyRole(BURN_BLOCKED_ROLE) + { if (!_isPrivileged()) { - if (_isPaused(PausableFeature.BURN)) revert ContractPaused(PausableFeature.BURN); // The point of burnBlocked is to seize from policy-blocked // accounts. Read the transfer-sender policy ID out of the // transfer-side packed slot and reject if the target is @@ -359,8 +458,7 @@ contract MockB20 is IB20 { return _isPaused(feature); } - function pause(PausableFeature[] calldata features) external onlyRole(PAUSE_ROLE) { - if (features.length == 0) revert EmptyFeatureSet(); + function pause(PausableFeature[] calldata features) external onlyRole(PAUSE_ROLE) nonEmptyFeatures(features) { MockB20Storage.Layout storage $ = MockB20Storage.layout(); for (uint256 i = 0; i < features.length; i++) { $.pausedVectors |= uint256(1) << uint8(features[i]); @@ -368,8 +466,7 @@ contract MockB20 is IB20 { emit Paused(msg.sender, features); } - function unpause(PausableFeature[] calldata features) external onlyRole(UNPAUSE_ROLE) { - if (features.length == 0) revert EmptyFeatureSet(); + function unpause(PausableFeature[] calldata features) external onlyRole(UNPAUSE_ROLE) nonEmptyFeatures(features) { MockB20Storage.Layout storage $ = MockB20Storage.layout(); for (uint256 i = 0; i < features.length; i++) { $.pausedVectors &= ~(uint256(1) << uint8(features[i])); @@ -621,12 +718,19 @@ contract MockB20 is IB20 { } } + /// @dev Pure mechanics: policy (with bootstrap bypass) + balance + + /// effects. Pause, role, and input validation are enforced by + /// the entrypoint modifiers (`whenNotPaused`, `validReceiver`, + /// `validSender`); every external caller of this helper — + /// `transfer`, `transferFrom`, `transferWithMemo`, + /// `transferFromWithMemo` — declares those modifiers, so the + /// gates run before this function is entered. `transferFrom` / + /// `transferFromWithMemo` additionally consume allowance and + /// check the executor policy in their bodies before calling + /// here; both of those checks ALSO honor the bootstrap bypass, + /// consistent with the policy bypass below. function _transfer(address from, address to, uint256 amount) internal { - if (to == address(0)) revert InvalidReceiver(to); - if (from == address(0)) revert InvalidSender(from); - if (!_isPrivileged()) { - if (_isPaused(PausableFeature.TRANSFER)) revert ContractPaused(PausableFeature.TRANSFER); // One SLOAD pulls both policy IDs we need for the transfer // check (and was already warmed if we came in via transferFrom, // which reads the executor lane of the same slot first). @@ -651,11 +755,16 @@ contract MockB20 is IB20 { emit Transfer(from, to, amount); } - function _mint(address to, uint256 amount) internal onlyRole(MINT_ROLE) { - if (to == address(0)) revert InvalidReceiver(to); - + /// @dev Pure mechanics: policy (with bootstrap bypass) + supply cap + /// + effects. Pause, role, and input validation are enforced + /// by the entrypoint modifiers (`whenNotPaused`, `onlyRole`, + /// `validReceiver`) on `mint` / `mintWithMemo`. The security + /// variant's `batchMint` declares the same entry-level + /// modifiers ONCE for the whole batch and validates per-element + /// receivers inline before invoking this helper, so `_mint` + /// itself never re-checks them. + function _mint(address to, uint256 amount) internal { if (!_isPrivileged()) { - if (_isPaused(PausableFeature.MINT)) revert ContractPaused(PausableFeature.MINT); uint64 mintReceiverPolicyId = MockB20Storage.layout().mintPolicyIds.receiver; if (!IPolicyRegistry(POLICY_REGISTRY).isAuthorized(mintReceiverPolicyId, to)) { revert PolicyForbids(MINT_RECEIVER_POLICY, mintReceiverPolicyId); @@ -672,13 +781,13 @@ contract MockB20 is IB20 { emit Transfer(address(0), to, amount); } - function _burnSelf(address from, uint256 amount) internal onlyRole(BURN_ROLE) { - if (!_isPrivileged()) { - if (_isPaused(PausableFeature.BURN)) revert ContractPaused(PausableFeature.BURN); - } - _burnRaw(from, amount); - } - + /// @dev Pure mechanics: balance + effects. Pause and role gates are + /// enforced by entrypoint modifiers (`whenNotPaused`, + /// `onlyRole`) on every caller (`burn`, `burnWithMemo`, + /// `burnBlocked`, and `MockB20Security`'s `batchBurn` / + /// `_redeemBurn`); see those functions for the per-caller + /// authorization surface. This helper never authorizes the + /// destruction on its own. function _burnRaw(address from, uint256 amount) internal { MockB20Storage.Layout storage $ = MockB20Storage.layout(); uint256 fromBalance = $.balances[from]; diff --git a/test/lib/mocks/MockB20Security.sol b/test/lib/mocks/MockB20Security.sol index fe5a72ed..6368d7eb 100644 --- a/test/lib/mocks/MockB20Security.sol +++ b/test/lib/mocks/MockB20Security.sol @@ -115,6 +115,19 @@ contract MockB20Security is MockB20, IB20Security { _; } + /// @dev Like the base `whenNotPaused` but without the factory + /// bootstrap bypass: the pause check is unconditional, even + /// for the factory in the init window. Reserved for variant + /// paths that deliberately reject the bypass — currently just + /// the `redeem` / `redeemWithMemo` family, which is + /// holder-initiated and has no legitimate init-time use case + /// (see this contract's natspec). Lives here, not on the + /// base, because no base function needs the stricter gate. + modifier whenNotPausedStrict(PausableFeature feature) { + if (_isPaused(feature)) revert ContractPaused(feature); + _; + } + // ============================================================ // ANNOUNCEMENTS // ============================================================ @@ -173,31 +186,51 @@ contract MockB20Security is MockB20, IB20Security { // BATCHED ISSUANCE / CLAWBACK // ============================================================ - function batchMint(address[] calldata recipients, uint256[] calldata amounts) external { + /// @dev `whenNotPaused(MINT)` + `onlyRole(MINT_ROLE)` modifiers run + /// ONCE for the entire batch — the underlying `_mint` helper + /// no longer carries its own pause/role gates (those were + /// hoisted to per-entrypoint modifiers as part of the + /// `pause → role → input → ...` canonical ordering). Length / + /// empty-batch checks fire next inside the body. The + /// per-element `validReceiver` guard is inlined in the loop + /// because modifier parameters evaluate once at function + /// entry, not per-iteration; this preserves the per-recipient + /// `InvalidReceiver` revert that callers depend on. + function batchMint(address[] calldata recipients, uint256[] calldata amounts) + external + whenNotPaused(PausableFeature.MINT) + onlyRole(MINT_ROLE) + { if (recipients.length != amounts.length) revert LengthMismatch(recipients.length, amounts.length); if (recipients.length == 0) revert EmptyBatch(); - // Per-element call into _mint: role / pause checks repeat (idempotent), - // but MINT_RECEIVER_POLICY policy and supply-cap accumulation are correctly - // applied per recipient. Cleaner than re-deriving the per-element body. for (uint256 i = 0; i < recipients.length; i++) { + // Per-element zero-receiver guard: `_mint` no longer checks + // input (modifier-only on the single-recipient `mint` / + // `mintWithMemo` entrypoints). + if (recipients[i] == address(0)) revert InvalidReceiver(recipients[i]); _mint(recipients[i], amounts[i]); } } - /// @dev `onlyRoleStrict` (not `onlyRole`): the factory bootstrap + /// @dev Modifier order: `pause → role` (canonical for the variant). + /// `onlyRoleStrict` (not `onlyRole`): the factory bootstrap /// bypass is deliberately NOT honored here, per the contract /// natspec — clawback against existing balances has no init-time /// use case, so granting the factory a bypass would only widen - /// the attack surface. + /// the attack surface. `whenNotPaused` retains the bypass; the + /// practical effect is identical because the factory never + /// reaches the body anyway (role-strict blocks it first), and + /// mirroring the rest of the contract on the pause vector + /// keeps the modifier set uniform. function batchBurn(address[] calldata accounts, uint256[] calldata amounts) external + whenNotPaused(PausableFeature.BURN) onlyRoleStrict(BURN_FROM_ROLE) { if (accounts.length != amounts.length) { revert LengthMismatch(accounts.length, amounts.length); } if (accounts.length == 0) revert EmptyBatch(); - if (_isPaused(PausableFeature.BURN)) revert ContractPaused(PausableFeature.BURN); for (uint256 i = 0; i < accounts.length; i++) { // Zero amounts are allowed per ERC-20 conventions (`transfer(0)` is valid); // `_burnRaw` is a no-op for amount == 0 and emits `Transfer(account, 0, 0)`. @@ -210,12 +243,12 @@ contract MockB20Security is MockB20, IB20Security { // REDEMPTION // ============================================================ - function redeem(uint256 amount) external { + function redeem(uint256 amount) external whenNotPausedStrict(PausableFeature.REDEEM) { uint256 ratio = _redeemBurn(amount); emit Redeemed(msg.sender, amount, ratio); } - function redeemWithMemo(uint256 amount, bytes32 memo) external { + function redeemWithMemo(uint256 amount, bytes32 memo) external whenNotPausedStrict(PausableFeature.REDEEM) { uint256 ratio = _redeemBurn(amount); // Order matters: Transfer (in _redeemBurn), then Memo, then Redeemed. emit Memo(msg.sender, memo); diff --git a/test/unit/B20/erc20/transferFrom_revertOrder.t.sol b/test/unit/B20/erc20/transferFrom_revertOrder.t.sol index d573d003..b9c937e4 100644 --- a/test/unit/B20/erc20/transferFrom_revertOrder.t.sol +++ b/test/unit/B20/erc20/transferFrom_revertOrder.t.sol @@ -9,18 +9,139 @@ import {PolicyRegistryConstants} from "test/lib/mocks/MockPolicyRegistry.sol"; /// @title Differential check-order tests for `transferFrom`. /// -/// @notice `transferFrom` adds two preconditions on top of `_transfer`'s checks -/// (already pinned by `transfer_revertOrder.t.sol`): +/// @notice `transferFrom` layers two body-level preconditions +/// (ALLOWANCE and EXECUTOR-POLICY) on top of `_transfer`'s policy / +/// balance checks. The cross-cutting gates (PAUSE, ZERO-RECEIVER, +/// ZERO-SENDER) live on the entrypoint as modifiers and therefore +/// fire BEFORE the body's allowance / executor-policy work runs. /// -/// **Canonical order (Solidity reference, when `msg.sender != from`):** -/// 1. ALLOWANCE (`_consumeAllowance`) → `InsufficientAllowance` -/// 2. EXECUTOR-POLICY (`isAuthorized(executorPolicyId, msg.sender)`) → `PolicyForbids(EXECUTOR, ...)` -/// 3..N. (all `_transfer` body checks — see `transfer_revertOrder.t.sol`) +/// **Canonical order (Solidity reference — modifier-led, when +/// `msg.sender != from`):** +/// 1. PAUSE (`whenNotPaused(TRANSFER)` modifier) → `ContractPaused` +/// 2. ZERO-RECEIVER (`validReceiver(to)` modifier) → `InvalidReceiver` +/// 3. ZERO-SENDER (`validSender(from)` modifier) → `InvalidSender` +/// 4. ALLOWANCE (body, `_consumeAllowance`) → `InsufficientAllowance` +/// 5. EXECUTOR-POLICY (body, `isAuthorized(executorPolicyId, msg.sender)`) +/// → `PolicyForbids(EXECUTOR, ...)` +/// 6..N. All `_transfer` body checks — see `transfer_revertOrder.t.sol` +/// (SENDER-POLICY → RECEIVER-POLICY → BALANCE). /// -/// These tests pin the two new preconditions against each other and -/// against a representative `_transfer` body check (ZERO-RECEIVER) to -/// prove they fire before `_transfer` is entered. +/// The full pair matrix between body-level ALLOWANCE/EXECUTOR-POLICY +/// and the modifier-led PAUSE/ZERO-RECEIVER/ZERO-SENDER is pinned +/// below to lock in the modifier-first precedence; one test against +/// a representative `_transfer` body check (SENDER-POLICY) proves +/// ALLOWANCE and EXECUTOR-POLICY both fire before `_transfer` is +/// entered. contract B20TransferFromRevertOrderTest is B20Test { + // --- Pairs where PAUSE wins (PAUSE is canonical first) --- + + /// @notice PAUSE beats ALLOWANCE. + function test_transferFrom_revertOrder_pause_beats_allowance( + address caller, + address from, + address to, + uint256 amount + ) public { + _assumeValidCaller(caller); + _assumeValidActor(from); + _assumeValidActor(to); + vm.assume(caller != from); + amount = bound(amount, 1, type(uint128).max); + _pause(IB20.PausableFeature.TRANSFER); + // No allowance set → ALLOWANCE would fire if PAUSE didn't. + + vm.prank(caller); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.TRANSFER)); + token.transferFrom(from, to, amount); + } + + /// @notice PAUSE beats EXECUTOR-POLICY. + function test_transferFrom_revertOrder_pause_beats_executorPolicy( + address caller, + address from, + address to, + uint256 amount + ) public { + _assumeValidCaller(caller); + _assumeValidActor(from); + _assumeValidActor(to); + vm.assume(caller != from); + amount = bound(amount, 1, type(uint128).max); + _pause(IB20.PausableFeature.TRANSFER); + _setPolicy(B20Constants.TRANSFER_EXECUTOR_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); + + vm.prank(caller); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.TRANSFER)); + token.transferFrom(from, to, amount); + } + + // --- Pairs where ZERO-RECEIVER wins (PAUSE not violated) --- + + /// @notice ZERO-RECEIVER beats ALLOWANCE. + /// @dev `validReceiver` modifier fires before the body's allowance check. + function test_transferFrom_revertOrder_zeroReceiver_beats_allowance(address caller, address from, uint256 amount) + public + { + _assumeValidCaller(caller); + _assumeValidActor(from); + vm.assume(caller != from); + amount = bound(amount, 1, type(uint128).max); + // No allowance AND `to == address(0)`. + + vm.prank(caller); + vm.expectRevert(abi.encodeWithSelector(IB20.InvalidReceiver.selector, address(0))); + token.transferFrom(from, address(0), amount); + } + + /// @notice ZERO-RECEIVER beats EXECUTOR-POLICY. + function test_transferFrom_revertOrder_zeroReceiver_beats_executorPolicy( + address caller, + address from, + uint256 amount + ) public { + _assumeValidCaller(caller); + _assumeValidActor(from); + vm.assume(caller != from); + amount = bound(amount, 1, type(uint128).max); + _setPolicy(B20Constants.TRANSFER_EXECUTOR_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); + + vm.prank(caller); + vm.expectRevert(abi.encodeWithSelector(IB20.InvalidReceiver.selector, address(0))); + token.transferFrom(from, address(0), amount); + } + + // --- Pairs where ZERO-SENDER wins (PAUSE + ZERO-RECEIVER not violated) --- + + /// @notice ZERO-SENDER beats ALLOWANCE. + /// @dev `validSender` modifier fires before the body's allowance check. + function test_transferFrom_revertOrder_zeroSender_beats_allowance(address caller, address to, uint256 amount) + public + { + _assumeValidCaller(caller); + _assumeValidActor(to); + amount = bound(amount, 1, type(uint128).max); + + vm.prank(caller); + vm.expectRevert(abi.encodeWithSelector(IB20.InvalidSender.selector, address(0))); + token.transferFrom(address(0), to, amount); + } + + /// @notice ZERO-SENDER beats EXECUTOR-POLICY. + function test_transferFrom_revertOrder_zeroSender_beats_executorPolicy(address caller, address to, uint256 amount) + public + { + _assumeValidCaller(caller); + _assumeValidActor(to); + amount = bound(amount, 1, type(uint128).max); + _setPolicy(B20Constants.TRANSFER_EXECUTOR_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); + + vm.prank(caller); + vm.expectRevert(abi.encodeWithSelector(IB20.InvalidSender.selector, address(0))); + token.transferFrom(address(0), to, amount); + } + + // --- Pairs where ALLOWANCE wins (PAUSE + input not violated) --- + /// @notice ALLOWANCE beats EXECUTOR-POLICY. function test_transferFrom_revertOrder_allowance_beats_executorPolicy( address caller, @@ -41,37 +162,51 @@ contract B20TransferFromRevertOrderTest is B20Test { token.transferFrom(from, to, amount); } - /// @notice ALLOWANCE beats anything in `_transfer` (representative: ZERO-RECEIVER). - function test_transferFrom_revertOrder_allowance_beats_transferBody(address caller, address from, uint256 amount) - public - { + /// @notice ALLOWANCE beats anything in `_transfer` (representative: SENDER-POLICY). + /// @dev `_transfer`'s body now contains policy + balance + effects only (input + /// validation hoisted to entrypoint modifier); SENDER-POLICY is the + /// first body check inside `_transfer` post-refactor. + function test_transferFrom_revertOrder_allowance_beats_transferBody( + address caller, + address from, + address to, + uint256 amount + ) public { _assumeValidCaller(caller); _assumeValidActor(from); + _assumeValidActor(to); vm.assume(caller != from); amount = bound(amount, 1, type(uint128).max); - // Allowance is 0 AND `to` is address(0). + // Allowance is 0 AND sender policy blocks `from`. + _setPolicy(B20Constants.TRANSFER_SENDER_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); vm.prank(caller); vm.expectRevert(abi.encodeWithSelector(IB20.InsufficientAllowance.selector, caller, 0, amount)); - token.transferFrom(from, address(0), amount); + token.transferFrom(from, to, amount); } - /// @notice EXECUTOR-POLICY beats anything in `_transfer` (representative: ZERO-RECEIVER). - /// @dev Allowance is set high enough to pass the allowance check, so the executor-policy - /// check is reached next, and it fires before `_transfer` is entered. + // --- Pair where EXECUTOR-POLICY wins (everything earlier satisfied) --- + + /// @notice EXECUTOR-POLICY beats anything in `_transfer` (representative: SENDER-POLICY). + /// @dev Allowance is set high enough to pass the allowance check, so the + /// executor-policy check runs next and fires before `_transfer` is + /// entered. function test_transferFrom_revertOrder_executorPolicy_beats_transferBody( address caller, address from, + address to, uint256 amount ) public { _assumeValidCaller(caller); _assumeValidActor(from); + _assumeValidActor(to); vm.assume(caller != from); amount = bound(amount, 1, type(uint128).max); - // Sufficient allowance, executor policy blocks caller, `to` is address(0). + // Sufficient allowance, executor policy blocks caller, sender policy also blocks `from`. vm.prank(from); token.approve(caller, amount); _setPolicy(B20Constants.TRANSFER_EXECUTOR_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); + _setPolicy(B20Constants.TRANSFER_SENDER_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); vm.prank(caller); vm.expectRevert( @@ -81,6 +216,6 @@ contract B20TransferFromRevertOrderTest is B20Test { PolicyRegistryConstants.ALWAYS_BLOCK_ID ) ); - token.transferFrom(from, address(0), amount); + token.transferFrom(from, to, amount); } } diff --git a/test/unit/B20/erc20/transfer_revertOrder.t.sol b/test/unit/B20/erc20/transfer_revertOrder.t.sol index f1a7313f..f1d56840 100644 --- a/test/unit/B20/erc20/transfer_revertOrder.t.sol +++ b/test/unit/B20/erc20/transfer_revertOrder.t.sol @@ -9,31 +9,42 @@ import {PolicyRegistryConstants} from "test/lib/mocks/MockPolicyRegistry.sol"; /// @title Differential check-order tests for `transfer`. /// -/// @notice **Canonical order (Solidity reference `_transfer`):** -/// 1. ZERO-RECEIVER (`to == address(0)`) → `InvalidReceiver` -/// 2. ZERO-SENDER (`from == address(0)`) → `InvalidSender` -/// 3. PAUSE (`_isPaused(TRANSFER)`) → `ContractPaused` -/// 4. SENDER-POLICY (`isAuthorized(senderPolicyId, from)`) → `PolicyForbids(SENDER, ...)` -/// 5. RECEIVER-POLICY (`isAuthorized(receiverPolicyId, to)`) → `PolicyForbids(RECEIVER, ...)` -/// 6. BALANCE (`fromBalance < amount`) → `InsufficientBalance` +/// @notice **Canonical order (Solidity reference — modifier-led):** +/// 1. PAUSE (`whenNotPaused(TRANSFER)` modifier) → `ContractPaused` +/// 2. ZERO-RECEIVER (`validReceiver(to)` modifier) → `InvalidReceiver` +/// 3. ZERO-SENDER (`validSender(from)` modifier) → `InvalidSender` +/// 4. SENDER-POLICY (body, in `_transfer`) → `PolicyForbids(SENDER, ...)` +/// 5. RECEIVER-POLICY (body, in `_transfer`) → `PolicyForbids(RECEIVER, ...)` +/// 6. BALANCE (body, in `_transfer`) → `InsufficientBalance` /// /// The public `transfer(to, amount)` entry sets `from = msg.sender`, so /// pairs involving ZERO-SENDER require pranking `address(0)`. C(6, 2) = 15 pairs. contract B20TransferRevertOrderTest is B20Test { - // --- Pairs where ZERO-RECEIVER wins --- + // --- Pairs where PAUSE wins (PAUSE is canonical first) --- - function test_transfer_revertOrder_zeroReceiver_beats_zeroSender(uint256 amount) public { - // Both `to` and `from` are address(0) — receiver check fires first. - vm.prank(address(0)); - vm.expectRevert(abi.encodeWithSelector(IB20.InvalidReceiver.selector, address(0))); + function test_transfer_revertOrder_pause_beats_zeroReceiver(address from, uint256 amount) public { + _assumeValidActor(from); + _pause(IB20.PausableFeature.TRANSFER); + + vm.prank(from); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.TRANSFER)); token.transfer(address(0), amount); } - function test_transfer_revertOrder_zeroReceiver_beats_pause(address from, uint256 amount) public { - _assumeValidActor(from); + function test_transfer_revertOrder_pause_beats_zeroSender(address to, uint256 amount) public { + _assumeValidActor(to); _pause(IB20.PausableFeature.TRANSFER); - vm.prank(from); + vm.prank(address(0)); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.TRANSFER)); + token.transfer(to, amount); + } + + // --- Pairs where ZERO-RECEIVER wins (PAUSE not violated) --- + + function test_transfer_revertOrder_zeroReceiver_beats_zeroSender(uint256 amount) public { + // Both `to` and `from` are address(0) — receiver check fires first. + vm.prank(address(0)); vm.expectRevert(abi.encodeWithSelector(IB20.InvalidReceiver.selector, address(0))); token.transfer(address(0), amount); } @@ -66,16 +77,7 @@ contract B20TransferRevertOrderTest is B20Test { token.transfer(address(0), amount); } - // --- Pairs where ZERO-SENDER wins (requires pranking address(0)) --- - - function test_transfer_revertOrder_zeroSender_beats_pause(address to, uint256 amount) public { - _assumeValidActor(to); - _pause(IB20.PausableFeature.TRANSFER); - - vm.prank(address(0)); - vm.expectRevert(abi.encodeWithSelector(IB20.InvalidSender.selector, address(0))); - token.transfer(to, amount); - } + // --- Pairs where ZERO-SENDER wins (PAUSE not violated; requires pranking address(0)) --- function test_transfer_revertOrder_zeroSender_beats_senderPolicy(address to, uint256 amount) public { _assumeValidActor(to); diff --git a/test/unit/B20/supply/burnBlocked_revertOrder.t.sol b/test/unit/B20/supply/burnBlocked_revertOrder.t.sol index c1895e4f..ffd51496 100644 --- a/test/unit/B20/supply/burnBlocked_revertOrder.t.sol +++ b/test/unit/B20/supply/burnBlocked_revertOrder.t.sol @@ -9,11 +9,11 @@ import {PolicyRegistryConstants} from "test/lib/mocks/MockPolicyRegistry.sol"; /// @title Differential check-order tests for `burnBlocked`. /// -/// @notice **Canonical order (Solidity reference):** -/// 1. ROLE (`onlyRole(BURN_BLOCKED_ROLE)` modifier) → `AccessControlUnauthorizedAccount` -/// 2. PAUSE (`_isPaused(BURN)`) → `ContractPaused` -/// 3. BLOCKED (`isAuthorized(senderPolicyId, from) == true` reverts) → `AccountNotBlocked` -/// 4. BALANCE (`fromBalance < amount` in `_burnRaw`) → `InsufficientBalance` +/// @notice **Canonical order (Solidity reference — modifier-led):** +/// 1. PAUSE (`whenNotPaused(BURN)` modifier) → `ContractPaused` +/// 2. ROLE (`onlyRole(BURN_BLOCKED_ROLE)` modifier) → `AccessControlUnauthorizedAccount` +/// 3. BLOCKED (body, `isAuthorized(senderPolicyId, from) == true` reverts) → `AccountNotBlocked` +/// 4. BALANCE (body, `fromBalance < amount` in `_burnRaw`) → `InsufficientBalance` /// /// Note on BLOCKED semantics: `burnBlocked` is a clawback function — it /// only succeeds when `from` is currently NOT authorized by the @@ -21,25 +21,24 @@ import {PolicyRegistryConstants} from "test/lib/mocks/MockPolicyRegistry.sol"; /// violation by setting the policy to `ALWAYS_ALLOW` so every account is /// authorized, which makes the function revert `AccountNotBlocked`. contract B20BurnBlockedRevertOrderTest is B20Test { - // --- Pairs where ROLE wins --- + // --- Pair where PAUSE wins (PAUSE is canonical first) --- - /// @notice ROLE beats PAUSE. - function test_burnBlocked_revertOrder_role_beats_pause(address caller, address from, uint256 amount) public { + /// @notice PAUSE beats ROLE. + /// @dev `whenNotPaused` modifier is listed first; fires before `onlyRole`. + function test_burnBlocked_revertOrder_pause_beats_role(address caller, address from, uint256 amount) public { _assumeValidCaller(caller); _assumeValidActor(from); vm.assume(caller != admin); _pause(IB20.PausableFeature.BURN); - // No BURN_BLOCKED_ROLE granted. + // No BURN_BLOCKED_ROLE granted AND BURN is paused — pause fires first. vm.prank(caller); - vm.expectRevert( - abi.encodeWithSelector( - IB20.AccessControlUnauthorizedAccount.selector, caller, B20Constants.BURN_BLOCKED_ROLE - ) - ); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.BURN)); token.burnBlocked(from, amount); } + // --- Pairs where ROLE wins (PAUSE not violated) --- + /// @notice ROLE beats BLOCKED. function test_burnBlocked_revertOrder_role_beats_blocked(address caller, address from, uint256 amount) public { _assumeValidCaller(caller); diff --git a/test/unit/B20/supply/burn_revertOrder.t.sol b/test/unit/B20/supply/burn_revertOrder.t.sol index 79a8b764..8bf70e74 100644 --- a/test/unit/B20/supply/burn_revertOrder.t.sol +++ b/test/unit/B20/supply/burn_revertOrder.t.sol @@ -12,32 +12,32 @@ import {MockB20, B20Constants} from "test/lib/mocks/MockB20.sol"; /// canonical first-firing revert selector. See `mint_revertOrder.t.sol` /// for the harness rationale. /// -/// **Canonical order (Solidity reference, `_burnSelf` → `_burnRaw`):** -/// 1. ROLE (`onlyRole(BURN_ROLE)` modifier) → `AccessControlUnauthorizedAccount` -/// 2. PAUSE (`_isPaused(BURN)`) → `ContractPaused` -/// 3. BALANCE (`fromBalance < amount` in `_burnRaw`) → `InsufficientBalance` +/// **Canonical order (Solidity reference — modifier-led):** +/// 1. PAUSE (`whenNotPaused(BURN)` modifier) → `ContractPaused` +/// 2. ROLE (`onlyRole(BURN_ROLE)` modifier) → `AccessControlUnauthorizedAccount` +/// 3. BALANCE (body, `fromBalance < amount` in `_burnRaw`) → `InsufficientBalance` contract B20BurnRevertOrderTest is B20Test { - /// @notice ROLE beats PAUSE. - /// @dev Modifier runs before any body check, including the pause guard. - function test_burn_revertOrder_role_beats_pause(address caller, uint256 amount) public { + /// @notice PAUSE beats ROLE. + /// @dev The pause modifier is listed FIRST in the modifier set, so it fires + /// before the role modifier. With both violated, ContractPaused surfaces. + function test_burn_revertOrder_pause_beats_role(address caller, uint256 amount) public { _assumeValidCaller(caller); vm.assume(caller != admin); _pause(IB20.PausableFeature.BURN); + // Caller has no role AND BURN is paused — pause fires first. vm.prank(caller); - vm.expectRevert( - abi.encodeWithSelector(IB20.AccessControlUnauthorizedAccount.selector, caller, B20Constants.BURN_ROLE) - ); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.BURN)); token.burn(amount); } /// @notice ROLE beats BALANCE. - /// @dev Modifier runs before `_burnRaw` is reached; insufficient balance never gets checked. + /// @dev `onlyRole` modifier runs before `_burnRaw` is reached; insufficient balance never gets checked. function test_burn_revertOrder_role_beats_balance(address caller, uint256 amount) public { _assumeValidCaller(caller); vm.assume(caller != admin); amount = bound(amount, 1, type(uint128).max); - // Caller has zero balance and no role — role check fires first. + // Caller has zero balance and no role — role check fires first (pause not set). vm.prank(caller); vm.expectRevert( @@ -47,7 +47,7 @@ contract B20BurnRevertOrderTest is B20Test { } /// @notice PAUSE beats BALANCE. - /// @dev Pause guard in `_burnSelf` runs before `_burnRaw` is invoked. + /// @dev `whenNotPaused` modifier on the entrypoint fires before `_burnRaw` is invoked. function test_burn_revertOrder_pause_beats_balance(uint256 amount) public { amount = bound(amount, 1, type(uint128).max); _grantRole(B20Constants.BURN_ROLE, alice); diff --git a/test/unit/B20/supply/mint_revertOrder.t.sol b/test/unit/B20/supply/mint_revertOrder.t.sol index b42feccf..d6d0c1e8 100644 --- a/test/unit/B20/supply/mint_revertOrder.t.sol +++ b/test/unit/B20/supply/mint_revertOrder.t.sol @@ -16,51 +16,88 @@ import {MockPolicyRegistry, PolicyRegistryConstants} from "test/lib/mocks/MockPo /// between the two backends surfaces as a fork-mode-only failure with a /// clear "selector A vs selector B" diff. /// -/// **Canonical order (Option A — Solidity's defensive-depth order):** -/// 1. ROLE (`onlyRole(MINT_ROLE)` modifier) → `AccessControlUnauthorizedAccount` -/// 2. ZERO-RECEIVER (`to == address(0)`) → `InvalidReceiver` -/// 3. PAUSE (`_isPaused(MINT)`) → `ContractPaused` -/// 4. POLICY (`isAuthorized(mintReceiverPolicyId, to)`) → `PolicyForbids` -/// 5. SUPPLY-CAP (`totalSupply + amount > supplyCap`) → `SupplyCapExceeded` +/// **Canonical order (Solidity reference — modifier-led):** +/// 1. PAUSE (`whenNotPaused(MINT)` modifier) → `ContractPaused` +/// 2. ROLE (`onlyRole(MINT_ROLE)` modifier) → `AccessControlUnauthorizedAccount` +/// 3. ZERO-RECEIVER (`validReceiver(to)` modifier) → `InvalidReceiver` +/// 4. POLICY (body, in `_mint`) → `PolicyForbids` +/// 5. SUPPLY-CAP (body, in `_mint`) → `SupplyCapExceeded` /// /// A `mint` call that violates two or more preconditions must always /// revert with the selector for the earliest-listed violation. The 10 /// tests below enumerate every pair (C(5, 2) = 10). contract B20MintRevertOrderTest is B20Test { - // --- Pairs where ROLE wins (ROLE is canonical first) --- + // --- Pairs where PAUSE wins (PAUSE is canonical first) --- - /// @notice With both ROLE and ZERO-RECEIVER violated, ROLE fires first. - /// @dev Canonical: the `onlyRole` modifier runs before the `to == 0` body check. - function test_mint_revertOrder_role_beats_zeroRecipient(address caller, uint256 amount) public { + /// @notice With both PAUSE and ROLE violated, PAUSE fires first. + /// @dev `whenNotPaused` modifier is listed before `onlyRole`; runs first. + function test_mint_revertOrder_pause_beats_role(address caller, address to, uint256 amount) public { _assumeValidCaller(caller); - vm.assume(caller != admin); // admin holds DEFAULT_ADMIN_ROLE but not MINT_ROLE on fresh token - // No MINT_ROLE granted; recipient is address(0). + _assumeValidActor(to); + vm.assume(caller != admin); + _pause(IB20.PausableFeature.MINT); + // No MINT_ROLE granted AND MINT is paused — pause fires first. vm.prank(caller); - vm.expectRevert( - abi.encodeWithSelector(IB20.AccessControlUnauthorizedAccount.selector, caller, B20Constants.MINT_ROLE) - ); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.MINT)); + token.mint(to, amount); + } + + /// @notice With both PAUSE and ZERO-RECEIVER violated, PAUSE fires first. + /// @dev `whenNotPaused` modifier runs before `validReceiver` modifier. + function test_mint_revertOrder_pause_beats_zeroRecipient(uint256 amount) public { + _grantRole(B20Constants.MINT_ROLE, minter); + _pause(IB20.PausableFeature.MINT); + // minter has role; recipient is address(0); MINT is paused — pause fires first. + + vm.prank(minter); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.MINT)); token.mint(address(0), amount); } - /// @notice With both ROLE and PAUSE violated, ROLE fires first. - /// @dev Canonical: modifier runs before any body check, including the pause guard. - function test_mint_revertOrder_role_beats_pause(address caller, address to, uint256 amount) public { - _assumeValidCaller(caller); + /// @notice With PAUSE and POLICY violated, PAUSE fires first. + function test_mint_revertOrder_pause_beats_policy(address to, uint256 amount) public { _assumeValidActor(to); - vm.assume(caller != admin); + _grantRole(B20Constants.MINT_ROLE, minter); _pause(IB20.PausableFeature.MINT); - // No MINT_ROLE granted. + _setPolicy(B20Constants.MINT_RECEIVER_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); + + vm.prank(minter); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.MINT)); + token.mint(to, amount); + } + + /// @notice With PAUSE and CAP violated, PAUSE fires first. + function test_mint_revertOrder_pause_beats_cap(address to, uint256 amount) public { + _assumeValidActor(to); + _grantRole(B20Constants.MINT_ROLE, minter); + _pause(IB20.PausableFeature.MINT); + amount = bound(amount, 1, type(uint128).max); + vm.prank(admin); + token.updateSupplyCap(0); + + vm.prank(minter); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.MINT)); + token.mint(to, amount); + } + + // --- Pairs where ROLE wins (PAUSE not violated) --- + + /// @notice With both ROLE and ZERO-RECEIVER violated, ROLE fires first. + /// @dev `onlyRole` modifier runs before `validReceiver` modifier. + function test_mint_revertOrder_role_beats_zeroRecipient(address caller, uint256 amount) public { + _assumeValidCaller(caller); + vm.assume(caller != admin); // admin holds DEFAULT_ADMIN_ROLE but not MINT_ROLE on fresh token + // No MINT_ROLE granted; recipient is address(0); pause not set. vm.prank(caller); vm.expectRevert( abi.encodeWithSelector(IB20.AccessControlUnauthorizedAccount.selector, caller, B20Constants.MINT_ROLE) ); - token.mint(to, amount); + token.mint(address(0), amount); } /// @notice With both ROLE and POLICY violated, ROLE fires first. - /// @dev Canonical: modifier runs before the receiver-policy check. function test_mint_revertOrder_role_beats_policy(address caller, address to, uint256 amount) public { _assumeValidCaller(caller); _assumeValidActor(to); @@ -76,7 +113,6 @@ contract B20MintRevertOrderTest is B20Test { } /// @notice With both ROLE and CAP violated, ROLE fires first. - /// @dev Canonical: modifier runs before the supply-cap arithmetic check. function test_mint_revertOrder_role_beats_cap(address caller, address to, uint256 amount) public { _assumeValidCaller(caller); _assumeValidActor(to); @@ -93,21 +129,10 @@ contract B20MintRevertOrderTest is B20Test { token.mint(to, amount); } - // --- Pairs where ZERO-RECEIVER wins (ROLE satisfied) --- - - /// @notice With ZERO-RECEIVER and PAUSE violated, ZERO-RECEIVER fires first. - /// @dev Canonical: zero-receiver guard runs before the pause guard inside `_mint`. - function test_mint_revertOrder_zeroRecipient_beats_pause(uint256 amount) public { - _grantRole(B20Constants.MINT_ROLE, minter); - _pause(IB20.PausableFeature.MINT); - - vm.prank(minter); - vm.expectRevert(abi.encodeWithSelector(IB20.InvalidReceiver.selector, address(0))); - token.mint(address(0), amount); - } + // --- Pairs where ZERO-RECEIVER wins (PAUSE + ROLE satisfied) --- /// @notice With ZERO-RECEIVER and POLICY violated, ZERO-RECEIVER fires first. - /// @dev Canonical: zero-receiver guard runs before the receiver-policy check. + /// @dev `validReceiver` modifier runs before the receiver-policy check in `_mint`. function test_mint_revertOrder_zeroRecipient_beats_policy(uint256 amount) public { _grantRole(B20Constants.MINT_ROLE, minter); _setPolicy(B20Constants.MINT_RECEIVER_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); @@ -118,7 +143,7 @@ contract B20MintRevertOrderTest is B20Test { } /// @notice With ZERO-RECEIVER and CAP violated, ZERO-RECEIVER fires first. - /// @dev Canonical: zero-receiver guard runs before the supply-cap arithmetic. + /// @dev `validReceiver` modifier runs before the supply-cap arithmetic in `_mint`. function test_mint_revertOrder_zeroRecipient_beats_cap(uint256 amount) public { _grantRole(B20Constants.MINT_ROLE, minter); amount = bound(amount, 1, type(uint128).max); @@ -130,40 +155,10 @@ contract B20MintRevertOrderTest is B20Test { token.mint(address(0), amount); } - // --- Pairs where PAUSE wins (ROLE + ZERO-RECEIVER satisfied) --- - - /// @notice With PAUSE and POLICY violated, PAUSE fires first. - /// @dev Canonical: pause guard runs before the receiver-policy check. - function test_mint_revertOrder_pause_beats_policy(address to, uint256 amount) public { - _assumeValidActor(to); - _grantRole(B20Constants.MINT_ROLE, minter); - _pause(IB20.PausableFeature.MINT); - _setPolicy(B20Constants.MINT_RECEIVER_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); - - vm.prank(minter); - vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.MINT)); - token.mint(to, amount); - } - - /// @notice With PAUSE and CAP violated, PAUSE fires first. - /// @dev Canonical: pause guard runs before the supply-cap arithmetic. - function test_mint_revertOrder_pause_beats_cap(address to, uint256 amount) public { - _assumeValidActor(to); - _grantRole(B20Constants.MINT_ROLE, minter); - _pause(IB20.PausableFeature.MINT); - amount = bound(amount, 1, type(uint128).max); - vm.prank(admin); - token.updateSupplyCap(0); - - vm.prank(minter); - vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.MINT)); - token.mint(to, amount); - } - - // --- Pair where POLICY wins (ROLE + ZERO + PAUSE satisfied) --- + // --- Pair where POLICY wins (PAUSE + ROLE + ZERO satisfied) --- /// @notice With POLICY and CAP violated, POLICY fires first. - /// @dev Canonical: receiver-policy check runs before the supply-cap arithmetic. + /// @dev Receiver-policy check runs before the supply-cap arithmetic in `_mint`. function test_mint_revertOrder_policy_beats_cap(address to, uint256 amount) public { _assumeValidActor(to); _grantRole(B20Constants.MINT_ROLE, minter); diff --git a/test/unit/B20Security/batch/batchBurn_revertOrder.t.sol b/test/unit/B20Security/batch/batchBurn_revertOrder.t.sol index ea3f9624..a002208e 100644 --- a/test/unit/B20Security/batch/batchBurn_revertOrder.t.sol +++ b/test/unit/B20Security/batch/batchBurn_revertOrder.t.sol @@ -8,11 +8,11 @@ import {B20SecurityTest} from "test/lib/B20SecurityTest.sol"; /// @title Differential check-order tests for `batchBurn` (security variant). /// -/// @notice **Canonical order (Solidity reference):** -/// 1. ROLE (`onlyRoleStrict(BURN_FROM_ROLE)` modifier) → `AccessControlUnauthorizedAccount` -/// 2. LENGTH_MISMATCH (`accounts.length != amounts.length`) → `LengthMismatch` -/// 3. EMPTY_BATCH (`accounts.length == 0`) → `EmptyBatch` -/// 4. PAUSE (`_isPaused(BURN)`) → `ContractPaused` +/// @notice **Canonical order (Solidity reference — modifier-led):** +/// 1. PAUSE (`whenNotPaused(BURN)` modifier) → `ContractPaused` +/// 2. ROLE (`onlyRoleStrict(BURN_FROM_ROLE)` modifier) → `AccessControlUnauthorizedAccount` +/// 3. LENGTH_MISMATCH (body, `accounts.length != amounts.length`) → `LengthMismatch` +/// 4. EMPTY_BATCH (body, `accounts.length == 0`) → `EmptyBatch` /// 5. BALANCE (per-element `_burnRaw`) → `InsufficientBalance` /// /// Some pairs are not reachable because the violations are mutually @@ -33,7 +33,54 @@ contract B20SecurityBatchBurnRevertOrderTest is B20SecurityTest { _twoUints.push(1); } - // --- Pairs where ROLE wins (via modifier, before any body check) --- + // --- Pairs where PAUSE wins (PAUSE is canonical first) --- + + /// @notice PAUSE beats ROLE. + /// @dev `whenNotPaused` modifier is listed before `onlyRoleStrict`. + function test_batchBurn_revertOrder_pause_beats_role(address caller, uint256 amount) public { + _assumeValidCaller(caller); + vm.assume(caller != admin); + vm.assume(caller != burnFromActor); + _pause(IB20.PausableFeature.BURN); + + vm.prank(caller); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.BURN)); + security().batchBurn(_singletonAddresses(alice), _singletonUints(amount)); + } + + /// @notice PAUSE beats LENGTH_MISMATCH. + function test_batchBurn_revertOrder_pause_beats_lengthMismatch() public { + _grantBurnFrom(); + _pause(IB20.PausableFeature.BURN); + + vm.prank(burnFromActor); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.BURN)); + security().batchBurn(_twoAddrs, _twoUints); + } + + /// @notice PAUSE beats EMPTY_BATCH. + function test_batchBurn_revertOrder_pause_beats_emptyBatch() public { + _grantBurnFrom(); + _pause(IB20.PausableFeature.BURN); + + vm.prank(burnFromActor); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.BURN)); + security().batchBurn(_emptyAddrs, _emptyUints); + } + + /// @notice PAUSE beats BALANCE. + function test_batchBurn_revertOrder_pause_beats_balance(uint256 amount) public { + amount = bound(amount, 1, type(uint128).max); + _grantBurnFrom(); + _pause(IB20.PausableFeature.BURN); + // alice has zero balance → BALANCE would fire if PAUSE didn't. + + vm.prank(burnFromActor); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.BURN)); + security().batchBurn(_singletonAddresses(alice), _singletonUints(amount)); + } + + // --- Pairs where ROLE wins (PAUSE not violated) --- // // Note: BURN_FROM_ROLE() is resolved before vm.prank because the view call // would otherwise consume the prank intended for batchBurn (same pattern as @@ -62,18 +109,6 @@ contract B20SecurityBatchBurnRevertOrderTest is B20SecurityTest { security().batchBurn(_emptyAddrs, _emptyUints); } - function test_batchBurn_revertOrder_role_beats_pause(address caller, uint256 amount) public { - _assumeValidCaller(caller); - vm.assume(caller != admin); - vm.assume(caller != burnFromActor); - _pause(IB20.PausableFeature.BURN); - bytes32 role = security().BURN_FROM_ROLE(); - - vm.prank(caller); - vm.expectRevert(abi.encodeWithSelector(IB20.AccessControlUnauthorizedAccount.selector, caller, role)); - security().batchBurn(_singletonAddresses(alice), _singletonUints(amount)); - } - function test_batchBurn_revertOrder_role_beats_balance(address caller, uint256 amount) public { _assumeValidCaller(caller); vm.assume(caller != admin); @@ -86,39 +121,4 @@ contract B20SecurityBatchBurnRevertOrderTest is B20SecurityTest { vm.expectRevert(abi.encodeWithSelector(IB20.AccessControlUnauthorizedAccount.selector, caller, role)); security().batchBurn(_singletonAddresses(alice), _singletonUints(amount)); } - - // --- Pairs where LENGTH_MISMATCH wins --- - - function test_batchBurn_revertOrder_lengthMismatch_beats_pause() public { - _grantBurnFrom(); - _pause(IB20.PausableFeature.BURN); - - vm.prank(burnFromActor); - vm.expectRevert(abi.encodeWithSelector(IB20Security.LengthMismatch.selector, uint256(2), uint256(1))); - security().batchBurn(_twoAddrs, _twoUints); - } - - // --- Pairs where EMPTY_BATCH wins --- - - function test_batchBurn_revertOrder_emptyBatch_beats_pause() public { - _grantBurnFrom(); - _pause(IB20.PausableFeature.BURN); - - vm.prank(burnFromActor); - vm.expectRevert(IB20Security.EmptyBatch.selector); - security().batchBurn(_emptyAddrs, _emptyUints); - } - - // --- Pair where PAUSE wins --- - - function test_batchBurn_revertOrder_pause_beats_balance(uint256 amount) public { - amount = bound(amount, 1, type(uint128).max); - _grantBurnFrom(); - _pause(IB20.PausableFeature.BURN); - // alice has zero balance → BALANCE would fire if PAUSE didn't. - - vm.prank(burnFromActor); - vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.BURN)); - security().batchBurn(_singletonAddresses(alice), _singletonUints(amount)); - } } diff --git a/test/unit/B20Security/batch/batchMint.t.sol b/test/unit/B20Security/batch/batchMint.t.sol index 561e358d..49e08f38 100644 --- a/test/unit/B20Security/batch/batchMint.t.sol +++ b/test/unit/B20Security/batch/batchMint.t.sol @@ -11,29 +11,38 @@ import {MockPolicyRegistry, PolicyRegistryConstants} from "test/lib/mocks/MockPo contract B20SecurityBatchMintTest is B20SecurityTest { /// @notice Verifies batchMint reverts when recipients.length != amounts.length - /// @dev Length-mismatch guard fires before the empty-batch guard; checks - /// LengthMismatch(recipients.length, amounts.length). + /// @dev Length-mismatch guard fires in batchMint's body, after the entrypoint + /// modifiers (PAUSE + MINT_ROLE) and before the empty-batch guard. + /// Caller is granted MINT_ROLE so the role modifier passes and the body + /// reaches the length check. function test_batchMint_revert_lengthMismatch() public { + _grantRole(B20Constants.MINT_ROLE, minter); + address[] memory recipients = _singletonAddresses(alice); uint256[] memory amounts = new uint256[](2); amounts[0] = 1; amounts[1] = 2; + vm.prank(minter); vm.expectRevert(abi.encodeWithSelector(IB20Security.LengthMismatch.selector, uint256(1), uint256(2))); security().batchMint(recipients, amounts); } /// @notice Verifies batchMint reverts when both arrays are empty - /// @dev EmptyBatch guard: no-op corp-actions transactions are rejected to keep the - /// log stream meaningful. + /// @dev EmptyBatch guard fires in batchMint's body, after PAUSE + MINT_ROLE + /// modifiers. Caller is granted MINT_ROLE so the role modifier passes. function test_batchMint_revert_emptyBatch() public { + _grantRole(B20Constants.MINT_ROLE, minter); + + vm.prank(minter); vm.expectRevert(IB20Security.EmptyBatch.selector); security().batchMint(new address[](0), new uint256[](0)); } - /// @notice Verifies batchMint surfaces _mint's role-check revert when caller lacks MINT_ROLE - /// @dev batchMint has no outer role check; per-element `_mint` enforces MINT_ROLE. Any - /// non-minter caller hits the inner revert on element 0. + /// @notice Verifies batchMint reverts when caller lacks MINT_ROLE + /// @dev `onlyRole(MINT_ROLE)` is now an entrypoint modifier on batchMint + /// itself (was per-element via `_mint` before the pause→role→input + /// hoist). Any non-minter caller is rejected before the body runs. function test_batchMint_revert_unauthorized(address caller, address to, uint256 amount) public { _assumeValidCaller(caller); _assumeValidActor(to); diff --git a/test/unit/B20Security/batch/batchMint_revertOrder.t.sol b/test/unit/B20Security/batch/batchMint_revertOrder.t.sol index f2941dd8..d1ad4ed5 100644 --- a/test/unit/B20Security/batch/batchMint_revertOrder.t.sol +++ b/test/unit/B20Security/batch/batchMint_revertOrder.t.sol @@ -9,17 +9,20 @@ import {B20Constants} from "test/lib/mocks/MockB20.sol"; /// @title Differential check-order tests for `batchMint` (security variant). /// -/// @notice **Canonical order (Solidity reference):** -/// 1. LENGTH_MISMATCH (`recipients.length != amounts.length`) → `LengthMismatch` -/// 2. EMPTY_BATCH (`recipients.length == 0`) → `EmptyBatch` -/// 3..N. Per-element `_mint` checks (see `mint_revertOrder.t.sol`): -/// ROLE → ZERO-RECEIVER → PAUSE → POLICY → CAP +/// @notice **Canonical order (Solidity reference — modifier-led):** +/// 1. PAUSE (`whenNotPaused(MINT)` modifier) → `ContractPaused` +/// 2. ROLE (`onlyRole(MINT_ROLE)` modifier) → `AccessControlUnauthorizedAccount` +/// 3. LENGTH_MISMATCH (body, `recipients.length != amounts.length`) → `LengthMismatch` +/// 4. EMPTY_BATCH (body, `recipients.length == 0`) → `EmptyBatch` +/// 5. ZERO-RECEIVER (per-element inline guard) → `InvalidReceiver` +/// 6..N. Per-element `_mint` checks (see `mint_revertOrder.t.sol`): +/// POLICY → CAP /// /// The pairs within `_mint`'s body are already pinned by -/// `mint_revertOrder.t.sol`. This file only pins the two batchMint-specific -/// preconditions (LENGTH_MISMATCH, EMPTY_BATCH) against the first per-element -/// check they encounter (ROLE), which transitively guarantees they fire -/// before every `_mint` body check. +/// `mint_revertOrder.t.sol`. This file pins the batch-level +/// preconditions (PAUSE, ROLE, LENGTH_MISMATCH, EMPTY_BATCH) +/// against each other and against the first per-element check +/// they encounter. contract B20SecurityBatchMintRevertOrderTest is B20SecurityTest { address[] internal _emptyAddrs; uint256[] internal _emptyUints; @@ -33,27 +36,87 @@ contract B20SecurityBatchMintRevertOrderTest is B20SecurityTest { _twoUints.push(1); } - /// @notice LENGTH_MISMATCH beats ROLE (and transitively everything inside `_mint`). - /// @dev Caller lacks MINT_ROLE; arrays have mismatched lengths. LENGTH check - /// fires in `batchMint` body before `_mint` is invoked, so the role - /// modifier on `_mint` never runs. - function test_batchMint_revertOrder_lengthMismatch_beats_mintBody(address caller) public { + // --- Pairs where PAUSE wins (PAUSE is canonical first) --- + + /// @notice PAUSE beats ROLE. + function test_batchMint_revertOrder_pause_beats_role(address caller) public { _assumeValidCaller(caller); vm.assume(caller != admin); vm.assume(caller != minter); + _pause(IB20.PausableFeature.MINT); vm.prank(caller); - vm.expectRevert(abi.encodeWithSelector(IB20Security.LengthMismatch.selector, uint256(2), uint256(1))); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.MINT)); + security().batchMint(_twoAddrs, _twoUints); + } + + /// @notice PAUSE beats LENGTH_MISMATCH. + function test_batchMint_revertOrder_pause_beats_lengthMismatch() public { + _grantRole(B20Constants.MINT_ROLE, minter); + _pause(IB20.PausableFeature.MINT); + + vm.prank(minter); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.MINT)); security().batchMint(_twoAddrs, _twoUints); } - /// @notice EMPTY_BATCH beats ROLE (and transitively everything inside `_mint`). - function test_batchMint_revertOrder_emptyBatch_beats_mintBody(address caller) public { + /// @notice PAUSE beats EMPTY_BATCH. + function test_batchMint_revertOrder_pause_beats_emptyBatch() public { + _grantRole(B20Constants.MINT_ROLE, minter); + _pause(IB20.PausableFeature.MINT); + + vm.prank(minter); + vm.expectRevert(abi.encodeWithSelector(IB20.ContractPaused.selector, IB20.PausableFeature.MINT)); + security().batchMint(_emptyAddrs, _emptyUints); + } + + // --- Pairs where ROLE wins (PAUSE not violated) --- + + /// @notice ROLE beats LENGTH_MISMATCH. + /// @dev `onlyRole` modifier fires before the body's length check. + function test_batchMint_revertOrder_role_beats_lengthMismatch(address caller) public { _assumeValidCaller(caller); vm.assume(caller != admin); vm.assume(caller != minter); vm.prank(caller); + vm.expectRevert( + abi.encodeWithSelector(IB20.AccessControlUnauthorizedAccount.selector, caller, B20Constants.MINT_ROLE) + ); + security().batchMint(_twoAddrs, _twoUints); + } + + /// @notice ROLE beats EMPTY_BATCH. + function test_batchMint_revertOrder_role_beats_emptyBatch(address caller) public { + _assumeValidCaller(caller); + vm.assume(caller != admin); + vm.assume(caller != minter); + + vm.prank(caller); + vm.expectRevert( + abi.encodeWithSelector(IB20.AccessControlUnauthorizedAccount.selector, caller, B20Constants.MINT_ROLE) + ); + security().batchMint(_emptyAddrs, _emptyUints); + } + + // --- Pairs where LENGTH_MISMATCH / EMPTY_BATCH win (PAUSE + ROLE satisfied) --- + + /// @notice LENGTH_MISMATCH beats per-element `_mint` body checks. + /// @dev With role granted and pause not set, the length check in batchMint's + /// body fires before the per-element loop runs. + function test_batchMint_revertOrder_lengthMismatch_beats_mintBody() public { + _grantRole(B20Constants.MINT_ROLE, minter); + + vm.prank(minter); + vm.expectRevert(abi.encodeWithSelector(IB20Security.LengthMismatch.selector, uint256(2), uint256(1))); + security().batchMint(_twoAddrs, _twoUints); + } + + /// @notice EMPTY_BATCH beats per-element `_mint` body checks. + function test_batchMint_revertOrder_emptyBatch_beats_mintBody() public { + _grantRole(B20Constants.MINT_ROLE, minter); + + vm.prank(minter); vm.expectRevert(IB20Security.EmptyBatch.selector); security().batchMint(_emptyAddrs, _emptyUints); } From 7c129654e2ca132d6e9cf543f27ab99bb0b26992 Mon Sep 17 00:00:00 2001 From: Amie Corso Date: Fri, 29 May 2026 11:54:51 -0700 Subject: [PATCH 2/5] review: drop input-validation modifiers, inline the checks Per review feedback: `validReceiver` / `validSender` / `validApprover` / `validSpender` / `nonEmptyFeatures` were misleading (the same address also has policy checks fired against it elsewhere, so a name like `validReceiver` implies completeness it doesn't have). Removed. Kept: `whenNotPaused` and `whenNotPausedStrict` (the pause gate is the only new modifier this PR introduces). Inlined zero-address / empty-feature checks at each entrypoint; revert order is unchanged. Tightened docstrings in mocks + tests to drop references to the removed modifiers. --- src/interfaces/IB20Security.sol | 4 +- test/lib/mocks/MockB20.sol | 130 +++++------------- test/lib/mocks/MockB20Security.sol | 26 +--- .../B20/erc20/transferFrom_revertOrder.t.sol | 37 +++-- .../unit/B20/erc20/transfer_revertOrder.t.sol | 12 +- .../B20/supply/burnBlocked_revertOrder.t.sol | 8 +- test/unit/B20/supply/burn_revertOrder.t.sol | 7 +- test/unit/B20/supply/mint_revertOrder.t.sol | 18 +-- .../batch/batchBurn_revertOrder.t.sol | 8 +- .../batch/batchMint_revertOrder.t.sol | 10 +- 10 files changed, 94 insertions(+), 166 deletions(-) diff --git a/src/interfaces/IB20Security.sol b/src/interfaces/IB20Security.sol index 6f7b4e89..29cb6d9f 100644 --- a/src/interfaces/IB20Security.sol +++ b/src/interfaces/IB20Security.sol @@ -387,8 +387,8 @@ interface IB20Security is IB20 { /// `SupplyCapExceeded` — see `mint`'s natspec for the /// per-element precedence. /// The pause and role gates are evaluated ONCE for the - /// whole batch (entrypoint modifiers); per-element gates - /// fire per recipient inside the loop. + /// whole batch; per-element gates fire per recipient inside + /// the loop. /// All-or-nothing: if any element reverts, the entire /// transaction reverts and no partial state is committed. /// Emits `Transfer(address(0), recipients[i], amounts[i])` diff --git a/test/lib/mocks/MockB20.sol b/test/lib/mocks/MockB20.sol index 5bf22529..d2f4cf50 100644 --- a/test/lib/mocks/MockB20.sol +++ b/test/lib/mocks/MockB20.sol @@ -97,15 +97,11 @@ contract MockB20 is IB20 { /// @dev Reverts `ContractPaused(feature)` if `feature` is paused, /// unless the call is from the factory in the bootstrap window - /// (`_isPrivileged()`). This is the canonical pause gate and - /// is always listed FIRST in a function's modifier set so - /// pause-vs-anything precedence reads off the modifier list - /// directly (modifiers run left-to-right, body at `_`). Token - /// invariants (balance, supply cap) are never bypassed and are - /// enforced regardless. Variants that reject the bootstrap - /// bypass (currently `MockB20Security`'s `redeem` path) declare - /// their own `whenNotPausedStrict` modifier rather than - /// overriding this one. + /// (see `_isPrivileged()`). Always listed FIRST in a gated + /// function's modifier set so pause precedence reads off the + /// signature directly (modifiers run left-to-right, body at + /// `_`). Token invariants (balance, supply cap) are never + /// bypassed and are enforced regardless. modifier whenNotPaused(PausableFeature feature) { if (!_isPrivileged() && _isPaused(feature)) revert ContractPaused(feature); _; @@ -131,45 +127,6 @@ contract MockB20 is IB20 { _; } - /// @dev Input-validation modifier: reverts `InvalidReceiver(to)` - /// if `to == address(0)`. ALWAYS enforced; never bypassed by - /// the factory bootstrap window. Occupies the third slot in - /// the canonical `pause → role → input` modifier order. - modifier validReceiver(address to) { - if (to == address(0)) revert InvalidReceiver(to); - _; - } - - /// @dev Input-validation modifier: reverts `InvalidSender(from)` - /// if `from == address(0)`. ALWAYS enforced; never bypassed. - modifier validSender(address from) { - if (from == address(0)) revert InvalidSender(from); - _; - } - - /// @dev Input-validation modifier: reverts `InvalidApprover(owner)` - /// if `owner == address(0)`. ALWAYS enforced; never bypassed. - modifier validApprover(address owner) { - if (owner == address(0)) revert InvalidApprover(owner); - _; - } - - /// @dev Input-validation modifier: reverts `InvalidSpender(spender)` - /// if `spender == address(0)`. ALWAYS enforced; never bypassed. - modifier validSpender(address spender) { - if (spender == address(0)) revert InvalidSpender(spender); - _; - } - - /// @dev Input-validation modifier: reverts `EmptyFeatureSet()` if - /// `features.length == 0`. ALWAYS enforced; never bypassed. - modifier nonEmptyFeatures(PausableFeature[] calldata features) { - if (features.length == 0) revert EmptyFeatureSet(); - _; - } - - // ============================================================ - // ERC-20: VIEWS // ============================================================ // ERC-20: VIEWS // ============================================================ @@ -203,13 +160,9 @@ contract MockB20 is IB20 { // ERC-20: MUTATIONS // ============================================================ - function transfer(address to, uint256 amount) - external - whenNotPaused(PausableFeature.TRANSFER) - validReceiver(to) - validSender(msg.sender) - returns (bool) - { + function transfer(address to, uint256 amount) external whenNotPaused(PausableFeature.TRANSFER) returns (bool) { + if (to == address(0)) revert InvalidReceiver(to); + if (msg.sender == address(0)) revert InvalidSender(msg.sender); _transfer(msg.sender, to, amount); return true; } @@ -217,10 +170,10 @@ contract MockB20 is IB20 { function transferFrom(address from, address to, uint256 amount) external whenNotPaused(PausableFeature.TRANSFER) - validReceiver(to) - validSender(from) returns (bool) { + if (to == address(0)) revert InvalidReceiver(to); + if (from == address(0)) revert InvalidSender(from); if (!_isPrivileged()) { // Allowance is consumed unconditionally outside the factory // bootstrap window. Matches OZ ERC20 and the Rust precompile, @@ -241,12 +194,9 @@ contract MockB20 is IB20 { return true; } - function approve(address spender, uint256 amount) - external - validApprover(msg.sender) - validSpender(spender) - returns (bool) - { + function approve(address spender, uint256 amount) external returns (bool) { + if (msg.sender == address(0)) revert InvalidApprover(msg.sender); + if (spender == address(0)) revert InvalidSpender(spender); MockB20Storage.layout().allowances[msg.sender][spender] = amount; emit Approval(msg.sender, spender, amount); return true; @@ -259,10 +209,10 @@ contract MockB20 is IB20 { function transferWithMemo(address to, uint256 amount, bytes32 memo) external whenNotPaused(PausableFeature.TRANSFER) - validReceiver(to) - validSender(msg.sender) returns (bool) { + if (to == address(0)) revert InvalidReceiver(to); + if (msg.sender == address(0)) revert InvalidSender(msg.sender); _transfer(msg.sender, to, amount); emit Memo(msg.sender, memo); return true; @@ -271,10 +221,10 @@ contract MockB20 is IB20 { function transferFromWithMemo(address from, address to, uint256 amount, bytes32 memo) external whenNotPaused(PausableFeature.TRANSFER) - validReceiver(to) - validSender(from) returns (bool) { + if (to == address(0)) revert InvalidReceiver(to); + if (from == address(0)) revert InvalidSender(from); if (!_isPrivileged()) { _consumeAllowance(from, msg.sender, amount); if (msg.sender != from) { @@ -308,12 +258,8 @@ contract MockB20 is IB20 { // MINT / BURN // ============================================================ - function mint(address to, uint256 amount) - external - whenNotPaused(PausableFeature.MINT) - onlyRole(MINT_ROLE) - validReceiver(to) - { + function mint(address to, uint256 amount) external whenNotPaused(PausableFeature.MINT) onlyRole(MINT_ROLE) { + if (to == address(0)) revert InvalidReceiver(to); _mint(to, amount); } @@ -321,8 +267,8 @@ contract MockB20 is IB20 { external whenNotPaused(PausableFeature.MINT) onlyRole(MINT_ROLE) - validReceiver(to) { + if (to == address(0)) revert InvalidReceiver(to); _mint(to, amount); emit Memo(msg.sender, memo); } @@ -458,7 +404,8 @@ contract MockB20 is IB20 { return _isPaused(feature); } - function pause(PausableFeature[] calldata features) external onlyRole(PAUSE_ROLE) nonEmptyFeatures(features) { + function pause(PausableFeature[] calldata features) external onlyRole(PAUSE_ROLE) { + if (features.length == 0) revert EmptyFeatureSet(); MockB20Storage.Layout storage $ = MockB20Storage.layout(); for (uint256 i = 0; i < features.length; i++) { $.pausedVectors |= uint256(1) << uint8(features[i]); @@ -466,7 +413,8 @@ contract MockB20 is IB20 { emit Paused(msg.sender, features); } - function unpause(PausableFeature[] calldata features) external onlyRole(UNPAUSE_ROLE) nonEmptyFeatures(features) { + function unpause(PausableFeature[] calldata features) external onlyRole(UNPAUSE_ROLE) { + if (features.length == 0) revert EmptyFeatureSet(); MockB20Storage.Layout storage $ = MockB20Storage.layout(); for (uint256 i = 0; i < features.length; i++) { $.pausedVectors &= ~(uint256(1) << uint8(features[i])); @@ -719,16 +667,14 @@ contract MockB20 is IB20 { } /// @dev Pure mechanics: policy (with bootstrap bypass) + balance + - /// effects. Pause, role, and input validation are enforced by - /// the entrypoint modifiers (`whenNotPaused`, `validReceiver`, - /// `validSender`); every external caller of this helper — - /// `transfer`, `transferFrom`, `transferWithMemo`, - /// `transferFromWithMemo` — declares those modifiers, so the - /// gates run before this function is entered. `transferFrom` / - /// `transferFromWithMemo` additionally consume allowance and - /// check the executor policy in their bodies before calling - /// here; both of those checks ALSO honor the bootstrap bypass, - /// consistent with the policy bypass below. + /// effects. Pause + input validation are enforced upstream by + /// every external caller (`transfer`, `transferFrom`, + /// `transferWithMemo`, `transferFromWithMemo`) before reaching + /// this helper. `transferFrom` / `transferFromWithMemo` + /// additionally consume allowance and check the executor + /// policy in their bodies before calling here; both of those + /// checks ALSO honor the bootstrap bypass, consistent with + /// the policy bypass below. function _transfer(address from, address to, uint256 amount) internal { if (!_isPrivileged()) { // One SLOAD pulls both policy IDs we need for the transfer @@ -756,13 +702,11 @@ contract MockB20 is IB20 { } /// @dev Pure mechanics: policy (with bootstrap bypass) + supply cap - /// + effects. Pause, role, and input validation are enforced - /// by the entrypoint modifiers (`whenNotPaused`, `onlyRole`, - /// `validReceiver`) on `mint` / `mintWithMemo`. The security - /// variant's `batchMint` declares the same entry-level - /// modifiers ONCE for the whole batch and validates per-element - /// receivers inline before invoking this helper, so `_mint` - /// itself never re-checks them. + /// + effects. Pause, role, and the zero-receiver check are + /// enforced upstream by `mint` / `mintWithMemo`. The security + /// variant's `batchMint` carries the same `whenNotPaused` + + /// `onlyRole` modifiers ONCE for the whole batch and validates + /// per-element receivers inline before invoking this helper. function _mint(address to, uint256 amount) internal { if (!_isPrivileged()) { uint64 mintReceiverPolicyId = MockB20Storage.layout().mintPolicyIds.receiver; diff --git a/test/lib/mocks/MockB20Security.sol b/test/lib/mocks/MockB20Security.sol index 6368d7eb..2f119c06 100644 --- a/test/lib/mocks/MockB20Security.sol +++ b/test/lib/mocks/MockB20Security.sol @@ -186,16 +186,10 @@ contract MockB20Security is MockB20, IB20Security { // BATCHED ISSUANCE / CLAWBACK // ============================================================ - /// @dev `whenNotPaused(MINT)` + `onlyRole(MINT_ROLE)` modifiers run - /// ONCE for the entire batch — the underlying `_mint` helper - /// no longer carries its own pause/role gates (those were - /// hoisted to per-entrypoint modifiers as part of the - /// `pause → role → input → ...` canonical ordering). Length / - /// empty-batch checks fire next inside the body. The - /// per-element `validReceiver` guard is inlined in the loop - /// because modifier parameters evaluate once at function - /// entry, not per-iteration; this preserves the per-recipient - /// `InvalidReceiver` revert that callers depend on. + /// @dev Pause + role enforced ONCE for the entire batch via the + /// entrypoint modifiers. Per-element zero-receiver guard is + /// inlined in the loop since `_mint` no longer carries an + /// input check. function batchMint(address[] calldata recipients, uint256[] calldata amounts) external whenNotPaused(PausableFeature.MINT) @@ -204,24 +198,16 @@ contract MockB20Security is MockB20, IB20Security { if (recipients.length != amounts.length) revert LengthMismatch(recipients.length, amounts.length); if (recipients.length == 0) revert EmptyBatch(); for (uint256 i = 0; i < recipients.length; i++) { - // Per-element zero-receiver guard: `_mint` no longer checks - // input (modifier-only on the single-recipient `mint` / - // `mintWithMemo` entrypoints). if (recipients[i] == address(0)) revert InvalidReceiver(recipients[i]); _mint(recipients[i], amounts[i]); } } - /// @dev Modifier order: `pause → role` (canonical for the variant). - /// `onlyRoleStrict` (not `onlyRole`): the factory bootstrap + /// @dev `onlyRoleStrict` (not `onlyRole`): the factory bootstrap /// bypass is deliberately NOT honored here, per the contract /// natspec — clawback against existing balances has no init-time /// use case, so granting the factory a bypass would only widen - /// the attack surface. `whenNotPaused` retains the bypass; the - /// practical effect is identical because the factory never - /// reaches the body anyway (role-strict blocks it first), and - /// mirroring the rest of the contract on the pause vector - /// keeps the modifier set uniform. + /// the attack surface. function batchBurn(address[] calldata accounts, uint256[] calldata amounts) external whenNotPaused(PausableFeature.BURN) diff --git a/test/unit/B20/erc20/transferFrom_revertOrder.t.sol b/test/unit/B20/erc20/transferFrom_revertOrder.t.sol index b9c937e4..63cb0231 100644 --- a/test/unit/B20/erc20/transferFrom_revertOrder.t.sol +++ b/test/unit/B20/erc20/transferFrom_revertOrder.t.sol @@ -10,28 +10,27 @@ import {PolicyRegistryConstants} from "test/lib/mocks/MockPolicyRegistry.sol"; /// @title Differential check-order tests for `transferFrom`. /// /// @notice `transferFrom` layers two body-level preconditions -/// (ALLOWANCE and EXECUTOR-POLICY) on top of `_transfer`'s policy / -/// balance checks. The cross-cutting gates (PAUSE, ZERO-RECEIVER, -/// ZERO-SENDER) live on the entrypoint as modifiers and therefore -/// fire BEFORE the body's allowance / executor-policy work runs. +/// (ALLOWANCE and EXECUTOR-POLICY) on top of `_transfer`'s +/// policy / balance checks. The PAUSE / ZERO-RECEIVER / +/// ZERO-SENDER guards run before the allowance / executor-policy +/// work in the entrypoint body. /// -/// **Canonical order (Solidity reference — modifier-led, when +/// **Canonical order (Solidity reference, when /// `msg.sender != from`):** /// 1. PAUSE (`whenNotPaused(TRANSFER)` modifier) → `ContractPaused` -/// 2. ZERO-RECEIVER (`validReceiver(to)` modifier) → `InvalidReceiver` -/// 3. ZERO-SENDER (`validSender(from)` modifier) → `InvalidSender` -/// 4. ALLOWANCE (body, `_consumeAllowance`) → `InsufficientAllowance` -/// 5. EXECUTOR-POLICY (body, `isAuthorized(executorPolicyId, msg.sender)`) +/// 2. ZERO-RECEIVER (`to == address(0)`) → `InvalidReceiver` +/// 3. ZERO-SENDER (`from == address(0)`) → `InvalidSender` +/// 4. ALLOWANCE (`_consumeAllowance`) → `InsufficientAllowance` +/// 5. EXECUTOR-POLICY (`isAuthorized(executorPolicyId, msg.sender)`) /// → `PolicyForbids(EXECUTOR, ...)` /// 6..N. All `_transfer` body checks — see `transfer_revertOrder.t.sol` /// (SENDER-POLICY → RECEIVER-POLICY → BALANCE). /// /// The full pair matrix between body-level ALLOWANCE/EXECUTOR-POLICY -/// and the modifier-led PAUSE/ZERO-RECEIVER/ZERO-SENDER is pinned -/// below to lock in the modifier-first precedence; one test against -/// a representative `_transfer` body check (SENDER-POLICY) proves -/// ALLOWANCE and EXECUTOR-POLICY both fire before `_transfer` is -/// entered. +/// and the PAUSE/ZERO-RECEIVER/ZERO-SENDER guards is pinned below; +/// one test against a representative `_transfer` body check +/// (SENDER-POLICY) proves ALLOWANCE and EXECUTOR-POLICY both +/// fire before `_transfer` is entered. contract B20TransferFromRevertOrderTest is B20Test { // --- Pairs where PAUSE wins (PAUSE is canonical first) --- @@ -78,7 +77,7 @@ contract B20TransferFromRevertOrderTest is B20Test { // --- Pairs where ZERO-RECEIVER wins (PAUSE not violated) --- /// @notice ZERO-RECEIVER beats ALLOWANCE. - /// @dev `validReceiver` modifier fires before the body's allowance check. + /// @dev Zero-receiver check fires before the allowance check. function test_transferFrom_revertOrder_zeroReceiver_beats_allowance(address caller, address from, uint256 amount) public { @@ -113,7 +112,7 @@ contract B20TransferFromRevertOrderTest is B20Test { // --- Pairs where ZERO-SENDER wins (PAUSE + ZERO-RECEIVER not violated) --- /// @notice ZERO-SENDER beats ALLOWANCE. - /// @dev `validSender` modifier fires before the body's allowance check. + /// @dev Zero-sender check fires before the allowance check. function test_transferFrom_revertOrder_zeroSender_beats_allowance(address caller, address to, uint256 amount) public { @@ -163,9 +162,9 @@ contract B20TransferFromRevertOrderTest is B20Test { } /// @notice ALLOWANCE beats anything in `_transfer` (representative: SENDER-POLICY). - /// @dev `_transfer`'s body now contains policy + balance + effects only (input - /// validation hoisted to entrypoint modifier); SENDER-POLICY is the - /// first body check inside `_transfer` post-refactor. + /// @dev `_transfer`'s body is policy + balance + effects (input validation + /// lives on the entrypoint); SENDER-POLICY is the first body check + /// inside `_transfer`. function test_transferFrom_revertOrder_allowance_beats_transferBody( address caller, address from, diff --git a/test/unit/B20/erc20/transfer_revertOrder.t.sol b/test/unit/B20/erc20/transfer_revertOrder.t.sol index f1d56840..af36a778 100644 --- a/test/unit/B20/erc20/transfer_revertOrder.t.sol +++ b/test/unit/B20/erc20/transfer_revertOrder.t.sol @@ -9,13 +9,13 @@ import {PolicyRegistryConstants} from "test/lib/mocks/MockPolicyRegistry.sol"; /// @title Differential check-order tests for `transfer`. /// -/// @notice **Canonical order (Solidity reference — modifier-led):** +/// @notice **Canonical order (Solidity reference):** /// 1. PAUSE (`whenNotPaused(TRANSFER)` modifier) → `ContractPaused` -/// 2. ZERO-RECEIVER (`validReceiver(to)` modifier) → `InvalidReceiver` -/// 3. ZERO-SENDER (`validSender(from)` modifier) → `InvalidSender` -/// 4. SENDER-POLICY (body, in `_transfer`) → `PolicyForbids(SENDER, ...)` -/// 5. RECEIVER-POLICY (body, in `_transfer`) → `PolicyForbids(RECEIVER, ...)` -/// 6. BALANCE (body, in `_transfer`) → `InsufficientBalance` +/// 2. ZERO-RECEIVER (`to == address(0)`) → `InvalidReceiver` +/// 3. ZERO-SENDER (`from == address(0)`) → `InvalidSender` +/// 4. SENDER-POLICY (`_transfer` body) → `PolicyForbids(SENDER, ...)` +/// 5. RECEIVER-POLICY (`_transfer` body) → `PolicyForbids(RECEIVER, ...)` +/// 6. BALANCE (`_transfer` body) → `InsufficientBalance` /// /// The public `transfer(to, amount)` entry sets `from = msg.sender`, so /// pairs involving ZERO-SENDER require pranking `address(0)`. C(6, 2) = 15 pairs. diff --git a/test/unit/B20/supply/burnBlocked_revertOrder.t.sol b/test/unit/B20/supply/burnBlocked_revertOrder.t.sol index ffd51496..94f572c3 100644 --- a/test/unit/B20/supply/burnBlocked_revertOrder.t.sol +++ b/test/unit/B20/supply/burnBlocked_revertOrder.t.sol @@ -9,11 +9,11 @@ import {PolicyRegistryConstants} from "test/lib/mocks/MockPolicyRegistry.sol"; /// @title Differential check-order tests for `burnBlocked`. /// -/// @notice **Canonical order (Solidity reference — modifier-led):** +/// @notice **Canonical order (Solidity reference):** /// 1. PAUSE (`whenNotPaused(BURN)` modifier) → `ContractPaused` /// 2. ROLE (`onlyRole(BURN_BLOCKED_ROLE)` modifier) → `AccessControlUnauthorizedAccount` -/// 3. BLOCKED (body, `isAuthorized(senderPolicyId, from) == true` reverts) → `AccountNotBlocked` -/// 4. BALANCE (body, `fromBalance < amount` in `_burnRaw`) → `InsufficientBalance` +/// 3. BLOCKED (`isAuthorized(senderPolicyId, from) == true` reverts) → `AccountNotBlocked` +/// 4. BALANCE (`fromBalance < amount` in `_burnRaw`) → `InsufficientBalance` /// /// Note on BLOCKED semantics: `burnBlocked` is a clawback function — it /// only succeeds when `from` is currently NOT authorized by the @@ -24,7 +24,7 @@ contract B20BurnBlockedRevertOrderTest is B20Test { // --- Pair where PAUSE wins (PAUSE is canonical first) --- /// @notice PAUSE beats ROLE. - /// @dev `whenNotPaused` modifier is listed first; fires before `onlyRole`. + /// @dev Pause modifier is listed before the role modifier; fires first. function test_burnBlocked_revertOrder_pause_beats_role(address caller, address from, uint256 amount) public { _assumeValidCaller(caller); _assumeValidActor(from); diff --git a/test/unit/B20/supply/burn_revertOrder.t.sol b/test/unit/B20/supply/burn_revertOrder.t.sol index 8bf70e74..ee5e786d 100644 --- a/test/unit/B20/supply/burn_revertOrder.t.sol +++ b/test/unit/B20/supply/burn_revertOrder.t.sol @@ -12,14 +12,13 @@ import {MockB20, B20Constants} from "test/lib/mocks/MockB20.sol"; /// canonical first-firing revert selector. See `mint_revertOrder.t.sol` /// for the harness rationale. /// -/// **Canonical order (Solidity reference — modifier-led):** +/// **Canonical order (Solidity reference):** /// 1. PAUSE (`whenNotPaused(BURN)` modifier) → `ContractPaused` /// 2. ROLE (`onlyRole(BURN_ROLE)` modifier) → `AccessControlUnauthorizedAccount` -/// 3. BALANCE (body, `fromBalance < amount` in `_burnRaw`) → `InsufficientBalance` +/// 3. BALANCE (`fromBalance < amount` in `_burnRaw`) → `InsufficientBalance` contract B20BurnRevertOrderTest is B20Test { /// @notice PAUSE beats ROLE. - /// @dev The pause modifier is listed FIRST in the modifier set, so it fires - /// before the role modifier. With both violated, ContractPaused surfaces. + /// @dev Pause modifier is listed before the role modifier; fires first. function test_burn_revertOrder_pause_beats_role(address caller, uint256 amount) public { _assumeValidCaller(caller); vm.assume(caller != admin); diff --git a/test/unit/B20/supply/mint_revertOrder.t.sol b/test/unit/B20/supply/mint_revertOrder.t.sol index d6d0c1e8..fa75e330 100644 --- a/test/unit/B20/supply/mint_revertOrder.t.sol +++ b/test/unit/B20/supply/mint_revertOrder.t.sol @@ -16,12 +16,12 @@ import {MockPolicyRegistry, PolicyRegistryConstants} from "test/lib/mocks/MockPo /// between the two backends surfaces as a fork-mode-only failure with a /// clear "selector A vs selector B" diff. /// -/// **Canonical order (Solidity reference — modifier-led):** +/// **Canonical order (Solidity reference):** /// 1. PAUSE (`whenNotPaused(MINT)` modifier) → `ContractPaused` /// 2. ROLE (`onlyRole(MINT_ROLE)` modifier) → `AccessControlUnauthorizedAccount` -/// 3. ZERO-RECEIVER (`validReceiver(to)` modifier) → `InvalidReceiver` -/// 4. POLICY (body, in `_mint`) → `PolicyForbids` -/// 5. SUPPLY-CAP (body, in `_mint`) → `SupplyCapExceeded` +/// 3. ZERO-RECEIVER (`to == address(0)`) → `InvalidReceiver` +/// 4. POLICY (`_mint` body) → `PolicyForbids` +/// 5. SUPPLY-CAP (`_mint` body) → `SupplyCapExceeded` /// /// A `mint` call that violates two or more preconditions must always /// revert with the selector for the earliest-listed violation. The 10 @@ -30,7 +30,7 @@ contract B20MintRevertOrderTest is B20Test { // --- Pairs where PAUSE wins (PAUSE is canonical first) --- /// @notice With both PAUSE and ROLE violated, PAUSE fires first. - /// @dev `whenNotPaused` modifier is listed before `onlyRole`; runs first. + /// @dev Pause modifier is listed before the role modifier; runs first. function test_mint_revertOrder_pause_beats_role(address caller, address to, uint256 amount) public { _assumeValidCaller(caller); _assumeValidActor(to); @@ -44,7 +44,7 @@ contract B20MintRevertOrderTest is B20Test { } /// @notice With both PAUSE and ZERO-RECEIVER violated, PAUSE fires first. - /// @dev `whenNotPaused` modifier runs before `validReceiver` modifier. + /// @dev Pause modifier runs before the body's zero-receiver check. function test_mint_revertOrder_pause_beats_zeroRecipient(uint256 amount) public { _grantRole(B20Constants.MINT_ROLE, minter); _pause(IB20.PausableFeature.MINT); @@ -84,7 +84,7 @@ contract B20MintRevertOrderTest is B20Test { // --- Pairs where ROLE wins (PAUSE not violated) --- /// @notice With both ROLE and ZERO-RECEIVER violated, ROLE fires first. - /// @dev `onlyRole` modifier runs before `validReceiver` modifier. + /// @dev Role modifier runs before the body's zero-receiver check. function test_mint_revertOrder_role_beats_zeroRecipient(address caller, uint256 amount) public { _assumeValidCaller(caller); vm.assume(caller != admin); // admin holds DEFAULT_ADMIN_ROLE but not MINT_ROLE on fresh token @@ -132,7 +132,7 @@ contract B20MintRevertOrderTest is B20Test { // --- Pairs where ZERO-RECEIVER wins (PAUSE + ROLE satisfied) --- /// @notice With ZERO-RECEIVER and POLICY violated, ZERO-RECEIVER fires first. - /// @dev `validReceiver` modifier runs before the receiver-policy check in `_mint`. + /// @dev Zero-receiver check runs before the receiver-policy check in `_mint`. function test_mint_revertOrder_zeroRecipient_beats_policy(uint256 amount) public { _grantRole(B20Constants.MINT_ROLE, minter); _setPolicy(B20Constants.MINT_RECEIVER_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); @@ -143,7 +143,7 @@ contract B20MintRevertOrderTest is B20Test { } /// @notice With ZERO-RECEIVER and CAP violated, ZERO-RECEIVER fires first. - /// @dev `validReceiver` modifier runs before the supply-cap arithmetic in `_mint`. + /// @dev Zero-receiver check runs before the supply-cap arithmetic in `_mint`. function test_mint_revertOrder_zeroRecipient_beats_cap(uint256 amount) public { _grantRole(B20Constants.MINT_ROLE, minter); amount = bound(amount, 1, type(uint128).max); diff --git a/test/unit/B20Security/batch/batchBurn_revertOrder.t.sol b/test/unit/B20Security/batch/batchBurn_revertOrder.t.sol index a002208e..e0ff9126 100644 --- a/test/unit/B20Security/batch/batchBurn_revertOrder.t.sol +++ b/test/unit/B20Security/batch/batchBurn_revertOrder.t.sol @@ -8,11 +8,11 @@ import {B20SecurityTest} from "test/lib/B20SecurityTest.sol"; /// @title Differential check-order tests for `batchBurn` (security variant). /// -/// @notice **Canonical order (Solidity reference — modifier-led):** +/// @notice **Canonical order (Solidity reference):** /// 1. PAUSE (`whenNotPaused(BURN)` modifier) → `ContractPaused` /// 2. ROLE (`onlyRoleStrict(BURN_FROM_ROLE)` modifier) → `AccessControlUnauthorizedAccount` -/// 3. LENGTH_MISMATCH (body, `accounts.length != amounts.length`) → `LengthMismatch` -/// 4. EMPTY_BATCH (body, `accounts.length == 0`) → `EmptyBatch` +/// 3. LENGTH_MISMATCH (`accounts.length != amounts.length`) → `LengthMismatch` +/// 4. EMPTY_BATCH (`accounts.length == 0`) → `EmptyBatch` /// 5. BALANCE (per-element `_burnRaw`) → `InsufficientBalance` /// /// Some pairs are not reachable because the violations are mutually @@ -36,7 +36,7 @@ contract B20SecurityBatchBurnRevertOrderTest is B20SecurityTest { // --- Pairs where PAUSE wins (PAUSE is canonical first) --- /// @notice PAUSE beats ROLE. - /// @dev `whenNotPaused` modifier is listed before `onlyRoleStrict`. + /// @dev Pause modifier is listed before the role-strict modifier; fires first. function test_batchBurn_revertOrder_pause_beats_role(address caller, uint256 amount) public { _assumeValidCaller(caller); vm.assume(caller != admin); diff --git a/test/unit/B20Security/batch/batchMint_revertOrder.t.sol b/test/unit/B20Security/batch/batchMint_revertOrder.t.sol index d1ad4ed5..59200a0a 100644 --- a/test/unit/B20Security/batch/batchMint_revertOrder.t.sol +++ b/test/unit/B20Security/batch/batchMint_revertOrder.t.sol @@ -9,13 +9,13 @@ import {B20Constants} from "test/lib/mocks/MockB20.sol"; /// @title Differential check-order tests for `batchMint` (security variant). /// -/// @notice **Canonical order (Solidity reference — modifier-led):** +/// @notice **Canonical order (Solidity reference):** /// 1. PAUSE (`whenNotPaused(MINT)` modifier) → `ContractPaused` /// 2. ROLE (`onlyRole(MINT_ROLE)` modifier) → `AccessControlUnauthorizedAccount` -/// 3. LENGTH_MISMATCH (body, `recipients.length != amounts.length`) → `LengthMismatch` -/// 4. EMPTY_BATCH (body, `recipients.length == 0`) → `EmptyBatch` +/// 3. LENGTH_MISMATCH (`recipients.length != amounts.length`) → `LengthMismatch` +/// 4. EMPTY_BATCH (`recipients.length == 0`) → `EmptyBatch` /// 5. ZERO-RECEIVER (per-element inline guard) → `InvalidReceiver` -/// 6..N. Per-element `_mint` checks (see `mint_revertOrder.t.sol`): +/// 6..N. Per-element `_mint` body (see `mint_revertOrder.t.sol`): /// POLICY → CAP /// /// The pairs within `_mint`'s body are already pinned by @@ -73,7 +73,7 @@ contract B20SecurityBatchMintRevertOrderTest is B20SecurityTest { // --- Pairs where ROLE wins (PAUSE not violated) --- /// @notice ROLE beats LENGTH_MISMATCH. - /// @dev `onlyRole` modifier fires before the body's length check. + /// @dev Role modifier fires before the body's length check. function test_batchMint_revertOrder_role_beats_lengthMismatch(address caller) public { _assumeValidCaller(caller); vm.assume(caller != admin); From 14602cd1227f77906883b98d0fcc81dad9a15571 Mon Sep 17 00:00:00 2001 From: Amie Corso Date: Fri, 29 May 2026 12:06:31 -0700 Subject: [PATCH 3/5] review: extract _requireNonZeroActors helper for transfer family MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replaces the inline `InvalidReceiver` + `InvalidSender` checks in `transfer` / `transferFrom` / `transferWithMemo` / `transferFromWithMemo` with a single shared helper. Order preserved (receiver first, then sender). Mint paths keep the inline `InvalidReceiver` check — only one line per site. --- test/lib/mocks/MockB20.sol | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/test/lib/mocks/MockB20.sol b/test/lib/mocks/MockB20.sol index d2f4cf50..ef475bd0 100644 --- a/test/lib/mocks/MockB20.sol +++ b/test/lib/mocks/MockB20.sol @@ -161,8 +161,7 @@ contract MockB20 is IB20 { // ============================================================ function transfer(address to, uint256 amount) external whenNotPaused(PausableFeature.TRANSFER) returns (bool) { - if (to == address(0)) revert InvalidReceiver(to); - if (msg.sender == address(0)) revert InvalidSender(msg.sender); + _requireNonZeroActors(msg.sender, to); _transfer(msg.sender, to, amount); return true; } @@ -172,8 +171,7 @@ contract MockB20 is IB20 { whenNotPaused(PausableFeature.TRANSFER) returns (bool) { - if (to == address(0)) revert InvalidReceiver(to); - if (from == address(0)) revert InvalidSender(from); + _requireNonZeroActors(from, to); if (!_isPrivileged()) { // Allowance is consumed unconditionally outside the factory // bootstrap window. Matches OZ ERC20 and the Rust precompile, @@ -211,8 +209,7 @@ contract MockB20 is IB20 { whenNotPaused(PausableFeature.TRANSFER) returns (bool) { - if (to == address(0)) revert InvalidReceiver(to); - if (msg.sender == address(0)) revert InvalidSender(msg.sender); + _requireNonZeroActors(msg.sender, to); _transfer(msg.sender, to, amount); emit Memo(msg.sender, memo); return true; @@ -223,8 +220,7 @@ contract MockB20 is IB20 { whenNotPaused(PausableFeature.TRANSFER) returns (bool) { - if (to == address(0)) revert InvalidReceiver(to); - if (from == address(0)) revert InvalidSender(from); + _requireNonZeroActors(from, to); if (!_isPrivileged()) { _consumeAllowance(from, msg.sender, amount); if (msg.sender != from) { @@ -666,6 +662,15 @@ contract MockB20 is IB20 { } } + /// @dev Receiver-then-sender zero-address check shared by every + /// transfer-family entrypoint. Reverts `InvalidReceiver(to)` + /// before `InvalidSender(from)` so the precedence between the + /// two matches the canonical order. + function _requireNonZeroActors(address from, address to) internal pure { + if (to == address(0)) revert InvalidReceiver(to); + if (from == address(0)) revert InvalidSender(from); + } + /// @dev Pure mechanics: policy (with bootstrap bypass) + balance + /// effects. Pause + input validation are enforced upstream by /// every external caller (`transfer`, `transferFrom`, From 0741474a2f5247149a7c72d4574ea6ef3e094d41 Mon Sep 17 00:00:00 2001 From: Amie Corso Date: Fri, 29 May 2026 12:23:42 -0700 Subject: [PATCH 4/5] review: remove factory-bootstrap bypass on pause checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Pause now always fires, even from the factory during the bootstrap window. Unlike role + policy bypass (which have real bootstrap motivations — factory needs to act before having roles, factory needs to seed initial supply before policies are configured), pause has no equivalent need: pause defaults to 'nothing paused' at creation, and any pause state during bootstrap is explicitly opted into by the operator's initCalls. Start-paused configurations must sequence the `pause(...)` call last. - `whenNotPaused` modifier drops its `_isPrivileged()` wrapper. - `whenNotPausedStrict` deleted (now redundant — `whenNotPaused` is always strict). - `redeem` / `redeemWithMemo` switched from `whenNotPausedStrict(REDEEM)` to `whenNotPaused(REDEEM)`. - Contract-level natspec updated to call out the pause-no-bypass rule. All 592 tests still pass — no test relied on the factory bypass for pause, which is strong evidence the bypass was unintentional / unused. Rust precompile gets the matching change via BOP-217. --- test/lib/mocks/MockB20.sol | 28 +++++++++++++++++++--------- test/lib/mocks/MockB20Security.sol | 17 ++--------------- 2 files changed, 21 insertions(+), 24 deletions(-) diff --git a/test/lib/mocks/MockB20.sol b/test/lib/mocks/MockB20.sol index ef475bd0..672e1b01 100644 --- a/test/lib/mocks/MockB20.sol +++ b/test/lib/mocks/MockB20.sol @@ -36,7 +36,11 @@ import {B20Constants} from "src/lib/B20Constants.sol"; /// writes `initialized = true` directly (also via `vm.store`) /// once `initCalls` have run, closing the privileged window. /// Token invariants (supply-cap math, balance accounting) -/// are NOT bypassed during the window. +/// are NOT bypassed during the window. Pause is also NOT +/// bypassed: pause defaults to "nothing paused" at creation, +/// and any pause state during bootstrap is explicitly opted +/// into by the operator's initCalls, so start-paused +/// configurations must sequence the `pause(...)` call last. /// - Variant tokens (e.g. `MockB20Stablecoin`) extend by adding /// a disjoint storage namespace; the factory writes the /// variant-specific slots directly, no virtual hook needed. @@ -95,15 +99,21 @@ contract MockB20 is IB20 { // MODIFIERS // ============================================================ - /// @dev Reverts `ContractPaused(feature)` if `feature` is paused, - /// unless the call is from the factory in the bootstrap window - /// (see `_isPrivileged()`). Always listed FIRST in a gated - /// function's modifier set so pause precedence reads off the - /// signature directly (modifiers run left-to-right, body at - /// `_`). Token invariants (balance, supply cap) are never - /// bypassed and are enforced regardless. + /// @dev Reverts `ContractPaused(feature)` if `feature` is paused. + /// Always listed FIRST in a gated function's modifier set so + /// pause precedence reads off the signature directly (modifiers + /// run left-to-right, body at `_`). + /// + /// Unlike `onlyRole` and the policy checks, this guard does + /// NOT honor the factory bootstrap bypass: pause defaults to + /// "nothing paused" at creation, and any pause state during + /// bootstrap is explicitly opted into by the operator's + /// initCalls. There's no legitimate flow where the factory + /// pauses a feature in initCall N and then needs to use that + /// feature in initCall N+1 — start-paused configurations + /// should sequence the `pause(...)` call last. modifier whenNotPaused(PausableFeature feature) { - if (!_isPrivileged() && _isPaused(feature)) revert ContractPaused(feature); + if (_isPaused(feature)) revert ContractPaused(feature); _; } diff --git a/test/lib/mocks/MockB20Security.sol b/test/lib/mocks/MockB20Security.sol index 2f119c06..50d72034 100644 --- a/test/lib/mocks/MockB20Security.sol +++ b/test/lib/mocks/MockB20Security.sol @@ -115,19 +115,6 @@ contract MockB20Security is MockB20, IB20Security { _; } - /// @dev Like the base `whenNotPaused` but without the factory - /// bootstrap bypass: the pause check is unconditional, even - /// for the factory in the init window. Reserved for variant - /// paths that deliberately reject the bypass — currently just - /// the `redeem` / `redeemWithMemo` family, which is - /// holder-initiated and has no legitimate init-time use case - /// (see this contract's natspec). Lives here, not on the - /// base, because no base function needs the stricter gate. - modifier whenNotPausedStrict(PausableFeature feature) { - if (_isPaused(feature)) revert ContractPaused(feature); - _; - } - // ============================================================ // ANNOUNCEMENTS // ============================================================ @@ -229,12 +216,12 @@ contract MockB20Security is MockB20, IB20Security { // REDEMPTION // ============================================================ - function redeem(uint256 amount) external whenNotPausedStrict(PausableFeature.REDEEM) { + function redeem(uint256 amount) external whenNotPaused(PausableFeature.REDEEM) { uint256 ratio = _redeemBurn(amount); emit Redeemed(msg.sender, amount, ratio); } - function redeemWithMemo(uint256 amount, bytes32 memo) external whenNotPausedStrict(PausableFeature.REDEEM) { + function redeemWithMemo(uint256 amount, bytes32 memo) external whenNotPaused(PausableFeature.REDEEM) { uint256 ratio = _redeemBurn(amount); // Order matters: Transfer (in _redeemBurn), then Memo, then Redeemed. emit Memo(msg.sender, memo); From 5aa3766c402a80bff0da35f00b6760b2416676be Mon Sep 17 00:00:00 2001 From: katzman Date: Fri, 29 May 2026 17:40:09 -0700 Subject: [PATCH 5/5] remove redundant pause check --- test/lib/mocks/MockB20Security.sol | 1 - 1 file changed, 1 deletion(-) diff --git a/test/lib/mocks/MockB20Security.sol b/test/lib/mocks/MockB20Security.sol index 50d72034..81ef02ca 100644 --- a/test/lib/mocks/MockB20Security.sol +++ b/test/lib/mocks/MockB20Security.sol @@ -295,7 +295,6 @@ contract MockB20Security is MockB20, IB20Security { /// path that the factory has no legitimate reason to invoke /// during bootstrap, so the bypass is omitted by design. function _redeemBurn(uint256 amount) internal returns (uint256 ratio) { - if (_isPaused(PausableFeature.REDEEM)) revert ContractPaused(PausableFeature.REDEEM); MockB20RedeemStorage.Layout storage $ = MockB20RedeemStorage.layout(); uint64 REDEEMSenderPolicyId = $.redeemPolicyIds.sender; if (!IPolicyRegistry(POLICY_REGISTRY).isAuthorized(REDEEMSenderPolicyId, msg.sender)) {