feat: add atomic local rebalancing bridge#8894
Open
xeno097 wants to merge 54 commits into
Open
Conversation
The source router was a caller-supplied argument to localRebalance, gated only by that router's own isAllowedRebalancer. An unauthorized caller could pass a forged router that allowlisted them, reach the arbitrary-calls window in the bridge's context, and puppet the bridge's standing rebalancer privilege on co-enrolled routes. Bind the source router as an immutable constructor argument and drop it from localRebalance, so auth binds to the real route and one bridge serves one source. Tests construct their own source-bound bridge where needed.
The reentrancy check reused _CALLBACK_RECIPIENT_SLOT, which is cleared before the arbitrary calls run, leaving the calls window unguarded against a nested localRebalance. Add a dedicated _REENTRANCY_GUARD_SLOT set at entry and cleared at the end of localRebalance, so the guard spans the whole body, and leave the recipient slot to do only the source-to-destination handoff.
- Use forceApprove for the rebalancer's output-token allowance so the mainnet USDT fork test no longer reverts on USDT's non-bool-returning approve. - Re-pin the Base fork block to one where the USDC/USDT Aerodrome pool quotes ~1:1, so the swap plus top-up funds the destination (the old block had an imbalanced pool, correctly tripping InsufficientOutput). - Default RPC_URL_MAINNET to an archive-capable endpoint so the mainnet fork tests can instantiate the historical fork from .env.default.
localRebalance checked output funding and refunded using absolute balances, so a token donation sitting on the wrapper could satisfy the destination while the escrowed input was refunded to the rebalancer, letting an authorized rebalancer extract source collateral funded by a third-party donation. Snapshot the wrapper balances before the calls and require the calls to produce at least requiredDelta of new output; refund only the per-call delta so donations are never counted toward funding nor swept. For a shared input/output token the entry snapshot is reused so the escrow legitimately counts as funding. Covered by donation-funding revert, shared-token escrow funding with donation preserved, and surplus-refund-with-donation-kept tests.
localRebalance is payable so calls can spend native (e.g. DEX fees), but any unspent msg.value was stranded and could be spent by a later caller via the native-balance sentinel. Refund this call's unspent native to the rebalancer, excluding pre-existing/force-fed native, reverting if the send fails. The refund runs while the reentrancy guard is still set so the recipient hook cannot re-enter.
rebalance granted an exact per-call allowance but never revoked it, so a bridge that consumed less than its quoted collateral left a residual standing allowance - the opposite of the per-call approval goal. Revoke to zero after transferRemote returns. All in-repo bridges consume the full quoted collateral synchronously, so this is defense-in-depth and also clears any legacy max allowance carried by an upgraded router on its first rebalance.
quoteTransferRemote returned two entries for the input token, violating the ITokenFee no-duplicate-token expectation. The router extracts by summing per token so behavior is unchanged; return a single collateral entry plus the zero native entry.
The test asserted a standing max allowance from the removed _addBridge override. Expect no standing allowance before rebalance and assert none remains after, matching the exact per-call approval plus post-transfer revoke.
The deploy no longer emits bridge approval txs, so the per-bridge allowance is zero. Update the e2e assertions to expect 0 and drop the now-unused MAX_UINT256 constant; the approvedTokens config entries are kept to exercise that they are ignored. Verified via CLI_E2E_TEST=warp-deploy-2 (14 passing).
Rename the changeset to match the contract name and describe the final behavior: the bridge binds its source router at construction, guards the whole local-rebalance flow against reentrancy, funds the destination only from output produced by the calls, refunds unspent input/output/native, and rebalance revokes any unconsumed allowance. Rephrased in present-tense declarative.
The native refund excluded pre-existing native, but calls could still spend it (e.g. via the native-balance sentinel) without reverting. Require the wrapper's balance after the calls to be at least its pre-existing native, so calls can spend at most this call's msg.value and donated native stays untouched.
Replace the hand-rolled transient reentrancy slot with the repo's ReentrancyGuardTransient nonReentrant modifier (already used by QuotedCalls). transferRemote stays unguarded so the legitimate rebalance callback still re-enters; only nested localRebalance is blocked, now via ReentrancyGuardReentrantCall.
Use the repo-wide Address.sendValue convention (as in TokenBridgeOft and QuotedCalls) for the native refund instead of a hand-rolled call/return-check, and drop the now-unused NativeRefundFailed error.
The single callback slot held the source router and was then overwritten with the resolved destination recipient, which was hard to follow. Use a dedicated slot for each. Since the source slot now stays set for the whole callback window, reject a second callback explicitly so a source cannot pull collateral twice, and revert with MissingCallback when the source never calls back.
paulbalaji
approved these changes
Jun 12, 2026
paulbalaji
left a comment
paulbalaji
left a comment
Collaborator
There was a problem hiding this comment.
Review — atomic local rebalancing bridge
Reviewed at c19e83a via a parallel red-team (value-accounting, arbitrary-call/reentrancy/privilege-escalation, approval-model/backwards-compat, SDK/TS, test-adequacy). Approving — the design is sound and I could not construct a fund-drain, under-collateralization, value-stranding, or caller-extraction path under the stated trust model (immutable source router, owner-configured recipient, balance-stable ERC20s). A few things I'd like addressed as part of rollout / follow-up (inline), none of which block the contract being correct as written.
What holds up well
- Immutable
sourceRoutergenuinely bounds blast radius;localRebalanceconsults only it for auth and the pull. - Privilege escalation through the arbitrary
callswindow is blocked on every vector traced: re-enteringsource.rebalance(slots cleared →NoActiveRebalance), pulling via another bridge (InvalidInputDelta),transferFromthe source (allowance already revoked beforecallsrun), and inflating the destination via its own bridge path (InsufficientOutput). The caller is already a source rebalancer, so gains no new authority. - Delta-based accounting correctly excludes pre-existing input/output/native donations from funding and refunds, handles the shared input==output branch, and the reentrancy guard + transient state-machine (2nd-callback reject /
MissingCallback) hold. - Native: pre-existing balance can't be spent (
InvalidNativeDelta); onlymsg.valueaccrual is refunded. - Exact-allowance model:
collateralFeesprovably matches what every in-tree bridge (base TokenRouter, OFT, Everclear) pulls; the post-transfer revoke is robust across revert / native / no-op paths and is strictly safer than the prior standing-max-approval. - SDK removals are complete (no dangling refs), no type casts introduced, and old configs still parse + apply.
Please address (see inline)
- Legacy standing allowances on already-deployed routes (most important). The whole model assumes a route holds no bridge allowance outside the active
rebalancecall, but routes upgraded in place keep their oldtype(uint256).maxapprovals until governance clears them per-pair. The contract is correct either way, but this is the load-bearing security premise and is enforced only by the upgrade note. Recommend the SDK emit revocation txs for existing positive router→bridge allowances during apply/upgrade (even when the bridge stays allowlisted), plus a runbook step. One reviewer would treat this as blocking; at minimum it should ship alongside rollout, not as docs. decimals()DoS for tokens reportingdecimals() >= 78or not implementing it (liveness only, config-dependent).- Fuzz invariant is weaker than the PR text claims — single shared 6-decimal token,
catch {}swallows the (many) reverting inputs with no success counter, and CI runs at the repo default of 50 runs, not 1000. - Minor: no event on
localRebalance; quote returns 2 entries (not "single"); arbitrarycallscan move non-input/output token donations off the bridge; changeset tense; stale e2e test name.
Pre-existing, not introduced here (FYI)
allowedRebalancingBridges[].approvedTokens is now fully vestigial (no longer consumed on deploy), but it's still schema-accepted, the reader returns only { bridge }, and transformConfigToCheck doesn't strip it — so a config that sets approvedTokens can drift on warp check. types.ts / EvmWarpRouteReader.ts / configUtils.ts are untouched by this PR, so this is a pre-existing latent issue, not a regression. Worth a follow-up to deprecate/strip the field now that it does nothing.
…e approveTokenForBridge - Document the configuration-only par invariant on the bridge and _requiredDelta: input and output must be economically at par since conversion is decimals-only. - Add testFuzz_localRebalance_crossDecimalPreservesValue asserting decimal- normalized value across routers does not decrease for distinct input/output decimals and a fuzzed swap price. - Mark approveTokenForBridge @Custom:deprecated and note it revokes despite the name; the selector is retained for upgrade/governance compatibility.
Routes upgraded in place still carried the pre-atomic-rebalancing type(uint256).max allowance for bridges that remain allowlisted. Add EvmWarpModule.createRevokeStaleBridgeAllowancesTxs, wired into update() after the upgrade tx, which revokes those allowances. Gated on (1) the router being on a legacy contract version (<= 11.3.1, the last grant-max approveTokenForBridge) and (2) an in-place upgrade being generated this run, so the revoke tx only ever runs against the new revoke-semantics impl. Hardhat tests drive the gate through the real update() flow via the version-spoof pattern.
Native collateral is unsupported (decimal conversion needs an ERC20). Reject a native source at construction (the source is immutable) and guard quoteTransferRemote against a caller whose collateral token is address(0), which would otherwise produce a duplicate zero-address quote entry.
localRebalance moved value (escrow, swap, destination funding, refund) but emitted no event of its own; only the source router's CollateralMoved fired. Emit LocalRebalanceExecuted(destinationRouter, amountIn, requiredDelta) on success so the resolved destination and amounts are observable.
Output donations (output funding floor) and native donations (InvalidNativeDelta) were already protected, but the wrapper's pre-call input balance had no floor, so a call could swap the escrow plus an input donation and the rebalancer would pocket the surplus output. Require the input balance after the calls to be at least the pre-call input snapshot, matching the output/native guards.
Bound the swap output at or above requiredDelta so the cross-decimal fuzz always succeeds and asserts every run (no swallowed reverts), add an upper-bound check that router value increases by at most the up-rounding of requiredDelta, and bump both rebalance fuzz tests to 1000 runs via per-test forge-config.
The test no longer creates token approvals (it asserts the allowance is 0), so rename it to 'should set the allowed bridges without creating token approvals'.
If a source router leaks a standing approval to the bridge, a call could pull extra collateral from the source beyond amountIn. Assert the source-drain guard reverts InvalidInputDelta when a call drains the source via such a leaked approval.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
solidity/contracts/token/AtomicLocalRebalancingBridge.sol (1)
252-255: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winGuard
decimals()before exponentiating.Line 254 still does
10 ** decimals(). Tokens returning>= 78overflow here, and tokens that don’t implementdecimals()still brick everylocalRebalanceon that route with a raw revert instead of a bridge-specific error. That’s a config-shaped liveness failure, not just a docs quirk.As per coding guidelines, "Validate all parameters within safe bounds in Solidity function implementations."Possible hardening
+ error InvalidDecimals(); + /// `@dev` Reverts if `token` does not implement `IERC20Metadata.decimals()`. function _decimalScale(address token) internal view returns (uint256) { - return 10 ** uint256(IERC20Metadata(token).decimals()); + try IERC20Metadata(token).decimals() returns (uint8 d) { + if (d > 77) revert InvalidDecimals(); + return 10 ** uint256(d); + } catch { + revert InvalidToken(); + } }🤖 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 `@solidity/contracts/token/AtomicLocalRebalancingBridge.sol` around lines 252 - 255, The _decimalScale function currently directly calls IERC20Metadata(token).decimals() and computes 10 ** decimals(), which can overflow for decimals >= 78 and will raw-revert if the token doesn't implement decimals(); update _decimalScale to first call IERC20Metadata(token).decimals() in a try/catch (or a low-level staticcall) and on failure revert with a bridge-specific error (e.g. UnsupportedToken or MissingDecimals for the token), then validate the returned decimals value is within safe bounds (require(decimals <= 77) or similar) before computing 10 ** uint256(decimals) to avoid overflow; reference symbols: _decimalScale and IERC20Metadata.decimals().Source: Coding guidelines
🧹 Nitpick comments (1)
solidity/contracts/token/AtomicLocalRebalancingBridge.sol (1)
49-54: 📐 Maintainability & Code Quality | ⚡ Quick winInclude the initiating rebalancer in
LocalRebalanceExecuted.Right now this only indexes
destinationRouter, so off-chain consumers still can’t tell who kicked the rebalance without replaying the whole tx. Adding an indexed caller keeps the event properly queryable and lines up with the repo’s event rules.As per coding guidelines, "Index action creator and operated-upon users/IDs in Solidity events."
🤖 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 `@solidity/contracts/token/AtomicLocalRebalancingBridge.sol` around lines 49 - 54, The LocalRebalanceExecuted event must include the initiating rebalancer as an indexed address so off-chain consumers can query who triggered the action; update the event declaration in AtomicLocalRebalancingBridge.sol (LocalRebalanceExecuted) to add an address indexed rebalancer (or caller) parameter, and update every emit LocalRebalanceExecuted(...) call to pass the rebalancer (use the function's rebalancer param or msg.sender where appropriate) so the emitted event matches the new signature.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@solidity/contracts/token/AtomicLocalRebalancingBridge.sol`:
- Around line 252-255: The _decimalScale function currently directly calls
IERC20Metadata(token).decimals() and computes 10 ** decimals(), which can
overflow for decimals >= 78 and will raw-revert if the token doesn't implement
decimals(); update _decimalScale to first call IERC20Metadata(token).decimals()
in a try/catch (or a low-level staticcall) and on failure revert with a
bridge-specific error (e.g. UnsupportedToken or MissingDecimals for the token),
then validate the returned decimals value is within safe bounds
(require(decimals <= 77) or similar) before computing 10 ** uint256(decimals) to
avoid overflow; reference symbols: _decimalScale and IERC20Metadata.decimals().
---
Nitpick comments:
In `@solidity/contracts/token/AtomicLocalRebalancingBridge.sol`:
- Around line 49-54: The LocalRebalanceExecuted event must include the
initiating rebalancer as an indexed address so off-chain consumers can query who
triggered the action; update the event declaration in
AtomicLocalRebalancingBridge.sol (LocalRebalanceExecuted) to add an address
indexed rebalancer (or caller) parameter, and update every emit
LocalRebalanceExecuted(...) call to pass the rebalancer (use the function's
rebalancer param or msg.sender where appropriate) so the emitted event matches
the new signature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 741bbcb6-9081-4e96-9bfd-ed140cd3eae1
📒 Files selected for processing (7)
.changeset/atomic-local-rebalancing-bridge.mdsolidity/contracts/token/AtomicLocalRebalancingBridge.solsolidity/contracts/token/libs/MovableCollateralRouter.solsolidity/test/token/AtomicLocalRebalancingBridge.t.soltypescript/cli/src/tests/ethereum/warp/warp-deploy-2.e2e-test.tstypescript/sdk/src/token/EvmWarpModule.hardhat-test.tstypescript/sdk/src/token/EvmWarpModule.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- .changeset/atomic-local-rebalancing-bridge.md
- solidity/contracts/token/libs/MovableCollateralRouter.sol
Reconcile the multi-recipient design (#8863) onto the hardened atomic local rebalancing bridge: - Add IRebalanceTargets and CrossCollateralRouter rebalance-target enrollment (addRebalanceTarget/removeRebalanceTarget/isRebalanceTarget), storage-appended. Cross-collateral routers must be added explicitly. - Reshape the bridge entry to the canonical-mirroring rebalance(uint32,uint256,ITokenBridge,bytes32,bytes): the source arg is a checked echo of the immutable source, the destination recipient is supplied per call and validated via isRebalanceTarget, and the trailing bytes carry the abi-encoded calls. The source must be a CrossCollateralRouter, asserted at construction. - Keep the immutable source, nonReentrant guard, consumed-after-escrow callback flag, and the donation/native/input/output invariants.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@solidity/contracts/token/AtomicLocalRebalancingBridge.sol`:
- Around line 140-142: The conversion of destinationRecipient to address using
TypeCasts.bytes32ToAddress() does not validate that the value is canonically
encoded (upper 96 bits must be zero). Add a canonicality check before the
conversion to verify that destinationRecipient has all zeros in its upper 96
bits, ensuring it represents a valid canonical address encoding. This validation
should occur before the TypeCasts.bytes32ToAddress() call to enforce proper
parameter bounds as per your coding guidelines.
In `@solidity/contracts/token/CrossCollateralRouter.sol`:
- Around line 189-196: The isRebalanceTarget function has a parameter validation
issue: when routers(_domain) is unset (defaults to bytes32(0)), the function
incorrectly returns true for a _target of bytes32(0), authorizing an invalid
zero target implicitly. Add a validation check at the beginning of the
isRebalanceTarget function to ensure _target is not bytes32(0), and return false
for invalid zero targets. This prevents false-positive authorization results and
aligns with the requirement to validate all parameters within safe bounds in
Solidity function implementations.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2a25e007-e8d9-48c0-a946-ab454c0e72ab
📒 Files selected for processing (7)
.changeset/atomic-local-rebalancing-bridge.mdsolidity/contracts/token/AtomicLocalRebalancingBridge.solsolidity/contracts/token/CrossCollateralRouter.solsolidity/contracts/token/interfaces/IRebalanceTargets.solsolidity/test/token/AtomicLocalRebalancingBridge.fork.t.solsolidity/test/token/AtomicLocalRebalancingBridge.t.solsolidity/test/token/CrossCollateralRouter.t.sol
✅ Files skipped from review due to trivial changes (1)
- .changeset/atomic-local-rebalancing-bridge.md
🚧 Files skipped from review as they are similar to previous changes (1)
- solidity/test/token/AtomicLocalRebalancingBridge.fork.t.sol
nambrot
reviewed
Jun 16, 2026
| ) { | ||
| revert InvalidInputDelta(); | ||
| } | ||
| // Calls must not spend the wrapper's pre-call input donation. |
Contributor
There was a problem hiding this comment.
What is pre-call input donation? Is it just the pulling of the funds from the source router? Donation seems misleading then?
Contributor
Author
There was a problem hiding this comment.
yup changed comment wording here
| } | ||
| // Calls must not spend the wrapper's pre-call input donation. | ||
| if ( | ||
| IERC20(inputToken).balanceOf(address(this)) < selfBefore.inputToken |
Contributor
There was a problem hiding this comment.
I'm not sure I understand this invariant, why is it wrong if this contract ends up with inputToken?
Contributor
Author
There was a problem hiding this comment.
This check is more about avoiding the rebalancer to use already existing funds on the bridge to complete a rebalance. Rebalance might be called with an amountIn that is less than the required amount to complete a swap so the already existing funds on the contract would be used, but that wouldn't be a proper rebalance, no (source router would have more tokens than the destination because less was pulled)?
- add IRebalancingBridge interface defining the rebalance entrypoint signature - rename for clarity (drop "delta"/"input"/"output"): sourceToken/ destinationToken, amount, requiredOutputAmount, allowedSourceRouter immutable + plain sourceRouter param, InvalidSourceTokenBalance, _refundTokenBalance - narrow the constructor to an is-contract check; IRebalanceTargets support is a documented precondition surfaced at rebalance - remove the pre-existing-native floor (native can only accrue via exotic force-send); refund unspent native in full - keep the source-token floor and destination receipt check; tidy comments
Re-add getAllowedBridgesApprovalTxs so warp apply grants the standing approvedTokens allowances that pre-atomic-rebalancing routers still need. Gated on the on-chain contract version being legacy (<= MAX_LEGACY_BRIDGE_ APPROVAL_VERSION), mirroring createRevokeStaleBridgeAllowancesTxs: newer impls grant per rebalance and the same selector revokes, so grants are suppressed there.
…voke Export MAX_LEGACY_BRIDGE_APPROVAL_VERSION and bridgeApprovalGrantsMaxAllowance so callers can tell whether approveTokenForBridge grants or revokes for a given contract version. The governance transaction reader now reads the target router's version and labels the call a grant or a revoke accordingly, instead of always describing it as a clear. Broaden createRevokeStaleBridgeAllowancesTxs to also revoke any configured approvedTokens (not just the collateral token) for bridges that remain allowlisted after an in-place upgrade, so no stale legacy max approval survives.
isRebalanceTarget returned true for bytes32(0) on a domain with no enrolled router, since routers(domain) is itself zero. Guard the read so the zero address is never a valid rebalance target. The setter (addRebalanceTarget) is intentionally left unguarded.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
solidity/test/token/AtomicLocalRebalancingBridge.t.sol (1)
708-742: 💤 Low valueConsider adding a test for pre-existing native balance on the bridge.
The
test_rebalance_refundsUnspentNativetest handles the happy path where msg.value gets returned. But if that native balance check I mentioned in the implementation gets added, you'd want a test here to make sure pre-existing ETH on the bridge doesn't get swept up by the rebalancer. Just somethin' to keep in mind, yeah?function test_rebalance_preservesPreExistingNativeBalance() public { swapTarget.setOutputAmount(100e6); // Pre-fund bridge with some ETH vm.deal(address(bridge), 0.5 ether); vm.deal(rebalancer, 1 ether); vm.prank(rebalancer); bridge.rebalance{value: 1 ether}( LOCAL_DOMAIN, 100e6, ITokenBridge(address(sourceRouter)), _toBytes32(address(destinationRouter)), abi.encode(_rebalancerCalls(100e6)) ); // Rebalancer gets back only what they sent assertEq(rebalancer.balance, 1 ether); // Pre-existing ETH stays on bridge assertEq(address(bridge).balance, 0.5 ether); }🤖 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 `@solidity/test/token/AtomicLocalRebalancingBridge.t.sol` around lines 708 - 742, Add a new test function called test_rebalance_preservesPreExistingNativeBalance after the existing test_rebalance_revertsWhenNativeRefundFails function to verify that pre-existing native balance on the bridge contract is preserved during a rebalance operation. The test should pre-fund the bridge with ETH, then execute a rebalance with additional ETH from the rebalancer, and assert that only the rebalancer's sent ETH is returned while the pre-existing bridge balance remains unchanged. This ensures the implementation correctly distinguishes between msg.value and pre-existing contract balance when processing refunds.
🤖 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 `@solidity/contracts/token/AtomicLocalRebalancingBridge.sol`:
- Line 75: The `InvalidNativeDelta` error definition in the contract is declared
but never thrown, and the native balance refund code at lines 218-221 sends the
entire contract balance without validating against pre-existing balance. Either
remove the unused `InvalidNativeDelta` error if native balance protection is not
required, or implement proper native balance snapshotting by capturing the
contract's balance before the call that receives native tokens, then validating
or refunding only the difference between pre-existing and new balance while
throwing `InvalidNativeDelta` if validation fails. This ensures the refund logic
aligns with the PR's stated intention of preventing pre-existing balance
spending.
---
Nitpick comments:
In `@solidity/test/token/AtomicLocalRebalancingBridge.t.sol`:
- Around line 708-742: Add a new test function called
test_rebalance_preservesPreExistingNativeBalance after the existing
test_rebalance_revertsWhenNativeRefundFails function to verify that pre-existing
native balance on the bridge contract is preserved during a rebalance operation.
The test should pre-fund the bridge with ETH, then execute a rebalance with
additional ETH from the rebalancer, and assert that only the rebalancer's sent
ETH is returned while the pre-existing bridge balance remains unchanged. This
ensures the implementation correctly distinguishes between msg.value and
pre-existing contract balance when processing refunds.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3fa5e268-ca6f-4488-b58f-a03c31d559f6
📒 Files selected for processing (10)
.changeset/atomic-local-rebalancing-bridge.mdsolidity/contracts/token/AtomicLocalRebalancingBridge.solsolidity/contracts/token/CrossCollateralRouter.solsolidity/contracts/token/interfaces/IRebalancingBridge.solsolidity/test/token/AtomicLocalRebalancingBridge.t.solsolidity/test/token/CrossCollateralRouter.t.soltypescript/infra/src/tx/govern-transaction-reader.tstypescript/sdk/src/index.tstypescript/sdk/src/token/EvmWarpModule.hardhat-test.tstypescript/sdk/src/token/EvmWarpModule.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- solidity/contracts/token/CrossCollateralRouter.sol
- solidity/test/token/CrossCollateralRouter.t.sol
- .changeset/atomic-local-rebalancing-bridge.md
- typescript/sdk/src/token/EvmWarpModule.hardhat-test.ts
Convert the four post-call invariant checks (previously a doubled InvalidSourceTokenBalance and a doubled InsufficientOutput) to require statements with distinct ALRB-prefixed messages, matching the MCR/CCR convention and disambiguating which invariant failed. The messages are named string constants on the contract, mirrored as bytes constants in the tests. Also group the function's comments and drop the "wrapper" naming.
Refund the bridge's entire token balances (not just the amount accrued during the call) to the rebalancer, mirroring the existing full native-balance refund, so no tokens are left stranded on the wrapper between calls. The funding invariants stay snapshot-based, so pre-existing balances still cannot fund the destination; only the post-invariant refund now sweeps everything. Tests updated accordingly (and the "donation" wording dropped for "pre-existing balance").
Make the post-call section read as two verify-settle cycles: the three checks that verify the rebalancer's calls (one of which gates the destination transfer) run first, then the destination funding and balance sweeps run as one uninterrupted settlement block, then the destination-gain check verifies the settlement. Comments label each phase precisely rather than implying a single linear checks/effects split. Behavior unchanged.
createRevokeStaleBridgeAllowancesTxs matched remaining bridges per domain, so a bridge moved between domains fell out of every intersection and kept its stale legacy allowance. Match by bridge address across all domains instead (ERC20 allowances are router+bridge and domain-agnostic), revoking the collateral token and any configured approvedTokens. Removed bridges are still left to _removeBridge and added bridges still never held a legacy allowance. Adds a moved-bridge test.
The post-settlement destination-gain assertion could only fail if safeTransfer under-delivered to the destination router — i.e. a non-standard (fee-on-transfer/ rebasing) destination token, which the contract already documents as unsupported. The arbitrary calls cannot reduce the destination router's balance (the wrapper has no authority over it), so under the standard-ERC20-at-par precondition the check is always true and no test ever exercised it. Removed it along with the now-unused destination-router snapshot and ERR_DESTINATION_UNDERFUNDED; source-drain and output-produced carry the safety weight.
createRevokeStaleBridgeAllowancesTxs only emitted revokes when an in-place upgrade was scheduled AND the router was still on a legacy impl. If a prior run upgraded the impl but its revoke txs never executed, a rerun saw the new version (or no scheduled upgrade) and emitted nothing, stranding the stale allowance. Now it skips only the unsafe case (legacy impl not being upgraded, where approveTokenForBridge would grant); otherwise it revokes against the new impl — whether upgrading in place or already upgraded — gated by the existing non-zero on-chain allowance check. Updates the already-upgraded test to the retry case and adds a no-stale-allowance negative.
nambrot
reviewed
Jun 17, 2026
| sourceToken: sourceBalanceBefore, | ||
| // Shared source/destination token reuses the source snapshot; | ||
| // otherwise exclude any pre-existing destination-token balance. | ||
| destinationToken: destinationToken == sourceToken |
Contributor
There was a problem hiding this comment.
I just noticed this now but when would source and destinationToken be the same?
Contributor
Author
There was a problem hiding this comment.
Couldn't we use this bridge to replenish collateral between 2 different warp routes that have the same collateral token (i know it is not the main use case for this contract)?
Revert the full-balance sweep: the rebalancer is refunded only the balances accrued during the call, leaving any balance the wrapper held before escrow untouched (per Nam's review). This restores the "pre-existing untouched" property end-to-end. Native now gets the same treatment as the tokens: snapshot the pre-existing native (excluding msg.value), reject calls that spend it (ERR_PREEXISTING_NATIVE_SPENT), and refund only the unspent remainder. The native snapshot lives on SelfBalanceSnapshot to avoid stack-too-deep. Restores the NativeSink helper and the pre-existing-native tests.
Break the dense rebalance body into named internal helpers, following the repo's conventions (thin orchestrator + _validate*/_verbNoun helpers, explicit params in the 4-6 range, reusing the existing SelfBalanceSnapshot rather than a context struct): _validateAndParseSourceRouter, _validateAndParseDestinationAddress, _snapshotSelfTokenBalances, _validatePostCallBalances, _refundAccruedBalances. The destination funding transfer stays inline (the one core mutation). Rename _pullSourceCollateral -> _pullSourceRouterCollateral (per review). Document the operating invariants in a contract-level @dev block and clarify the shared source/destination-token case. No behavior change.
Convert the four post-call invariant require-strings to custom errors so the whole contract reverts uniformly with custom errors (input validation already did), resolving the mixed-style inconsistency: SourceRouterOverdrawn, PreexistingSourceTokenSpent, PreexistingNativeBalanceSpent, InsufficientOutputTokenProduced. Tests assert the error selectors instead of the dropped string mirrors.
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
4 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.
Replaces #8806
This PR supersedes #8806, which was opened from a fork I don't own (so I can't push the follow-up changes there). It targets the same base branch and contains the same feature, with the #8806 review feedback resolved, the #8863 multi-recipient design folded in, and additional security hardening applied from review (see "Review feedback & hardening" below).
Summary
AtomicLocalRebalancingBridgefor same-chain local rebalances, exposing a singlerebalance(...)entrypoint defined by the newIRebalancingBridgeinterfacesourceRoutercall argument is a checked echo of that immutablebytes32argument, validated against the source's rebalance targets viaIRebalanceTargets.isRebalanceTarget(localDomain, destinationRecipient)— the bridge funds the validated argument, not the source's callback recipientMovableCollateralRouterallowed rebalancers for caller authorizationCallLibcalls (ABI-decoded from the trailingbytes) after the same-chain rebalance callback; calls must produce the required output-token balance on the wrapperCrossCollateralRouterimplementsIRebalanceTargets, supporting multiple local rebalance recipients per domain beyond the enrolled remote routerEntrypoint
The signature mirrors the canonical
MovableCollateralRouter.rebalance(uint32,uint256,ITokenBridge)and carries the destination + calls as additional arguments.datais opaque to the interface so future flavors can decode it differently.Rebalance targets (
IRebalanceTargets)CrossCollateralRouterimplementsisRebalanceTarget(uint32 domain, bytes32 target).addRebalanceTarget/removeRebalanceTarget(owner-only, events emitted), surfaced byrebalanceTargets(domain).enrollCrossCollateralRouters) are not automatically valid targets and must be added explicitly.IRebalanceTargets; the constructor verifies the source is a contract.Standing approval model
addBridgenow only allowlists the bridge; it no longer creates max token allowance.HypERC20Collateralno longer approves bridges during bridge enrollment.MovableCollateralRouter.rebalancegrants only the exact collateral-token allowance quoted by the selected bridge, and revokes any unconsumed allowance aftertransferRemote.approveTokenForBridge(address,address)is retained for upgrade/governance compatibility but now revokes legacy approval instead of setting max approval (the selector is overloaded by impl version).<= MAX_LEGACY_BRIDGE_APPROVAL_VERSION),allowedRebalancingBridges[].approvedTokensstill emitsapproveTokenForBridgegrants, since those impls grant max. On newer routers the field is ignored (allowances are granted per rebalance).approvedTokens. A bridge being removed in the same run has its collateral cleared on-chain by_removeBridge, but its extraapprovedTokensare not discoverable from config and must be revoked while the bridge is still configured.MAX_LEGACY_BRIDGE_APPROVAL_VERSIONandbridgeApprovalGrantsMaxAllowance(version); the governance transaction reader uses them to read the target router's on-chain version and describeapproveTokenForBridgeas a grant or a revoke accordingly (instead of always labelling it a clear).Atomic bridge call model
Rebalancer calls perform approvals/swaps/top-ups and must leave at least the required output-token balance on
AtomicLocalRebalancingBridge, produced by the calls (balances already on the wrapper cannot satisfy the requirement). The wrapper pays the destination router directly, so a call bundle cannot satisfy the local balance requirement by invoking the destination router's outbound bridge path with an attacker-controlled remote recipient. The whole flow (including the calls window) isnonReentrant; a single transient callback flag authorizes exactly onetransferRemoteescrow and is consumed before the token pull (CEI), so the post-invariant native refund cannot re-enter to escrow again.sequenceDiagram participant R as Rebalancer participant B as AtomicLocalRebalancingBridge participant S as Source router participant C as Call targets and DEX participant D as Destination router R->>B: rebalance(localDomain, amountIn, sourceRouter, destinationRecipient, data) B->>B: require sourceRouter equals allowedSourceRouter, caller is an allowed rebalancer B->>S: isRebalanceTarget(localDomain, destinationRecipient), else InvalidRecipient B->>B: set callback-active flag, snapshot balances B->>S: rebalance(localDomain, amountIn, bridge) S->>B: transferRemote(localDomain, recipient, amountIn) B->>B: consume callback flag, reject a second callback B->>S: transferFrom(sourceRouter, bridge, amountIn) B->>B: assert callback consumed, else MissingCallback B->>C: multicall(decoded calls) produces output token on the bridge B->>B: enforce source-drain and output-produced invariants B->>D: transfer(destinationRouter, requiredOutputAmount) B->>R: sweep all remaining token and native balances S->>S: revoke any unconsumed allowanceReview feedback & hardening
Addresses the #8806/#8894 review feedback (CodeRabbit, Nam, security review):
sourceRouterargument is a checked echo, so the arbitrarycallscannot puppet privileges on another route.ReentrancyGuardTransient(nonReentrant) plus a single transient callback flag consumed before escrow (CEI); the legitimatetransferRemotecallback is authorized once, a second callback reverts, and a missing callback revertsMissingCallback.transferRemoteinsidecallsinflating the destination — the destination is funded only by output the calls produce; routing output through the destination drains the wrapper and revertsInsufficientOutputTokenProduced.msg.valueis refunded viaAddress.sendValueafter the callback flag is consumed, so it cannot re-enter to escrow.rebalancerevokes any unconsumed allowance.destinationRecipient, validated viaisRebalanceTarget; the zero address is rejected.requiredOutputAmount); a non-par destination reverts or demands absurd top-ups. Par is a configuration-only invariant.Decisions
approveTokenForBridgenaming — kept (the selector is relied on by upgrade/governance tooling). Rather than rename, the human-review hazard is mitigated: the governance transaction reader now reads the target's version and labels the call a grant or a revoke correctly.isRebalanceTarget(domain, bytes32(0))— rejected at the read;addRebalanceTargetis intentionally left unguarded (the read is the authoritative oracle, the bridge fails closed regardless).EIP-170
The Solidity optimizer
runswas lowered from 10,000 to 9,990 (Foundry + Hardhat). At 10,000 runs the optimizer crossed a discrete cost-model threshold that inflatedCrossCollateralRouterto 24,607 bytes (31 over the limit); output is byte-identical across 9,900–9,990, bringing CCR to 24,457 bytes with a negligible runtime gas trade-off.Upgrade note
Not an old-major-incompatible upgrade. Existing routes can upgrade in place because the
approveTokenForBridge(address,address)selector remains available; calling it after upgrade clears legacy standing allowances (collateral token and anyapprovedTokens). The atomic bridge assumes route approvals are not left available outside the activerebalancecall.Changesets
@hyperlane-xyz/core: major — bridge approval behavior changes and standing allowances are removed;IRebalanceTargets/IRebalancingBridgeadded; optimizer runs lowered.@hyperlane-xyz/sdk: minor — legacyapprovedTokensgrants are version-gated, stale-allowance revoke is broadened, and version helpers are exported.Tests
forge test --match-contract AtomicLocalRebalancingBridgeforge test --match-contract CrossCollateralRouterforge test --match-path test/token/MovableCollateralRouter.t.solforge test --match-path test/token/HypERC20MovableCollateral.t.solforge test --match-test testFuzz_localRebalance_doesNotDecreaseRouterTokenSum --fuzz-runs 1000test/token/AtomicLocalRebalancingBridge.fork.t.solpnpm -C typescript/sdk test:hardhat(rebalancing-bridge grant/revoke gating, version spoofing)CLI_E2E_TEST=warp-deploy-2 pnpm -C typescript/cli test:ethereum:e2e