improve: parameterize Arbitrum_RescueAdapter L2 recipient - #1496
improve: parameterize Arbitrum_RescueAdapter L2 recipient#1496droplet-rl wants to merge 2 commits into
Conversation
The deployed (Dec 2022) Arbitrum_RescueAdapter hardcodes its L2 recipient to the deployer address and was compiled against the pre-Nitro inbox interface. This updates the adapter to take the recipient as an explicit constructor arg (instead of msg.sender) so a fresh deployment can direct rescued ETH to a chosen address, refreshes the stale classic-era comments (the source already targets the post-Nitro unsafeCreateRetryableTicket), and adds a Foundry deploy script plus tests covering the HubPool delegatecall flow. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: droplet-rl <284132418+droplet-rl@users.noreply.github.com>
12da511 to
38df356
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 12da511cc3
ℹ️ 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".
| l1Inbox = _l1ArbitrumInbox; | ||
|
|
||
| l2RefundL2Address = msg.sender; | ||
| l2Recipient = _l2Recipient; |
There was a problem hiding this comment.
When _l2Recipient is accidentally passed as address(0), the immutable recipient cannot be corrected and relayMessage will send the rescued l2CallValue plus all retryable refunds to the zero address, permanently burning the ETH during the one-off rescue. Since this constructor now parameterizes the fund recipient, add a nonzero check before storing it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — added if (_l2Recipient == address(0)) revert InvalidL2Recipient(); in the constructor, plus a Foundry test (test_constructor_RevertsOnZeroRecipient) covering it.
Addresses Codex review feedback: a zero _l2Recipient would permanently burn the rescued ETH and all retryable refunds, and the immutable cannot be corrected after deployment. Revert with InvalidL2Recipient in the constructor and add a test covering it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Addressed the Codex review feedback: the constructor now reverts with @codex review — please re-review the updated diff. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 38df356044
ℹ️ 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".
| constructor(ArbitrumL1InboxLike _l1ArbitrumInbox, address _l2Recipient) { | ||
| l1Inbox = _l1ArbitrumInbox; | ||
|
|
||
| l2RefundL2Address = msg.sender; | ||
| l2Recipient = _l2Recipient; |
There was a problem hiding this comment.
When a deployment supplies address(0) for _l2Recipient, this immutable is accepted and later used as the retryable ticket destination plus both refund addresses, so the rescue call would send the aliased HubPool ETH and all refunds to the zero address on Arbitrum. This regresses from the previous msg.sender recipient, which could not be zero, and makes a misdeployment able to permanently lose the funds this adapter is meant to recover.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This review ran against the pre-fix commit 38df356 (race with the push). Already addressed in 1f283ab: the constructor reverts with InvalidL2Recipient() on a zero _l2Recipient, with a Foundry test (test_constructor_RevertsOnZeroRecipient) covering it.
|
This second Codex review ran against the pre-fix commit |
1 participant
Motivation
~13.92 ETH of gas-refund dust has accrued at the HubPool's aliased address on Arbitrum (
0xd297fA914353c44B2e33EBE05F21846f1048CFeB), becauserelayTokens→l1ERC20GatewayRouter.outboundTransfergives no way to direct L2 refunds. The existing mainnetArbitrum_RescueAdapterdeployment (0xC6fA0a4EBd802c01157d6E7fB1bbd2ae196ae375, Dec 2022) can't direct the rescue: its recipient is immutable and set tomsg.senderat construction (the deployer EOA), and its deployed bytecode was compiled against the pre-Nitro inbox interface (createRetryableTicketNoRefundAliasRewrite). A fresh deployment is needed to send the rescued ETH to the intended recipient (the Risk Labs relayer0x07aE…70E67, matching where the productionArbitrum_Adapteralready sends its L2 gas refunds).Context: Slack thread
C0BHMM63D9Q/1784437086.818009(Arbitrum orphaned funds recovery).Changes
Arbitrum_RescueAdapter.sol: take the L2 recipient as an explicit constructor arg (l2Recipient) instead ofmsg.sender; import the inbox interface directly frominterfaces/ArbitrumBridge.sol; drop unused SafeERC20/IERC20 imports; refresh classic-era comments (the source already targets post-NitrounsafeCreateRetryableTicket). Gas parameters are unchanged from the battle-tested 2022 values — overprovisioning is refunded to the recipient on L2.ArbitrumMocks.sol: addunsafeCreateRetryableTicketto the mockInboxwith the same call-tracking pattern ascreateRetryableTicket.DeployArbitrumRescueAdapter.s.sol: new Foundry deploy script mirroringDeployArbitrumAdapter.s.sol, recipient defaulted to the Risk Labs relayer.Arbitrum_RescueAdapter.t.sol: new tests covering the HubPool delegatecall flow (correct inbox args and L1 call value from HubPool balance), insufficient-ETH revert (both wrapped and direct), andrelayTokensrevert.Intended usage
Owner multisig executes atomically:
setCrossChainContracts(42161, rescueAdapter, spoke)→relaySpokePoolAdminFunction(42161, abi.encode(amount))→ restore production adapter. Theamountis funded from the aliased address's L2 balance; the 0.02 ETH L1 call value comes from the HubPool's existing ETH.Test
yarn test-evm-foundry -- --match-path "*Arbitrum*"→ 31 passed, 0 failed (4 new). Deploy script compiles. Prettier/solhint clean (remaining solhint warnings match pre-existing file style).No repo docs reference the rescue adapter; contract natspec updated in place, so no separate doc updates needed.
🤖 Generated with Claude Code