From e24a9bff7f1e6798e051db2218ef4c7e6aec71a5 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 10 Aug 2022 11:28:47 -0400 Subject: [PATCH 1/4] improve(arbitrum-adapter): Use outboundTransferCustomRefund when sending tokens to L2 --- contracts/chain-adapters/Arbitrum_Adapter.sol | 36 +++++++++++++------ 1 file changed, 25 insertions(+), 11 deletions(-) diff --git a/contracts/chain-adapters/Arbitrum_Adapter.sol b/contracts/chain-adapters/Arbitrum_Adapter.sol index c9ccbc39c..278a43d60 100644 --- a/contracts/chain-adapters/Arbitrum_Adapter.sol +++ b/contracts/chain-adapters/Arbitrum_Adapter.sol @@ -59,7 +59,7 @@ contract Arbitrum_Adapter is AdapterInterface { uint32 public immutable l2GasLimit = 2_000_000; // This address on L2 receives extra ETH that is left over after relaying a message via the inbox. - address public immutable l2RefundL2Address; + address public immutable l2RefundL2Address = 0x428AB2BA90Eba0a4Be7aF34C9Ac451ab061AC010; ArbitrumL1InboxLike public immutable l1Inbox; @@ -73,8 +73,6 @@ contract Arbitrum_Adapter is AdapterInterface { constructor(ArbitrumL1InboxLike _l1ArbitrumInbox, ArbitrumL1ERC20GatewayLike _l1ERC20GatewayRouter) { l1Inbox = _l1ArbitrumInbox; l1ERC20GatewayRouter = _l1ERC20GatewayRouter; - - l2RefundL2Address = msg.sender; } /** @@ -130,14 +128,30 @@ contract Arbitrum_Adapter is AdapterInterface { // Note: outboundTransfer() will ultimately create a retryable ticket and set this contract's address as the // refund address. This means that the excess ETH to pay for the L2 transaction will be sent to the aliased // contract address on L2 and lost. - l1ERC20GatewayRouter.outboundTransfer{ value: requiredL1CallValue }( - l1Token, - to, - amount, - l2GasLimit, - l2GasPrice, - data - ); + + // Note: Legacy routers don't have the outboundTransferCustomRefund method, so default to using + // outboundTransfer(). Legacy routers are used for: + // - DAI + if (l1Token == 0x6b175474e89094c44da98b954eedeac495271d0f) { + l1ERC20GatewayRouter.outboundTransfer{ value: requiredL1CallValue }( + l1Token, + to, + amount, + l2GasLimit, + l2GasPrice, + data + ); + } else { + l1ERC20GatewayRouter.outboundTransferCustomRefund{ value: requiredL1CallValue }( + l1Token, + l2RefundL2Address, + to, + amount, + l2GasLimit, + l2GasPrice, + data + ); + } emit TokensRelayed(l1Token, l2Token, amount, to); } From 99e850b8ceac9654b8c206f0031ccb9b389c81e8 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 10 Aug 2022 12:07:28 -0400 Subject: [PATCH 2/4] Fix tests --- contracts/chain-adapters/Arbitrum_Adapter.sol | 16 ++++++++++++++-- contracts/test/ArbitrumMocks.sol | 3 ++- test/chain-adapters/Arbitrum_Adapter.ts | 5 +++-- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/contracts/chain-adapters/Arbitrum_Adapter.sol b/contracts/chain-adapters/Arbitrum_Adapter.sol index 278a43d60..c8c125852 100644 --- a/contracts/chain-adapters/Arbitrum_Adapter.sol +++ b/contracts/chain-adapters/Arbitrum_Adapter.sol @@ -29,6 +29,16 @@ interface ArbitrumL1ERC20GatewayLike { bytes calldata _data ) external payable returns (bytes memory); + function outboundTransferCustomRefund( + address _token, + address _refundTo, + address _to, + uint256 _amount, + uint256 _maxGas, + uint256 _gasPriceBid, + bytes calldata _data + ) external payable returns (bytes memory); + function getGateway(address _token) external view returns (address); } @@ -59,7 +69,7 @@ contract Arbitrum_Adapter is AdapterInterface { uint32 public immutable l2GasLimit = 2_000_000; // This address on L2 receives extra ETH that is left over after relaying a message via the inbox. - address public immutable l2RefundL2Address = 0x428AB2BA90Eba0a4Be7aF34C9Ac451ab061AC010; + address public immutable l2RefundL2Address; ArbitrumL1InboxLike public immutable l1Inbox; @@ -73,6 +83,8 @@ contract Arbitrum_Adapter is AdapterInterface { constructor(ArbitrumL1InboxLike _l1ArbitrumInbox, ArbitrumL1ERC20GatewayLike _l1ERC20GatewayRouter) { l1Inbox = _l1ArbitrumInbox; l1ERC20GatewayRouter = _l1ERC20GatewayRouter; + + l2RefundL2Address = msg.sender; } /** @@ -132,7 +144,7 @@ contract Arbitrum_Adapter is AdapterInterface { // Note: Legacy routers don't have the outboundTransferCustomRefund method, so default to using // outboundTransfer(). Legacy routers are used for: // - DAI - if (l1Token == 0x6b175474e89094c44da98b954eedeac495271d0f) { + if (l1Token == 0x6B175474E89094C44Da98b954EedeAC495271d0F) { l1ERC20GatewayRouter.outboundTransfer{ value: requiredL1CallValue }( l1Token, to, diff --git a/contracts/test/ArbitrumMocks.sol b/contracts/test/ArbitrumMocks.sol index 230b9ee34..3ef75bf92 100644 --- a/contracts/test/ArbitrumMocks.sol +++ b/contracts/test/ArbitrumMocks.sol @@ -2,7 +2,8 @@ pragma solidity ^0.8.0; contract ArbitrumMockErc20GatewayRouter { - function outboundTransfer( + function outboundTransferCustomRefund( + address, address, address, uint256, diff --git a/test/chain-adapters/Arbitrum_Adapter.ts b/test/chain-adapters/Arbitrum_Adapter.ts index bc20e5b00..bdfc3f18d 100644 --- a/test/chain-adapters/Arbitrum_Adapter.ts +++ b/test/chain-adapters/Arbitrum_Adapter.ts @@ -91,14 +91,15 @@ describe("Arbitrum Chain Adapter", function () { ); // The correct functions should have been called on the arbitrum contracts. - expect(l1ERC20GatewayRouter.outboundTransfer).to.have.been.calledOnce; // One token transfer over the canonical bridge. + expect(l1ERC20GatewayRouter.outboundTransferCustomRefund).to.have.been.calledOnce; // One token transfer over the canonical bridge. // Adapter should have approved gateway to spend its ERC20. expect(await dai.allowance(hubPool.address, gatewayAddress)).to.equal(tokensSendToL2); const message = defaultAbiCoder.encode(["uint256", "bytes"], [consts.sampleL2MaxSubmissionCost, "0x"]); - expect(l1ERC20GatewayRouter.outboundTransfer).to.have.been.calledWith( + expect(l1ERC20GatewayRouter.outboundTransferCustomRefund).to.have.been.calledWith( dai.address, + owner.address, mockSpoke.address, tokensSendToL2, consts.sampleL2Gas, From 8c97c83fa66a69f3a1703836c57dbc1416950174 Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Wed, 10 Aug 2022 12:18:09 -0400 Subject: [PATCH 3/4] fix test --- contracts/chain-adapters/Arbitrum_Adapter.sol | 7 +++---- contracts/test/ArbitrumMocks.sol | 11 +++++++++++ 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/contracts/chain-adapters/Arbitrum_Adapter.sol b/contracts/chain-adapters/Arbitrum_Adapter.sol index c8c125852..0e8b7759a 100644 --- a/contracts/chain-adapters/Arbitrum_Adapter.sol +++ b/contracts/chain-adapters/Arbitrum_Adapter.sol @@ -137,14 +137,13 @@ contract Arbitrum_Adapter is AdapterInterface { // maxSubmissionCost to use when creating an L2 retryable ticket: https://github.com/OffchainLabs/arbitrum/blob/e98d14873dd77513b569771f47b5e05b72402c5e/packages/arb-bridge-peripherals/contracts/tokenbridge/ethereum/gateway/L1GatewayRouter.sol#L232 bytes memory data = abi.encode(l2MaxSubmissionCost, ""); - // Note: outboundTransfer() will ultimately create a retryable ticket and set this contract's address as the - // refund address. This means that the excess ETH to pay for the L2 transaction will be sent to the aliased - // contract address on L2 and lost. - // Note: Legacy routers don't have the outboundTransferCustomRefund method, so default to using // outboundTransfer(). Legacy routers are used for: // - DAI if (l1Token == 0x6B175474E89094C44Da98b954EedeAC495271d0F) { + // Note: outboundTransfer() will ultimately create a retryable ticket and set this contract's address as the + // refund address. This means that the excess ETH to pay for the L2 transaction will be sent to the aliased + // contract address on L2 and lost. l1ERC20GatewayRouter.outboundTransfer{ value: requiredL1CallValue }( l1Token, to, diff --git a/contracts/test/ArbitrumMocks.sol b/contracts/test/ArbitrumMocks.sol index 3ef75bf92..43a87bb8f 100644 --- a/contracts/test/ArbitrumMocks.sol +++ b/contracts/test/ArbitrumMocks.sol @@ -14,6 +14,17 @@ contract ArbitrumMockErc20GatewayRouter { return _data; } + function outboundTransfer( + address, + address, + uint256, + uint256, + uint256, + bytes calldata _data + ) external payable returns (bytes memory) { + return _data; + } + function getGateway(address) external view returns (address) { return address(this); } From 28bb5c92417f991512154f29e5790ffdb9ca813f Mon Sep 17 00:00:00 2001 From: nicholaspai Date: Mon, 15 Aug 2022 12:38:37 -0400 Subject: [PATCH 4/4] Update Arbitrum_Adapter.sol --- contracts/chain-adapters/Arbitrum_Adapter.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/chain-adapters/Arbitrum_Adapter.sol b/contracts/chain-adapters/Arbitrum_Adapter.sol index 0e8b7759a..51027aa9e 100644 --- a/contracts/chain-adapters/Arbitrum_Adapter.sol +++ b/contracts/chain-adapters/Arbitrum_Adapter.sol @@ -138,12 +138,13 @@ contract Arbitrum_Adapter is AdapterInterface { bytes memory data = abi.encode(l2MaxSubmissionCost, ""); // Note: Legacy routers don't have the outboundTransferCustomRefund method, so default to using - // outboundTransfer(). Legacy routers are used for: + // outboundTransfer(). Legacy routers are used for the following tokens that are currently enabled: // - DAI if (l1Token == 0x6B175474E89094C44Da98b954EedeAC495271d0F) { // Note: outboundTransfer() will ultimately create a retryable ticket and set this contract's address as the // refund address. This means that the excess ETH to pay for the L2 transaction will be sent to the aliased - // contract address on L2 and lost. + // contract address on L2, which we'd have to retrieve via a custom adapter + // (i.e. the Arbitrum_RescueAdapter). l1ERC20GatewayRouter.outboundTransfer{ value: requiredL1CallValue }( l1Token, to,