feat: Counterfactual route policies#1434
Conversation
|
Closes ACP-106 |
tbwebb22
requested review from
bmzig,
mrice32,
nicholaspai and
pxrl
as code owners
![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: 54e6ffe789
ℹ️ 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".
| if (implementation == WITHDRAW_IMPL && msg.sender == cloneArgs.withdrawUser) { | ||
| _delegate(implementation, cloneArgs, params, submitterData); | ||
| return; | ||
| } |
There was a problem hiding this comment.
When implementation == WITHDRAW_IMPL but msg.sender != cloneArgs.withdrawUser, execution falls through to the normal proof path instead of being rejected. In this commit, WithdrawImplementation.execute no longer performs any caller authorization, so if a route policy ever includes a withdraw leaf, any caller with that proof can choose arbitrary (token,to,amount) in submitterData and sweep clone funds. Previously this was mitigated by auth inside WithdrawImplementation; now the dispatcher should explicitly reject non-escape withdraw calls to preserve that safety invariant.
Useful? React with 👍 / 👎.
| if (leafDestinationChainId != cloneArgs.destinationChainId || leafOutputToken != cloneArgs.outputToken) | ||
| revert InvalidIdentity(); |
There was a problem hiding this comment.
submitter passed in both params and cloneArgs kind of strange that we are checking them against each other, is there a way you think we can just have the submitter pass them once?
There was a problem hiding this comment.
Well the leaves on the policy (params) can point to different destination chains and tokens. We have to ensure that the selected leaf targets the same destination as this clone is. If we don't the submitter could select a leaf that sends to the wrong destination chain or token.
There was a problem hiding this comment.
right, I was just thinking more along the lines submitter provides destinationChainId and outputToken and the function then constructs params/cloneArgs, but that might be uglier
There was a problem hiding this comment.
I think we can remove duplication here, see slack
| uint256 executionFee; | ||
| uint32 signatureDeadline; | ||
| bytes peripherySignature; | ||
| bytes signature; |
There was a problem hiding this comment.
| bytes signature; | |
| bytes authSignature; |
or something similar to distinguish it form the other signature
There was a problem hiding this comment.
yeah good call, maybe rename it to counterfactualSignature?
There was a problem hiding this comment.
renamed to counterfactualSignature here 0c5f57d
| if (sd.executionFee > 0) IERC20(inputToken).safeTransfer(sd.executionFeeRecipient, sd.executionFee); | ||
|
|
||
| uint256 depositAmount = sd.amount - dp.executionFee; | ||
| uint256 depositAmount = sd.amount - sd.executionFee; |
There was a problem hiding this comment.
what happens if executionFee > amount? I guess the funds will just get stuck until more funds are sent
There was a problem hiding this comment.
Thats right. And I think thats fine. Basically maxExecutionFee is a lower bound for how much the user should be sending through the counterfactual.
| uint256 relayerFee = depositAmount > outputInInputToken ? depositAmount - outputInInputToken : 0; | ||
| uint256 totalFee = relayerFee + dp.executionFee; | ||
| uint256 totalFee = relayerFee + executionFee; | ||
| uint256 maxFee = dp.maxFeeFixed + (dp.maxFeeBps * inputAmount) / BPS_SCALAR; |
There was a problem hiding this comment.
why does only SpokePool have a fixed and dynamic fee but the rest CCTP/OFT have only fixed?
There was a problem hiding this comment.
So the CCTP / OFT implementations take maxFee as a param, so we're kind of delegating that dynamic fee part to the sponsored bridging contracts
| * independent `activeRoot` storage. | ||
| * @custom:security-contact bugs@across.to | ||
| */ | ||
| contract RoutePolicy is IRoutePolicy, Ownable { |
There was a problem hiding this comment.
Should all the leaf verification happen here? like even the leaf construction? that way when a RouterPolicy is replaced the leaf computation also gets updated
There was a problem hiding this comment.
Can you elaborate on what you mean here? I'm not sure I follow
There was a problem hiding this comment.
mainly thinking if these lines from CounterfactualDeposit.sol can be moved here?
bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(implementation, keccak256(params)))));
bytes32 root = IRoutePolicy(cloneArgs.routePolicyAddress).activeRoot();
if (!MerkleProof.verify(proof, root, leaf)) revert InvalidProof();
There was a problem hiding this comment.
yeah that definitely could work. Do you think its cleaner?
It would be a little more gas (Claude is telling me +1k gas) since we're passing more calldata in the external call
There was a problem hiding this comment.
actually now that I think about this, its not going to work since RoutePolicy isn't upgradable so doesn't give us any benefit with moving the logic there
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fb438b6ea
ℹ️ 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".
| abi.encode(EXECUTE_CCTP_TYPEHASH, routeParamsHash, sd.executionFee, sd.signatureDeadline) | ||
| ); |
There was a problem hiding this comment.
The new local EIP-712 check only signs (routeParamsHash, executionFee, signatureDeadline), but the executed quote still carries a mutable sd.nonce and sd.cctpDeadline later in _depositForBurn. Because sd.nonce is not part of the signed payload, the same local signature can be reused with different periphery quotes/nonces before signatureDeadline, allowing repeated fee-authorized executions against the same clone balance instead of one intended authorization.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
I actually think that structHash = abi.encode(routeParams.nonce, sd.executionFee, sd.signatureDeadline can be all we need here.
Will tie the two signatures together effectively, since API commits to once nonce per unique quote
| abi.encode(EXECUTE_OFT_TYPEHASH, routeParamsHash, sd.executionFee, sd.signatureDeadline) | ||
| ); |
There was a problem hiding this comment.
As implemented, the OFT local signature covers only (routeParamsHash, executionFee, signatureDeadline) and excludes sd.nonce/sd.oftDeadline, even though those values are consumed in the downstream quote. That means a valid local fee signature can be replayed across multiple distinct OFT quotes (new nonces) until expiry, enabling repeated executions that were not uniquely authorized per quote.
Useful? React with 👍 / 👎.
|
@codex review - note the changes to the IRoutePolicy interface, and the new RoutePolicyImmutableRoot implementation |
|
Codex Review: Didn't find any major issues. Swish! ℹ️ 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". |
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Re-reviewed against the head of taylor/counterfactual-route-policy (9d6aee9). I focused on what changed since my prior review (59e71b6): the CCTP/OFT replay mitigation, the new RoutePolicyImmutableRoot, the admin arg added to ICounterfactualImplementation, and the WithdrawImplementation defense-in-depth check.
What the new diff resolves cleanly
- CCTP/OFT replay (OQ #2 partial close). Binding the local
counterfactualSignaturetononceinstead ofrouteParamsHashis the right shape. I verified both peripheries (SponsoredCCTPSrcPeriphery.sol:85,89andSponsoredOFTSrcPeriphery.sol:221,98) reject duplicate nonces and mark them used, so once the periphery consumes the nonce, the local sig is also burned — no on-chain bookkeeping needed in the bridge impl itself. The replay window is genuinely closed for these two paths. WithdrawImplementationdefense-in-depth. The explicitif (msg.sender != admin) revert Unauthorized()covers the "policy owner accidentally adds a withdraw leaf" footgun.adminflows through the dispatcher's verifiedcloneArgs, so this is safe to trust here.RoutePolicyImmutableRoot. Bytecode-immutable root + UUPS upgrade-as-rotation is a reasonable trade — reads become bytecode constant loads instead of SLOADs, at the cost of a small impl deploy per rotation. The interface adds theclonearg for forward compatibility with per-clone roots without a future breaking change. Good move.- Cross-chain consistency test.
testIdenticalDay0ArgsProduceIdenticalProxyAddressnow actually exercises the property (impl → proxy CREATE2 chain, then asserts an arg change cascades through to a different proxy address). My prior comment about the tautological test is closed.
Still open
- OQ #2 for
SpokePool+AdminWithdrawManager.signedWithdraw. Neither has a nonce-like field that something downstream consumes single-use, so both remain replayable withinsignatureDeadlineif the clone is re-funded. The PR description acknowledges this. My recommendation hasn't changed: land a one-time signature-mark mapping for these two before mainnet.SignedWithdrawis the more dangerous of the pair (committed(token, to, amount)triple → re-drain to the sametoon refund). - Single-step
Ownable→Ownable2Step. Still applies, now onOwnableUpgradeable. The deployment story calls a single-steptransferOwnership(chainLocalMultisig)per chain (RoutePolicies.mdstep 4); a typo permanently bricks the policy on that chain, which then can't be recovered without breaking cross-chain address consistency.Ownable2StepUpgradeableremoves that class of fat-finger. AdminWithdrawManager.withdrawImplimmutable. Still vestigial in the sense I flagged — sincecloneArgs.admin = manageris in the clone's argsHash for life and the manager pins itswithdrawImpl, the withdraw impl is effectively immutable per-clone. Either making it owner-settable or dropping it (and having the manager pass it in per call) would eliminate the only deployment-ordering invariant left on this contract.- Inconsistent typehash binding. SpokePool typehash now includes
address clonewhile CCTP/OFT bind clone only via the EIP-712 domain separator. Both are safe; the inconsistency is the only thing worth tidying up.
Drive-by on the new code
RoutePolicyImmutableRoot._authorizeUpgrade(address)doesn't constrain what the new impl looks like beyondonlyOwner. Documented in OQ #1. The owner can in principle deploy an impl whoseactiveRootdoes anything; the admin escape covers fund safety regardless. Fine._disableInitializers()in the impl constructor is the right move (line 39). Test at line 52 covers it.- The genesis-root-must-be-identical-across-chains invariant is now load-bearing for cross-chain proxy address consistency. Worth a sanity check in
CheckCounterfactualDeployments.s.solwhen that's wired up: assert the proxy's current impl matches a chain-specific impl after first rotation, and that the proxy address itself matches a reference chain.
Overall: the design has tightened substantially since my first pass. With OQ #2 closed for SpokePool + the manager, and Ownable2Step on the policy, this is solidly ship-ready.
| * The proxy address stays constant on each chain throughout all rotations. | ||
| * @custom:security-contact bugs@across.to | ||
| */ | ||
| contract RoutePolicyImmutableRoot is IRoutePolicy, Initializable, OwnableUpgradeable, UUPSUpgradeable { |
There was a problem hiding this comment.
Same recommendation as my prior review, now applied to the upgradeable variant: Ownable2StepUpgradeable instead of OwnableUpgradeable. The deployment story (RoutePolicies.md step 4) has the deployer EOA calling transferOwnership(chainLocalMultisig) separately per chain — single-step transfer leaves a fat-finger trap that bricks the policy on the affected chain, and recovery requires a fresh policy at a new address, which breaks cross-chain address consistency. Two-step transfer eliminates this class of footgun cheaply.
| /// (and cleanly gives single-use replay protection — once the periphery consumes the | ||
| /// nonce, the local sig can never be replayed). | ||
| bytes32 public constant EXECUTE_CCTP_TYPEHASH = | ||
| keccak256("ExecuteCCTP(bytes32 nonce,uint256 executionFee,uint32 signatureDeadline)"); |
There was a problem hiding this comment.
Confirmed this is now safe against in-deadline replay: SponsoredCCTPSrcPeriphery.sol:85,89 rejects duplicate nonces, so binding the local sig to nonce transitively closes the replay window. Nice idea — no extra storage in the impl. (The comment block already explains this; flagging for reviewers' confidence.)
| /// (and cleanly gives single-use replay protection — once the periphery consumes the | ||
| /// nonce, the local sig can never be replayed). | ||
| bytes32 public constant EXECUTE_OFT_TYPEHASH = | ||
| keccak256("ExecuteOFT(bytes32 nonce,uint256 executionFee,uint32 signatureDeadline)"); |
There was a problem hiding this comment.
Same as CCTP — SponsoredOFTSrcPeriphery.sol:221,98 consumes nonces single-use, so the nonce-binding here closes the local-sig replay too. Both peripheries verified.
|
|
||
| /// @notice EIP-712 typehash for execute deposit signature verification. | ||
| /// @notice EIP-712 typehash binding the signature to (clone, leaf, runtime fields). | ||
| bytes32 public constant EXECUTE_DEPOSIT_TYPEHASH = |
There was a problem hiding this comment.
Still replayable in deadline (OQ #2). Unlike CCTP/OFT, the SpokePool path has no downstream nonce consumption to lean on, so the routeParamsHash-bound sig remains valid against the same clone until signatureDeadline — a refund of inputAmount within that window enables a second deposit. Strong recommend either (a) adding a bytes32 nonce to the typehash + a mapping(bytes32 => bool) in clone storage (ERC-7201 to avoid colliding with other impls), or (b) committing block.timestamp lower-bound the signer must move forward for each subsequent sig. (a) is the cleaner fit for the broadcast-and-pick-up executor model.
| * @dev The signer fixes the recipient (`to`) in the EIP-712 message; the caller cannot redirect. | ||
| */ | ||
| function signedWithdrawToUser( | ||
| function signedWithdraw( |
There was a problem hiding this comment.
Still replayable in deadline (OQ #2). This path is the more dangerous of the two open replays — the signature commits a fixed (token, to, amount) triple, so any refund of amount during the deadline window enables a second drain to the same to with no further coordination. A regular mapping(bytes32 sigHash => bool used) in the manager's storage (no ERC-7201 since this contract isn't delegatecalled) checked + set after ECDSA.recover is the minimal fix.
| keccak256("SignedWithdraw(address depositAddress,address token,address to,uint256 amount,uint256 deadline)"); | ||
|
|
||
| /// @notice Canonical `WithdrawImplementation` address (passed to the dispatcher's escape path). | ||
| address public immutable withdrawImpl; |
There was a problem hiding this comment.
Same observation as my prior review: this immutable plus the fact that cloneArgs.admin = address(this) is itself in the clone's argsHash for life effectively pins the withdraw implementation per-clone. To rotate the withdraw impl you'd need a new manager and a redeploy of every clone (which changes their addresses and defeats the address-persistence goal). Making this owner-settable, or moving it to a per-call argument, removes that constraint. Not blocking, but it costs nothing structurally and unlocks future flexibility.
| bytes32 public constant EXECUTE_DEPOSIT_TYPEHASH = | ||
| keccak256( | ||
| "ExecuteDeposit(uint256 inputAmount,uint256 outputAmount,bytes32 exclusiveRelayer,uint32 exclusivityDeadline,uint32 quoteTimestamp,uint32 fillDeadline,uint32 signatureDeadline)" | ||
| "ExecuteDeposit(address clone,bytes32 routeParamsHash,uint256 inputAmount,uint256 outputAmount,bytes32 exclusiveRelayer,uint32 exclusivityDeadline,uint32 quoteTimestamp,uint32 fillDeadline,uint32 signatureDeadline,uint256 executionFee)" |
There was a problem hiding this comment.
Mild inconsistency with CCTP/OFT typehashes — those bind clone implicitly via the EIP-712 domain separator alone, while this one redundantly includes address clone in the struct. Both are safe; consider dropping clone here and relying on the domain separator (matches the CCTP/OFT pattern and shortens the typehash).
| /// @dev Required by UUPS — only the owner can upgrade the implementation. Upgrading is the | ||
| /// mechanism by which the active root changes: the owner deploys a new implementation | ||
| /// carrying the new root and points the proxy at it. | ||
| function _authorizeUpgrade(address) internal override onlyOwner {} |
There was a problem hiding this comment.
Worth verifying as part of the deployment runbook: a chain whose policy proxy points at an impl whose code is somehow lost (selfdestruct of the impl, etc.) would be unable to upgradeToAndCall because the proxy's _dispatchUpgradeToAndCall selfdelegate calls into the current impl's upgradeTo via UUPS. The impls here have no selfdestruct, so this is theoretical, but the CheckCounterfactualDeployments.s.sol (still pending) should at least assert each chain's proxy has a non-zero current impl with the expected code hash for the deployed impl revision.
mrice32
left a comment
mrice32
left a comment
There was a problem hiding this comment.
Overall looks great, just a few comments!
| bytes32 outputToken; | ||
| uint256 destinationChainId; | ||
| bytes32 recipient; | ||
| address admin; |
There was a problem hiding this comment.
Who do we intend the admin to be set to? Some admin multisig or the user?
There was a problem hiding this comment.
so this is intended to be set to the AdminWithdrawManager contract - which in turn allows itsdirectWithdrawer (should be a multsig) to call directWithdraw, or for an executor to call signedWithdraw
| cloneArgs.outputToken, | ||
| cloneArgs.destinationChainId, |
There was a problem hiding this comment.
Can we drop these and let the leaf validate that it knows how to handle this particular outputToken and destinationChainId (via routeParams or otherwise)?
| * @custom:security-contact bugs@across.to | ||
| */ | ||
| contract CounterfactualDepositCCTP is ICounterfactualImplementation { | ||
| contract CounterfactualDepositCCTP is ICounterfactualImplementation, EIP712 { |
There was a problem hiding this comment.
Does this only work with chains where destination SponsoredBridging contracts are deployed?
I'm assuming we don't have a way to do a vanilla CCTP leaf, right? I could see that being nice for destination Solana, for instance.
There was a problem hiding this comment.
Yep thats right, this is specific to our SponsoredBridging contracts. And yeah for a vanilla CCTP leaf we would need a new implementation for that specifically
| * @custom:security-contact bugs@across.to | ||
| */ | ||
| contract CounterfactualDepositOFT is ICounterfactualImplementation { | ||
| contract CounterfactualDepositOFT is ICounterfactualImplementation, EIP712 { |
There was a problem hiding this comment.
Similar to CCTP, we don't have a way to talk to non sponsored bridging OFT contracts, right?
I'm assuming there's nothing stopping us from doing that in the future, right?
There was a problem hiding this comment.
Correct, and yeah no reason we couldn't add an implementation for this in the future
| bytes calldata /* routeParams */, | ||
| bytes calldata submitterData | ||
| ) external payable { | ||
| if (msg.sender != admin) revert Unauthorized(); |
There was a problem hiding this comment.
This would be the AdminWithdrawManager, right? Does that mean that we would need to have a distinct AdminWithdrawManager contract deployed for each counterfactual address that has their EOA encoded? Or am I missing something?
There was a problem hiding this comment.
And if it doesn't, then does that mean the signed withdrawer (i.e. the API) can authorize a withdrawal to any address (since the unique refund address isn't known/stored onchain)?
There was a problem hiding this comment.
This would be the AdminWithdrawManager, right? Does that mean that we would need to have a distinct AdminWithdrawManager contract deployed for each counterfactual address that has their EOA encoded? Or am I missing something?
so I'm envisioning that there would generally be a global AdminWithdrawManager instance per chain that all counterfactual clones point to as their admin
There was a problem hiding this comment.
does that mean the signed withdrawer (i.e. the API) can authorize a withdrawal to any address (since the unique refund address isn't known/stored onchain)?
thats correct. Do you think this is placing to much trust in the API? And that the user's refund address should be store onchain?
|
@codex review the last commit that restructured the merkle tree and moved outputToken and destinationChain into routeParams |
|
Codex Review: Didn't find any major issues. Nice work! ℹ️ 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 review the last commit that updated the withdraw flow |
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ 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". |
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Re-reviewed against af3fe4d. Two new commits since my last pass: the withdraw-flow rewrite and the dispatcher-leaf simplification (outputToken/destinationChainId moved out of the leaf, into per-impl routeParams with opt-in binding via CloneIdentity.enforce).
This iteration is a notable security improvement. The two design changes work together to materially shrink the trust footprint of every authority in the system:
Why this is meaningfully better
WithdrawImplementation now forces destination to userAddress. The destination is no longer chosen at call time — it's the clone-bound userAddress, period. This collapses several of my prior concerns at once:
SignedWithdrawreplay (OQ #2) is now benign: a replayed signature still routes to the user, not to an attacker. The "blast radius bounded" note in the open-questions section is correct.- A compromised
directWithdrawercan force withdrawals but cannot redirect them. - A compromised
signercan force withdrawals but cannot redirect them. - The trust model collapses to "when and how much," never "where." Clean.
AdminWithdrawManager no longer pins withdrawImpl. Passed per-call now, with the signed typehash committing withdrawImpl so a compromised submitter can't substitute. The construction-order constraint I flagged is replaced with a clean dependency: manager deploys first, impl deploys second pointing at the manager. My prior concern about the immutable being vestigial / pinning impl per-clone is closed.
Identity binding moved from dispatcher to per-impl CloneIdentity.enforce. I went back and forth on this trade. The dispatcher's leaf is now simpler ((impl, keccak256(routeParams))), and each impl declares whether it needs identity binding. The footgun is real — a future impl author who needs binding and forgets to call CloneIdentity.enforce ships a vulnerability — but the convention is explicit in the lib's natspec and in the per-impl natspec, and the three current impls that need binding all call it. Auditors can grep for CloneIdentity.enforce to verify coverage on future additions. Acceptable trade.
The SpokePoolRouteParams natspec now spells out why binding is required there (the stableExchangeRate per-pair assumption). Important context that was implicit before; making it explicit reduces the chance a future maintainer drops the enforcement.
What's still open (all flagged, none blocking)
RoutePolicyImmutableRootstill uses single-stepOwnableUpgradeable. I flagged this twice. The deployment story still has the deployer EOA single-step-transferring to a per-chain multisig; a typo bricks the policy on that chain, and recovery breaks cross-chain address consistency.Ownable2StepUpgradeableremoves that class of fat-finger for free. Not blocking — it's an operational risk you can offset with paranoid deployment scripting + reviews — but it's the cheapest hardening left on the table.AdminWithdrawManager's single-stepOwnableis less load-bearing (per-chain manager, no cross-chain invariant) but worth flipping in the same pass for consistency.- OQ #2 SpokePool replay is the last unresolved replay window, and it too is now mostly benign (the replay funds go to
cloneArgs.recipient, which is the legitimate user destination). Worth still doing the one-time-marks fix before mainnet — "user receives their own money again" is still a UX/liveness footgun (someone can involuntarily move funds out of a clone the user wanted to keep funded) — but no longer security-load-bearing. - OQ #1 owner-compromise / timelock split is the genuine remaining design question. Not for this PR.
SpokePooltypehash redundantly includesaddress clonewhile CCTP/OFT bind clone only via the domain separator. Pure cosmetic at this point.
Process-level observation
The implementation plan's Scripts checklist (RoutePolicies.md) is still entirely unchecked. The deploy/check/rotate scripts (DeployRoutePolicy.s.sol, RotateRoutePolicyRoot.s.sol, CheckCounterfactualDeployments.s.sol, etc.) aren't in this PR. That's fine — splitting into a follow-up keeps this PR reviewable — but make sure the follow-up lands before any testnet deployment, and that CheckCounterfactualDeployments.s.sol asserts current-impl code hashes on every chain (the cross-chain proxy address consistency invariant depends on no chain having upgraded prematurely).
A defensive item for the security-review checklist: every new ICounterfactualImplementation either calls CloneIdentity.enforce or has an explicit comment justifying why identity binding is unnecessary (e.g. WithdrawImplementation because destination is forced to userAddress). Catches the future-maintainer footgun cheaply.
Approving. Nice work tightening this through three review cycles.
| bytes calldata /* routeParams */, | ||
| bytes calldata submitterData | ||
| ) external payable { | ||
| if (msg.sender != admin && msg.sender != userAddress) revert Unauthorized(); |
There was a problem hiding this comment.
This is the right shape. Forcing the destination to userAddress instead of a caller-supplied to collapses three separate trust concerns (signer compromise, directWithdrawer compromise, signature replay) into a single benign outcome: the user receives their own funds. Strong improvement over the prior (token, to, amount) triple.
| bytes32 public constant SIGNED_WITHDRAW_TYPEHASH = | ||
| keccak256("SignedWithdraw(address depositAddress,address token,uint256 amount,uint256 deadline)"); | ||
| keccak256( | ||
| "SignedWithdraw(address depositAddress,address withdrawImpl,address token,uint256 amount,uint256 deadline)" |
There was a problem hiding this comment.
Good that withdrawImpl is committed in the typehash — a compromised submitter can't substitute a malicious impl on a valid signature. Combined with the policy's merkle gate (only policy-authorized impls can be reached) and the impl's msg.sender == admin check (only the manager or user can trigger this impl), the signer's authority is genuinely "when + how much" only. The destination question is resolved structurally inside the impl.
| * @param routeDestinationChainId `destinationChainId` committed inside `routeParams`. | ||
| * @param cloneDestinationChainId `destinationChainId` forwarded by the dispatcher from `cloneArgs`. | ||
| */ | ||
| function enforce( |
There was a problem hiding this comment.
The opt-in model relies on impl authors remembering to call this. The current three impls that need binding (SpokePool, CCTP, OFT) all do, and the natspec spells out the contract. Worth adding to the security-review checklist: every new ICounterfactualImplementation either calls CloneIdentity.enforce or has an explicit comment justifying why identity binding is unnecessary (e.g. WithdrawImplementation because destination is forced to userAddress).
| if (msg.sender != cloneArgs.userAddress) { | ||
| // Verify merkle proof against the policy's active root. The leaf commits only | ||
| // `(implementation, keccak256(routeParams))` | ||
| bytes32 leaf = keccak256(bytes.concat(keccak256(abi.encode(implementation, keccak256(routeParams))))); |
There was a problem hiding this comment.
The leaf format change is a real design pivot — worth a paragraph in the changelog (or PR description) calling out that off-chain leaf builders must be updated to match the new commitment keccak256(impl, keccak256(routeParams)) (without outputToken / destinationChainId at the dispatcher level). Easy to miss when porting indexers / SDK encoders.
| /// @notice EIP-712 typehash for execute deposit signature verification. | ||
| /// @notice EIP-712 typehash binding the signature to (clone, leaf, runtime fields). | ||
| bytes32 public constant EXECUTE_DEPOSIT_TYPEHASH = | ||
| keccak256( |
There was a problem hiding this comment.
Pure cosmetic: typehash still binds address clone explicitly while CCTP/OFT typehashes bind clone implicitly via the EIP-712 domain separator. Both are safe — the inconsistency is the only thing left to tidy. Drop clone here for symmetry, or add it to CCTP/OFT, but pick one.
|
@codex review |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. ℹ️ 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". |
5 participants
Closes ACP-106