From 83dff6b8cdd560242f464a30219961e78c4fc0a4 Mon Sep 17 00:00:00 2001 From: katzman Date: Wed, 27 May 2026 15:46:56 -0700 Subject: [PATCH] fix: consume allowance on transferFrom when caller == from MockB20.transferFrom and transferFromWithMemo gated _consumeAllowance behind `msg.sender != from`, silently skipping allowance burn when the owner was the caller. OZ ERC20 and the Rust precompile both carve no such exception. Lift _consumeAllowance to fire unconditionally outside the factory bootstrap window; keep the executor-policy check gated on `msg.sender != from` (sender-policy already covers `from`). Regression tests pin the corrected behavior on both transferFrom and transferFromWithMemo: self-caller now reverts InsufficientAllowance without a self-approval and consumes the slot when one exists. Co-Authored-By: Claude Opus 4.7 --- test/lib/mocks/MockB20.sol | 30 ++++--- test/unit/B20/erc20/transferFrom.t.sol | 80 +++++++++++++++++++ test/unit/B20/memo/transferFromWithMemo.t.sol | 54 +++++++++++++ 3 files changed, 153 insertions(+), 11 deletions(-) diff --git a/test/lib/mocks/MockB20.sol b/test/lib/mocks/MockB20.sol index 14c78e5..4427f39 100644 --- a/test/lib/mocks/MockB20.sol +++ b/test/lib/mocks/MockB20.sol @@ -154,14 +154,20 @@ contract MockB20 is IB20 { } function transferFrom(address from, address to, uint256 amount) external returns (bool) { - if (!_isPrivileged() && msg.sender != from) { + 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); - // 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. - uint64 executorPolicyId = MockB20Storage.layout().transferPolicyIds.executor; - if (!IPolicyRegistry(POLICY_REGISTRY).isAuthorized(executorPolicyId, msg.sender)) { - revert PolicyForbids(TRANSFER_EXECUTOR_POLICY, executorPolicyId); + 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); + } } } _transfer(from, to, amount); @@ -187,11 +193,13 @@ contract MockB20 is IB20 { } function transferFromWithMemo(address from, address to, uint256 amount, bytes32 memo) external returns (bool) { - if (!_isPrivileged() && msg.sender != from) { + if (!_isPrivileged()) { _consumeAllowance(from, msg.sender, amount); - uint64 executorPolicyId = MockB20Storage.layout().transferPolicyIds.executor; - if (!IPolicyRegistry(POLICY_REGISTRY).isAuthorized(executorPolicyId, msg.sender)) { - revert PolicyForbids(TRANSFER_EXECUTOR_POLICY, executorPolicyId); + if (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); diff --git a/test/unit/B20/erc20/transferFrom.t.sol b/test/unit/B20/erc20/transferFrom.t.sol index 59f83ed..138fdc6 100644 --- a/test/unit/B20/erc20/transferFrom.t.sol +++ b/test/unit/B20/erc20/transferFrom.t.sol @@ -280,4 +280,84 @@ contract B20TransferFromTest is B20Test { vm.prank(caller); assertTrue(token.transferFrom(from, to, amount), "transferFrom must return true"); } + + // ============================================================ + // REGRESSION: SELF-CALLER ALLOWANCE + // ============================================================ + // + // transferFrom MUST consume allowance even when msg.sender == from. + // OZ ERC20 and the Rust precompile both carve no exception for the + // self-caller case, so the mock must not either. Earlier the mock + // gated `_consumeAllowance` behind `msg.sender != from`, silently + // letting an owner-as-caller transfer without burning approval — + // these tests pin the corrected behavior. + + /// @notice Verifies transferFrom reverts InsufficientAllowance when caller == from and no self-approval exists + /// @dev Regression: without this, msg.sender == from skipped allowance consumption entirely. + function test_transferFrom_revert_selfCaller_insufficientAllowance(address from, address to, uint256 amount) + public + { + _assumeValidActor(from); + _assumeValidActor(to); + amount = bound(amount, 1, type(uint256).max); + // No self-approval; allowance[from][from] is 0. + + vm.prank(from); + vm.expectRevert(abi.encodeWithSelector(IB20.InsufficientAllowance.selector, from, 0, amount)); + token.transferFrom(from, to, amount); + } + + /// @notice Verifies transferFrom decreases self-allowance by the spent amount when caller == from + /// @dev Regression: matches OZ / Rust — allowance is spent against `allowances[from][from]`. + function test_transferFrom_success_selfCaller_decreasesAllowance( + 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, 1, allowanceAmount); + + _mint(from, spendAmount); + vm.prank(from); + token.approve(from, allowanceAmount); + + vm.prank(from); + token.transferFrom(from, to, spendAmount); + + assertEq( + token.allowance(from, from), allowanceAmount - spendAmount, "self-allowance must decrease by spent amount" + ); + assertEq( + uint256(vm.load(address(token), MockB20Storage.allowanceSlot(from, from))), + allowanceAmount - spendAmount, + "allowances[from][from] slot must reflect the consumed amount" + ); + assertEq(token.balanceOf(to), spendAmount, "to must receive the spent amount"); + } + + /// @notice Verifies transferFrom with self-caller skips the executor policy check + /// @dev Self-caller is not an executor distinct from `from`; sender-policy already + /// covers `from` inside _transfer. Executor policy MUST NOT fire — pins the + /// one carve-out we intentionally keep around `msg.sender == from`. + function test_transferFrom_success_selfCaller_skipsExecutorPolicy(address from, address to, uint256 amount) public { + _assumeValidActor(from); + _assumeValidActor(to); + vm.assume(from != to); + amount = bound(amount, 1, type(uint128).max); + + _mint(from, amount); + vm.prank(from); + token.approve(from, amount); + _setPolicy(B20Constants.TRANSFER_EXECUTOR_POLICY, PolicyRegistryConstants.ALWAYS_BLOCK_ID); + + vm.prank(from); + token.transferFrom(from, to, amount); + + assertEq(token.balanceOf(to), amount, "transfer must succeed despite blocked executor policy"); + } } diff --git a/test/unit/B20/memo/transferFromWithMemo.t.sol b/test/unit/B20/memo/transferFromWithMemo.t.sol index 0adf8dd..15c202f 100644 --- a/test/unit/B20/memo/transferFromWithMemo.t.sol +++ b/test/unit/B20/memo/transferFromWithMemo.t.sol @@ -121,4 +121,58 @@ contract B20TransferFromWithMemoTest is B20Test { vm.prank(caller); assertTrue(token.transferFromWithMemo(from, to, amount, memo), "transferFromWithMemo must return true"); } + + // ============================================================ + // REGRESSION: SELF-CALLER ALLOWANCE + // ============================================================ + // + // Mirrors the transferFrom regression: msg.sender == from MUST still + // consume allowance. See transferFrom.t.sol for the canonical + // regression discussion. + + /// @notice Verifies transferFromWithMemo reverts InsufficientAllowance when caller == from and no self-approval exists + /// @dev Regression: inherits the corrected allowance-consumption invariant from transferFrom. + function test_transferFromWithMemo_revert_selfCaller_insufficientAllowance( + address from, + address to, + uint256 amount, + bytes32 memo + ) public { + _assumeValidActor(from); + _assumeValidActor(to); + amount = bound(amount, 1, type(uint256).max); + + vm.prank(from); + vm.expectRevert(abi.encodeWithSelector(IB20.InsufficientAllowance.selector, from, 0, amount)); + token.transferFromWithMemo(from, to, amount, memo); + } + + /// @notice Verifies transferFromWithMemo decreases self-allowance by the spent amount when caller == from + /// @dev Regression: spend-tracking parity with transferFrom on the self-caller path. + function test_transferFromWithMemo_success_selfCaller_decreasesAllowance( + address from, + address to, + uint256 amount, + bytes32 memo + ) public { + _assumeValidActor(from); + _assumeValidActor(to); + vm.assume(from != to); + amount = bound(amount, 1, type(uint128).max); + + _mint(from, amount); + vm.prank(from); + token.approve(from, amount); + + vm.prank(from); + token.transferFromWithMemo(from, to, amount, memo); + + assertEq(token.allowance(from, from), 0, "self-allowance must be consumed"); + assertEq( + uint256(vm.load(address(token), MockB20Storage.allowanceSlot(from, from))), + 0, + "allowances[from][from] slot must reflect the consumption" + ); + assertEq(token.balanceOf(to), amount, "to must receive the transferred amount"); + } }