fix(b20): consume allowance on privileged transferFrom (L-04)

ilikesymmetry · claude · ilikesymmetry · commit e95e1e3df94d · 2026-06-02T10:38:18.000-04:00
Audit finding L-04 (BOP-230): allowance deduction in privileged
transferFrom diverged between the Rust precompile and the Solidity
reference. The Rust precompile consumes the spender's allowance on every
transferFrom — there is no `privileged` exception for allowance
accounting; a privileged caller only bypasses the executor-policy check
(an infinite allowance is never decremented). The Solidity reference
instead skipped the entire allowance block during the factory bootstrap
window, neither checking nor decrementing.

Align the Solidity reference to the Rust behavior: consume allowance
unconditionally, gating only the executor-policy check on the bootstrap
window.

- transferFrom / transferFromWithMemo: call _consumeAllowance
  unconditionally; keep the executor-policy check behind
  `!_isPrivileged() &amp;&amp; msg.sender != from`
- Correct the _transfer natspec to reflect that allowance consumption is
  unconditional and only the executor-policy check honors the bypass
- Add fuzz regression tests (transferFrom + transferFromWithMemo): a
  privileged call decrements allowance by the spent amount, and still
  reverts InsufficientAllowance when the allowance is insufficient

Co-Authored-By: Claude Opus 4.8 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/test/lib/mocks/MockB20.sol b/test/lib/mocks/MockB20.sol
@@ -186,20 +186,21 @@ abstract contract MockB20 is IB20 {
         returns (bool)
     {
         _requireNonZeroActors(from, to);
-        if (!_isPrivileged()) {
-            // Allowance is consumed unconditionally outside the factory
-            // bootstrap window. Matches OZ ERC20 and the Rust precompile,
-            // both of which carve no exception for `msg.sender == from`.
-            _consumeAllowance(from, msg.sender, amount);
-            if (msg.sender != from) {
-                // Read the executor policy ID out of the transfer-side packed
-                // slot. Cold here; warm by the time _transfer reads the same
-                // slot for sender + receiver. Skipped when the caller is the
-                // owner — sender-policy already covers `from` inside _transfer.
-                uint64 executorPolicyId = MockB20Storage.layout().transferPolicyIds.executor;
-                if (!IPolicyRegistry(POLICY_REGISTRY).isAuthorized(executorPolicyId, msg.sender)) {
-                    revert PolicyForbids(TRANSFER_EXECUTOR_POLICY, executorPolicyId);
-                }
+        // Allowance is consumed unconditionally — including during the factory
+        // bootstrap window (`_isPrivileged()`). Matches the Rust precompile,
+        // which carves no `privileged` exception for allowance accounting
+        // (BOP-230 / L-04); only the executor-policy check below is bypassed
+        // for a privileged caller. An infinite allowance is still not
+        // decremented (handled inside `_consumeAllowance`).
+        _consumeAllowance(from, msg.sender, amount);
+        if (!_isPrivileged() && msg.sender != from) {
+            // Read the executor policy ID out of the transfer-side packed
+            // slot. Cold here; warm by the time _transfer reads the same
+            // slot for sender + receiver. Skipped when the caller is the
+            // owner — sender-policy already covers `from` inside _transfer.
+            uint64 executorPolicyId = MockB20Storage.layout().transferPolicyIds.executor;
+            if (!IPolicyRegistry(POLICY_REGISTRY).isAuthorized(executorPolicyId, msg.sender)) {
+                revert PolicyForbids(TRANSFER_EXECUTOR_POLICY, executorPolicyId);
             }
         }
         _transfer(from, to, amount);
@@ -235,13 +236,15 @@ abstract contract MockB20 is IB20 {
         returns (bool)
     {
         _requireNonZeroActors(from, to);
-        if (!_isPrivileged()) {
-            _consumeAllowance(from, msg.sender, amount);
-            if (msg.sender != from) {
-                uint64 executorPolicyId = MockB20Storage.layout().transferPolicyIds.executor;
-                if (!IPolicyRegistry(POLICY_REGISTRY).isAuthorized(executorPolicyId, msg.sender)) {
-                    revert PolicyForbids(TRANSFER_EXECUTOR_POLICY, executorPolicyId);
-                }
+        // Allowance is consumed unconditionally — including during the factory
+        // bootstrap window — matching the Rust precompile (BOP-230 / L-04).
+        // Only the executor-policy check below is bypassed for a privileged
+        // caller; infinite allowance is still not decremented.
+        _consumeAllowance(from, msg.sender, amount);
+        if (!_isPrivileged() && msg.sender != from) {
+            uint64 executorPolicyId = MockB20Storage.layout().transferPolicyIds.executor;
+            if (!IPolicyRegistry(POLICY_REGISTRY).isAuthorized(executorPolicyId, msg.sender)) {
+                revert PolicyForbids(TRANSFER_EXECUTOR_POLICY, executorPolicyId);
             }
         }
         _transfer(from, to, amount);
@@ -702,10 +705,12 @@ abstract contract MockB20 is IB20 {
     ///      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.
+    ///      additionally consume the allowance (unconditionally —
+    ///      including in the bootstrap window, matching the Rust
+    ///      precompile, see BOP-230 / L-04) and check the executor
+    ///      policy in their bodies before calling here; only the
+    ///      executor-policy check honors the bootstrap bypass,
+    ///      consistent with the sender/receiver 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
diff --git a/test/unit/B20/erc20/transferFrom.t.sol b/test/unit/B20/erc20/transferFrom.t.sol
@@ -364,4 +364,91 @@ contract B20TransferFromTest is B20Test {
 
         assertEq(token.balanceOf(to), amount, "transfer must succeed despite blocked executor policy");
     }
+
+    // ============================================================
+    //        REGRESSION: PRIVILEGED BOOTSTRAP ALLOWANCE (BOP-230 / L-04)
+    // ============================================================
+    //
+    // A privileged transferFrom (factory caller during the bootstrap
+    // window) consumes allowance exactly like an ordinary transferFrom:
+    // the allowance is both CHECKED and DECREMENTED. The Rust precompile
+    // carves no `privileged` exception for allowance accounting — only the
+    // executor-policy check is bypassed for a privileged caller. Before
+    // BOP-230 the Solidity reference skipped the entire allowance block
+    // during the window (neither checking nor decrementing); these tests
+    // pin the corrected, Rust-aligned behavior.
+    //
+    // To enter the window: set allowance/balance while still initialized,
+    // then reopen the bootstrap window via vm.store on the initialized
+    // slot, then call as the factory. The privileged spender is the
+    // factory address.
+
+    /// @notice Verifies a privileged transferFrom reverts InsufficientAllowance when allowance is below the spend
+    /// @dev Pins that the allowance check is unconditional — a privileged caller is still
+    ///      rejected for insufficient allowance; before BOP-230 the privileged path skipped
+    ///      the check entirely. Regression: BOP-230 / L-04.
+    function test_transferFrom_revert_privileged_insufficientAllowance(
+        address from,
+        address to,
+        uint256 allowanceAmount,
+        uint256 spendAmount
+    ) public {
+        _assumeValidActor(from);
+        _assumeValidActor(to);
+        allowanceAmount = bound(allowanceAmount, 0, type(uint128).max - 1);
+        spendAmount = bound(spendAmount, allowanceAmount + 1, type(uint128).max);
+
+        _mint(from, spendAmount);
+        vm.prank(from);
+        token.approve(address(factory), allowanceAmount);
+
+        // Reopen the factory bootstrap window so the factory caller is privileged.
+        vm.store(address(token), MockB20Storage.initializedSlot(), bytes32(0));
+
+        vm.prank(address(factory));
+        vm.expectRevert(
+            abi.encodeWithSelector(IB20.InsufficientAllowance.selector, address(factory), allowanceAmount, spendAmount)
+        );
+        token.transferFrom(from, to, spendAmount);
+    }
+
+    /// @notice Verifies a privileged transferFrom decrements allowance by the spent amount
+    /// @dev Allowance is consumed during the bootstrap window exactly as outside it, matching
+    ///      the Rust precompile. Before BOP-230 the privileged path left the allowance
+    ///      untouched. Regression: BOP-230 / L-04.
+    function test_transferFrom_success_privileged_decrementsAllowance(
+        address from,
+        address to,
+        uint256 allowanceAmount,
+        uint256 spendAmount
+    ) public {
+        _assumeValidActor(from);
+        _assumeValidActor(to);
+        vm.assume(from != to);
+        allowanceAmount = bound(allowanceAmount, 1, type(uint128).max);
+        vm.assume(allowanceAmount != type(uint256).max);
+        spendAmount = bound(spendAmount, 0, allowanceAmount);
+
+        _mint(from, spendAmount);
+        vm.prank(from);
+        token.approve(address(factory), allowanceAmount);
+
+        // Reopen the factory bootstrap window so the factory caller is privileged.
+        vm.store(address(token), MockB20Storage.initializedSlot(), bytes32(0));
+
+        vm.prank(address(factory));
+        token.transferFrom(from, to, spendAmount);
+
+        assertEq(
+            token.allowance(from, address(factory)),
+            allowanceAmount - spendAmount,
+            "privileged transferFrom must decrement allowance by the spent amount"
+        );
+        assertEq(token.balanceOf(to), spendAmount, "to must receive the spent amount");
+        assertEq(
+            uint256(vm.load(address(token), MockB20Storage.allowanceSlot(from, address(factory)))),
+            allowanceAmount - spendAmount,
+            "allowances[from][factory] slot must reflect the consumed amount"
+        );
+    }
 }
diff --git a/test/unit/B20/memo/transferFromWithMemo.t.sol b/test/unit/B20/memo/transferFromWithMemo.t.sol
@@ -177,4 +177,85 @@ contract B20TransferFromWithMemoTest is B20Test {
         );
         assertEq(token.balanceOf(to), amount, "to must receive the transferred amount");
     }
+
+    // ============================================================
+    //        REGRESSION: PRIVILEGED BOOTSTRAP ALLOWANCE (BOP-230 / L-04)
+    // ============================================================
+    //
+    // Mirrors the transferFrom regression: a privileged transferFromWithMemo
+    // (factory caller during the bootstrap window) consumes allowance exactly
+    // like an ordinary call — both checked and decremented — while only the
+    // executor-policy check stays bypassed, matching the Rust precompile. See
+    // transferFrom.t.sol for the canonical discussion.
+
+    /// @notice Verifies a privileged transferFromWithMemo reverts InsufficientAllowance when allowance is below the spend
+    /// @dev Pins that the allowance check is unconditional — a privileged caller is still
+    ///      rejected for insufficient allowance; before BOP-230 the privileged path skipped
+    ///      the check entirely. Regression: BOP-230 / L-04.
+    function test_transferFromWithMemo_revert_privileged_insufficientAllowance(
+        address from,
+        address to,
+        uint256 allowanceAmount,
+        uint256 spendAmount,
+        bytes32 memo
+    ) public {
+        _assumeValidActor(from);
+        _assumeValidActor(to);
+        allowanceAmount = bound(allowanceAmount, 0, type(uint128).max - 1);
+        spendAmount = bound(spendAmount, allowanceAmount + 1, type(uint128).max);
+
+        _mint(from, spendAmount);
+        vm.prank(from);
+        token.approve(address(factory), allowanceAmount);
+
+        // Reopen the factory bootstrap window so the factory caller is privileged.
+        vm.store(address(token), MockB20Storage.initializedSlot(), bytes32(0));
+
+        vm.prank(address(factory));
+        vm.expectRevert(
+            abi.encodeWithSelector(IB20.InsufficientAllowance.selector, address(factory), allowanceAmount, spendAmount)
+        );
+        token.transferFromWithMemo(from, to, spendAmount, memo);
+    }
+
+    /// @notice Verifies a privileged transferFromWithMemo decrements allowance by the spent amount
+    /// @dev Allowance is consumed during the bootstrap window exactly as outside it, matching
+    ///      the Rust precompile. Before BOP-230 the privileged path left the allowance
+    ///      untouched. Regression: BOP-230 / L-04.
+    function test_transferFromWithMemo_success_privileged_decrementsAllowance(
+        address from,
+        address to,
+        uint256 allowanceAmount,
+        uint256 spendAmount,
+        bytes32 memo
+    ) public {
+        _assumeValidActor(from);
+        _assumeValidActor(to);
+        vm.assume(from != to);
+        allowanceAmount = bound(allowanceAmount, 1, type(uint128).max);
+        vm.assume(allowanceAmount != type(uint256).max);
+        spendAmount = bound(spendAmount, 0, allowanceAmount);
+
+        _mint(from, spendAmount);
+        vm.prank(from);
+        token.approve(address(factory), allowanceAmount);
+
+        // Reopen the factory bootstrap window so the factory caller is privileged.
+        vm.store(address(token), MockB20Storage.initializedSlot(), bytes32(0));
+
+        vm.prank(address(factory));
+        token.transferFromWithMemo(from, to, spendAmount, memo);
+
+        assertEq(
+            token.allowance(from, address(factory)),
+            allowanceAmount - spendAmount,
+            "privileged transferFromWithMemo must decrement allowance by the spent amount"
+        );
+        assertEq(token.balanceOf(to), spendAmount, "to must receive the spent amount");
+        assertEq(
+            uint256(vm.load(address(token), MockB20Storage.allowanceSlot(from, address(factory)))),
+            allowanceAmount - spendAmount,
+            "allowances[from][factory] slot must reflect the consumed amount"
+        );
+    }
 }
