feat(SpokePoolPeriphery): support ERC-6492 counterfactual contract-wallet signers#1467
Conversation
The bytes-signature ERC-3009 entry points (depositWithAuthorizationBytes, swapAndBridgeWithAuthorizationBytes) delegate signature verification to the token (e.g. USDC v2.2), which supports EIP-1271 contract wallets but has no concept of ERC-6492. A *counterfactual* (not-yet-deployed) contract wallet therefore cannot authorize a transfer, since there is no code at the signer's address to run isValidSignature on. We cannot change the token's verification, so we replicate only the deploy half of the ERC-6492 flow: a new ERC6492SignatureHandler base contract detects the ERC-6492 magic suffix and, if present, runs the wrapped factory call to materialize the wallet before handing the unwrapped inner signature to the token. The token's own EIP-1271 check then succeeds against the deployed signer. The factory target/calldata are attacker-controlled, so the prepare call is routed through the canonical Multicall3 singleton (supplied as an immutable constructor arg) — this contract is never the msg.sender of that call, so it cannot be tricked into abusing its own allowances. The call uses requireSuccess=false so an already-deployed wallet is tolerated; the token remains the source of truth on validity. Mirrors Coinbase's ERC6492SignatureHandler (base/commerce-payments). - New IMulticall3 interface and ERC6492SignatureHandler base contract. - SpokePoolPeriphery inherits it; constructor now takes the Multicall3 address. - Deploy script + Constants.getMulticall3() pass the canonical 0xcA11...CA11. - Non-ERC-6492 signatures pass through unchanged (existing bytes/EIP-1271/EOA tests continue to exercise the passthrough path). Test plan: - yarn build-evm-foundry - yarn test-evm-foundry -- --match-contract "SpokePoolPeripheryTest|TransferProxy" 53 passed (49 existing + 4 new) - New tests: counterfactual-wallet deposit & swap-and-bridge, already-deployed wallet with wrapped sig (idempotent prepare), and failing-prepare tolerance. - yarn lint-fix Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7836ec32d7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
🤖 Babysitting this PR — picked up the assignment. Working from |
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Self-review on the babysit pass.
- Addressed codex's P2 thread by updating the Tron deployment script (and the Tron README) to accept and forward the new
multicall3constructor argument. Without it any deployment viayarn tron-deploy-spoke-pool-periphery <permit2>would have reverted in the constructor's zero-address check. - Otherwise the diff looks solid: the design routes the attacker-controlled prepare call through the canonical Multicall3 so the periphery is never
msg.sender, therequireSuccess=falsetolerates already-deployed wallets, thenonReentrantguard on the entry points prevents prepare-driven reentry, and theabstracthandler doesn't pretend to be a verifier. - One small nit on
_handleERC6492Signatureflagged inline — the manual byte-by-byte copy loop is correct but gas-wasteful and easy to replace with a memory copy. Non-blocking.
The new SpokePoolPeriphery constructor requires a Multicall3 address; the Tron deployment script was still encoding only `<permit2>`, so the documented `yarn tron-deploy-spoke-pool-periphery` flow would have reverted in the constructor's zero-address check. Take a second Tron Base58Check address, validate it, and ABI-encode it as the second arg. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Babysit pass complete. Reviewer threads (1 unresolved → 0)
Self-review
Commits this pass
Push will be handled by the checkpoint; the codex resolve is queued and will fire on push success. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Acknowledged codex's clean re-review of |
|
@droplet-rl make the optimization you called out. |
Take the signature as calldata and decode the (factory, factoryCalldata, innerSignature) tuple directly from a calldata slice. Drops the entry calldata-to-memory copy and the byte-by-byte loop into a memory scratch buffer that used to precede abi.decode. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Done — applied the optimization in |
|
Ask again to re-run the unfinished work. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Hooray! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Codex came back clean on the optimization commit ( |
|
@droplet-rl please make the optimization you mentioned in your review. |
|
Already in — pushed as The handler now takes Nothing more to do this pass; let me know if you had a different optimization in mind. |
fusmanii
left a comment
fusmanii
left a comment
There was a problem hiding this comment.
Looks good to me, just two (optional) comments
| constructor( | ||
| IPermit2 _permit2, | ||
| address _multicall3 | ||
| ) EIP712("ACROSS-PERIPHERY", "1.0.0") ERC6492SignatureHandler(_multicall3) { |
There was a problem hiding this comment.
should we change the version here?
There was a problem hiding this comment.
I don't think a bump is required here, but it's your call. The "1.0.0" version feeds the periphery's own EIP-712 domain (_domainSeparatorV4), which is used to verify the DepositData/SwapAndDepositData typed-data signatures the periphery itself checks. This PR doesn't change any of those typed-data structs — the ERC-6492 work only deploys the counterfactual signer before handing the unwrapped signature to the token, and the token's receiveWithAuthorization verifies against the token's EIP-712 domain (e.g. USDC's), not ours. So nothing the periphery signs over changed.
There's also no replay concern motivating a bump: this is a fresh deployment (new bytecode + new multicall3 constructor arg → new address), so the domain separator already differs from the existing periphery by verifyingContract. A version bump would only be warranted if we'd changed the typed-data schema while keeping the same address.
Happy to bump to "1.1.0" purely as a human-facing signal of the redeploy if you'd prefer — just let me know and I'll add it. Leaving this open for your call.
…l 32 Replace the hard-coded 32 magic-suffix length with _ERC6492_MAGIC_VALUE.length (a compile-time constant for the bytes32) so the length check and the calldata slices are self-documenting and stay in sync with the constant. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Babysit pass done — handled fusmanii's two (optional) review comments. 1. 2. Self-review: no issues. Walked the handler edge cases (exact-32-byte input → Verification: Commits this pass:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 13f38536de
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // same address on virtually all EVM chains (see https://github.com/mds1/multicall). Used by the | ||
| // SpokePoolPeriphery to route ERC-6492 prepare/deploy calls through a neutral contract. | ||
| function getMulticall3() public pure returns (address) { | ||
| return 0xcA11bde05977b3631167028862bE2a173976CA11; |
There was a problem hiding this comment.
When this deploy helper is used for ZK_SYNC (chain 324; generated/constants.json already has a Permit2 entry, so the script can target it), this hard-coded address is not the Multicall3 deployment—the Multicall3 deployment table lists zkSync Era at 0xF9cda624FBC7e059355ce98a31693d299FACd963, while SpokePoolPeriphery now rejects a non-contract _multicall3 in its constructor. That makes the deployment script revert on zkSync-like chains with non-standard Multicall3 addresses; please look the address up per chain instead of always returning 0xcA11....
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Made getMulticall3 chain-aware: it now takes a chainId and returns the zkSync-stack Multicall3 deployment (0xF9cda624FBC7e059355ce98a31693d299FACd963) for ZK_SYNC (324) and ZK_SYNC_SEPOLIA (300), falling back to the canonical 0xcA11…CA11 everywhere else. DeploySpokePoolPeriphery.s.sol now passes block.chainid through.
I verified those two addresses against the canonical mds1/multicall deployments.json (both zkSync networks share 0xF9cda624…). LENS/LENS_SEPOLIA aren't in that registry, so I left them on the canonical fallback rather than guess — and since the periphery constructor rejects a non-contract _multicall3, an unmapped chain whose Multicall3 turns out to be non-canonical fails loudly at deploy time instead of deploying with a dead reference. Easy to add a mapping there once an address is confirmed.
|
Ask again to re-run the unfinished work. |
grasphoper
left a comment
grasphoper
left a comment
There was a problem hiding this comment.
Looks good, 2 Qs:
- move
isContractcheck to theERC6492SignatureHandlerconstructor instead of SpokePoolPeriphery? - should we add the same logic to
Permit2signatures?
|
Added Permit2 support here #1488 if we want it |
|
Ask again to re-run the unfinished work. |
Signed-off-by: Ihor Farion <ihor@umaproject.org>
Signed-off-by: Ihor Farion <ihor@umaproject.org>
4 participants
Summary
PR #1420 added the bytes-signature ERC-3009 entry points (
depositWithAuthorizationBytes,swapAndBridgeWithAuthorizationBytes) which pull tokens via the extendedreceiveWithAuthorization(...,bytes signature)overload (e.g. USDC v2.2). Those delegate signature verification to the token, which supports EIP-1271 contract wallets — but the token has no concept of ERC-6492. A counterfactual (not-yet-deployed) smart-contract wallet therefore can't authorize a transfer: there's no code at the signer's address to runisValidSignatureon.We can't change the token's verification. So, following the approach in Coinbase's
ERC6492SignatureHandler, this PR implements only the deploy half of ERC-6492 in the periphery — it is not the signature verifier:ERC6492SignatureHandlerbase contract._handleERC6492Signaturedetects the ERC-6492 magic suffix (0x6492…6492) and, if present, runs the wrapped(factory, factoryCalldata)to materialize the wallet, then returns the unwrapped inner signature.SpokePoolPeripheryinherits it and runs_handleERC6492Signatureon the signature in both*WithAuthorizationBytesentry points before forwarding it to the token. Once the wallet exists, the token's own EIP-1271 check against the signer succeeds.factory/factoryCalldataare attacker-controlled, so the prepare call is routed through the canonical Multicall3 singleton (supplied as an immutable constructor arg) rather than made directly — the periphery is never themsg.senderof that call and so can't be tricked into abusing its own allowances. The call usesrequireSuccess=false, so an already-deployed wallet is tolerated; the token remains the source of truth on validity.IMulticall3interface.Constants.getMulticall3()supply the canonical0xcA11bde05977b3631167028862bE2a173976CA11.Non-ERC-6492 signatures (EOA / already-deployed EIP-1271) fall straight through
_handleERC6492Signatureunchanged, so existing behavior is preserved.Scope
Only the two
*WithAuthorizationBytesflows need this — they're the only entry points where the token performs EIP-1271 verification. The v/r/s, permit, and permit2 flows are unaffected.Test plan
yarn build-evm-foundryyarn test-evm-foundry -- --match-contract "SpokePoolPeripheryTest|TransferProxy"— 53 passed (49 existing + 4 new)testTransferWithAuthBytesDepositERC6492CounterfactualWallet— wallet deployed by the prepare step, deposit succeedstestTransferWithAuthBytesSwapAndBridgeERC6492CounterfactualWallet— same for swap-and-bridgetestTransferWithAuthBytesDepositERC6492AlreadyDeployedWallet— wrapped sig + already-deployed wallet (idempotent prepare noop)testTransferWithAuthBytesDepositERC6492FailedPrepareTolerated— failing prepare target is swallowed (requireSuccess=false), deposit still succeeds via the already-deployed signeryarn lint-fix🤖 Generated with Claude Code