Repository navigation
feat(FraxFacet): address Frax external review — deprecated spokes, non-EVM (Solana) receivers, approvedOft pre-check (EXSC-382) - #2138
Merged
gvladika merged 5 commits intoAug 10, 2026
Conversation
…-only receiver, approvedOft pre-check (EXSC-382) Addresses the external review from the Frax team on PR #2048. Blocking fix - config/frax.json: remove berachain, mode and scroll from the "hop" address map and drop the mode (34443) / scroll (534352) "mappings" entries — Frax is deprecating those spokes from the hop mesh. DEV NOTICE records why so they do not get re-added. Hardening - EVM-only receiver: reject bridgeData.receiver == NON_EVM_ADDRESS with InvalidReceiver. HopV2's recipient is bytes32 and Frax routes to Solana (EID 30168) via the Fraxtal hub, but this version derives the recipient from the 20-byte receiver, so the sentinel would otherwise be bridged literally and the funds delivered to 0x11f1...f1. The inverse (non-EVM destination with an EVM receiver) is already covered: no non-EVM chain ID is in "mappings", so getFraxChainIdToEid reverts UnsupportedChainId. Adding a bytes32 non-EVM receiver to FraxData is deliberately left as a scoped follow-up. - HOP.approvedOft(oft) pre-check (added to IFraxHopV2, verified live on both the Fraxtal hub and the Arbitrum spoke): an OFT the hop does not route now reverts TokenNotSupported in _validateFraxData. Moved the oft.token() == sendingAssetId check there too, so both OFT invariants are enforced before any deposit or swap rather than after _depositAndSwap. Both paths revert atomically either way — this is a gas/error-clarity win, not a fund-safety one. - Re-pinned the Arbitrum fork block 483300000 -> 488500000 so the fork tests run against the upgraded RemoteHopV2 implementation. All tests pass. - docs/FraxFacet.md: document the approval trust assumption explicitly — LibAsset.maxApproveERC20 sets the allowance to type(uint256).max (the amount argument is only the required-allowance threshold), so the diamond carries a standing unlimited allowance to HopV2, an upgradeable proxy. Corrects the prior "scoped to the bridged amount" wording. Also documents the EVM-only scoping, the approvedOft pre-check, the deprecated spokes and the source-only chains (katana/somnia/stable/tempo have a hop but no destination mapping). Tests: 6 new (non-EVM sentinel and unapproved-OFT rejection on both entry points, plus both on the Tempo branch); 59 pass, 0 fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
lifi-action-bot
marked this pull request as draft
July 28, 2026 06:44
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe Frax facet adds centralized validation for receiver encoding, destination EIDs, OFT compatibility, and HopV2 approval, alongside non-EVM routing and chain-mapping removal. Configurations, signing formats, documentation, demos, mocks, and standard and Tempo tests were updated. ChangesFrax HopV2 validation and routing
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
requires-types
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
Test Coverage ReportLine Coverage: 90.50% (3460 / 3823 lines) |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
…val, fork-only tests (EXSC-382) Follow-up to the external-review round, addressing Daniel's review of #2138. removeFraxChainIdToEid — deprecation is now enforceable - initFrax copies config/frax.json mappings into diamond storage, so dropping a chain from config did nothing to diamonds already seeded: mode (34443) and scroll (534352) are still live on the Arbitrum staging diamond (verified on-chain). Both write paths reject lzEid == 0 and no removal path existed, so a seeded destination was permanent. - Adds owner-only removeFraxChainIdToEid(uint256[]) + FraxChainIdToEidRemoved. Reverts UnsupportedChainId on an unset entry so a mistyped removal fails loudly. Docs record that retiring a spoke is a two-step op (config + on-chain call) and that staging still needs the call. approvedOft before oft.token() - oft is caller-supplied, so token() was the first external call the facet made into an unvalidated address. Reordered: the hop's allowlist now gates it. Full non-EVM (Solana) support — replaces the EVM-only scoping - Solana is live on the hop today: quote(frxUSD, 30168, ...) returns a fee from the Arbitrum spoke (a bogus EID reverts), so the previous "unreachable" framing was wrong. - FraxData gains bytes32 nonEVMReceiver, passed to sendOFT verbatim, with BridgeToNonEVMChainBytes32 emitted. Sentinel and destination kind are cross-checked in BOTH directions: a 20-byte receiver on a non-EVM route would left-pad into an unspendable Solana account, and the sentinel on an EVM route would deliver to NON_EVM_ADDRESS. A populated nonEVMReceiver on an EVM route reverts rather than being silently ignored. - Non-EVM destinations are identified by _isNonEVMDestination (LIFI_CHAIN_ID_SOLANA); config seeds 1151111081099710 -> 30168. Ripples: clearSigningProposal.json struct signatures and demoFrax.ts updated for the new field. Fork-first tests (rule 400 [CONV:FORK-FIRST]) — no mocked counterparty - Unapproved OFT: real Arbitrum USDC. It has no token(), so reverting TokenNotSupported rather than a failed decode also proves approvedOft is evaluated first. - token() mismatch: real sfrxUSD OFT (hop-approved, underlying != frxUSD) instead of MockFraxOFT. - Solana routes bridge through the real hop on both entry points. - Dropped the mock-toggle approvedOft test; the Tempo mock now returns true unconditionally and MockFraxOFT is confined to the non-forkable Tempo suite. Version stays 1.0.0 (still iterating on the facet's first version). Tests: 72 pass, 0 fail (was 59). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| bool isNonEVM = _bridgeData.receiver == NON_EVM_ADDRESS; | ||
| bytes32 recipient = isNonEVM | ||
| ? _fraxData.nonEVMReceiver | ||
| : bytes32(uint256(uint160(_bridgeData.receiver))); |
Contributor
Author
There was a problem hiding this comment.
False positive — no narrowing cast is present on this line, and the line is pre-existing.
_bridgeData.receiver is an address (160 bits). uint160(address) is an exact-width conversion, uint256(uint160) widens, and bytes32(uint256) is same-width. Every step is width-preserving or widening, so no bit truncation is reachable. The detector appears to pattern-match nested casts that include uint160(...) rather than checking the source width.
This expression is also unchanged from the base branch (feature/exsc-382-frax-sc-implementation, line 320) — this commit only moved it into a ternary to select between the EVM and non-EVM recipient, which is why the scanner sees the line as new. And it is the established idiom in this repo for EVM-address-to-bytes32: AcrossFacetV4.sol:268, AllBridgeFacet.sol:175, CelerCircleBridgeFacet.sol:128, AcrossV4SwapFacet.sol:829, LiFiIntentEscrowFacet.sol:205.
No code change. The alert can be dismissed as a false positive.
…signing proposal (EXSC-382) Fixes the verify-clear-signing failure on #2138. Two distinct mistakes in the previous commit: 1. removeFraxChainIdToEid failed the clear-signing generator's classifier: "unrecognized prefix — doesn't match any classifier and isn't in the explicit non-user-facing skip-list". Rather than adding a generator exception, renamed it to unsetFraxChainIdToEid (event FraxChainIdToEidUnset) to match the existing `unset` prefix, documented in NON_USER_FACING_PREFIXES as "inverse of set*" and already used by PolymerCCTPFacet.unsetChainIdToDomainId for the identical operation. No shared-tooling change needed. Batch input and the loud UnsupportedChainId on an unset entry are kept. 2. config/clearSigningProposal.json is GENERATED by tasks/buildClearSigningProposal.ts (plain JSON.stringify, and lint-staged does not format *.json). It had been hand-edited and then run through prettier, which compacted arrays and would have failed the generator's `git diff --exit-code` check even after fix 1. Regenerated from the ABI: the diff against the base branch is now exactly the two Frax struct signatures, and re-running the generator is byte-idempotent. Tests: 72 pass, 0 fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/solidity/Facets/FraxFacet.t.sol (1)
532-559: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen assertions in
test_CanSwapAndBridgeToSolanaWithBytes32Receiver.This test only checks
frxUSD.balanceOf(_facetTestContractAddress) == 0after the call, whereas its non-swap sibling (test_CanBridgeToSolanaWithBytes32Receiver, lines 494-530) and the pre-existingtestBase_CanSwapAndBridgeTokensboth verifyAssetSwapped/LiFiTransferStartedevents and sender/diamond balance deltas. As written, this test would still pass even if the swap output or the final bridged amount silently diverged from expectations.♻️ Suggested strengthening
vm.expectEmit(true, true, true, true, _facetTestContractAddress); emit BridgeToNonEVMChainBytes32( bridgeData.transactionId, LIFI_CHAIN_ID_SOLANA, SOLANA_RECEIVER ); + vm.expectEmit(true, true, true, true, _facetTestContractAddress); + emit LiFiTransferStarted(bridgeData); initiateSwapAndBridgeTxWithFacet(false); vm.stopPrank(); assertEq(frxUSD.balanceOf(_facetTestContractAddress), 0); + assertEq(frxUSD.balanceOf(address(diamond)), 0); + assertEq(address(diamond).balance, 0);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/solidity/Facets/FraxFacet.t.sol` around lines 532 - 559, Strengthen test_CanSwapAndBridgeToSolanaWithBytes32Receiver by asserting the expected AssetSwapped and LiFiTransferStarted events, matching the patterns in test_CanBridgeToSolanaWithBytes32Receiver and testBase_CanSwapAndBridgeTokens. Also capture the relevant pre-call balances and verify sender and facet balance deltas, while retaining the existing bridge event and zero frxUSD balance assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@config/clearSigningProposal.json`:
- Line 737: Add a visible _fraxData.nonEVMReceiver field to the fields
definitions for both Frax clear-signing formats:
config/clearSigningProposal.json lines 737-737 and 3750-3750. Keep the existing
ABI tuple entries and expose the actual non-EVM recipient alongside the other
Frax data in each format.
---
Nitpick comments:
In `@test/solidity/Facets/FraxFacet.t.sol`:
- Around line 532-559: Strengthen
test_CanSwapAndBridgeToSolanaWithBytes32Receiver by asserting the expected
AssetSwapped and LiFiTransferStarted events, matching the patterns in
test_CanBridgeToSolanaWithBytes32Receiver and testBase_CanSwapAndBridgeTokens.
Also capture the relevant pre-call balances and verify sender and facet balance
deltas, while retaining the existing bridge event and zero frxUSD balance
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a22855c4-1bb5-4a02-a5c7-b72306666fcc
📒 Files selected for processing (7)
config/clearSigningProposal.jsonconfig/frax.jsondocs/FraxFacet.mdscript/demoScripts/demoFrax.tssrc/Facets/FraxFacet.soltest/solidity/Facets/FraxFacet.t.soltest/solidity/utils/MockFraxHopV2Tempo.sol
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/FraxFacet.md
Addresses CodeRabbit on #2138: adding nonEVMReceiver to the ABI tuple without exposing it in `fields` left signers unable to verify a Solana recipient — _bridgeData.receiver is only the NON_EVM_ADDRESS sentinel on those routes, so the one field a signer could see carried no information about the destination. Fixed in the generator, not the generated file. CodeRabbit's suggested remedy was to hand-edit config/clearSigningProposal.json lines 737/3750, but that file is emitted by tasks/buildClearSigningProposal.ts and the exposure is driven by the BRIDGE_EXTRA_RECEIVERS registry, which had no Frax entry. Added Frax as TYPE-B (nonEVMReceiver-style, 0x0 on EVM) alongside Mayan/LayerSwap/Chainflip/Eco/ NEARIntents/PolymerCCTP, and regenerated. Both Frax formats now carry `_fraxData.nonEVMReceiver | "Non-EVM Recipient" | raw | visible: always`, identical to the other nine facets that expose a non-EVM recipient. FraxFacet is a slightly stronger case for TYPE-B than its peers: it *enforces* nonEVMReceiver == bytes32(0) on EVM destinations (reverting InvalidCallData), so the "0x0 reads as N/A" property is guaranteed rather than conventional. Noted at the registry entry. Also synced the TYPE-B list in docs/ClearSigningProposal.md. Generator output verified byte-idempotent; the generated file was NOT run through prettier this time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
…anches (EXSC-382) - assert the exact bytes32 recipient forwarded to HOP.sendOFT on the Solana path (both entry points) and the left-padded receiver on the EVM path; the emitted event reads fraxData directly, so a mis-wired recipient ternary was previously invisible to every test (mutation-verified) - add the LiFiTransferStarted expectEmit and native-balance check to the swap-path Solana test, matching its non-swap sibling - docs: destinationChainId is enforced (bound to dstEid), not metadata Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
0xDEnYO
changed the title
Contributor
Author
Gate-review residuals (automated pre-review pass, commit b5ec7bf)Escalated / not auto-fixed — need a human decision:
Lower-confidence — human judgment:
Auto-fixed in b5ec7bf (tests/docs only, no contract changes): |
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
🔍 QA Review — EXSC-382 — FraxFacet Non-EVM (Solana) Routing via LayerZero OFT
SummaryPR #2138 extends Acceptance Criteria Verification
FindingsNone blocking. Implementation is correct. Process Items (non-blocking — must be resolved before PR #2048 merges to
|
| # | Item | Severity | Location |
|---|---|---|---|
| P1 | External review archival — if a third-party review of FraxFacet was performed, the report should be committed to audit/reports/ and registered in audit/auditLog.json before main merge. versionControlAndAuditCheck.yml will block merge on main if @custom:version changed without a corresponding audit entry. |
Process | audit/auditLog.json |
| P2 | Version bump required at main merge — FraxFacet.sol still carries @custom:version 1.0.0. If logic changed relative to the last audited version, the version must be bumped before the feature branch merges to main. CI enforces this on main-targeting PRs — acceptable to defer until #2048 is retargeted. |
Process | src/Facets/FraxFacet.sol |
| P3 | Arbitrum staging cleanup — any staging deployments used for Frax integration testing should be documented or cleaned up before production deployment. | Process | Deployment notes |
🤖 Automated QA review by
lifi-qa-agent[bot]. Methodology: Linear AC verification + diff analysis + test coverage audit. Note: this PR targets a feature branch (notmain); version/audit CI runs at finalmain-targeting PR merge.
gvladika
marked this pull request as ready for review
August 10, 2026 08:09
gvladika
merged commit Aug 10, 2026
f08d37f
into
feature/exsc-382-frax-sc-implementation
48 of 50 checks passed
4 of 9 tasks
4 of 8 tasks
0xDEnYO
added a commit
that referenced
this pull request
Aug 20, 2026
…2) [FraxFacet v1.0.0, IFraxHopV2 v1.0.0] (#2048) * feat(FraxFacet): add Frax HopV2 (LayerZero OFT) bridge facet + config + deploy scripts (EXSC-382) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(FraxFacet): harden swap-path guards + Tempo fee sweep; add docs + clearSigning (EXSC-382) - swap entrypoint: reject stray msg.value on Tempo; require final swap output == sendingAssetId - Tempo: return unpulled fee-token remainder to refundRecipient (invariant holds across hop upgrades) - docs/FraxFacet.md; regenerate clearSigningProposal.json (both Frax entrypoints) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(FraxFacet): fork + mock suite, 100% line coverage incl. Tempo ERC20-fee path (EXSC-382) - 29 Arbitrum-fork tests (real HopV2 + frxUSD self-OFT): dust floor+refund, excess-native refund, diamond-retains-nothing, oft/token mismatch, validation reverts, swap-output guard - 7 local Tempo tests via MockFraxHopV2Tempo (precompile TIP20 can't fork; rule 400): msg.value==0 guard, ERC20 fee pull, opted-in fee token, unused-fee sweep Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(FraxFacet): add demo script; flag unverified API slug + fix Tempo guard wording in docs (EXSC-382) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(FraxFacet): address CodeRabbit review — demo aborts on quote-routing mismatch; docs clarify enforced BridgeData fields (EXSC-382) - demoFrax: source-chain Hop is always the arbitrum spoke for both routes; validate oft/dstEid/recipient/amount/approvalAddress/to and throw on mismatch instead of warn - docs: BridgeData is not strictly analytics — sendingAssetId/receiver/minAmount are enforced Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(FraxFacet): validate destinationChainId via governed EID mapping; harden fee-token + modifier consistency (EXSC-382) - add owner-governed chainId -> LayerZero EID diamond-storage mapping (initFrax / setChainIdToEid / getChainIdToEid), mirroring SupersetFacet, and cross-check bridgeData.destinationChainId against fraxData.dstEid (reverts UnsupportedChainId when unmapped, InformationMismatch on eid mismatch) - reject the Tempo fee-token == bridged-token collision (InformationMismatch) so the unused-fee balance-delta accounting cannot strand the bridged amount - align swapAndStartBridgeTokensViaFrax modifier order with the non-swap entrypoint (validateBridgeData first) - seed config/frax.json mappings with verified LayerZero EIDs + a DEV NOTICE; wire initFrax into the Update deploy scripts (EVM + zksync) as the diamondCut init call - tests: +16 (mapping admin, destinationChainId mismatch/unsupported, fee-token collision); docs updated Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(config): single-source LayerZero EIDs in config/layerzero.json (EXSC-382) - add config/layerzero.json as the one source of truth for chainId -> LayerZero EID, built from AcrossV4SwapFacet's audited values + fork-verified fraxtal - tasks/syncLayerZeroEids.ts generates the `mappings` arrays in frax.json / superset.json from it (subset = each facet's own supported networks: hop / poolManager); deploy scripts still read `.mappings` unchanged - script/tasks/layerZeroEids.test.ts pins all three consumers (frax, superset, AcrossV4's hardcoded _chainIdToLzEid) to layerzero.json so a wrong/stale EID fails in CI instead of on-chain - fix two wrong Frax EIDs surfaced by the reconciliation: sei 30300 -> 30280, xLayer 30196 -> 30274 (they disagreed with the audited AcrossV4 values); drop unverified abstract until its EID is confirmed - no facet .sol changes (SupersetFacet/AcrossV4SwapFacet bytecode unchanged); docs updated Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(deploy): register FraxFacet v1.0.0 on arbitrum staging (EXSC-382) Deployed FraxFacet at 0x3B76d15Eb68a7a0E09b86D9aee3c49fD006cdFB2 (CREATE3) and diamondCut into the arbitrum staging LiFiDiamond (0xD3b2b0aC0AFdd0d166a495f5E9fca4eCc715a782); initFrax seeded the chainId->LayerZero-EID mapping (verified on-chain, e.g. getChainIdToEid(252)=30255). Enables the demoFrax.ts staging smoke test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(demoFrax): lower demo amount to 5 frxUSD for staging smoke test (EXSC-382) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(demoFrax): record verified arbitrum-staging bridge tx hash (EXSC-382) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(config): sparse optional per-network config values + _getOptionalConfigContractAddress (EXSC-382) frax.json's tipFeeManager/pathUsd were all-zero on 26 of 27 networks (non-zero only on Tempo). Explicit all-zero maps are pure noise and a footgun: the strict config reader reverts on a MISSING key, so the first FraxFacet deploy on any future network (added via /add-network, which never touches bridge configs) would revert on a value that was always meant to be zero. - Add _getOptionalConfigContractAddress to both ScriptBase files: returns address(0) when the key is absent (stdJson.keyExists guard), no zero/contract check on present values (Tempo's precompiles have no bytecode). - Sparsify tipFeeManager/pathUsd in frax.json to list only Tempo; the DEV NOTICE documents the sparse convention. - Switch both DeployFraxFacet scripts to the optional reader. - Codify the required-vs-optional sparsity rule in 004-config-structure so future configs are created in the optimal shape. The bash checkDeployRequirements layer already tolerates a missing key when allowToDeployWithZeroAddress is "true" (already set for both keys), so the deploy pipeline is unaffected end-to-end. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * refactor(config): inline LayerZero EIDs per-facet; drop layerzero.json + sync task (EXSC-382) Revert the EID single-sourcing indirection: the chainId -> LayerZero EID mappings live directly in each facet's config again, as hand-maintained arrays. Removes config/layerzero.json, tasks/syncLayerZeroEids.ts, and the script/tasks/layerZeroEids.test.ts drift guard. The mapping DATA (including the earlier sei/xLayer EID corrections and the dropped-until-verified abstract) is kept as-is in config/frax.json / config/superset.json — only the "generated" tooling and notices are removed. No .sol changes; deploy scripts already read `.mappings` directly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(config): add abstract (2741 -> EID 30324) to FraxFacet mappings (EXSC-382) Abstract is listed under `hop` but was dropped from `mappings` pending EID verification. EID 30324 confirmed against the LayerZero deployments list (https://docs.layerzero.network/v2/deployments/deployed-contracts), so routes to Abstract can now be seeded. Matches AcrossV4SwapFacet where present (Across does not list Abstract, so no cross-facet conflict). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(FraxFacet): address peer review — selector collision, fee over-pull guard, calldata (EXSC-382) Resolves the three review comments from @gvladika on PR #2048: - Selector collision with SupersetFacet: setChainIdToEid/getChainIdToEid used byte-identical selectors to SupersetFacet's. Since both facets are cut into the same diamond, UpdateScriptBase.buildDiamondCut would reassign the colliding selectors and remove the previously-owning facet's remaining selectors — silently uninstalling a live bridge facet. Renamed to setFraxChainIdToEid / getFraxChainIdToEid (and event FraxChainIdToEidSet) so both facets coexist. - Tempo unused-fee sweep underflow: guard the balance-delta subtraction so a HopV2 over-pull degrades to "no refund" instead of an arithmetic-panic revert of an otherwise-valid bridge. Adds regression test test_Tempo_FeeOverPullDegradesToNoRefundInsteadOfReverting. - Non-swap startBridgeTokensViaFrax _bridgeData: memory -> calldata (matches SupersetFacet; drops an unnecessary calldata->memory copy on the hot path). Also adds rule 102-facets [CONV:FACET-SELECTORS]: facet-specific admin/config functions must carry the facet identifier to prevent selector collisions. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(deploy): arbitrum staging — redeploy renamed FraxFacet, restore SupersetFacet (EXSC-382) Redeployed FraxFacet to arbitrum staging carrying the renamed, non-colliding selectors (setFraxChainIdToEid/getFraxChainIdToEid). The diamondCut removed the old generic selectors (setChainIdToEid 0x5b1ee840 / getChainIdToEid 0xedc3f41e) from the diamond and added the Frax-specific ones. FraxFacet 0x3B76…dFB2 -> 0xCDFE2fc28F6f824F9E619728A52363bd558aB4d0 Because the earlier collision had already uninstalled SupersetFacet on this diamond (0 selectors), also re-cut SupersetFacet (current repo version) so both bridges coexist. Superset reclaims its generic selectors cleanly now that Frax no longer owns them; the two facets share no selectors. SupersetFacet 0x54AE…D222D -> 0x962125b0b3c2329EAd744a0ad383D37F78C608e9 Verified on-chain via the diamond loupe: no selector overlap, both facets live. Staging only (direct cut); no production proposals. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(FraxFacet): address Frax external review — deprecated spokes, non-EVM (Solana) receivers, approvedOft pre-check (EXSC-382) (#2138) * fix(FraxFacet): address Frax external review — deprecated spokes, EVM-only receiver, approvedOft pre-check (EXSC-382) Addresses the external review from the Frax team on PR #2048. Blocking fix - config/frax.json: remove berachain, mode and scroll from the "hop" address map and drop the mode (34443) / scroll (534352) "mappings" entries — Frax is deprecating those spokes from the hop mesh. DEV NOTICE records why so they do not get re-added. Hardening - EVM-only receiver: reject bridgeData.receiver == NON_EVM_ADDRESS with InvalidReceiver. HopV2's recipient is bytes32 and Frax routes to Solana (EID 30168) via the Fraxtal hub, but this version derives the recipient from the 20-byte receiver, so the sentinel would otherwise be bridged literally and the funds delivered to 0x11f1...f1. The inverse (non-EVM destination with an EVM receiver) is already covered: no non-EVM chain ID is in "mappings", so getFraxChainIdToEid reverts UnsupportedChainId. Adding a bytes32 non-EVM receiver to FraxData is deliberately left as a scoped follow-up. - HOP.approvedOft(oft) pre-check (added to IFraxHopV2, verified live on both the Fraxtal hub and the Arbitrum spoke): an OFT the hop does not route now reverts TokenNotSupported in _validateFraxData. Moved the oft.token() == sendingAssetId check there too, so both OFT invariants are enforced before any deposit or swap rather than after _depositAndSwap. Both paths revert atomically either way — this is a gas/error-clarity win, not a fund-safety one. - Re-pinned the Arbitrum fork block 483300000 -> 488500000 so the fork tests run against the upgraded RemoteHopV2 implementation. All tests pass. - docs/FraxFacet.md: document the approval trust assumption explicitly — LibAsset.maxApproveERC20 sets the allowance to type(uint256).max (the amount argument is only the required-allowance threshold), so the diamond carries a standing unlimited allowance to HopV2, an upgradeable proxy. Corrects the prior "scoped to the bridged amount" wording. Also documents the EVM-only scoping, the approvedOft pre-check, the deprecated spokes and the source-only chains (katana/somnia/stable/tempo have a hop but no destination mapping). Tests: 6 new (non-EVM sentinel and unapproved-OFT rejection on both entry points, plus both on the Tempo branch); 59 pass, 0 fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(FraxFacet): full non-EVM (Solana) support, on-chain mapping removal, fork-only tests (EXSC-382) Follow-up to the external-review round, addressing Daniel's review of #2138. removeFraxChainIdToEid — deprecation is now enforceable - initFrax copies config/frax.json mappings into diamond storage, so dropping a chain from config did nothing to diamonds already seeded: mode (34443) and scroll (534352) are still live on the Arbitrum staging diamond (verified on-chain). Both write paths reject lzEid == 0 and no removal path existed, so a seeded destination was permanent. - Adds owner-only removeFraxChainIdToEid(uint256[]) + FraxChainIdToEidRemoved. Reverts UnsupportedChainId on an unset entry so a mistyped removal fails loudly. Docs record that retiring a spoke is a two-step op (config + on-chain call) and that staging still needs the call. approvedOft before oft.token() - oft is caller-supplied, so token() was the first external call the facet made into an unvalidated address. Reordered: the hop's allowlist now gates it. Full non-EVM (Solana) support — replaces the EVM-only scoping - Solana is live on the hop today: quote(frxUSD, 30168, ...) returns a fee from the Arbitrum spoke (a bogus EID reverts), so the previous "unreachable" framing was wrong. - FraxData gains bytes32 nonEVMReceiver, passed to sendOFT verbatim, with BridgeToNonEVMChainBytes32 emitted. Sentinel and destination kind are cross-checked in BOTH directions: a 20-byte receiver on a non-EVM route would left-pad into an unspendable Solana account, and the sentinel on an EVM route would deliver to NON_EVM_ADDRESS. A populated nonEVMReceiver on an EVM route reverts rather than being silently ignored. - Non-EVM destinations are identified by _isNonEVMDestination (LIFI_CHAIN_ID_SOLANA); config seeds 1151111081099710 -> 30168. Ripples: clearSigningProposal.json struct signatures and demoFrax.ts updated for the new field. Fork-first tests (rule 400 [CONV:FORK-FIRST]) — no mocked counterparty - Unapproved OFT: real Arbitrum USDC. It has no token(), so reverting TokenNotSupported rather than a failed decode also proves approvedOft is evaluated first. - token() mismatch: real sfrxUSD OFT (hop-approved, underlying != frxUSD) instead of MockFraxOFT. - Solana routes bridge through the real hop on both entry points. - Dropped the mock-toggle approvedOft test; the Tempo mock now returns true unconditionally and MockFraxOFT is confined to the non-forkable Tempo suite. Version stays 1.0.0 (still iterating on the facet's first version). Tests: 72 pass, 0 fail (was 59). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(FraxFacet): rename to unsetFraxChainIdToEid and regenerate clear-signing proposal (EXSC-382) Fixes the verify-clear-signing failure on #2138. Two distinct mistakes in the previous commit: 1. removeFraxChainIdToEid failed the clear-signing generator's classifier: "unrecognized prefix — doesn't match any classifier and isn't in the explicit non-user-facing skip-list". Rather than adding a generator exception, renamed it to unsetFraxChainIdToEid (event FraxChainIdToEidUnset) to match the existing `unset` prefix, documented in NON_USER_FACING_PREFIXES as "inverse of set*" and already used by PolymerCCTPFacet.unsetChainIdToDomainId for the identical operation. No shared-tooling change needed. Batch input and the loud UnsupportedChainId on an unset entry are kept. 2. config/clearSigningProposal.json is GENERATED by tasks/buildClearSigningProposal.ts (plain JSON.stringify, and lint-staged does not format *.json). It had been hand-edited and then run through prettier, which compacted arrays and would have failed the generator's `git diff --exit-code` check even after fix 1. Regenerated from the ABI: the diff against the base branch is now exactly the two Frax struct signatures, and re-running the generator is byte-idempotent. Tests: 72 pass, 0 fail. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(clearSigning): expose FraxData.nonEVMReceiver to signers (EXSC-382) Addresses CodeRabbit on #2138: adding nonEVMReceiver to the ABI tuple without exposing it in `fields` left signers unable to verify a Solana recipient — _bridgeData.receiver is only the NON_EVM_ADDRESS sentinel on those routes, so the one field a signer could see carried no information about the destination. Fixed in the generator, not the generated file. CodeRabbit's suggested remedy was to hand-edit config/clearSigningProposal.json lines 737/3750, but that file is emitted by tasks/buildClearSigningProposal.ts and the exposure is driven by the BRIDGE_EXTRA_RECEIVERS registry, which had no Frax entry. Added Frax as TYPE-B (nonEVMReceiver-style, 0x0 on EVM) alongside Mayan/LayerSwap/Chainflip/Eco/ NEARIntents/PolymerCCTP, and regenerated. Both Frax formats now carry `_fraxData.nonEVMReceiver | "Non-EVM Recipient" | raw | visible: always`, identical to the other nine facets that expose a non-EVM recipient. FraxFacet is a slightly stronger case for TYPE-B than its peers: it *enforces* nonEVMReceiver == bytes32(0) on EVM destinations (reverting InvalidCallData), so the "0x0 reads as N/A" property is guaranteed rather than conventional. Noted at the registry entry. Also synced the TYPE-B list in docs/ClearSigningProposal.md. Generator output verified byte-idempotent; the generated file was NOT run through prettier this time. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(FraxFacet): pin the sendOFT recipient with expectCall on both branches (EXSC-382) - assert the exact bytes32 recipient forwarded to HOP.sendOFT on the Solana path (both entry points) and the left-padded receiver on the EVM path; the emitted event reads fraxData directly, so a mis-wired recipient ternary was previously invisible to every test (mutation-verified) - add the LiFiTransferStarted expectEmit and native-balance check to the swap-path Solana test, matching its non-swap sibling - docs: destinationChainId is enforced (bound to dstEid), not metadata Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> * refactor(FraxFacet): route EVM recipient cast through LibBytes.toBytes32 (EXSC-382) Replace the hand-rolled bytes32(uint256(uint160(receiver))) with LibBytes.toBytes32 per [CONV:ADDR-BYTES32]. Byte-identical widening cast; centralises the Olympix unsafe-downcast finding in LibBytes instead of a per-call-site dismissal. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(deploy): FraxFacet v1.0.0 to arbitrum + base staging (EXSC-382) Redeploy on arbitrum (bytecode changed since prior staging deploy) and new deploy on base; CREATE3 address 0x401Aaf6A650A17677821E665838367EBD5bE77BD on both, registered via diamondCut. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(FraxFacet): Frax-scope immutable getter selectors (audit sujithsomraaj/lifi-frax-facet-v1.0.0#3) Rename public immutables HOP/TIP_FEE_MANAGER/PATH_USD to FRAX_HOP/FRAX_TIP_FEE_MANAGER/FRAX_PATH_USD so their auto-generated diamond getter selectors carry the facet identifier, per [CONV:FACET-SELECTORS]. Generic selectors risk a silent cross-facet collision in UpdateScriptBase.buildDiamondCut, which reassigns a colliding selector and removes the previously-owning facet's other selectors. Updates the facet test getter assertions and doc references. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(FraxFacet): clarify Tempo fee-token funding is caller approve, not pre-transfer (audit sujithsomraaj/lifi-frax-facet-v1.0.0#4) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(FraxFacet): drop 'to be confirmed' hedge from Tempo fee-funding requirement (audit sujithsomraaj/lifi-frax-facet-v1.0.0#4) State the caller approve/no-pretransfer requirement as the contract fact it is; demote EXP-514 to the operational tracking pointer it actually is. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(FraxFacet): trim Tempo fee-funding note to the approval requirement (audit sujithsomraaj/lifi-frax-facet-v1.0.0#4) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(FraxFacet): cap Tempo LZ fee via BE-supplied nativeFee instead of on-chain quote (audit sujithsomraaj/lifi-frax-facet-v1.0.0#1) On Tempo the facet quoted the TIP20 fee on-chain via HopV2.quoteStatic and pulled that uncapped amount from the caller. Mirror the native-fee path instead: pull the caller-supplied FraxData.nativeFee, approve HopV2, and let HopV2 requote and pull its actual fee. depositAsset pulls exactly nativeFee from the caller, so that is the most the caller can be charged regardless of HopV2's quote. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(audit): file FraxFacet v1.0.0 audit report (EXSC-382) Add Sujith Somraaj independent security review (audit20260820) for FraxFacet v1.0.0 and IFraxHopV2 v1.0.0, audited at commit 3ae7e5f. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(clear-signing): regenerate proposal after main merge (EXSC-382) Frax destinationChainId entries now use the chainId display format the current generator emits, matching tasks/buildClearSigningProposal.ts output. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: Goran Vladika <goran.vladika@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which Linear task belongs to this PR?
EXSC-382
Why did I implement it this way?
External review of #2048 by the Frax team. One blocking fix, the rest hardening — and one item that started as "scope v1 to EVM" and was later upgraded to full non-EVM support (see below).
Blocking fix
Remove the deprecated spokes.
berachain,modeandscrollare gone from thehopaddress map, and themode(34443) /scroll(534352) entries are gone frommappings.berachainhad no mapping entry to begin with. TheDEV NOTICEinconfig/frax.jsonnow records why they are absent so nobody re-adds them from the LayerZero deployment list.Config removal alone does not retire a spoke on diamonds already seeded via
initFrax, so the facet gainedunsetFraxChainIdToEid(uint256[])(owner-only, new selector, emitsFraxChainIdToEidUnset, revertsUnsupportedChainIdon an unset entry so a mistyped removal fails loudly). The Arbitrum staging diamond still hasmode/scrollseeded from the pre-deprecation config and needs that removal call (plus a facet redeploy) before further staging testing.Non-EVM (Solana) receiver support
The reviewer flagged that
bytes32(uint256(uint160(receiver)))would have encoded theNON_EVM_ADDRESSsentinel literally and delivered funds to0x11f1…f1. Round 1 scoped v1 to EVM (rejecting the sentinel); the follow-up round replaced that with full non-EVM support, matching the TYPE-B pattern of Mayan/Chainflip/PolymerCCTP:FraxData.nonEVMReceiver(bytes32) carries the real recipient (e.g. a Solana pubkey);config/frax.jsonseedsLIFI_CHAIN_ID_SOLANA (1151111081099710) → LayerZero EID 30168(via the Fraxtal hub).InvalidReceiver; a 20-byte receiver on a non-EVM route revertsInvalidReceiver(it would be left-padded into an unspendable Solana account); a zerononEVMReceiveron a non-EVM route revertsInvalidNonEVMReceiver; a populated one on an EVM route revertsInvalidCallData.BridgeToNonEVMChainBytes32per [CONV:EVENTS], and the clear-signing generator (tasks/buildClearSigningProposal.tsBRIDGE_EXTRA_RECEIVERS) exposes_fraxData.nonEVMReceiveras a visible "Non-EVM Recipient" field in both Frax formats so signers can verify the actual destination (CodeRabbit finding, fixed in the generator and regenerated)._isNonEVMDestinationcurrently lists onlyLIFI_CHAIN_ID_SOLANA— documented indocs/FraxFacet.mdas the place to extend when Frax adds further non-EVM spokes.HOP.approvedOft(oft)pre-check. Added toIFraxHopV2and verified live before wiring it in — the selector exists on both the Fraxtal hub (0x0000…6B36) and the Arbitrum spoke (0x0000…de659), returnstruefor frxUSD andfalsefor a non-OFT. An unapproved OFT now revertsTokenNotSupportedin_validateFraxData.I also moved the existing
oft.token() == sendingAssetIdcheck into_validateFraxData, since it had exactly the same late-failure shape. Both OFT invariants now run before any deposit or swap, andapprovedOftruns beforeoft.token()so the facet never calls into a caller-supplied OFT the hop has not allowlisted.One correction to the framing, though: both entry points revert atomically, so an unapproved OFT discovered inside
sendOFTwould already have rolled the user's swap back — no funds were ever at risk. The pre-check is a gas and error-clarity win (a bad route costs validation gas, not deposit + swap +sendOFT), and that is how it's documented rather than as a fund-safety fix.Re-ran the facet tests against the upgraded RemoteHopV2. The Arbitrum fork block was pinned at 483300000, which predates the upgrade (the hop proxy's implementation slot changed between that block and now). Re-pinned to 488500000 so the fork suite exercises the current hop bytecode. Everything passes unchanged — no adaptation to the new implementation was needed.
Documented the approval trust assumption. This one also surfaced a docs inaccuracy in #2048: the old text said the allowance is "scoped to the bridged amount". It isn't —
LibAsset.maxApproveERC20'samountargument is only the required-allowance threshold, and when the current allowance is insufficient the allowance is set totype(uint256).max. So after the first bridge of a token, the diamond carries a standing unlimited allowance to HopV2, an upgradeable proxy with an adminrecover().docs/FraxFacet.mdnow states this plainly, including what an upgraded malicious HopV2 could reach (any balance the diamond happens to hold in an approved token at that moment) and why the residual risk is bounded by the "diamond holds nothing between transactions" invariant. Mirrored as a@devnote on the facet.Items left open — for you, not code changes
katana(747474),somnia(5031),stable(988) andtempo(4217) all have a live hop entry but nomappingsentry, so they work as sources and revertUnsupportedChainIdas destinations. Tempo is the odd one — the full TIP20 source path is implemented but it can't be a destination. Documented as deliberate launch scoping resolved alongside EXP-514, with a pointer tosetFraxChainIdToEidfor adding them later. If any of these should be destinations at launch, the mapping entries need adding — tell me which and I'll add them.unsetFraxChainIdToEid([34443, 534352])on the Arbitrum staging diamond + facet redeploy (see blocking-fix section).Tests
Fork tests (Arbitrum, real hop) cover Solana bridging on both entry points end-to-end, all four sentinel/receiver cross-check reverts, unapproved-OFT rejection on both entry points (real USDC as the bogus OFT, proving
approvedOftis evaluated beforetoken()), theoft.token()mismatch against a second real hop-approved OFT (sfrxUSD), and the fullunsetFraxChainIdToEidlifecycle (remove/re-add/multi/auth/init guards). The Tempo mock suite covers the ERC20-fee path to Solana.vm.expectCallpins the exactbytes32recipient forwarded toHOP.sendOFTon both the non-EVM and EVM branches (mutation-verified: inverting the recipient ternary fails all three pinning tests).Facet version stays at
1.0.0— v1.0.0 is unmerged and only on Arbitrum staging, matching how the earlier peer-review round in #2048 was handled. The staging diamond needs a redeploy of the facet before any further staging testing.Checklist before requesting a review
Checklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)
🤖 Generated with Claude Code