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
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Solid PR — the spec doc reads cleanly and the contract changes track it 1:1. I went through the dispatcher, factory, RoutePolicy, all three bridge impls, WithdrawImplementation, AdminWithdrawManager, and the tests. No blocking bugs found. The two open questions you flagged are the right ones to focus on; opinions inline.
On the open questions
1. Withdraw architecture — recommend (a) structural + separate (current), with a tweak.
The fund-recovery guarantee is the load-bearing property here, and (a) is the cheapest way to preserve it without coupling the dispatcher to transfer logic. The vestigial address(this), recipient, outputToken, destinationChainId args on WithdrawImplementation.execute are a minor cost relative to keeping the dispatcher's "every execute goes through an impl" invariant clean.
That said, the withdrawImpl immutable in AdminWithdrawManager is mostly cosmetic: since admin can call any impl through the dispatcher's escape, the immutable doesn't actually pin anything. It does make the manager's two paths slightly less flexible (can't migrate to a new withdraw impl without redeploying the manager — and since cloneArgs.admin = manager is itself immutable in the clone's argsHash, that effectively pins withdrawImpl per-clone for life). Consider whether you'd rather make it mutable (owner-set) or drop the field entirely and have callers pass the impl. (c) policy-committed is correctly ruled out for the reason you noted.
2. Signature replay — strong recommend the one-time signature marks (option c) before mainnet.
The replay surface is real and asymmetric across the four paths:
SignedWithdrawis the worst — committed(token, to, amount)triple → a single refund ofamountlets the same sig drain again to the sameto, no further coordination.- CCTP/OFT — replayable if the clone is refunded to
≥ sd.amountwithin the deadline. - SpokePool — same shape; refund-then-redeposit is the attack.
The "rely on signer-side bookkeeping + short deadlines" path puts the entire safety property in a single off-chain process. A leaked or buffered signature inside its deadline is exploitable with zero on-chain protection. ~1 SSTORE per execute is cheap insurance, and the ERC-7201 mixin for the three bridge impls is a clean ~30 LOC. I'd push to land this in this PR rather than ship the design with the known footgun.
Other observations
A few smaller things worth considering — none blocking.
-
RoutePolicyuses single-stepOwnable. The deployment story has a deployer EOA callingtransferOwnership(chainLocalMultisig)per chain. A typo there permanently bricks the policy on that chain (no way back — and the address is supposed to be uniform cross-chain, so recovering means deploying a new policy at a new address and re-rooting every clone, breaking address consistency for affected chains).Ownable2Stepshifts that risk meaningfully without adding much complexity. -
Inconsistent EIP-712 binding of
clone.CounterfactualDepositSpokePool.EXECUTE_DEPOSIT_TYPEHASHincludesaddress cloneas a struct field; CCTP / OFT / SignedWithdraw rely on the EIP-712 domain separator'sverifyingContractalone. Both are safe (domain separator already bindsaddress(this)), but the inconsistency is a future-confusion vector. Either pick one and apply it everywhere, or note the asymmetry in the impl comments. -
Test for cross-chain address consistency is tautological.
RoutePolicy.t.sollines 108–126 computecomputeCreate2Address(...)twice with identical inputs and assert equality — that's a no-op. The interesting property is "same(deployer, salt, codeHash)across chains where onlyblock.chainiddiffers gives the same address" or "different deployer addresses give different addresses." Worth either fixing or dropping. -
updateRoot(bytes32(0))is unguarded. Open Question #1 covers the broader compromise mitigation, but worth noting the specific footgun: an owner can brick the policy in one tx. Admin escape covers fund recovery, so this is a liveness issue rather than a safety issue. A min-non-zero check would catch fat-fingers cheaply. -
AdminWithdrawManager.setSigner/setDirectWithdraweracceptaddress(0). OZ ECDSA v5 reverts on signatures recovering toaddress(0), so no forgery risk — just a liveness footgun. -
RootUpdated(bytes32 newRoot)is not indexed. Indexers can filter on it regardless, butindexedmakes per-root subscriptions cheaper. Drive-by suggestion. -
CounterfactualDeposit.executeispayableandmsg.valuecarries through the delegatecall. Fine for OFT (LayerZero fees) and native SpokePool deposits; just confirming this is intentional — the dispatcher itself does no msg.value accounting and trusts each impl to handle it. (It does.)
| * independent `activeRoot` storage. | ||
| * @custom:security-contact bugs@across.to | ||
| */ | ||
| contract RoutePolicy is IRoutePolicy, Ownable { |
There was a problem hiding this comment.
Consider Ownable2Step instead of Ownable. The deployment flow has the deployer EOA calling transferOwnership(chainLocalMultisig) separately per chain (per the spec and Deployment section of RoutePolicies.md); single-step transfer means a typo or wrong-address transfer permanently bricks the policy on that chain. Recovery would require deploying a new RoutePolicy at a new address, which breaks the cross-chain address-consistency invariant for the affected chain. Ownable2Step requires the multisig to acceptOwnership() and eliminates this class of fat-finger.
| } | ||
|
|
||
| /// @inheritdoc IRoutePolicy | ||
| function updateRoot(bytes32 newRoot) external onlyOwner { |
There was a problem hiding this comment.
updateRoot(bytes32(0)) is allowed and bricks the policy in one tx (admin escape still recovers funds, so this is liveness, not safety). The PR's Open Question #1 already covers the broader compromise mitigation; if you want the cheapest possible fat-finger guard without committing to a timelock, an explicit if (newRoot == bytes32(0)) revert here would catch it. Note that this would also block the legitimate "freeze the policy" use case — your call.
| 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.
withdrawImpl is immutable and cloneArgs.admin = address(this) is committed in the clone's argsHash for life. Together that effectively pins the withdraw implementation per clone — to migrate to a fixed-up withdraw impl, you'd have to deploy a new manager and redeploy every clone (different argsHash → different CREATE2 address). Worth either (a) making this an owner-settable field, or (b) dropping it and having directWithdraw / signedWithdraw take the impl as a parameter. Option (b) costs ~1 calldata word per call and removes the only deployment-ordering invariant for this contract.
| * @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.
Option (c) one-time signature marks (per the PR description) is the right fix here — the EIP-712 message commits (depositAddress, token, to, amount, deadline) with no nonce or used-flag, so a refund of amount during the deadline window enables the same (to, amount) drain a second time. This path is the most replay-prone of the four. For the manager specifically, a regular mapping(bytes32 sigHash => bool used) in normal storage (no ERC-7201 needed since the manager isn't delegatecalled) is the minimal fix — set the flag after ECDSA.recover succeeds.
| /// @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.
The typehash includes address clone as a struct field; CCTP / OFT typehashes rely on the EIP-712 domain separator (verifyingContract = address(this) = clone) alone. Both are equivalent for replay safety, but the inconsistency between the three bridge impls is a future-confusion vector. Either drop clone here (domain separator already binds it) or add the field to CCTP/OFT for symmetry. Mild preference for dropping — the typehash gets shorter and the binding lives in exactly one place (the domain separator).
| * @param depositAmount Amount to deposit after deducting the execution fee. | ||
| */ | ||
| function _depositForBurn(CCTPDepositParams memory dp, CCTPSubmitterData memory sd, uint256 depositAmount) private { | ||
| function _verifySignature(bytes32 routeParamsHash, CCTPSubmitterData memory sd) private view { |
There was a problem hiding this comment.
Same replay concern as AdminWithdrawManager.signedWithdraw and the other bridge impls: the EIP-712 sig is reusable until signatureDeadline if the clone is re-funded with ≥ sd.amount of the burn token during that window. The shared ERC-7201 SignatureMarks mixin you described in Open Question #2 is the cleanest fix — adding if (used[sigHash]) revert; used[sigHash] = true; here after recover would shut this path. Worth landing in the same PR rather than shipping with the known footgun.
| * @param depositAmount Amount to deposit after deducting the execution fee. | ||
| */ | ||
| function _deposit(OFTDepositParams memory dp, OFTSubmitterData memory sd, uint256 depositAmount) private { | ||
| function _verifySignature(bytes32 routeParamsHash, OFTSubmitterData memory sd) private view { |
There was a problem hiding this comment.
Same replay concern as CCTP — (paramsHash, executionFee, signatureDeadline) plus the clone-bound domain separator is sufficient to bind identity, but nothing prevents a second execute with the same sig during the deadline window if the clone is re-funded. Recommend the shared SignatureMarks ERC-7201 mixin per Open Question #2.
|
|
||
| // --- Cross-chain address consistency: identical constructor args → identical address --- | ||
|
|
||
| function testIdenticalArgsProduceIdenticalAddress() public { |
There was a problem hiding this comment.
This test is tautological — computeCreate2Address(salt, codeHash, address(this)) returns the same value when called twice with identical inputs by construction. The interesting cross-chain consistency property (constructor args identical across chains → same address regardless of block.chainid) is implicit in how CREATE2 works (it doesn't use chainid) but isn't actually exercised here. Either rewrite to vary an input that would matter on different chains, or drop this test and rely on the production-side CheckCounterfactualDeployments.s.sol for cross-chain verification.
| */ | ||
| interface IRoutePolicy { | ||
| /// @notice Emitted on every successful root update. | ||
| event RootUpdated(bytes32 newRoot); |
There was a problem hiding this comment.
Drive-by: event RootUpdated(bytes32 indexed newRoot) makes per-root subscriptions cheaper for indexers. Non-blocking.
|
@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 👍 / 👎.
4 participants
Open Questions:
WithdrawImplementation vs. inlined in dispatcher). Currently "structural + separate" — but the impl's address is pinned by WITHDRAW_IMPL (not actually swappable) and
two of three execute args are unused. Three options:
a) Structural + separate (current): strongest fund guarantee — withdraw works regardless of policy state. Costs: extra contract, dispatcher immutable,
deployment-ordering invariant, vestigial interface conformance on the impl.
b) Structural + inlined: same fund guarantee, drops the impl, immutable, delegatecall hop, and unused-arg conformance. Costs: dispatcher gains a transfer path; "every
execute goes through an impl" abstraction is broken.
c) Policy-committed: simplest dispatcher (no escape, no immutable). Costs: policy owner becomes load-bearing for fund safety; the current impl is unsafe to expose
this way (submitterData-controlled to would let any caller drain), so the path also requires committing to/token/amount into the leaf — a redesign, not a drop-in.
one-time mark. Replay is bounded only by deadline and clone balance — a refund of the signed amount lets the same signature drain again. SignedWithdraw is worst-case
(committed (token, to, amount) triple → re-drains the same to). Options:
a) Operational only (current): short deadlines + signer-side bookkeeping. Zero overhead, fully reliant on signer discipline.
b) Monotonic nonce: forces ordered consumption — bad fit for permissionless executors.
c) One-time signature marks (recommended): mapping(bytes32 sigHash => bool used), ~1 SSTORE per execute, out-of-order consumption. Shared ERC-7201 mixin across
CCTP/OFT/SpokePool; normal storage in AdminWithdrawManager.
Closes ACP-106