feat: chain-agnostic counterfactual leaves via beacon-provided config#1456
Conversation
Move every chain-specific value (bridge endpoints, CCTP domain / OFT EID, fee signer, token addresses) out of the leaf-implementation immutables and the merkle leaf params, and onto the per-chain CounterfactualBeacon as public immutables. Leaf implementations resolve the beacon from the proxy's ERC-1967 beacon slot (CounterfactualImplementationBase) and read what they need at runtime; the input token is fixed per implementation. Result: leaf implementations are byte-identical across chains (one CREATE2 address everywhere) and a single leaf is valid on every chain, so initialRoot carries one leaf per route instead of one per source chain. Drops sourceChainId and the SourceChainMismatch check; an unconfigured route reverts RouteNotConfigured. - CounterfactualBeacon: add immutable chain config (CounterfactualChainConfig) with named getters (signer, spokePool, wrappedNativeToken, cctp*, oft*, usdc, usdt); implementation/upgradeRoot stay mutable. Config changes are UUPS upgrades. - CounterfactualBeaconBootstrap: chain-identical no-arg UUPS impl so the beacon proxy deploys to the same address on every chain (bootstrap -> upgrade). - SpokePool impl is now abstract with per-token variants (Usdc, Native) and a distinct EIP-712 domain name each, preventing cross-variant signature replay. Tron variant resolves USDT. - Signer read from beacon.signer() (rotatable via beacon upgrade). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CounterfactualTestBase now exposes _deployBeacon(CounterfactualChainConfig) (the beacon config is immutable, so each test builds it from its mocks) and _baseConfig(). Bridge tests construct no-arg impls, set endpoints/tokens/signer on the beacon, and drop sourceChainId + the token field from leaf params. SpokePool tests split into Usdc/Native variants (distinct EIP-712 domain names) and add cross-variant and RouteNotConfigured coverage; Tron test uses the USDT variant. 91 tests pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- DeployCounterfactualBeacon (new): bootstrap -> ERC1967Proxy (chain-invariant init calldata) -> chain-specific CounterfactualBeacon impl -> upgradeToAndCall -> dispatcher -> setImplementation, keeping the proxy address identical per chain. - CounterfactualConfig._buildChainConfig() resolves the CounterfactualChainConfig from constants/deployed-addresses (signer, spokePool, wrappedNativeToken, cctp*, oft*, usdc, usdt), tolerating missing values as address(0). - Impl deploy scripts are now no-arg; SpokePool deploys Usdc + Native variants; add a VanillaCCTP deploy script. - CheckCounterfactualDeployments reads chain config from the beacon's getters (impls no longer expose them); impl checks become bytecode presence checks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Rewrite Address Determinism (one leaf per route, not per source chain), Route leaves (no sourceChainId/token; token fixed per impl), calldata injection (#3 chain-specific values from the beacon), Upgrade Mechanism (Route Trees are now chain-invariant; no sourceChainId defense-in-depth), Execution Fees (signer read from beacon.signer()), Vanilla CCTP, and the Trust Model (admin also owns the immutable, upgrade-gated chain config). Add a Chain Configuration section documenting the beacon getters, the immutable-config model, and the bootstrap->upgrade deploy for proxy parity. 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: 82dda39e79
ℹ️ 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".
Replace the per-token SpokePool variants (…Usdc / …Native) with a single input-token-agnostic CounterfactualDepositSpokePool. The leaf carries the beacon getter selector for its input token (`inputTokenGetter`); bytes4(0) means a native deposit (wrapped via beacon.wrappedNativeToken()). The impl resolves it via a guarded staticcall to the beacon, so one implementation serves any registered token. Because inputTokenGetter is part of params, it is committed in routeParamsHash (which the SpokePool EIP-712 fee signature binds) — so a signature for one token never validates for another, and cross-chain replay is independently prevented by the chainId in the EIP-712 domain. This removes the need for per-token variants and per-variant EIP-712 domain names; the Tron variant once again only overrides transfer semantics and inherits the mainline name. CCTP / Vanilla CCTP / OFT are unchanged (USDC-only, read beacon.usdc() directly). Updates the SpokePool deploy script (one impl), DeployAll/Check scripts, tests (adds arbitrary-token and cross-token-signature coverage), and DESIGN.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
🤖 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 pass after addressing the four codex threads. Diff vs base reads cleanly end-to-end (registry, leaf impls, deploy scripts, tests, docs). No additional bugs or regressions surfaced beyond what codex flagged; all 64 counterfactual local-foundry tests pass after the fixes.
Reviewed in particular:
- Beacon validation (
_validateImplementationrequires the target'sBEACON()to bind back) — sound, catches the catastrophic admin-misclick. - Bootstrap → UUPS upgrade flow on the registry proxy — chain-invariant init calldata is preserved (bootstrap owner = deterministic deployer, not the per-chain multisig).
- SpokePool's
inputTokenGetterstaticcall guard (ok && ret.length == 32) — correctly demotes malformed returns toaddress(0)→RouteNotConfigured. - Fee-signature scoping:
chainId(EIP-712 domain) +address(this)(clone viaverifyingContractor explicitclone) +routeParamsHash(which now commitsinputTokenGetter) — cross-chain, cross-clone, cross-token replay all blocked. - OFT impl after my fix: reads input token from
periphery.TOKEN()(single-token by construction), so chain-identical bytecode preserved, nobeacon.usdc()dependency, works for the USDT0 periphery deployments already on-chain.
Nothing further to flag. Ready for re-run of the codex review against the new HEAD.
- OFT impl resolves input token from periphery.TOKEN() (token-agnostic): every currently deployed SponsoredOFTSrcPeriphery is USDT0, so hardcoding beacon.usdc() bricked the OFT route on every chain. - Re-add deploy-time CCTP/OFT periphery guards in DeployAllCounterfactual so the beacon does not bake address(0) and silently brick routes. - Require Tron USDT in _buildChainConfig so the Tron SpokePool route cannot be deployed with usdt=0. - Transfer beacon ownership in the all-in-one transferRoles flow so the beacon admin does not stay on the deployer EOA. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Babysit pass on PR #1456 done — addressed all 4 codex review threads and self-reviewed. Codex threads addressed (all queued to resolve on push):
Self-review: no further bugs flagged. Submitted a body-only review (#4450937553) noting what was checked: beacon validation, bootstrap→UUPS flow, SpokePool selector guard, fee-signature scoping. Tests: all 64 local foundry counterfactual tests pass. Commit pushed by checkpoint: |
|
Ask again to re-run the unfinished work. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad8c10c5f5
ℹ️ 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".
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Self-review pass after addressing the four new codex threads. All 64 counterfactual local-foundry tests pass; counterfactual profile compiles clean.
Reviewed in particular:
_beaconWiredTostaticcall: guarded against revert + malformed return (!ok || ret.length != 32), and the underlying re-run path is safe (CREATE2s idempotent,upgradeToAndCallno-ops on same impl,setImplementationskips when already pointing at the dispatcher).- Beacon owner/
pendingOwnerreview in the readiness script: same shape as the existing AdminWithdrawManager owner review, so the operator sees consistent output and can spot an in-flight Ownable2Step transfer. - Extractor special-case scope: only fires when
scriptName === "DeployCounterfactualBeacon.s.sol", so other ERC1967Proxy deployments (SpokePool proxies) continue to be rewritten as before. - SpokePool require placement: in
_buildChainConfigso it triggers once at beacon-impl construction time, paired with the existing Tron USDT require, both giving the operator a precise message about which lookup source to fix.
Nothing further to flag.
- DeployAllCounterfactual: verify beacon proxy actually resolves the expected dispatcher (guarded staticcall) before skipping the beacon stack sub-script, so an interrupted previous deploy doesn't leave the proxy on the bootstrap or with a stale target. - ExtractDeployedFoundryAddresses: special-case the beacon deploy script so the ERC1967Proxy is recorded as `CounterfactualBeacon` (canonical address) and the per-chain implementation deploy is skipped; without this, the generic ERC1967Proxy→SpokePool rewrite would misfile the beacon proxy and overwrite SpokePool. - CheckCounterfactualDeployments: add manual review of the beacon owner and pendingOwner against config.toml and multisigs.json, mirroring the AdminWithdrawManager owner review so a deployer-EOA beacon admin is surfaced instead of silently passing. - CounterfactualConfig: require `spokePool != 0` in `_buildChainConfig` so a missing deployed-addresses.json entry fails at deploy time rather than baking address(0) into an immutable on the beacon implementation. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Babysit pass on PR #1456 done — addressed all 4 new codex threads from review #4451087262. Threads addressed (all queued to resolve on push):
Self-review: no further bugs flagged. Submitted body-only review #4451168978. Tests: all 64 local foundry counterfactual tests pass; counterfactual profile compiles clean. Commit pushed by checkpoint: |
|
Ask again to re-run the unfinished work. |
|
@codex review |
1 similar comment
|
@codex review |
|
Saw the |
Summary
Testing / Checks
|
|
No-op pass — codex's re-trigger comment confirmed all 8 prior review threads are resolved on HEAD (For the worktree state codex flagged: my checkout shows only the two unrelated submodule pointer drifts from session setup; the |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 22ea141485
ℹ️ 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".
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Self-review pass after addressing the two new codex threads on 22ea1414. All 64 counterfactual local-foundry tests pass; counterfactual profile compiles clean.
Reviewed in particular:
- Tron SpokePool branching: identical decision used in both the standalone deploy script and the orchestrator's prediction (
block.chainid == 728126428→CounterfactualDepositSpokePoolTr.creationCode). CREATE2 addresses correctly diverge from the mainline impl, so a Tron leaf must name the Tr address. _warnIfBeaconConfigStaleonly fires on the skip path, so a fresh beacon deploy doesn't run a redundant comparison. Covers all 10 immutable fields (signer/spokePool/wrappedNativeToken/cctp{SrcPeriphery,TokenMessenger,SourceDomain}/oft{SrcPeriphery,SrcEid}/usdc/usdt), and the remediation message points to the exact UUPS recipe.- The stale-config helper calls
_buildChainConfig(), which inherits the new SpokePool/Tron USDT requires — so a stale-config check on a chain missing SpokePool reverts loud with the precise lookup-source guidance. Desirable behavior. - Tron canonical-deploy boundary documented in the SpokePool script docstring: the Tron production path remains the Tron TS script (uses Tron's solc fork); the Foundry branch is the defensive path.
Nothing further to flag.
- DeployCounterfactualDepositSpokePool + DeployAllCounterfactual: branch on Tron (chainid 728126428) to deploy `CounterfactualDepositSpokePoolTr` instead of the mainline impl, so Tron USDT's non-standard `transfer` return value doesn't revert execution-fee payouts on the Foundry deploy path. The canonical Tron production deploy remains the TS script under `script/tron/counterfactual/`; this branch is the defensive Foundry path. - DeployAllCounterfactual: on the beacon-stack skip path, compare every field of the live beacon's immutable chain config against the values the current resolvers produce. Per-field mismatches are logged and a summary warning explains the UUPS-upgrade remediation, so a previously deployed beacon with a stale config no longer hides behind an "ALREADY DEPLOYED" message. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
Babysit pass on PR #1456 done — addressed both new codex threads from review #4451554232. Threads addressed (both queued to resolve on push):
Self-review: no further bugs flagged. Submitted body-only review #4451632558. Tests: all 64 local foundry counterfactual tests pass; counterfactual profile compiles clean. Commit pushed by checkpoint: |
|
Ask again to re-run the unfinished work. |
|
@codex review |
|
Codex Review: Didn't find any major issues. 🚀 ℹ️ 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". |
|
No-op pass — codex re-reviewed HEAD |
There was a problem hiding this comment.
Should maxExecutionFee be looked up by chain as well? I assume we'd have mainnet configured with a different maxExecutionFee than L2s.
There was a problem hiding this comment.
Good question — leaving this open for the design call, but here's how I read the trade-off:
Current design. maxExecutionFee is in the leaf and committed by routeParamsHash, which the EIP-712 fee signature binds. It's the load-bearing safety cap against a compromised signer: if the off-chain signer is exfiltrated, the worst they can do is sign executionFee = maxExecutionFee and drain that amount per deposit — the merkle root is the upper bound, and changing it requires a new upgradeRoot (which the current activeRoot doesn't authorize per clone). The signer is by design less-trusted than the beacon admin.
If we move it to the beacon (e.g. beacon.maxCctpExecutionFee()), the cap becomes per-chain and per-route by config rather than per-leaf, which matches the mainnet-vs-L2 cost reality you're pointing at. But the cap is then mutated by a UUPS upgrade of the registry implementation (the values are immutable), so:
- Tightening a cap downwards is easy and safe (multisig deploys a new beacon impl with a smaller value, upgrades).
- The backstop against the signer shifts from "merkle commitment" to "beacon admin won't raise this" — which is the same trust we already place in the beacon admin for retargeting
implementation()and for the entire chain config. So strictly: not weaker than the rest of the model.
Middle option. Keep maxExecutionFee in the leaf as the absolute backstop (sized to the most expensive chain, e.g. mainnet), and add an additional beacon.maxXxxExecutionFee() per-chain cap that the leaf also enforces. That gives you both per-chain tuning AND a merkle-committed absolute ceiling, at the cost of a few SLOAD-equivalents and three more beacon fields (CCTP, OFT, SpokePool execution-fee caps).
Leaning toward the middle option since it preserves the safety property the current design buys, but I don't want to ship the change without your + @mrice32 sign-off — it touches the CCTP/OFT/SpokePool leaf encodings (and therefore the merkle commitments callers have started building against). Happy to implement whichever you want.
|
Babysit pass on PR #1456 — replied to tbwebb22's open thread on Thread context: tbwebb22 asked whether My reply summarizes the trade-off:
Leaning toward the middle option in my reply, but explicitly declined to ship the change without their sign-off — the leaf-encoding change ripples through every consumer's merkle commitment, so it needs the design call to come from the PR author. Other threads: 16 prior threads remain resolved on HEAD |
| // Pull the input token from the periphery's immutable `TOKEN` so the leaf works for whichever | ||
| // ERC-20 the chain's periphery was deployed for (USDC, USDT0, etc.). The periphery enforces | ||
| // non-zero `_token` at construction, so we don't re-check here. | ||
| address inputToken = ISponsoredOFTSrcPeriphery(oftSrcPeriphery).TOKEN(); |
There was a problem hiding this comment.
so currently this can only support a single token through OFT per chain. Is that alright?
There was a problem hiding this comment.
Correct — as it stands, this impl is one OFT periphery per chain, so one token per chain. The beacon has a single oftSrcPeriphery field, the SponsoredOFTSrcPeriphery is single-token by construction (TOKEN is set in the constructor and validated against the OFT messenger), and the leaf trusts whatever that periphery says is its token. Matches Across' current deployments — every SponsoredOFTSrcPeriphery per script/mintburn/oft/usdt0.toml is USDT0.
If we ever need multi-token OFT per chain (e.g. USDC OFT + USDT0 OFT live on the same chain), the natural extension is the same inputTokenGetter selector pattern the SpokePool leaf uses:
- Add per-token periphery getters on the beacon (
oftUsdcPeriphery,oftUsdtPeriphery, …) —oftSrcEidstays single since it's a property of the chain's LayerZero endpoint, not the periphery. - Add a
bytes4 peripheryGettertoOFTRouteParams; the leaf resolves it through the existing guarded-staticcall helper, then reads.TOKEN()from the returned periphery. Same merkle-commitment property (the selector is bound byrouteParamsHash). - One leaf per (token, route) pair, still chain-agnostic.
For the current scope I think keeping single-token is fine — it matches the deployment reality and lets us ship without adding new beacon fields no one's using yet. Happy to ship the selector-based generalization in a follow-up the moment you decide a second OFT token is on the roadmap. Leaving this unresolved while you weigh it.
|
Babysit pass on PR #1456 — replied to tbwebb22's new thread on Thread context: tbwebb22 asked whether the OFT impl supporting only a single token per chain is acceptable. My reply:
Other threads:
No code changes this pass, no commit. |
|
So we have to upgrade the beacon impl every time we want to support a new src token right? This feels kind of weird since the beacon is so critical to the rest of the system (holding upgrade root and impl for all proxies). Thoughts on separating out the lookup logic into a separate contract than the beacon? |
|
Good push — agree the current shape concentrates "boring config tweaks" and "registry surgery" on the same UUPS proxy, which makes adding a new src token weightier than it should be. Concrete sketch of how I'd split it, leaving the call to @mrice32 / you on whether to land it in this PR or a follow-up: Two contracts:
Add-a-token workflow then becomes:
Blast-radius improvement:
Leaf-impl impact (small):
Cost:
Migration: since nothing's deployed yet on this PR, this can land in-PR with no compat shims. If you'd rather merge as-is and split in a follow-up, I'm fine either way — the touch is mechanical and well-tested by the existing 65 counterfactual tests once translated. Holding off on writing code until you and @mrice32 agree on direction. |
| address usdc; | ||
| address usdt; |
There was a problem hiding this comment.
Thoughts on assigning IDs to tokens? Feels a little weird to identify them by their symbol/ticker here since that isn't guaranteed to be unique. This is a nit tho
There was a problem hiding this comment.
Nit ack'd, and I think it dovetails nicely with your earlier "separate lookup from the beacon" comment — the fix is the same.
Today the field is address usdc; but what the merkle leaf actually commits to is the function selector keccak256("usdc()")[:4], which is genuinely unique. The ticker name is just human-readable shorthand for "whatever this chain's USDC ERC-20 is" — drift could happen if we renamed the function (compile breaks, merkle commitment breaks too — caught at deploy), but the on-chain identifier is the selector, not the string.
That said, the concern about identifying tokens by hard-coded fields at all (vs. an opaque ID space) is real, and the cleanest answer is a mapping pattern that also retires the "UUPS-upgrade-per-new-token" workflow:
// On the registry (or its split-out ChainConfig sibling, per your earlier comment):
mapping(bytes32 id => address) private _tokens;
function token(bytes32 id) external view returns (address);
function setToken(bytes32 id, address token) external onlyOwner; // mutable storage, not immutableThe leaf's inputTokenGetter switches from bytes4 selector to bytes32 id (e.g.
keccak256("USDC")), and resolution is a SLOAD instead of a staticcall to a baked-in getter. Adding USDT0 / FRAX / whatever then means one setter call by the multisig, not a fresh CounterfactualBeacon deploy + UUPS upgrade. Selectors-as-IDs were the right shape when the field list was hardcoded; once we want extensibility, an opaque ID space is the right shape.
The semantic identity question (USDT vs USDT0 vs Bridged-USDC) is operator-managed in either world: whoever issues the off-chain quote / leaf has to pick an ID that means a specific token on a specific chain, and the beacon binding makes it authoritative. With explicit IDs (keccak256("USDT0") vs keccak256("USDT")) the ambiguity is at least made visible at quote-construction time.
Leaving this open and tagging it with the earlier "split lookup out" thread — both want the same refactor and should probably be designed together. Holding off on writing it until you and @mrice32 land on a direction.
There was a problem hiding this comment.
I think readable names is probably best, but we may want to make them more specific if there are collision issues we're worried about.
|
Babysit pass on PR #1456 — replied to tbwebb22's new inline thread on Thread context: tbwebb22 — "Thoughts on assigning IDs to tokens? Feels a little weird to identify them by their symbol/ticker here since that isn't guaranteed to be unique. This is a nit tho." My reply:
Status of open threads (still 3 unresolved, all open design questions):
No code changes this pass, no commit. HEAD remains |
…ector The OFT leaf now carries a `peripheryGetter` — the 4-byte selector of the beacon getter for the SponsoredOFTSrcPeriphery to use. Each OFT periphery is single-token (immutable TOKEN()), so naming the periphery selects the input token, which the impl reads from the resolved periphery. Supporting another OFT token is a beacon upgrade adding another periphery getter — no leaf/impl change. - Beacon: add oftUsdcPeriphery alongside oftSrcPeriphery (per-token OFT peripheries; leaf names which via selector). Config resolver + DeployAll stale-check + Check updated. - Move the guarded selector→address staticcall into CounterfactualImplementationBase as the shared _resolveBeaconAddress; SpokePool and OFT both use it (SpokePool's private _resolveInputToken removed). - OFT test: two single-token peripheries wired; new test proves a second token routes through its periphery via OFT_USDC_GETTER while the primary periphery is untouched. - DESIGN.md: OFT route is now selector-driven like SpokePool. maxExecutionFee intentionally stays in the leaf params (per-chain move deferred). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Make the NatSpec and inline comments across the counterfactual contracts, deploy scripts, and tests more concise — cut redundancy and over-explanation while keeping the correctness/security rationale (fee-before-periphery ordering, RouteNotConfigured gating, EIP-712 binding, bytecode pinning, prank-before-external-call test gotchas). Comments only; no code changes (build + 94 tests unchanged). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
USDC bridges via CCTP, not LayerZero OFT, so a "USDC OFT periphery" doesn't exist; oftUsdcPeriphery was a wrongly-named placeholder added to demo multi-token. Remove it from the beacon (struct/immutable/getter), config resolver, deploy/check scripts, and DESIGN. The selector mechanism stays: the OFT leaf carries `peripheryGetter`, the beacon exposes oftSrcPeriphery (USDT0), and real additional OFT tokens are onboarded by adding a periphery getter via a beacon upgrade. The OFT test now proves the selector is genuinely consulted (RouteNotConfigured when the named getter is unset) instead of routing a fake USDC periphery. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Move the execution-fee cap out of the leaf as a hardcoded value and onto the per-chain beacon, named by a `bytes4 maxExecutionFeeGetter` the leaf carries (mirroring inputTokenGetter/peripheryGetter). The impl resolves it with the new `_resolveBeaconUint`. For CCTP/Vanilla/OFT it's the executionFee cap; for SpokePool it's the fixed component of the combined cap (`maxFeeBps` stays in the leaf). Add example per-(token,bridge) cap getters on the beacon (immutable uint256): usdcCctpMaxExecutionFee, usdtOftMaxExecutionFee, usdcSpokePoolMaxExecutionFee, usdtSpokePoolMaxExecutionFee, wethSpokePoolMaxExecutionFee. Vanilla CCTP reuses the USDC CCTP getter. Wired into the config resolver (optional config.toml reads, default 0), the DeployAll stale-check, and the Check script. Add a note on CounterfactualBeacon that its immutable config values are pure configuration — a new implementation changing only those is not subject to audit; only logic changes are. Tests updated to set caps on the beacon and name them via selectors. 94 pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fig) Move root/implementation management, UUPS, and Ownable2Step admin into a new abstract CounterfactualBeaconBase. CounterfactualBeacon now derives from it and holds only the chain-specific immutables (endpoints, tokens, fee signer, fee caps) plus the constructor wiring. This keeps the audit boundary clean: the base is the reviewable logic, while a config-only change is a new derived contract that touches no logic. Static references to inherited members (initialize, implementation, NotAContract, WrongBeacon) are repointed to CounterfactualBeaconBase, since inherited members are not reachable via the derived contract type. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb418142eb
ℹ️ 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 (dispatcher == address(0)) { | ||
| _fail("CounterfactualBeacon", "implementation", "CounterfactualDeposit not in deployed-addresses.json"); | ||
| } else { | ||
| _assertAddrEq("CounterfactualBeacon", "implementation", beacon.implementation(), dispatcher); |
There was a problem hiding this comment.
When the beacon proxy address exists but a previous DeployCounterfactualBeacon run stopped before upgrading it off CounterfactualBeaconBootstrap, this high-level beacon.implementation() call reverts because the bootstrap has no implementation() selector. That aborts CheckCounterfactualDeployments instead of reporting the intended [FAIL] for an incomplete deployment, so the verifier cannot diagnose exactly the partial-deploy state this new beacon bootstrap flow can leave behind; mirror the low-level staticcall pattern used by _beaconWiredTo before reading the rest of the beacon getters.
Useful? React with 👍 / 👎.
droplet-rl
left a comment
droplet-rl
left a comment
There was a problem hiding this comment.
Overall this is a strong refactor — the chain-agnostic-leaves architecture is correctly anchored on the BeaconProxy + per-chain CounterfactualBeacon shape, and the selector-based input-token / OFT-periphery / fee-cap pattern is consistent across all four bridge impls. Splitting CounterfactualBeaconBase (audit-scope logic) from CounterfactualBeacon (per-chain immutable config) is a clean separation and the audit-boundary docstring makes the intent explicit. 68 local foundry tests pass and exercise the relevant cross-leaf paths (cross-clone replay, RouteNotConfigured, native-vs-ERC20, alt-chain ERC-20-as-native, OFT periphery selector, etc.).
Security review focus areas — what I checked and what looked sound:
- Signature scope: every leaf (
SpokePool/CCTP/VanillaCCTP/OFT) now bindsrouteParamsHashin its EIP-712 typehash, so a fee signature cannot validate against a different leaf in the same clone (and the chain ID +verifyingContract = address(this)block cross-chain / cross-clone replay).cb418142was the right fix for OFT and CCTP — keep that property as more bridges are added. - Native vs ERC-20 branching by resolved value (rather than
bytes4(0)sentinel selector) is the right decoupling. Tests cover both flavors of thenativeToken.selectorleaf. - Beacon target validation (
_validateImplementationchecksIBeaconTarget(impl).BEACON() == address(this)) is a good guard against the catastrophic admin retargeting footgun. - Bootstrap → upgrade flow for chain-invariant proxy address is well thought out; doc on bytecode pinning is in the right place.
A few items I'd flag for follow-up (inline). None are blocking — all are either design-direction questions, sharper failure modes, or documentation tightening. The two open design threads from tbwebb22 (per-chain maxExecutionFee already addressed in 32091ecc; multi-token OFT addressed in 7ec8d2c6) are now resolved on HEAD; the remaining open thread about separating lookup logic from the beacon stays a future-direction discussion.
Verdict: COMMENT — looks good to ship once the Tron-impl-address note (and ideally a deploy-time safety net for Bootstrap pinning) are addressed.
| /// @dev Resolve a uint from a no-arg `() -> uint256` beacon getter named by `getter`'s selector (carried | ||
| /// in the leaf, e.g. `beacon.usdcCctpMaxExecutionFee.selector`). Reverts `RouteNotConfigured` if the | ||
| /// getter doesn't exist; a configured value of 0 is valid and returned as-is. Merkle/signature-bound. | ||
| function _resolveBeaconUint(bytes4 getter) internal view returns (uint256) { |
There was a problem hiding this comment.
_resolveBeaconUint only checks ret.length == 32, which makes any () -> uintN getter ABI-compatible — including ones whose semantic type is wrong for a fee cap (e.g. cctpSourceDomain.selector, which returns uint32). The selector is committed in routeParamsHash and signed by the off-chain signer, so this isn't an attack vector — it's a footgun for whoever bakes the leaf. If the signer accidentally signs a leaf with the wrong maxExecutionFeeGetter selector, the leaf would silently use a small domain ID as the fee cap and lock the route below realistic relayer fees.
No blocker, but worth either (a) a deploy-time check that any selector chosen as a maxExecutionFeeGetter matches one of the fee-cap getter names, or (b) tagging the cap getters in a separate namespace (e.g. all …MaxExecutionFee prefixed and asserted in CheckCounterfactualDeployments).
| * only transfer semantics, not the signed outcome. | ||
| * @custom:security-contact bugs@across.to | ||
| */ | ||
| contract CounterfactualDepositSpokePoolTr is CounterfactualDepositSpokePool { |
There was a problem hiding this comment.
The Tron variant lands at a different CREATE2 address than the mainline CounterfactualDepositSpokePool (different bytecode by construction), so a multi-chain route that includes Tron needs two leaves per route — one naming the mainline impl, one naming the Tr impl. This is the right answer for Tron USDT's transfer semantics, but it does break the headline "one leaf per route" property for any path that touches Tron.
Would be good to call this out explicitly in DESIGN.md (the only mention I can find is in the deploy-script comment in DeployCounterfactualDepositSpokePool.s.sol), so leaf authors building multi-chain trees know to include the Tr impl as a separate leaf when their route covers Tron. Documenting the exception keeps it from being a surprise during integration.
| * | ||
| * **Bytecode pinning:** the chain-invariant proxy address depends on this contract's exact creation | ||
| * code, so any bytecode change (solc/optimizer/imports/AST) moves the Bootstrap and every dependent | ||
| * address. Once the canonical Bootstrap ships on the first chain, deploy later chains from the saved |
There was a problem hiding this comment.
The bytecode-pinning rationale is correct and well-explained, but right now the only enforcement is a docstring. If a future PR bumps the solc version in [profile.counterfactual], changes an OZ import, or anything else AST-affecting, the Bootstrap creation code shifts → the proxy address shifts → every counterfactual proxy address shifts, all without obvious feedback at deploy time.
A cheap deploy-time safety net: commit the canonical Bootstrap creation-code hash (or its predicted CREATE2 address) somewhere consultable (e.g. a constant in CounterfactualConfig or a checked-in JSON), and have DeployCounterfactualBeacon require that the freshly-compiled hash / predicted address matches. Then any accidental drift halts deployment with a precise error instead of silently moving the canonical address.
| /// `.WRAPPED_NATIVE_TOKENS.<chainId>` entry the sentinel would brick at execution (wrapped native 0 → | ||
| /// `RouteNotConfigured`), so we fall back to `address(0)` so the leaf cleanly RouteNotConfigured's. | ||
| function _resolveNativeToken() internal view returns (address) { | ||
| string memory path = string.concat(".NATIVE_TOKEN.", vm.toString(block.chainid)); |
There was a problem hiding this comment.
The current default (sentinel when wrappedNative exists, zero otherwise) handles the Lens/Tempo case correctly. One thing I'd nail down: the inverse pairing — cfg.nativeToken = NATIVE_SENTINEL but a later registry-upgrade scenario where wrappedNativeToken somehow ends up zero — is currently allowed by the struct (any combination of fields is permissible). The leaf reverts cleanly via _requireConfigured(beacon.wrappedNativeToken()), so it's fail-soft, but a deploy-time invariant in _buildChainConfig would make this a 'can't happen' rather than 'reverts safely if it ever happens':
require(
cfg.nativeToken != NATIVE_SENTINEL || cfg.wrappedNativeToken != address(0),
"config: nativeToken=sentinel requires wrappedNativeToken"
);Matching the existing SpokePool + Tron USDT require pattern.
| uint256 usdcSpokePoolMaxExecutionFee; | ||
| uint256 usdtSpokePoolMaxExecutionFee; | ||
| uint256 wethSpokePoolMaxExecutionFee; | ||
| } |
There was a problem hiding this comment.
The expanded chain-config struct (now including per-(token,bridge) max-execution-fee fields) reaches 16 immutables — every config tweak (new fee cap, new token getter, new periphery) becomes a full CounterfactualBeacon deploy + UUPS upgrade. That's by design and the audit-boundary doc makes the cost legible, but it's also the surface tbwebb22 was getting at in the earlier "separate lookup logic from the beacon" comment.
Not a blocker for this PR — but if/when the team picks up that follow-up, the mapping-based chainConfig(bytes32 id) pattern (with a setter on the registry) retires both this UUPS-per-config-tweak workflow and the ticker-name nit in one go. Worth scoping as a follow-up issue so it doesn't get lost.
_buildChainConfig now rejects a config where nativeToken is the native sentinel but wrappedNativeToken is unset. _resolveNativeToken upholds this for the default path, but returns a .NATIVE_TOKEN.<chainId> override verbatim, so an operator override could otherwise bake a sentinel that bricks at execution (the native branch reverts on a zero wrappedNativeToken). Fail loud at deploy instead, matching the existing spokePool/Tron-USDT guards. Addresses a review comment on PR #1456. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| ) | ||
| ); | ||
| if (ECDSA.recover(_hashTypedDataV4(structHash), submitterData.counterfactualSignature) != signer) | ||
| if (ECDSA.recover(_hashTypedDataV4(structHash), submitterData.counterfactualSignature) != _beacon().signer()) |
There was a problem hiding this comment.
do we need to check for _requireConfigured() for signer here?
There was a problem hiding this comment.
If signer isn't configured, then the EDCSA should always revert
There was a problem hiding this comment.
In theory, yes, but I think this is mostly an error message thing, since it would default to 0x0, meaning any signature would be invalid. Will defer this to post-audit.
Co-authored-by: Taylor Webb <taylor@Taylors-MacBook-Pro.local>
* fix cctp maxFee * add maxFee and minFinalityThreshold to signature check
tbwebb22
merged commit d257650
into
taylor/counterfactual-upgradeable
4 participants
Summary
Makes the counterfactual leaf implementations chain-agnostic. Every chain-specific value (bridge endpoints, CCTP domain / OFT EID, the fee
signer, and token addresses) moves out of the leaf-implementation immutables and the merkle leafparamsand onto the per-chainCounterfactualBeaconaspublic immutables. Leaf implementations resolve the beacon from the proxy's ERC-1967 beacon slot and read what they need at runtime; the input token is fixed by which implementation a leaf names.Result: each leaf implementation compiles to identical bytecode and one CREATE2 address on every chain, and a single leaf is valid everywhere — so
initialRootcarries one leaf per route instead of one per source chain.sourceChainIdand theSourceChainMismatchcheck are gone; an unconfigured route revertsRouteNotConfigured.What changed
CounterfactualBeacon— adds an immutableCounterfactualChainConfig(named getters:signer,spokePool,wrappedNativeToken,cctpSrcPeriphery,cctpTokenMessenger,cctpSourceDomain,oftSrcPeriphery,oftSrcEid,usdc,usdt).implementation/upgradeRootstay mutable storage. Because config is immutable, changing a value or adding a token is a UUPS upgrade of the registry implementation, not a setter.CounterfactualBeaconBootstrap(new) — a chain-identical, no-arg UUPS impl. The beacon proxy is deployed against it (chain-invariant init calldata: initialize to the deterministic deployer), thenupgradeToAndCall-ed to the chain-specificCounterfactualBeacon, keeping the proxy address identical on every chain.CounterfactualImplementationBase(new) — resolves the beacon from the proxy's ERC-1967 beacon slot so leaf impls hold no immutables.CounterfactualDepositCCTP/…VanillaCCTP/…OFTare now no-arg and read endpoints +beacon.signer()+beacon.usdc()at runtime.CounterfactualDepositSpokePoolbecomes abstract with per-token variants…SpokePoolUsdcand…SpokePoolNative(one token hardcoded per impl), each with a distinct EIP-712 domain name to prevent cross-variant signature replay on a shared proxy. The Tron variant resolves USDT.CounterfactualTestBasebuilds the immutable config per test (_deployBeacon(config)); all suites migrated, including new cross-variant-replay andRouteNotConfiguredcoverage. 91/91 pass.DeployCounterfactualBeacon(bootstrap → upgrade → dispatcher → setImplementation),_buildChainConfig()resolver, no-arg impl scripts + SpokePool variants, andCheckCounterfactualDeploymentsreads config from the beacon's getters.DESIGN.mdrewritten (Address Determinism, Route leaves, Upgrade Mechanism, Execution Fees, Trust Model) + a new Chain Configuration section.Testing
FOUNDRY_PROFILE=local-test forge test --match-contract 'Counterfactual|Withdraw|AdminWithdraw'→ 91 passed, 0 failed.FOUNDRY_PROFILE=local-test forge build(contracts + tests + all scripts) andFOUNDRY_PROFILE=counterfactual forge buildboth pass.Notes / follow-ups
usdtis not yet ingenerated/constants.json, so_buildChainConfigresolves it toaddress(0)everywhere (only needed for the Tron USDT SpokePool variant).cctpTokenMessengerresolves from thecctpV2TokenMessengerconstants entry — worth a sanity check before any real deploy.🤖 Generated with Claude Code