feat: counterfactual enumerated routes

[tbwebb22]

No description provided.

[grasphoper]

Seems like we can just use address(this) as the identity?

[tbwebb22]

yep agreed 763cfff

[tbwebb22]

@droplet-rl review

[tbwebb22]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8d37606981

ℹ️ 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".

[chatgpt-codex-connector]

Prevent initializing the dispatcher implementation itself

initialize is permissionless and only checks merkleRoot == 0, so anyone can initialize the standalone dispatcher contract (not just clones) and then call execute against an attacker-chosen merkle root. Because execute now reads merkleRoot from storage instead of clone immutables, this makes the implementation address itself executable and allows draining any ETH/ERC20 mistakenly sent there via a crafted leaf (e.g., using WithdrawImplementation). The previous design implicitly prevented this by requiring clone args during execute; this regression should be blocked by rejecting direct use of the implementation instance (or restricting initialization to factory-created clones).

Useful? React with 👍 / 👎.

[droplet-rl]

Summary

Strong design write-up in ENUMERATED_ROUTES.md and the on-chain pieces (CounterfactualMigrationRegistry, identity-keyed dispatcher with initialRoot folded into the CREATE2 salt, in-impl EIP-712 for CCTP/OFT) implement that design cleanly. The front-run analysis at deploy/initialize is sound, and using OZ's EIP712 under delegatecall is fine because the cached-domain check rebuilds the separator when address(this) != _cachedThis.

That said, I'm requesting changes — the headline issue is that no test file was updated, so this branch doesn't compile, let alone exercise any of the new behavior. A couple of smaller doc/code mismatches and one missing constructor guard noted inline.

Blocking

1. Tests broken; no coverage for new behavior. Every existing counterfactual test file still calls the old constructors:

   • test/evm/foundry/local/CounterfactualDeposit.t.sol:54-55 — new CounterfactualDeposit() / new CounterfactualDepositFactory()
   • test/evm/foundry/local/CounterfactualDepositCCTP.t.sol:74-76 — same, plus new CounterfactualDepositCCTP(srcPeriphery, SOURCE_DOMAIN) (now needs _signer)
   • test/evm/foundry/local/CounterfactualDepositOFT.t.sol:91-93 — same, plus new CounterfactualDepositOFT(srcPeriphery, SRC_EID) (now needs _signer)
   • test/evm/foundry/local/CounterfactualDepositSpokePool.t.sol:124-126
   • test/evm/foundry/local/Tron_Counterfactual.t.sol:85-87

   The new constructors require _migrationRegistry, _dispatcher, and _signer respectively, so forge build fails before any test runs. Beyond the compile break, none of the surfaces introduced here have coverage:

   • initialize guard (AlreadyInitialized, InvalidInitialRoot)
   • migrate happy path + NoOpMigration + stale-metaRoot rejection
   • CounterfactualMigrationRegistry (owner gating, transferOwnership, setMetaRoot events, zero-owner constructor revert)
   • chainId-in-leaf cross-chain isolation (set vm.chainId and prove a leaf is only valid on its chainId)
   • Same-address invariant across vm.chainId values for a fixed (identityHash, initialRoot)
   • Front-run protection (different initialRoot → different address; honest predicted address still reachable)
   • deployAndMigrateAndExecute — both the "migrate skipped because already at target root" and "migrate executed" paths
   • Dynamic executionFee for CCTP/OFT — happy path, tampered fee → InvalidSignature, expired signatureDeadline → SignatureExpired, paramsHash binding so a signature for route A can't be replayed against route B on the same impl

   The spec's "Tests" section already lists most of these; please port that list into actual .t.sol and fix the existing fixtures.

Notable, non-blocking

2. Spec ↔ code disagreement on executionFeeRecipient in the typehash. The spec (§5, "Implementations — dynamic executionFee") says the typehash binds, at minimum, "every signed amount/fee field, executionFeeRecipient, the route's paramsHash, and signatureDeadline". All three impls deliberately omit executionFeeRecipient (and SpokePool's natspec documents that as intentional — "submitter-chosen so any relayer can claim the fee"). The chosen design seems right; the spec line is the thing that should change.

3. MIGRATION_REGISTRY "compile-time constant" in spec vs. immutable in code. The dispatcher takes _migrationRegistry as a constructor arg and stores it as immutable. Functionally equivalent to a constant for cross-chain consistency (deployer passes the same value everywhere → identical bytecode → identical CREATE2 address), but the spec language at lines 287 and 336 still says "compile-time constant." Worth aligning the wording so future readers don't expect a constant.

4. Stale-root execution gap (already noted as Q4). The spec calls this out explicitly; flagging it so it doesn't get lost: between setMetaRoot and per-clone migrate, revocation-style updates (signer rotation after compromise, fee tightening, withdraw-admin rotation) remain executable on unmigrated clones. The "off-chain sweeper watches MetaRootUpdated and migrates every affected clone" mitigation is fine as a v1 stance, but it needs to ship with the sweeper and a metaRoot-lag dashboard before this can be relied on for revocations.

5. CCTP/OFT impl signatures are replay-safe within a deadline window only via balance consumption. No nonce in the impl typehash; if a user re-funds the clone before signatureDeadline, the same implSignature can fire again. Matches the SpokePool design and the spec rationale, but worth a one-liner in the CCTP/OFT natspec so re-funding within the deadline window isn't a surprise.

6. Permissionless initialize is safe only via the factory's atomicity. Currently safe because Clones.cloneDeterministicWithImmutableArgs deploys from the factory and the factory initializes in the same tx — only the factory can land a clone at the predicted CREATE2 address. A comment to that effect on initialize's natspec would help auditors not have to reconstruct the argument.

7. delegatecall to a code-less address silently no-ops. CounterfactualDeposit.sol:76 — delegatecall to an EOA or precompile returns success = true with empty returndata. The merkle proof check guarantees implementation matches an admin-approved leaf, so this is admin-discipline rather than a contract bug, but a cheap if (implementation.code.length == 0) revert ... would catch an admin typo in a leaf and cost almost nothing relative to the bridge call.

[droplet-rl]

Suggest a _migrationRegistry == address(0) check here. Today nothing rejects a misconfigured dispatcher; with a zero registry, migrate would call metaRoot() on address(0), which reverts with empty returndata and bubbles up as an unhelpful failure. Since the dispatcher's address derives from this arg, a misconfigured deploy is unrecoverable — fail loud in the constructor.

[tbwebb22]

added here 8e4eace @droplet-rl

[droplet-rl]

Took a look at 8e4eace — all six inline points are addressed, and the dispatcher lock is a nicer answer than what I asked for.

Dispatcher (CounterfactualDeposit.sol)

• ✅ _migrationRegistry == address(0) revert in the constructor.
• ✅ Bonus: IMPLEMENTATION_LOCK = bytes32(uint256(1)) written to the dispatcher's own merkleRoot in the constructor. This makes a direct initialize on the dispatcher hit AlreadyInitialized, and a direct execute find no provable leaf (any leaf collision with 1 is cryptographically infeasible). One small note: migrate still passes if a metaProof against (dispatcherAddr, newRoot) ever existed — purely theoretical since the admin would have to publish that leaf, but worth knowing the lock isn't load-bearing for migrate.
• ✅ initialize natspec now spells out the three-part safety argument (factory-only CREATE2 + atomic init + merkleRoot != 0 guard + dispatcher lock). Reads cleanly.

CCTP / OFT / SpokePool natspec

• ✅ Replay-window semantics now written down. The CCTP/OFT switch to binding nonce (instead of paramsHash) is actually a nicer answer — single-use replay protection falls out for free once the periphery records the nonce. SpokePool's natspec correctly distinguishes itself as the deadline-bounded case (no nonce → re-funding within the deadline window re-arms the same sig).

Spec (ENUMERATED_ROUTES.md)

• ✅ MIGRATION_REGISTRY → migrationRegistry immutable everywhere (text, Q1, D5, implementation plan).
• ✅ executionFeeRecipient explicitly called out as intentionally not bound. D9 now reflects the per-bridge route-binding choice (paramsHash for SpokePool, nonce for CCTP/OFT).

Still outstanding (from the original review): the test files haven't been updated, so forge build still fails on the old constructor signatures (new CounterfactualDeposit() etc.) across all five .t.sol fixtures, and there's still no coverage for initialize/migrate/the registry/cross-chain leaf isolation/deployAndMigrateAndExecute/dynamic executionFee. That's the gate I'd still want to see before this lands — happy to re-review once it's in.

[droplet-rl]

This bullet says the typehash binds executionFeeRecipient, but none of the three impls actually bind it (intentionally — SpokePool's natspec explicitly calls it submitter-chosen so any relayer can earn the fee). Update the spec to drop executionFeeRecipient from the "at minimum" list, or call it out as a per-bridge choice.

[tbwebb22]

@droplet-rl fixed here 8e4eace

[droplet-rl]

MIGRATION_REGISTRY is described here (and in step 2 of the implementation plan, line 336) as a compile-time constant. The actual dispatcher uses a constructor-set immutable. Functionally equivalent for the cross-chain-address argument, but worth aligning the wording so reviewers don't go hunting for a constant.

[tbwebb22]

@droplet-rl fixed here 8e4eace

[droplet-rl]

Consider documenting in the natspec (or near the deadline check) that the impl signature replay window is bounded by signatureDeadline and balance consumption, but not by a nonce — so if the clone is re-funded before the deadline, the same implSignature can fire again. This matches the design rationale in ENUMERATED_ROUTES.md D8, but it's currently only visible to readers who've read the design doc.

[tbwebb22]

@droplet-rl For CCTP and OFT, the signature is single use because it is bound to the nonce of the periphery signature

[droplet-rl]

Confirmed — looking at the current CounterfactualDepositCCTP.sol (after fdb5ad45), the typehash is ExecuteCCTP(bytes32 nonce,uint256 executionFee,uint32 signatureDeadline), so the local sig is bound to the same nonce the SponsoredCCTPSrcPeriphery consumes. Once the periphery records the nonce, both sigs are permanently unreplayable. Same story for OFT. The natspec on line 109-111 already states this cleanly ("single-use replay protection through periphery nonce consumption").

My original comment was written against the pre-cleanup diff that bound paramsHash only — the fix in fdb5ad45 made the concern moot for CCTP/OFT. SpokePool is the remaining deadline-only case, and its natspec already calls that out explicitly. Nothing further needed here.

[droplet-rl]

Suggest extending the natspec on initialize to spell out the safety argument: initialize is external with no caller check, but the only address that can deploy a clone at the predicted CREATE2 address is the factory, which calls initialize atomically in the same tx (so the merkleRoot != 0 guard then rejects any subsequent call). Without that argument written down, this reads as a missing access control.

[tbwebb22]

@droplet-rl fixed here 8e4eace