fix: gate signed ERC20 transfers with allowance lockdown (Cantina 3.1.2)#63
fix: gate signed ERC20 transfers with allowance lockdown (Cantina 3.1.2)#63re1ro wants to merge 1 commit into
Conversation
A signed permit carrying a TransferERC20 op (modeOrExpiration == 0) called _transferFrom directly with no lock check, so lockdown() could not stop it. Reuse the existing owner->tokenKey->spender allowance lock: before a signed transfer, revert AllowanceLocked if the recipient-keyed allowance is locked. An owner blocks signed transfers to a recipient via lockdown(token, recipient). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
OverviewThis PR introduces a check in QA & Test CoverageNo issues found in this domain. Web SecuritySkipped: not applicable to this PR — PR addresses blockchain-specific permit/signature security, not web security concerns. Blockchain SecurityNo issues found in this domain. PerformanceSkipped: not applicable to this PR — PR adds a single conditional check with no algorithmic changes or performance implications. Stats
Automated review by Eco's specialized review team. Architecture, business logic, and correctness remain with the human reviewer. |
There was a problem hiding this comment.
Pull request overview
Fixes Cantina finding 3.1.2 by ensuring TransferERC20 permit operations respect the existing allowance lock (“lockdown”) mechanism, preventing signed transfer permits from bypassing lockdown.
Changes:
- Added a
LOCKED_ALLOWANCEgate in theTransferERC20branch of_processChainPermitsto revert withAllowanceLockedwhen the recipient-keyed slot is locked. - Added regression tests covering the locked-recipient revert case (and a happy-path transfer case).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/Permit3.sol | Adds an allowance-lock check before executing TransferERC20 permit transfers. |
| test/Permit3.t.sol | Adds tests for transfer-permit behavior with/without recipient lockdown. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Gate signed transfers with the existing allowance lock mechanism (Cantina 3.1.2). | ||
| // The recipient (p.account) is the only counterparty committed in the signature, so an | ||
| // owner blocks a signed transfer to a given recipient via lockdown(token, recipient), | ||
| // which writes the same owner->tokenKey->recipient allowance slot read here. |
| function test_permitTransferFrom_succeedsWithoutLockdown() public { | ||
| // Happy path with no lockdown still transfers successfully. | ||
| IPermit3.ChainPermits memory chainPermits = _createBasicTransferPermit(); | ||
|
|
||
| deal(address(token), recipient, 0); | ||
|
|
||
| uint48 deadline = uint48(block.timestamp + 1 hours); | ||
| uint48 timestamp = uint48(block.timestamp); | ||
| bytes memory signature = _signPermit(chainPermits, deadline, timestamp, SALT); | ||
|
|
||
| permit3.permit(owner, SALT, deadline, timestamp, chainPermits.permits, signature); | ||
|
|
||
| assertEq(token.balanceOf(recipient), AMOUNT); | ||
| } |
Summary
Addresses Cantina finding 3.1.2 (High). A signed
permit(...)carrying aTransferERC20op (modeOrExpiration == 0) called_transferFromdirectly in_processChainPermitswith no lock check, solockdown()could not stop it.This change adds a lock gate to the signed transfer path by reusing the existing
owner -> tokenKey -> spenderallowance lock mechanism (product decision: reuse, not a new mapping). Before performing the signed transfer it revertsAllowanceLockedif the recipient-keyed allowance is locked.Chosen key semantics
lockdown(token, recipient)blocks signed transfers to that recipient, where recipient =p.account.For a signed transfer, the only counterparty committed in the signature is the recipient
p.account—msg.senderis just an arbitrary relayer, unknown at lock time. So an owner blocks a signed transfer to a given recipient by callinglockdown([{token, recipient}]).Key derivation lines up.
lockdownwrites tobytes32(uint256(uint160(token)))(PermitBase.sol:159). The transfer branch requiresuint256(p.tokenKey) >> 160 == 0(Permit3.sol:338) and derivestoken = address(uint160(uint256(p.tokenKey))), so for a valid transfer opp.tokenKey == bytes32(uint256(uint160(token)))exactly. The check readsallowances[owner][p.tokenKey][p.account]— the same slotlockdown(token, recipient)writes. Confirmed they match.The signature still authorizes the transfer; this only adds a lock gate. The allowance is not consumed/decremented, and the happy path is unchanged when nothing is locked.
Residual limitation (explicit)
This does NOT stop a leaked signature that sends to an arbitrary/unknown recipient, because the owner cannot know the recipient at lock time. The general kill-switch for a leaked signature remains
invalidateNonces(salt).This change is defense-in-depth / parity with the NFT
CollectionLockedfix, not a complete mitigation of the leaked-signature threat.Tests
Added regression tests in
test/Permit3.t.sol:test_permitTransferFrom_blockedByLockdownOnRecipient— owner signs a transfer torecipient, callslockdown([{token, recipient}]), the permit revertsAllowanceLocked, recipient balance unchanged.test_permitTransferFrom_succeedsWithoutLockdown— happy path still transfers successfully.Full suite:
forge test-> 215 passed, 0 failed.🤖 Generated with Claude Code