LZ Bridge Upgrade - #220
Merged
0xJem merged 64 commits intoMar 23, 2026
Merged
Conversation
- Add fees parameter to Bridged event for fee tracking - Add IVersioned to supportsInterface in LZCrossChainBridge - Remove unused IEnabler import and inheritance from ILZCrossChainBridge - Rename getTrustedRemoteAddress to getTrustedRemote - Rename BridgeReceived event to Received - Reorder modifiers: onlyEnabled before onlyFacilitator - Improve NatSpec for setBridgedSupply and setFacilitator
…o lz-bridge-security-revamp
…ssaging libraries - Updated LayerZero integration to use V2 endpoints and messaging libraries. - Refactored contract interfaces and methods to accommodate new endpoint ID structure. - Enhanced peer management and message handling for improved security and efficiency. - Added migration documentation outlining changes and benefits of the upgrade.
…mporarily by renaming their files
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSplits the monolithic bridge into a LayerZero V2 policy ( Changes
Sequence Diagram(s)sequenceDiagram
actor User as User
participant Bridge as LZCrossChainBridge
participant Gateway as LZBridgeGateway
participant MINTR as OlympusMinter
participant LZEndpoint as LZEndpointV2
User->>Bridge: sendOhm(dstEid, to, amount) + native fee
Bridge->>Bridge: safeTransferFrom(user -> gateway, amount)
Bridge->>Gateway: burnAndSend(dstEid,to,amount,refundAddr,options) payable
Gateway->>Gateway: validate peer & rate-limit
Gateway->>Gateway: if canonical -> bridgedSupply += amount
Gateway->>MINTR: approve(amount) / burn(amount)
Gateway->>LZEndpoint: send(dstEid, peer, payload(type,to,amount), options) payable
LZEndpoint-->>Gateway: guid / delivery ack
Gateway-->>Bridge: emit Sent / Bridged event
Bridge-->>User: refund excess ETH (if any)
sequenceDiagram
participant LZEndpoint as LZEndpointV2
participant Gateway as LZBridgeGateway
participant MINTR as OlympusMinter
actor Recipient as Recipient
LZEndpoint->>Gateway: lzReceive(origin, guid, message)
Gateway->>Gateway: verify caller is endpoint & peer mapping
Gateway->>Gateway: decode payload -> (msgType,to,amount)
Gateway->>Gateway: validate msgType == MSG_BRIDGE_OHM
Gateway->>Gateway: if canonical -> bridgedSupply -= amount (revert on underflow)
Gateway->>MINTR: increaseAllowance / mint(to, amount)
Gateway-->>Recipient: OHM delivered
Gateway-->>LZEndpoint: emit Received event
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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: 10
🧹 Nitpick comments (11)
src/libraries/LZConfigLib.sol (2)
152-185: Use custom errors instead of string reverts per coding guidelines.The EID helper functions use
revert("LZConfigLib: unsupported EID")which doesn't follow the guideline to use custom errors instead ofrequire()for assertions.🔧 Suggested refactor
Add a custom error at the library level:
+/// `@notice` Thrown when an unsupported LayerZero EID is provided. +error LZConfigLib_UnsupportedEid(uint32 eid); + library LZConfigLib {Then replace revert strings:
- revert("LZConfigLib: unsupported EID"); + revert LZConfigLib_UnsupportedEid(eid_);As per coding guidelines: "Use custom errors instead of
require()for assertions; do not revert with blank messages."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libraries/LZConfigLib.sol` around lines 152 - 185, Declare a custom error at the top of the LZConfigLib (e.g., error UnsupportedEid(uint32 eid);) and replace the string reverts in sendUln302ForEid, recvUln302ForEid, outboundConfirmationsForEid, and lzDvnForEid with revert UnsupportedEid(eid_); so the functions throw the typed error with the EID value instead of revert("LZConfigLib: unsupported EID").
187-199: Hardcoded DVN ordering assumption may be fragile.The comment states "All chain-specific LZ DVN addresses are below GCLOUD_DVN (0xD56e...)" and the code handles both orderings. While the current addresses satisfy this, if LayerZero deploys new chain DVNs with higher addresses, the comment would become misleading.
Consider adding a brief comment acknowledging the else branch handles future cases, or add a compile-time assertion to validate the assumption for known chains.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/libraries/LZConfigLib.sol` around lines 187 - 199, The dvnsForEid function relies on the assumption that chain-specific LZ DVN addresses are below GCLOUD_DVN (see GCLOUD_DVN and lzDvnForEid), which is fragile; add a short clarifying comment above dvnsForEid that notes the current assumption and that the else branch handles future higher-address DVNs, and also add a defensive check (e.g., in the contract constructor or an init function) that asserts/validates known local DVN constants are less than GCLOUD_DVN so the assumption is explicitly verified at deployment time (use the lzDvnForEid mapping/constants for the known EIDs).src/scripts/ops/batches/LZCrossChainBridgeBatch.sol (1)
50-50: Consider using function selector instead of signature string for type safety.Using
encodeWithSignature("setBridgeStatus(bool)", false)is less type-safe than using a selector. If the oldCrossChainBridgeinterface is available, prefer:abi.encodeWithSelector(OldCrossChainBridge.setBridgeStatus.selector, false)This provides compile-time verification and avoids typo risks in the function signature string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/ops/batches/LZCrossChainBridgeBatch.sol` at line 50, Replace the string-based ABI encoding in the addToBatch call with a selector-based encoding for type safety: locate the addToBatch(oldBridge, abi.encodeWithSignature("setBridgeStatus(bool)", false)) call and change it to use abi.encodeWithSelector with the OldCrossChainBridge.setBridgeStatus.selector and the same boolean argument so the compiler verifies the function exists and avoids signature-typo risks.src/test/policies/bridge/LZBridgeGatewayFork.t.sol (1)
619-625: Test should verify specific revert reason for underpayment.The test expects a revert when sending with insufficient fee (
value: 1), but doesn't verify the specific error. Consider usingvm.expectRevert(ExpectedError.selector)to ensure it fails for the right reason rather than an unrelated issue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/policies/bridge/LZBridgeGatewayFork.t.sol` around lines 619 - 625, Replace the generic vm.expectRevert() with an assertion for the bridge's specific underpayment error so the test fails only for the correct reason: before calling ethBridge.sendOhm{value: 1}(LZConfigLib.ARB_EID, recipient, amount2) use vm.expectRevert(<BridgeContract>.InsufficientFee.selector) (or the actual custom error selector defined on the bridge contract, e.g., NotEnoughFee/InsufficientNativeToken) — alternatively use vm.expectRevert(abi.encodeWithSelector(<BridgeContract>.<ErrorName>.selector)) if the error is a custom error type; keep the rest of the test (sender, approve, amount2) the same.src/scripts/ops/batches/LZCrossChainBridgeL2Batch.sol (2)
31-31: Prefer type-safe encoding over string-based signature.Using
abi.encodeWithSignatureis less type-safe and more error-prone thanabi.encodeWithSelector. If the old bridge interface is available, consider importing it for compile-time signature verification.- addToBatch(oldBridge, abi.encodeWithSignature("setBridgeStatus(bool)", false)); + // If ICrossChainBridge interface is available: + addToBatch(oldBridge, abi.encodeWithSelector(ICrossChainBridge.setBridgeStatus.selector, false));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/ops/batches/LZCrossChainBridgeL2Batch.sol` at line 31, Replace the string-based call to abi.encodeWithSignature in the addToBatch invocation with a type-safe selector-based encoding: import or declare the old bridge interface that defines setBridgeStatus(bool) and use abi.encodeWithSelector(OldBridgeInterface.setBridgeStatus.selector, false) (or OldBridgeInterface(address(0)).setBridgeStatus.selector) when calling addToBatch(oldBridge, ...); this ensures compile-time signature verification and avoids brittle string signatures.
5-9: Import grouping could be improved.Per coding guidelines, imports should be grouped as: interfaces, libraries, contracts; sorted alphabetically within each group. Currently
console2(library) is mixed with contract imports.-import {LZBridgeL2BatchScript} from "./lib/LZBridgeL2BatchScript.sol"; -import {console2} from "@forge-std-1.9.6/console2.sol"; - -import {LZCrossChainBridge} from "src/periphery/bridge/LZCrossChainBridge.sol"; -import {IEnabler} from "src/periphery/interfaces/IEnabler.sol"; +// Interfaces +import {IEnabler} from "src/periphery/interfaces/IEnabler.sol"; + +// Libraries +import {console2} from "@forge-std-1.9.6/console2.sol"; + +// Contracts +import {LZBridgeL2BatchScript} from "./lib/LZBridgeL2BatchScript.sol"; +import {LZCrossChainBridge} from "src/periphery/bridge/LZCrossChainBridge.sol";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/ops/batches/LZCrossChainBridgeL2Batch.sol` around lines 5 - 9, Imports are not grouped/sorted per guidelines: move and group imports so interfaces first, then libraries, then contracts, and sort each group alphabetically; specifically put IEnabler (interface) first, then console2 (library) next, and then LZBridgeL2BatchScript and LZCrossChainBridge (contracts) afterward, each sorted alphabetically within their group to replace the current ordering that mixes console2 with contract imports.src/proposals/LZBridgeSecurityUpgradeProposal.sol (1)
49-55: TODO: Set constants before proposal submission.These placeholder values must be set before the proposal is submitted on-chain:
BRIDGED_SUPPLY_CAP: Currently 0, should be the maximum OHM allowed to be bridgedARB_GATEWAY,OPT_GATEWAY,BASE_GATEWAY: Remote gateway addresses after deploymentThe proposal will fail validation if
BRIDGED_SUPPLY_CAPis 0 and no peers are set.Would you like me to help calculate an appropriate bridged supply cap based on current cross-chain OHM distribution?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/proposals/LZBridgeSecurityUpgradeProposal.sol` around lines 49 - 55, Replace the placeholder constants with the real deployed values: set BRIDGED_SUPPLY_CAP to a non-zero uint256 representing the maximum OHM allowed to be bridged (ensure it’s the intended cap and > 0), and replace ARB_GATEWAY, OPT_GATEWAY, and BASE_GATEWAY with the actual deployed remote gateway addresses; update the constants in LZBridgeSecurityUpgradeProposal.sol (BRIDGED_SUPPLY_CAP, ARB_GATEWAY, OPT_GATEWAY, BASE_GATEWAY) before submitting the proposal so validation passes.audit/2026-03_lz-bridge-upgrade/README.md (1)
18-26: Add language specifier to fenced code blocks.Static analysis flagged these code blocks as missing language specifiers. Adding a language like
textorplaintextimproves markdown rendering consistency.-``` +```text User -> LZCrossChainBridge (facilitator) -> LZBridgeGateway (policy) -> LZ Endpoint V2 -> [destination]On receive:
-
+text
LZ Endpoint V2 -> LZBridgeGateway.lzReceive -> validate peer -> mint OHM to recipient🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@audit/2026-03_lz-bridge-upgrade/README.md` around lines 18 - 26, Update the two fenced code blocks that contain the sequences "User -> LZCrossChainBridge (facilitator) -> LZBridgeGateway (policy) -> LZ Endpoint V2 -> [destination]" and "LZ Endpoint V2 -> LZBridgeGateway.lzReceive -> validate peer -> mint OHM to recipient" to include a language specifier (e.g., replace ``` with ```text) so Markdown renders them consistently; locate the blocks around the lines referencing LZCrossChainBridge and LZBridgeGateway.lzReceive and change their opening fences to ```text.src/scripts/ops/batches/lib/LZBridgeBatchScript.sol (1)
30-31: TODO: Set INITIAL_BRIDGED_SUPPLY before execution.This value needs to be set to the current bridged OHM supply before running the migration batch. Setting it to the wrong value could cause supply tracking issues.
Would you like me to help generate a script to query the current bridged supply from the existing CrossChainBridge?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/ops/batches/lib/LZBridgeBatchScript.sol` around lines 30 - 31, Replace the placeholder INITIAL_BRIDGED_SUPPLY constant with the actual current bridged OHM supply by querying the deployed CrossChainBridge contract before running the migration: write a small script that calls the bridge's public view (e.g., bridgedSupply / totalBridged / equivalent getter) on the deployed CrossChainBridge, read the returned uint256, and then set that numeric value into the uint256 internal constant INITIAL_BRIDGED_SUPPLY in LZBridgeBatchScript.sol (remove the TODO). Alternatively, have the migration setup call the bridge getter at runtime and initialize the variable from that view to avoid hardcoding; ensure you update the symbol INITIAL_BRIDGED_SUPPLY in the file accordingly.src/test/policies/bridge/LZBridgeGateway.t.sol (2)
642-700: Use selector-based revert expectations for the LayerZero errors.These assertions are stringly typed, so a typo or upstream signature change will not get compile-time protection. Import the error types, or declare minimal local error interfaces for them in the test, and switch these expectations to
.selector-based encoding.As per coding guidelines, all test assertions must use error selectors, never string messages:
abi.encodeWithSelector(Error.selector).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/policies/bridge/LZBridgeGateway.t.sol` around lines 642 - 700, Replace stringly-typed revert expectations in tests test_burnAndSend_revertsIfEnforcedOptionsLackExecutorGas and test_burnAndSend_revertsIfInsufficientFee by using selector-based encoding: import or locally declare the error types for Executor_NoOptions() and LZ_InsufficientFee(uint256,uint256,uint256,uint256) (e.g., error Executor_NoOptions(); error LZ_InsufficientFee(uint256,uint256,uint256,uint256);), then change vm.expectRevert(abi.encodeWithSignature(...)) calls to vm.expectRevert(abi.encodeWithSelector(Executor_NoOptions.selector)) and vm.expectRevert(abi.encodeWithSelector(LZ_InsufficientFee.selector, fee.nativeFee, fee.nativeFee/2, 0, 0)) respectively so the tests use abi.encodeWithSelector(Error.selector) instead of string signatures.
28-1857: Please split this suite by entrypoint and adopt the repo test shape.This file aggregates most of
LZBridgeGateway’s external surface, and the new tests rely on ad hoc helpers instead of the repo’sgiven*modifiers /test_given...branching names. Splitting it into one file per external function will make future bridge changes much easier to extend and review.As per coding guidelines,
src/test/**/*.t.solshould use one test file per contract external function,given*modifiers for state setup, andtest_given<Condition>_<Action>_<ExpectedResult>()naming.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/policies/bridge/LZBridgeGateway.t.sol` around lines 28 - 1857, Single-line summary: The monolithic test file must be split into per-entrypoint test files and adopt the repo test shape (given* modifiers and test_given... naming). Split LZBridgeGatewayTests into separate test files focused on each external function/entrypoint (e.g., burnAndSend, lzReceive, setPeer, setDelegate, setFacilitator, setBridgedSupply, setBridgedSupplyCap, setEnforcedOptions, combineOptions, setRateLimits, resetRateLimits, endpoint admin proxies (setSendLibrary/setReceiveLibrary/setReceiveLibraryTimeout/setEndpointConfig), message management proxies (skip/nilify/burn/clear), estimateSendFee/getAmountCanBeSent, supportsInterface), move shared setup into LZBridgeGatewayTestBase or repo given* modifiers (replace ad-hoc helpers like _sendCanonicalToNonCanonical/_sendNonCanonicalToCanonical with given* fixtures), and rename tests to test_given<Condition>_<Action>_<ExpectedResult> using the existing helpers and state (LZBridgeGatewayTestBase, setUp, DEFAULT_OPTIONS, CANONICAL_EID/NONCANONICAL_EID, gateway/gateway2); ensure each new file contains only tests for one external function and uses the repository's modifiers and test naming conventions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/periphery/bridge/LZCrossChainBridge.sol`:
- Line 13: Update the unversioned solmate import to the versioned remapped path
used across the project: replace the import string "solmate/auth/Owned.sol" with
the versioned package path (e.g., "@solmate-6.2.0/auth/Owned.sol") in the
LZCrossChainBridge.sol file so the Owned contract import matches the project's
remappings and other imports.
In `@src/periphery/interfaces/ILZCrossChainBridge.sol`:
- Line 2: Update the Solidity pragma in the ILZCrossChainBridge interface from
">=0.8.4" to ">=0.8.24" to comply with the project's coding guidelines; locate
the pragma at the top of the ILZCrossChainBridge.sol file (interface
ILZCrossChainBridge) and replace the version constraint so the contract compiles
under Solidity versions 0.8.24 and above.
In `@src/policies/bridge/LZBridgeGateway.sol`:
- Around line 229-249: estimateSendFee currently builds a payload for
MSG_BRIDGE_OHM with to_ but doesn't enforce the same recipient validation as
burnAndSend, allowing quotes for payloads that would be rejected at execution;
update estimateSendFee to perform the same recipient check used by burnAndSend
(e.g., require(to_ != address(0)) or call the same validation helper) before
encoding the payload so quoting and execution validation remain aligned (refer
to estimateSendFee, burnAndSend, MSG_BRIDGE_OHM).
- Around line 484-486: Wrap the outer payload decode in a try/catch so decoding
failures map to the custom error: add an external pure helper (e.g.,
_tryDecode(bytes calldata) external pure returns (uint8, bytes memory)) that
simply does (uint8 msgType, bytes memory data) = abi.decode(payload, (uint8,
bytes)); return them, and then change _decodeAndRoute to call
this._tryDecode(payload_) inside a try block and on catch revert with
LZBridgeGateway_InvalidPayload(); ensure references to _tryDecode and
LZBridgeGateway_InvalidPayload() are used so malformed outer payloads produce
the stable gateway selector.
In `@src/policies/interfaces/ILZBridgeGateway.sol`:
- Line 2: Update the Solidity file declaring interface ILZBridgeGateway (the
pragma at the top) to raise the compiler floor from "pragma solidity >=0.8.18;"
to "pragma solidity >=0.8.24;" so the interface's advertised minimum matches the
repo coding guideline and the rest of the upgrade; locate the pragma in
ILZBridgeGateway.sol and change the version specifier accordingly.
In `@src/proposals/LZBridgeSecurityUpgradeProposal.sol`:
- Line 5: The pragma directive at the top of LZBridgeSecurityUpgradeProposal.sol
is too low; update the Solidity version requirement from "pragma solidity
>=0.8.20;" to require at least 0.8.24 (e.g., "pragma solidity >=0.8.24;") so the
contract LZBridgeSecurityUpgradeProposal compiles under the mandated minimum
compiler version and follows project coding guidelines.
In `@src/scripts/ops/batches/lib/LZBridgeL2BatchScript.sol`:
- Around line 92-102: The EOA broadcast path in LZBridgeL2BatchScript currently
calls vm.startBroadcast() without specifying the sender, causing inconsistency
with the Anvil fork and simulation paths; update the EOA branch to call
vm.startBroadcast(_owner) before invoking _runBatch() (and keep
vm.stopBroadcast() after) so the batch runs as the same _owner used by
vm.startPrank(_owner) and vm.startBroadcast(_owner) in the other execution
modes.
In `@src/test/policies/bridge/LZBridgeGateway.t.sol`:
- Around line 1410-1633: The proxy tests use vm.mockCall() but never assert the
endpoint was actually invoked; add a vm.expectCall(endpoint_,
abi.encodeWithSignature(...), bytes("")) immediately before each gateway method
call to ensure the gateway forwards to the endpoint. For each function listed
(test_setSendLibrary_proxiesToEndpoint,
test_setReceiveLibrary_proxiesToEndpoint,
test_setReceiveLibraryTimeout_proxiesToEndpoint,
test_setEndpointConfig_proxiesToEndpoint, test_skip_proxiesToEndpoint,
test_nilify_proxiesToEndpoint, test_burn_proxiesToEndpoint,
test_clear_proxiesToEndpoint) keep the existing vm.mockCall() but insert a
matching vm.expectCall(...) with the same target (endpoint_ or
gateway.LZ_ENDPOINT()) and signature/args just before vm.prank(bridgeAdmin);
gateway.<method>(...) so the test fails if the gateway method (e.g.,
gateway.setSendLibrary, gateway.setReceiveLibrary,
gateway.setReceiveLibraryTimeout, gateway.setEndpointConfig, gateway.skip,
gateway.nilify, gateway.burn, gateway.clear) does not actually call the
endpoint.
In `@src/test/policies/bridge/LZBridgeGatewayFork.t.sol`:
- Line 587: The assertion uses guid.length which is always 32 for a bytes32 and
therefore meaningless; change the check to assert that guid != bytes32(0) (or
use an assertNe/assertTrue equivalent) so the test verifies the GUID is not the
zero value—update the assertion referencing the guid variable in the test (the
line containing assertGt(guid.length, 0, "GUID should be non-zero"))
accordingly.
In `@src/test/proposals/LZBridgeSecurityUpgradeProposal.t.sol`:
- Line 3: Update the Solidity pragma in this test file: replace the existing
"pragma solidity ^0.8.0;" declaration with a pragma that requires compiler
version 0.8.24 or newer (e.g., use ^0.8.24 or a range like >=0.8.24 <0.9.0) so
the file complies with the guideline; the target token to change is the pragma
line containing "pragma solidity ^0.8.0;".
---
Nitpick comments:
In `@audit/2026-03_lz-bridge-upgrade/README.md`:
- Around line 18-26: Update the two fenced code blocks that contain the
sequences "User -> LZCrossChainBridge (facilitator) -> LZBridgeGateway (policy)
-> LZ Endpoint V2 -> [destination]" and "LZ Endpoint V2 ->
LZBridgeGateway.lzReceive -> validate peer -> mint OHM to recipient" to include
a language specifier (e.g., replace ``` with ```text) so Markdown renders them
consistently; locate the blocks around the lines referencing LZCrossChainBridge
and LZBridgeGateway.lzReceive and change their opening fences to ```text.
In `@src/libraries/LZConfigLib.sol`:
- Around line 152-185: Declare a custom error at the top of the LZConfigLib
(e.g., error UnsupportedEid(uint32 eid);) and replace the string reverts in
sendUln302ForEid, recvUln302ForEid, outboundConfirmationsForEid, and lzDvnForEid
with revert UnsupportedEid(eid_); so the functions throw the typed error with
the EID value instead of revert("LZConfigLib: unsupported EID").
- Around line 187-199: The dvnsForEid function relies on the assumption that
chain-specific LZ DVN addresses are below GCLOUD_DVN (see GCLOUD_DVN and
lzDvnForEid), which is fragile; add a short clarifying comment above dvnsForEid
that notes the current assumption and that the else branch handles future
higher-address DVNs, and also add a defensive check (e.g., in the contract
constructor or an init function) that asserts/validates known local DVN
constants are less than GCLOUD_DVN so the assumption is explicitly verified at
deployment time (use the lzDvnForEid mapping/constants for the known EIDs).
In `@src/proposals/LZBridgeSecurityUpgradeProposal.sol`:
- Around line 49-55: Replace the placeholder constants with the real deployed
values: set BRIDGED_SUPPLY_CAP to a non-zero uint256 representing the maximum
OHM allowed to be bridged (ensure it’s the intended cap and > 0), and replace
ARB_GATEWAY, OPT_GATEWAY, and BASE_GATEWAY with the actual deployed remote
gateway addresses; update the constants in LZBridgeSecurityUpgradeProposal.sol
(BRIDGED_SUPPLY_CAP, ARB_GATEWAY, OPT_GATEWAY, BASE_GATEWAY) before submitting
the proposal so validation passes.
In `@src/scripts/ops/batches/lib/LZBridgeBatchScript.sol`:
- Around line 30-31: Replace the placeholder INITIAL_BRIDGED_SUPPLY constant
with the actual current bridged OHM supply by querying the deployed
CrossChainBridge contract before running the migration: write a small script
that calls the bridge's public view (e.g., bridgedSupply / totalBridged /
equivalent getter) on the deployed CrossChainBridge, read the returned uint256,
and then set that numeric value into the uint256 internal constant
INITIAL_BRIDGED_SUPPLY in LZBridgeBatchScript.sol (remove the TODO).
Alternatively, have the migration setup call the bridge getter at runtime and
initialize the variable from that view to avoid hardcoding; ensure you update
the symbol INITIAL_BRIDGED_SUPPLY in the file accordingly.
In `@src/scripts/ops/batches/LZCrossChainBridgeBatch.sol`:
- Line 50: Replace the string-based ABI encoding in the addToBatch call with a
selector-based encoding for type safety: locate the addToBatch(oldBridge,
abi.encodeWithSignature("setBridgeStatus(bool)", false)) call and change it to
use abi.encodeWithSelector with the OldCrossChainBridge.setBridgeStatus.selector
and the same boolean argument so the compiler verifies the function exists and
avoids signature-typo risks.
In `@src/scripts/ops/batches/LZCrossChainBridgeL2Batch.sol`:
- Line 31: Replace the string-based call to abi.encodeWithSignature in the
addToBatch invocation with a type-safe selector-based encoding: import or
declare the old bridge interface that defines setBridgeStatus(bool) and use
abi.encodeWithSelector(OldBridgeInterface.setBridgeStatus.selector, false) (or
OldBridgeInterface(address(0)).setBridgeStatus.selector) when calling
addToBatch(oldBridge, ...); this ensures compile-time signature verification and
avoids brittle string signatures.
- Around line 5-9: Imports are not grouped/sorted per guidelines: move and group
imports so interfaces first, then libraries, then contracts, and sort each group
alphabetically; specifically put IEnabler (interface) first, then console2
(library) next, and then LZBridgeL2BatchScript and LZCrossChainBridge
(contracts) afterward, each sorted alphabetically within their group to replace
the current ordering that mixes console2 with contract imports.
In `@src/test/policies/bridge/LZBridgeGateway.t.sol`:
- Around line 642-700: Replace stringly-typed revert expectations in tests
test_burnAndSend_revertsIfEnforcedOptionsLackExecutorGas and
test_burnAndSend_revertsIfInsufficientFee by using selector-based encoding:
import or locally declare the error types for Executor_NoOptions() and
LZ_InsufficientFee(uint256,uint256,uint256,uint256) (e.g., error
Executor_NoOptions(); error
LZ_InsufficientFee(uint256,uint256,uint256,uint256);), then change
vm.expectRevert(abi.encodeWithSignature(...)) calls to
vm.expectRevert(abi.encodeWithSelector(Executor_NoOptions.selector)) and
vm.expectRevert(abi.encodeWithSelector(LZ_InsufficientFee.selector,
fee.nativeFee, fee.nativeFee/2, 0, 0)) respectively so the tests use
abi.encodeWithSelector(Error.selector) instead of string signatures.
- Around line 28-1857: Single-line summary: The monolithic test file must be
split into per-entrypoint test files and adopt the repo test shape (given*
modifiers and test_given... naming). Split LZBridgeGatewayTests into separate
test files focused on each external function/entrypoint (e.g., burnAndSend,
lzReceive, setPeer, setDelegate, setFacilitator, setBridgedSupply,
setBridgedSupplyCap, setEnforcedOptions, combineOptions, setRateLimits,
resetRateLimits, endpoint admin proxies
(setSendLibrary/setReceiveLibrary/setReceiveLibraryTimeout/setEndpointConfig),
message management proxies (skip/nilify/burn/clear),
estimateSendFee/getAmountCanBeSent, supportsInterface), move shared setup into
LZBridgeGatewayTestBase or repo given* modifiers (replace ad-hoc helpers like
_sendCanonicalToNonCanonical/_sendNonCanonicalToCanonical with given* fixtures),
and rename tests to test_given<Condition>_<Action>_<ExpectedResult> using the
existing helpers and state (LZBridgeGatewayTestBase, setUp, DEFAULT_OPTIONS,
CANONICAL_EID/NONCANONICAL_EID, gateway/gateway2); ensure each new file contains
only tests for one external function and uses the repository's modifiers and
test naming conventions.
In `@src/test/policies/bridge/LZBridgeGatewayFork.t.sol`:
- Around line 619-625: Replace the generic vm.expectRevert() with an assertion
for the bridge's specific underpayment error so the test fails only for the
correct reason: before calling ethBridge.sendOhm{value: 1}(LZConfigLib.ARB_EID,
recipient, amount2) use
vm.expectRevert(<BridgeContract>.InsufficientFee.selector) (or the actual custom
error selector defined on the bridge contract, e.g.,
NotEnoughFee/InsufficientNativeToken) — alternatively use
vm.expectRevert(abi.encodeWithSelector(<BridgeContract>.<ErrorName>.selector))
if the error is a custom error type; keep the rest of the test (sender, approve,
amount2) the same.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 897c1233-2b34-47fa-aac6-670dc5d3e6eb
⛔ Files ignored due to path filters (1)
soldeer.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
audit/2026-03_lz-bridge-upgrade/README.mdaudit/2026-03_lz-bridge-upgrade/scopefile.txtaudit/2026-03_lz-bridge-upgrade/solidity-metrics.htmlfoundry.tomlremappings.txtsrc/libraries/LZConfigLib.solsrc/periphery/bridge/LZCrossChainBridge.solsrc/periphery/interfaces/ILZCrossChainBridge.solsrc/policies/bridge/LZBridgeGateway.solsrc/policies/interfaces/ILZBridgeGateway.solsrc/policies/interfaces/ILZEndpointV2Admin.solsrc/proposals/LZBridgeSecurityUpgradeProposal.solsrc/proposals/addresses.jsonsrc/scripts/deploy/DeployV3.s.solsrc/scripts/deploy/savedDeployments/lz_bridge_canonical.jsonsrc/scripts/deploy/savedDeployments/lz_bridge_noncanonical.jsonsrc/scripts/env.jsonsrc/scripts/ops/batches/LZBridgeGatewayBatch.solsrc/scripts/ops/batches/LZBridgeGatewayL2Batch.solsrc/scripts/ops/batches/LZCrossChainBridgeBatch.solsrc/scripts/ops/batches/LZCrossChainBridgeL2Batch.solsrc/scripts/ops/batches/lib/LZBridgeBatchScript.solsrc/scripts/ops/batches/lib/LZBridgeL2BatchScript.solsrc/test/periphery/bridge/LZCrossChainBridge.t.solsrc/test/policies/bridge/LZBridgeGateway.t.solsrc/test/policies/bridge/LZBridgeGatewayFork.t.solsrc/test/policies/bridge/LZBridgeGatewayFork_LZConfig.t.solsrc/test/proposals/LZBridgeSecurityUpgradeProposal.t.sol
| @@ -0,0 +1,75 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity >=0.8.4; | |||
Contributor
There was a problem hiding this comment.
Update Solidity pragma to >=0.8.24.
The coding guidelines specify that Solidity version must be >= 0.8.24 for new contracts. The current pragma >=0.8.4 is too permissive.
Proposed fix
-pragma solidity >=0.8.4;
+pragma solidity >=0.8.24;As per coding guidelines: "Solidity version must be >= 0.8.24 (with some historical contracts on 0.8.15)"
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| pragma solidity >=0.8.4; | |
| pragma solidity >=0.8.24; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/periphery/interfaces/ILZCrossChainBridge.sol` at line 2, Update the
Solidity pragma in the ILZCrossChainBridge interface from ">=0.8.4" to
">=0.8.24" to comply with the project's coding guidelines; locate the pragma at
the top of the ILZCrossChainBridge.sol file (interface ILZCrossChainBridge) and
replace the version constraint so the contract compiles under Solidity versions
0.8.24 and above.
| @@ -0,0 +1,273 @@ | |||
| // SPDX-License-Identifier: MIT | |||
| pragma solidity >=0.8.18; | |||
Contributor
There was a problem hiding this comment.
Raise this interface pragma to the repo minimum.
>=0.8.18 advertises a lower compiler floor than the rest of this upgrade supports. Bumping it to at least >=0.8.24 avoids misleading downstream integrations about the minimum Solidity version they can safely compile against.
As per coding guidelines, Solidity version must be >= 0.8.24 (with some historical contracts on 0.8.15).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/policies/interfaces/ILZBridgeGateway.sol` at line 2, Update the Solidity
file declaring interface ILZBridgeGateway (the pragma at the top) to raise the
compiler floor from "pragma solidity >=0.8.18;" to "pragma solidity >=0.8.24;"
so the interface's advertised minimum matches the repo coding guideline and the
rest of the upgrade; locate the pragma in ILZBridgeGateway.sol and change the
version specifier accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@audit/2026-03_lz-bridge-upgrade/README.md`:
- Around line 18-20: The fenced code blocks showing the flow lines "User ->
LZCrossChainBridge (facilitator) -> LZBridgeGateway (policy) -> LZ Endpoint V2
-> [destination]" and "LZ Endpoint V2 -> LZBridgeGateway.lzReceive -> validate
peer -> mint OHM to recipient" are missing fence languages and trigger
markdownlint MD040; update both opening triple-backtick fences to include a
language identifier such as text (e.g., change ``` to ```text) so both code
blocks are fenced with a language and CI will pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 99aa32b3-6dd7-4315-a9b2-a93968349b35
📒 Files selected for processing (1)
audit/2026-03_lz-bridge-upgrade/README.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@audit/2026-03_lz-bridge-upgrade/README.md`:
- Line 138: The diagram text misstates the transfer method; update the flow line
to reflect the actual implementation by replacing `OHM.transferFrom(user,
gateway, amount)` with `IERC20(OHM).safeTransferFrom(user, gateway, amount)` (or
simply `OHM.safeTransferFrom(user, gateway, amount)`) to match the
`safeTransferFrom` call used in the LZCrossChainBridge contract (see
LZCrossChainBridge and its use of IERC20(OHM).safeTransferFrom).
- Line 71: The README currently references the wrong branch name
`lz-bridge-security-upgrade-v2`; update that occurrence to the correct PR branch
name `lz-bridge-upgrade` (replace the string `lz-bridge-security-upgrade-v2`
with `lz-bridge-upgrade` in the README content) so the branch reference in the
audit doc matches the actual PR branch.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 25350de3-d7a5-4fab-be79-16fea806989d
📒 Files selected for processing (2)
audit/2026-03_lz-bridge-upgrade/README.mdaudit/2026-03_lz-bridge-upgrade/scopefile.txt
…o addr in `estimateSendFee`
… explicit sender
…/message management tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/policies/bridge/LZBridgeGateway.sol (1)
488-491:⚠️ Potential issue | 🟡 MinorNormalize malformed outer payload decode failures to gateway custom error.
abi.decode(payload_, (uint8, bytes))can still bubble a generic decode revert for malformed payloads, which bypassesLZBridgeGateway_InvalidPayload()and weakens revert observability.Proposed fix
function _decodeAndRoute(uint32 srcEid_, bytes32 guid_, bytes calldata payload_) private { if (payload_.length < _MIN_PAYLOAD_LENGTH) revert LZBridgeGateway_InvalidPayload(); - (uint8 msgType, bytes memory data) = abi.decode(payload_, (uint8, bytes)); + uint8 msgType; + bytes memory data; + try this._decodeOuterPayload(payload_) returns (uint8 decodedMsgType, bytes memory decodedData) { + msgType = decodedMsgType; + data = decodedData; + } catch { + revert LZBridgeGateway_InvalidPayload(); + } if (msgType == MSG_BRIDGE_OHM) { _receiveBridgeOhm(srcEid_, guid_, data); } else { revert LZBridgeGateway_InvalidMessageType(msgType); } } + +function _decodeOuterPayload( + bytes calldata payload_ +) external pure returns (uint8 msgType_, bytes memory data_) { + return abi.decode(payload_, (uint8, bytes)); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/policies/bridge/LZBridgeGateway.sol` around lines 488 - 491, The abi.decode in _decodeAndRoute can bubble a generic decode revert; wrap the decode in a try/catch by moving the abi.decode into a small internal helper (e.g. add a private pure function _safeDecodePayload(bytes calldata) returns (uint8, bytes memory) that performs (uint8 msgType, bytes memory data) = abi.decode(payload_, (uint8, bytes)) and returns them), then call it from _decodeAndRoute using try this._safeDecodePayload(payload_) returns (uint8 msgType, bytes memory data) { ... } catch { revert LZBridgeGateway_InvalidPayload(); } so any malformed outer payload decodes are normalized to LZBridgeGateway_InvalidPayload().
🧹 Nitpick comments (2)
src/scripts/ops/batches/lib/LZBridgeL2BatchScript.sol (2)
5-7: Reorder imports per coding guidelines.Imports should be grouped as: interface, libraries, contracts; sorted alphabetically within each group.
Suggested import order
-import {LZBridgeBatchScript} from "./LZBridgeBatchScript.sol"; -import {console2} from "@forge-std-1.9.6/console2.sol"; import {VmSafe} from "@forge-std-1.9.6/Vm.sol"; +import {console2} from "@forge-std-1.9.6/console2.sol"; +import {LZBridgeBatchScript} from "./LZBridgeBatchScript.sol";As per coding guidelines: "Imports must be at the top of the file below the license and pragma, grouped as: interface, libraries, contracts; sorted alphabetically within each group".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/ops/batches/lib/LZBridgeL2BatchScript.sol` around lines 5 - 7, Reorder the three imports so they follow the project's import grouping and alphabetical rules: group interfaces first, then libraries, then contracts; within each group sort alphabetically. Specifically, place the VmSafe import (from "@forge-std-1.9.6/Vm.sol") in the interface group, the console2 import (from "@forge-std-1.9.6/console2.sol") in the libraries group, and the LZBridgeBatchScript import (from "./LZBridgeBatchScript.sol") in the contracts group, with each group sorted alphabetically.
56-65: Consider using_runBatch()instead of duplicating the loop.This loop is identical to the inherited
_runBatch()implementation (seeBatchScriptV2.sol:155-177). The EOA path at lines 99-101 correctly calls_runBatch()within the broadcast context, demonstrating the preferred approach.Suggested refactor
console2.log("\nBroadcasting batch to Anvil fork"); vm.startBroadcast(_owner); - for (uint256 i; i < _batchTargets.length; ++i) { - (bool success, bytes memory data) = _batchTargets[i].call(_batchData[i]); - if (!success) { - assembly { - let revertStringLength := mload(data) - let revertStringPtr := add(data, 0x20) - revert(revertStringPtr, revertStringLength) - } - } - } + _runBatch(); vm.stopBroadcast();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/scripts/ops/batches/lib/LZBridgeL2BatchScript.sol` around lines 56 - 65, The loop duplicating batch execution should be replaced by calling the inherited _runBatch() to avoid duplication; locate the for-loop that iterates over _batchTargets and _batchData in LZBridgeL2BatchScript.sol and remove it, invoking _runBatch() where that loop currently exists (consistent with how the EOA path calls _runBatch()); ensure the call is made within the same context (broadcast/transaction) so revert bubbling and return behavior remain identical to BatchScriptV2's implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@audit/2026-03_lz-bridge-upgrade/README.md`:
- Line 104: Update the README sentence to match current behavior: note that the
gateway now overrides _inflow (in addition to _outflow) to short-circuit when an
EID is unconfigured (limit == 0 && window == 0), so the previous claim that the
base _inflow is always safe without override is inaccurate; mention RateLimiter,
_inflow, _outflow, gateway, and per-EID zero-inflight short-circuiting so the
spec matches the implementation.
---
Duplicate comments:
In `@src/policies/bridge/LZBridgeGateway.sol`:
- Around line 488-491: The abi.decode in _decodeAndRoute can bubble a generic
decode revert; wrap the decode in a try/catch by moving the abi.decode into a
small internal helper (e.g. add a private pure function _safeDecodePayload(bytes
calldata) returns (uint8, bytes memory) that performs (uint8 msgType, bytes
memory data) = abi.decode(payload_, (uint8, bytes)) and returns them), then call
it from _decodeAndRoute using try this._safeDecodePayload(payload_) returns
(uint8 msgType, bytes memory data) { ... } catch { revert
LZBridgeGateway_InvalidPayload(); } so any malformed outer payload decodes are
normalized to LZBridgeGateway_InvalidPayload().
---
Nitpick comments:
In `@src/scripts/ops/batches/lib/LZBridgeL2BatchScript.sol`:
- Around line 5-7: Reorder the three imports so they follow the project's import
grouping and alphabetical rules: group interfaces first, then libraries, then
contracts; within each group sort alphabetically. Specifically, place the VmSafe
import (from "@forge-std-1.9.6/Vm.sol") in the interface group, the console2
import (from "@forge-std-1.9.6/console2.sol") in the libraries group, and the
LZBridgeBatchScript import (from "./LZBridgeBatchScript.sol") in the contracts
group, with each group sorted alphabetically.
- Around line 56-65: The loop duplicating batch execution should be replaced by
calling the inherited _runBatch() to avoid duplication; locate the for-loop that
iterates over _batchTargets and _batchData in LZBridgeL2BatchScript.sol and
remove it, invoking _runBatch() where that loop currently exists (consistent
with how the EOA path calls _runBatch()); ensure the call is made within the
same context (broadcast/transaction) so revert bubbling and return behavior
remain identical to BatchScriptV2's implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2d0685c2-0493-46e5-afd7-03bdc0b7011b
📒 Files selected for processing (8)
audit/2026-03_lz-bridge-upgrade/README.mdsrc/policies/bridge/LZBridgeGateway.solsrc/policies/interfaces/ILZBridgeGateway.solsrc/proposals/LZBridgeSecurityUpgradeProposal.solsrc/scripts/ops/batches/lib/LZBridgeL2BatchScript.solsrc/test/policies/bridge/LZBridgeGateway.t.solsrc/test/policies/bridge/LZBridgeGatewayFork.t.solsrc/test/proposals/LZBridgeSecurityUpgradeProposal.t.sol
✅ Files skipped from review due to trivial changes (1)
- src/proposals/LZBridgeSecurityUpgradeProposal.sol
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/proposals/LZBridgeSecurityUpgradeProposal.t.sol
- src/test/policies/bridge/LZBridgeGatewayFork.t.sol
- src/policies/interfaces/ILZBridgeGateway.sol
- src/test/policies/bridge/LZBridgeGateway.t.sol
Member
|
Merging this. We can do the review on the new PR. |
Merged
This was referenced Apr 14, 2026
This was referenced May 4, 2026
Merged
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
3 participants
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.
Summary by CodeRabbit
New Features
Documentation
Tests & Ops
Chores