fix(InventoryClient): clamp L2→L1 excess withdrawal to live on-chain balance#3505
fix(InventoryClient): clamp L2→L1 excess withdrawal to live on-chain balance#3505droplet-rl wants to merge 2 commits into
Conversation
… balance withdrawExcessBalances() derives the withdrawal amount from the cached/virtual inventory balance (getCurrentAllocationPct), which can exceed the relayer's settled on-chain balance - e.g. when funds have already been sent out via an in-flight rebalance, or the cached balance is stale relative to recent activity. The L2 bridge burns/transfers from the relayer's actual balance, so an over-estimate reverts with "ERC20: transfer amount exceeds balance" and the withdrawal re-attempts and fails every loop. Clamp the amount to a fresh on-chain balanceOf, always leaving the configured target allocation behind: amount = min(desired, max(0, liveBalance - target)). Skip entirely when the live balance is already at/under target. Mirrors the existing fresh-balance precaution already used on the L1->L2 rebalance path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbd3d6ff9a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| protected async getRemoteTokenBalance(chainId: number, l2Token: Address): Promise<BigNumber> { | ||
| const spokePoolClient = this.tokenClient.spokePoolManager.getClient(chainId); | ||
| assert(isDefined(spokePoolClient), `SpokePoolClient not found for chainId ${chainId}`); | ||
| assert(isEVMSpokePoolClient(spokePoolClient)); |
There was a problem hiding this comment.
Support SVM balance reads for SVM withdrawals
When withdrawExcessPeriod is enabled for Solana USDC, the withdrawal path reaches this helper because CUSTOM_L2_BRIDGE[CHAIN_IDs.SOLANA] maps USDC to the Solana CCTP L2 bridge and adapter.isSupportedL2Bridge() passes. This new EVM-only assertion makes withdrawExcessBalances() throw before queuing the Solana withdrawal instead of reading the SPL token balance, so excess withdrawals from SVM chains are disabled in that configuration.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — fixed in ad2a78c. I moved the fresh-balance read onto TokenClient.getRemoteTokenBalance(), which now dispatches per chain VM instead of asserting EVM-only. SVM clients read the SPL token balance via getSolanaTokenBalance(provider, mint, relayerSvmAddress) (mirroring BalanceAllocator._queryBalance), so when withdrawExcessPeriod is enabled for Solana USDC the helper returns the on-chain balance instead of throwing and aborting the whole withdrawExcessBalances() loop.
| const maxWithdrawable = liveBalance.gt(targetBalanceInL2TokenDecimals) | ||
| ? liveBalance.sub(targetBalanceInL2TokenDecimals) | ||
| : bnZero; |
There was a problem hiding this comment.
Preserve shortfall headroom in live-balance clamp
The allocation check above calls getCurrentAllocationPct(..., ignoreShortfall=false), so desiredWithdrawalAmount is reduced by the chain's token shortfall. When this new clamp is binding, however, maxWithdrawable only leaves the target balance behind; if the cached balance is inflated by more than the shortfall, the bot withdraws liveBalance - target and consumes inventory that was reserved for outstanding shortfalls. Include the shortfall converted to L2 decimals in this cap before queuing the withdrawal.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in ad2a78c. The clamp now reserves the chain's shortfall on top of the target:
maxWithdrawable = max(0, liveBalance - target - shortfall)
Since desiredWithdrawalAmount already nets out the shortfall (currentAllocPct uses ignoreShortfall=false), this leaves the same headroom behind when the clamp binds, so we no longer consume inventory earmarked for unfilled deposits. getShortfallTotalRequirement() is already denominated in l2Token decimals (same as liveBalance / target), so no conversion is needed. Added a regression test: "Reserves the chain's shortfall on top of the target when clamping".
| // Clamp the withdrawal to the relayer's actual on-chain balance, always leaving the target | ||
| // allocation behind. `desiredWithdrawalAmount` is derived from the cached/virtual inventory | ||
| // balance (getCurrentAllocationPct), which can exceed the settled on-chain balance — e.g. when | ||
| // funds have already been sent out via an in-flight rebalance, or the cached balance is stale | ||
| // relative to recent activity. Without this clamp the L2 bridge's burn/transferFrom can revert | ||
| // with "transfer amount exceeds balance", and the withdrawal could drain the chain below target. | ||
| const targetBalanceInL2TokenDecimals = cumulativeBalanceInL2TokenDecimals.mul(targetPct).div(this.scalar); |
There was a problem hiding this comment.
Document the changed excess-withdrawal behavior
This changes InventoryClient runtime behavior by adding a fresh-balance clamp before L2→L1 excess withdrawals, but the change does not update the relevant client/inventory docs. /workspace/relayer/AGENTS.md requires: “Keep all relevant AGENTS.md and README.md files updated in the same change whenever behavior, configuration, interfaces, or runtime flow changes.” Please update the InventoryClient documentation (for example src/clients/README.md or the inventory virtual-balance docs) so operators know excess withdrawals are now capped by live settled balance rather than only virtual allocation math.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Done in ad2a78c. Added a "Withdrawing Excess Balances to L1" section to src/clients/README.md and a "How excess L2->L1 withdrawals clamp to settled balance" section (plus a Common-pitfalls bullet) to docs/inventory-virtual-balance-model.md, documenting that excess withdrawals are now clamped to the live settled balance, leaving target + shortfall behind.
| const spokePoolClient = this.tokenClient.spokePoolManager.getClient(chainId); | ||
| assert(isDefined(spokePoolClient), `SpokePoolClient not found for chainId ${chainId}`); | ||
| assert(isEVMSpokePoolClient(spokePoolClient)); | ||
| const token = new Contract(l2Token.toNative(), ERC20.abi, spokePoolClient.spokePool.provider); |
There was a problem hiding this comment.
Normalize TVM token address before balanceOf
When withdrawExcessPeriod is enabled for Tron USDT, this fresh balance read constructs an ethers Contract with l2Token.toNative(). TVM token addresses are native/base58 in these configs while the ethers-based TRON paths normalize to an EVM-compatible hex address before creating contracts, so the balance read fails before the OFT withdrawal path can run. Normalize TVM addresses for ethers here before instantiating the token contract.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in ad2a78c. The balance read now normalizes to the ethers-compatible hex address via token.toEvmAddress() (and the relayer via relayerEvmAddress.toEvmAddress()) before constructing the contract, matching the existing TokenClient.resolveRemoteTokens() / getEthersCompatibleAddress() convention — so Tron USDT base58 addresses no longer break new Contract(...).
Note: the L2→L1 withdrawal dispatch for TVM (OFTL2Bridge / BaseChainAdapter.withdrawTokenFromL2) is a separate path that does not appear to be wired up for Tron yet, so end-to-end Tron excess withdrawals are still gated elsewhere — but the balance read itself is now correct and no longer the blocker.
…e reads in excess withdrawal Addresses Codex review feedback on the L2->L1 excess-withdrawal clamp: - Reserve the chain's token shortfall on top of the target when clamping. desiredWithdrawalAmount already nets out shortfall (currentAllocPct uses ignoreShortfall=false), but the clamp only left the target behind. When the live balance was stale enough for the clamp to bind, the bot could withdraw inventory earmarked for unfilled deposits. Now: amount = min(desired, max(0, liveBalance - target - shortfall)). - Make the fresh on-chain balance read VM-aware. The previous EVM-only assert in getRemoteTokenBalance would throw (aborting the whole withdrawal loop) for Solana USDC, and used the base58 .toNative() address for Tron USDT which breaks ethers Contract construction. The read now lives on TokenClient and dispatches per VM: EVM/TVM normalize to the ethers-compatible hex address (.toEvmAddress()), SVM reads the SPL token balance. - Add a regression test for the shortfall reservation and document the live-balance clamp in src/clients/README.md and the inventory virtual-balance model doc (per AGENTS.md doc-maintenance requirement). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
@codex thanks for the review — addressed all four points in commit
|
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
|
Thanks for the re-review — glad the updated diff is clean. No further changes; this should be ready for human review/merge. |
Problem
The relayer's L2→L1 excess-inventory withdrawal (
InventoryClient.withdrawExcessBalances) was attempting to withdraw more USDC than the relayer actually holds on the chain, causing the CCTPdepositForBurnto revert withERC20: transfer amount exceeds balance. Because nothing moves on a failed simulation, the inputs don't change and it re-attempts and fails every loop.Observed (Arbitrum USDC, prod
zion-across-fast-relayer-rebalancer)Actual on-chain wallet balance at the time: 167,601 USDC. The bot evaluated Arbitrum as holding ~413.5k when only ~167.6k was settled.
Root cause
desiredWithdrawalAmountiscumulativeBalance × (currentAllocPct − targetPct), i.e. it is derived from the cached/virtual inventory balance (getBalanceOnChain/getCurrentAllocationPct), not the relayer's settled wallet balance. The L2 bridge burns from the relayer's actual balance, so any positive gap between the virtual and settled balance produces an over-withdrawal.In the observed incident the gap was a source-side, in-flight rebalance: the relayer had already sent ~209,938 USDC out of Arbitrum → HyperEVM via CCTP (
pendingBridges: ["42161-0x4096…"]), but the balance used to evaluate Arbitrum's excess still counted those funds — so the same USDC was committed twice (once to the rebalance, once to the L1 withdrawal). The cached balance being stale relative to recent activity produces the same effect.There was previously no clamp to the live balance, and the target was effectively subtracted from the virtual balance, so the realized chain balance could be driven below target (toward zero).
Fix
Before queuing the withdrawal, clamp it to a fresh on-chain
balanceOf, always leaving the configured target allocation behind:This enforces both invariants regardless of why the virtual balance is inflated:
A fresh read is required because the relayer's cached balance is exactly the stale value that caused the issue (clamping to the cache would be a no-op). The new
getRemoteTokenBalance()mirrors the existing fresh-balance precaution already used on the L1→L2 rebalance path (the "🚧 Token balance on mainnet changed, skipping rebalance" guard).When
liveBalance == virtualBalance(the normal case) the clamp is a no-op, so steady-state behaviour is unchanged.Tests
test/InventoryClient.InventoryRebalance.ts:liveBalance − targetand is strictly less thandesiredWithdrawalAmount.All 18 tests in the file pass; the two pre-existing withdrawal tests are unaffected (clamp is a no-op when virtual == live).
yarn typecheckandeslint/prettierclean.Follow-up (not in this PR)
There is a separate, related latent issue on the destination side:
getBalanceOnChainaddspendingRebalances(in-flight inbound rebalances) outside theignoreL1ToL2PendingAmountguard (introduced in #2944), so a chain that is a rebalance destination can also be flagged for excess withdrawal on funds that haven't arrived yet. In the same incident, HyperEVM (credited the 209,938 inbound) was also flagged for excess. This live-balance clamp defends against that case too, but thegetBalanceOnChainguard is worth tightening separately.🤖 Generated with Claude Code