feat: Upgradeable Counterfactuals

[tbwebb22]

No description provided.

[tbwebb22]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e316447618

ℹ️ 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]

Pass the new constructor args in deploy scripts

Adding the mandatory beacon constructor argument here breaks the inspected counterfactual deployment flow: script/counterfactual/DeployCounterfactualDeposit.s.sol:18 still deploys type(CounterfactualDeposit).creationCode without abi.encode(beacon), so the init code is missing the constructor data and the deploy-all path predicts/deploys the wrong bytecode. The same stale pattern remains for CounterfactualDepositFactory (no beacon arg) and for CCTP/OFT (scripts still pass two args after the new signer arg), so this upgrade cannot be deployed via the repo's documented scripts until those init-code builders and predictions are updated.

Useful? React with 👍 / 👎.

[tbwebb22]

Yes, deploy scripts have not been updated yet

[droplet-rl]

Summary

Nice, well-scoped refactor. The architecture (BeaconProxy + dispatcher + per-bridge impls + signed runtime fees) is internally consistent and matches the very detailed DESIGN.md. All 48 counterfactual-related tests pass locally under FOUNDRY_PROFILE=local-test, including the new beacon-retargeting, updateRootAndExecute, checkStableExchangeRate flag, and Tron leaf-impl variants.

I have no blocking concerns. Verdict: COMMENT with a handful of small items, mostly stale docs/comments that don't match the post-D27/D28 (beacon + no-versioning) world, plus one minor consistency nit on the beacon initializer and one merge-hygiene note.

What I looked at

• Core: CounterfactualBeacon, CounterfactualDeposit, CounterfactualDepositFactory(Tron).
• Per-bridge impls: SpokePool / CCTP / OFT (route params, EIP-712 fee sig, _checkFee, checkStableExchangeRate gate, source-chain binding) and the Tron variants.
• Withdraw: WithdrawImplementation(Tron) + AdminWithdrawManager (direct + signed paths).
• Tests: all suites listed in Phase 4 of DESIGN.md.

Architectural notes (positive)

• Storage-slot hygiene: beacon (across.counterfactual.beacon.storage) vs proxy state (across.counterfactual.upgradeable.storage) are distinct ERC-7201 namespaces; no risk of collision when the beacon-target is delegatecalled into a proxy frame.
• OZ BeaconProxy's constructor calls IBeacon(beacon).implementation() and then delegatecalls into it, which means the factory's first deploy() will revert if the beacon's implementation is still address(0) — operationally enforces the deploy ordering (beacon → impl → setImplementation → factory).
• updateRootAndExecute's skip-when-current pattern cleanly avoids the RootUnchanged revert in races where two relayers each try to bump+execute.
• SpokePool's typehash binding everything explicitly vs CCTP/OFT relying transitively on the periphery quote signature is well-justified given there's no SpokePool periphery in this path.

Merge hygiene — please rebase

The PR is forked from 8a45d6eb, one commit before master's #1446 (init Tron MulticallHandler). As a side effect, this diff currently shows contracts/handlers/MulticallHandler.sol reverted to its pre-#1446 state and contracts/handlers/TronMulticallHandler.sol (plus its test, its deploy script, and the package.json / script/tron/README.md references) deleted. Almost certainly accidental — please rebase against current master so #1446 is preserved on merge.

Stale docs/comments that didn't have a clean anchor inside the diff hunks

A couple that I'd flag separately because the affected file isn't part of this PR's diff or the line is in an unchanged hunk:

• contracts/periphery/counterfactual/CounterfactualDepositSpokePoolTr.sol (unchanged on this branch, follow-up suggestion): the rationale comment "each clone's address is derived via CREATE2 from its implementation address" was true under the old EIP-1167 clone model, but under BeaconProxy the proxy's address depends on (factory, salt, beacon, initialRoot) — the implementation is not in the proxy preimage at all. The stated protection (cross-impl signature replay) still holds, but for a different reason: the route impl address is committed in the merkle leaf, and SpokePool's typehash binds routeParamsHash. The Tron variant also deliberately inherits the same EIP-712 domain name, so the domain-name argument doesn't add anything either.
• contracts/periphery/counterfactual/CounterfactualDepositSpokePool.sol around line 50: a couple of unchanged docstrings still say "the clone address" (leftover EIP-1167 wording — under the new model it's the BeaconProxy address). Cosmetic but inconsistent with the rest of the (well-written) DESIGN.md.

Optional hardening

CounterfactualDeposit._execute does implementation.delegatecall(...) without validating the target is a contract. A delegatecall to an EOA succeeds as a no-op with empty returndata. The merkle-tree authoring is the trust boundary here (the leaf commits the address), so this is fine in principle — but a one-line if (implementation.code.length == 0) revert NotAContract(); would catch tree-builder mistakes loudly. Up to you; if rejected, worth noting why in DESIGN.md.

One DESIGN.md sanity check

Phase 0 still flags pinning a fixed solc / optimizer combo and adding a forge inspect <c> bytecode cross-chain parity check for the factory and CounterfactualBeacon as remaining work. With salt now caller-supplied (D30), automatic cross-chain parity only holds when the caller reuses salt = 0; the determinism guarantee on the substrate (factory + beacon address) is what makes that caller obligation meaningful. Worth confirming this Phase-0 bytecode-parity check is on the deploy checklist before any production deployment.

[droplet-rl]

Stale NatSpec. Per D28 (versioning removed) and D29 (rename), this interface no longer holds version / minRequiredVersion. The docstring still claims it does. Suggest replacing the last sentence with something like:

It also holds the (proxy, latestRoot) upgrade tree root that authorizes per-proxy root updates. Root updates are best-effort — there is no on-chain version/freshness gate.

Keeping the stale text will mislead an auditor reading the interface in isolation.

[tbwebb22]

37f1aec

[droplet-rl]

Consistency gap. setImplementation enforces newImplementation.code.length != 0, but initialize calls _setImplementation(implementation_) directly with no such check. The docstring above explicitly permits address(0) for lazy init, which is fine — but it would also silently accept an arbitrary non-zero EOA (a typo'd address), which the public setter would have rejected.

In practice, the first factory.deploy() would revert because OZ's BeaconProxy constructor calls IBeacon(beacon).implementation() and then delegatecalls into it, so a bad address surfaces quickly — but it's still a UX gap. Suggest:

if (implementation_ != address(0) && implementation_.code.length == 0) revert NotAContract();
_setImplementation(implementation_);

Keeps the "may be address(0) and set later" affordance but rejects EOAs.

[tbwebb22]

98c9032

[droplet-rl]

Small precision nit on this caveat: the SpokePool impl's typehash does bind routeParamsHash, so the cross-leaf scenario you describe (prove leaf A, submit B's signature) is only a concern for CCTP/OFT — where the typehash is (nonce, executionFee, signatureDeadline) and the route/amount are bound only transitively via the periphery quote signature.

For CCTP/OFT it then collapses to a nonce-uniqueness invariant on the periphery: a single nonce can't authorize two different (route, amount) tuples. The tree-builder invariant (“never two leaves with the same implementation”) is still a fine belt-and-braces rule, but the failure mode is bridge-specific. Tightening the language here would make this easier to verify in audit.

[tbwebb22]

46c5d99

[droplet-rl]

Subtle but worth documenting: when newRoot == activeRoot(), the updateProof arg is never validated. The test testUpdateRootAndExecuteSkipsUpdateWhenCurrent deliberately passes a bogus proof and confirms it's ignored. That's the intended D32 behavior, but a caller reading the diff might think they're getting a fresh upgrade-tree authorization check on every call. A one-line @dev note like “updateProof is ignored when the proxy is already at newRoot” would make this less surprising.

[tbwebb22]

46c5d99

[droplet-rl]

Worth confirming explicitly in DESIGN.md (Open Question #2 / pre-deployment ordering) what happens if a relayer reuses a fee signature within signatureDeadline and the user re-funds the proxy. The CCTP periphery's nonce-uniqueness check is what blocks the bridge call, but the local _verifySignature here only binds (nonce, executionFee, signatureDeadline) — so a replay attempt would pay this execution fee, then fail at the periphery, then revert the whole tx (atomic). Good — but only because the fee payout precedes the periphery call in the same tx. Worth a one-line comment near this safeTransfer making that atomicity-ordering load-bearing claim explicit. (Same applies to OFT.)

[tbwebb22]

8a55bbc