From a42f94a8dbe1c8a27f61f04444b56e41a6430aef Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Tue, 13 Dec 2022 16:08:34 -0600 Subject: [PATCH 01/12] fix(contracts): make Arbitrum relayer nonce public --- src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol index 76a8014..a95df78 100644 --- a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol +++ b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol @@ -37,7 +37,7 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { uint256 public immutable maxGasLimit; /// @notice Nonce to uniquely idenfity each batch of calls. - uint256 internal nonce; + uint256 public nonce; /** * @notice Hash of transactions that were relayed in `relayCalls`. From b9747b07344be5e7d3bbc391a3907191580ab82c Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Tue, 13 Dec 2022 16:09:33 -0600 Subject: [PATCH 02/12] fix(tests): update Arbitrum scripts to bridge --- script/bridge/BridgeToArbitrumGoerli.ts | 68 +++++++++++------------ script/fork/bridge/BridgeToArbitrum.ts | 73 ++++++++++++++----------- 2 files changed, 76 insertions(+), 65 deletions(-) diff --git a/script/bridge/BridgeToArbitrumGoerli.ts b/script/bridge/BridgeToArbitrumGoerli.ts index 96ee910..99531cd 100644 --- a/script/bridge/BridgeToArbitrumGoerli.ts +++ b/script/bridge/BridgeToArbitrumGoerli.ts @@ -1,13 +1,14 @@ import { L1TransactionReceipt, L1ToL2MessageGasEstimator } from '@arbitrum/sdk/'; -import { hexDataLength } from '@ethersproject/bytes'; +import { getBaseFee } from '@arbitrum/sdk/dist/lib/utils/lib'; import { BigNumber, providers } from 'ethers'; import hre from 'hardhat'; import { ARBITRUM_GOERLI_CHAIN_ID, GOERLI_CHAIN_ID } from '../../Constants'; import { getContractAddress } from '../../helpers/getContract'; import { action, error as errorLog, info, success } from '../../helpers/log'; -import { CrossChainRelayerArbitrum, ICrossChainRelayer } from '../../types'; -import CrossChainRelayerArbitrumArtifact from '../../out/CrossChainRelayerArbitrum.sol/CrossChainRelayerArbitrum.json'; +import { CrossChainRelayerArbitrum } from '../../types'; +import { CallLib } from '../../types/ICrossChainRelayer'; +import CrossChainRelayerArbitrumArtifact from '../../out/EthereumToArbitrumRelayer.sol/CrossChainRelayerArbitrum.json'; const main = async () => { action('Relay calls from Ethereum to Arbitrum...'); @@ -16,7 +17,7 @@ const main = async () => { ethers: { getContractAt, provider: l1Provider, - utils: { defaultAbiCoder, Interface }, + utils: { Interface }, }, getNamedAccounts, } = hre; @@ -52,33 +53,44 @@ const main = async () => { [greeting], ); - const calls: ICrossChainRelayer.CallStruct[] = [ + const calls: CallLib.CallStruct[] = [ { target: greeterAddress, data: callData, }, ]; + const nextNonce = (await crossChainRelayerArbitrum.nonce()).add(1); + const executeCallsData = new Interface([ 'function executeCalls(uint256,address,(address,bytes)[])', - ]).encodeFunctionData('executeCalls', [ - BigNumber.from(1), - deployer, - [[greeterAddress, callData]], - ]); + ]).encodeFunctionData('executeCalls', [nextNonce, deployer, [[greeterAddress, callData]]]); const l1ToL2MessageGasEstimate = new L1ToL2MessageGasEstimator(l2Provider); + const baseFee = await getBaseFee(l1Provider); + + /** + * The estimateAll method gives us the following values for sending an L1->L2 message + * (1) maxSubmissionCost: The maximum cost to be paid for submitting the transaction + * (2) gasLimit: The L2 gas limit + * (3) deposit: The total amount to deposit on L1 to cover L2 gas and L2 call value + */ + const { deposit, gasLimit, maxSubmissionCost } = await l1ToL2MessageGasEstimate.estimateAll( + { + from: crossChainRelayerArbitrumAddress, + to: crossChainExecutorAddress, + l2CallValue: BigNumber.from(0), + excessFeeRefundAddress: deployer, + callValueRefundAddress: deployer, + data: executeCallsData, + }, + baseFee, + l1Provider, + ); - const maxGas = await l1ToL2MessageGasEstimate.estimateRetryableTicketGasLimit({ - from: crossChainRelayerArbitrumAddress, - to: crossChainExecutorAddress, - l2CallValue: BigNumber.from(0), - excessFeeRefundAddress: deployer, - callValueRefundAddress: deployer, - data: executeCallsData, - }); + info(`Current retryable base submission price is: ${maxSubmissionCost.toString()}`); - const relayCallsTransaction = await crossChainRelayerArbitrum.relayCalls(calls, maxGas); + const relayCallsTransaction = await crossChainRelayerArbitrum.relayCalls(calls, gasLimit); const relayCallsTransactionReceipt = await relayCallsTransaction.wait(); const relayedCallsEventInterface = new Interface([ @@ -96,33 +108,21 @@ const main = async () => { action('Process calls from Ethereum to Arbitrum...'); - const greetingBytes = defaultAbiCoder.encode(['string'], [greeting]); - const greetingBytesLength = hexDataLength(greetingBytes) + 4; // 4 bytes func identifier - - const submissionPriceWei = await l1ToL2MessageGasEstimate.estimateSubmissionFee( - l1Provider, - await l1Provider.getGasPrice(), - greetingBytesLength, - ); - - info(`Current retryable base submission price: ${submissionPriceWei.toString()}`); - - const maxSubmissionCost = submissionPriceWei.mul(5); const gasPriceBid = await l2Provider.getGasPrice(); info(`L2 gas price: ${gasPriceBid.toString()}`); - const callValue = maxSubmissionCost.add(gasPriceBid.mul(maxGas)); + info(`Sending greeting to L2 with ${deposit.toString()} callValue for L2 fees:`); const processCallsTransaction = await crossChainRelayerArbitrum.processCalls( relayCallsNonce, calls, deployer, - maxGas, + gasLimit, maxSubmissionCost, gasPriceBid, { - value: callValue, + value: deposit, }, ); diff --git a/script/fork/bridge/BridgeToArbitrum.ts b/script/fork/bridge/BridgeToArbitrum.ts index 318feeb..e55a618 100644 --- a/script/fork/bridge/BridgeToArbitrum.ts +++ b/script/fork/bridge/BridgeToArbitrum.ts @@ -1,7 +1,7 @@ import { task } from 'hardhat/config'; import { HardhatRuntimeEnvironment } from 'hardhat/types'; import { L1TransactionReceipt, L1ToL2MessageGasEstimator } from '@arbitrum/sdk/'; -import { hexDataLength } from '@ethersproject/bytes'; +import { getBaseFee } from '@arbitrum/sdk/dist/lib/utils/lib'; import { SignerWithAddress } from '@nomiclabs/hardhat-ethers/signers'; import { BigNumber, providers } from 'ethers'; import kill from 'kill-port'; @@ -68,58 +68,69 @@ export const relayCalls = task( }, ]; + const nextNonce = (await crossChainRelayerArbitrum.nonce()).add(1); + const executeCallsData = new Interface([ 'function executeCalls(uint256,address,(address,bytes)[])', - ]).encodeFunctionData('executeCalls', [ - BigNumber.from(1), - deployer, - [[greeterAddress, callData]], - ]); + ]).encodeFunctionData('executeCalls', [nextNonce, deployer, [[greeterAddress, callData]]]); const l1ToL2MessageGasEstimate = new L1ToL2MessageGasEstimator(l2Provider); + const baseFee = await getBaseFee(l1Provider); + + /** + * The estimateAll method gives us the following values for sending an L1->L2 message + * (1) maxSubmissionCost: The maximum cost to be paid for submitting the transaction + * (2) gasLimit: The L2 gas limit + * (3) deposit: The total amount to deposit on L1 to cover L2 gas and L2 call value + */ + const { deposit, gasLimit, maxSubmissionCost } = await l1ToL2MessageGasEstimate.estimateAll( + { + from: crossChainRelayerArbitrum.address, + to: crossChainExecutorAddress, + l2CallValue: BigNumber.from(0), + excessFeeRefundAddress: deployer, + callValueRefundAddress: deployer, + data: executeCallsData, + }, + baseFee, + l1Provider, + ); - const maxGas = await l1ToL2MessageGasEstimate.estimateRetryableTicketGasLimit({ - from: crossChainRelayerArbitrum.address, - to: crossChainExecutorAddress, - l2CallValue: BigNumber.from(0), - excessFeeRefundAddress: deployer, - callValueRefundAddress: deployer, - data: executeCallsData, - }); + info(`Current retryable base submission price is: ${maxSubmissionCost.toString()}`); - await crossChainRelayerArbitrum.relayCalls(calls, maxGas); + const relayCallsTransaction = await crossChainRelayerArbitrum.relayCalls(calls, gasLimit); + const relayCallsTransactionReceipt = await relayCallsTransaction.wait(); - success('Successfully relayed calls from Ethereum to Arbitrum!'); + const relayedCallsEventInterface = new Interface([ + 'event RelayedCalls(uint256 indexed nonce,address indexed sender, (address target,bytes data)[], uint256 gasLimit)', + ]); - action('Process calls from Ethereum to Arbitrum...'); + const relayedCallsEventLogs = relayedCallsEventInterface.parseLog( + relayCallsTransactionReceipt.logs[0], + ); - const greetingBytes = defaultAbiCoder.encode(['string'], [greeting]); - const greetingBytesLength = hexDataLength(greetingBytes) + 4; // 4 bytes func identifier + const [relayCallsNonce] = relayedCallsEventLogs.args; - const submissionPriceWei = await l1ToL2MessageGasEstimate.estimateSubmissionFee( - l1Provider, - await l1Provider.getGasPrice(), - greetingBytesLength, - ); + success('Successfully relayed calls from Ethereum to Arbitrum!'); + info(`Nonce: ${relayCallsNonce}`); - info(`Current retryable base submission price: ${submissionPriceWei.toString()}`); + action('Process calls from Ethereum to Arbitrum...'); - const maxSubmissionCost = submissionPriceWei.mul(5); const gasPriceBid = await l2Provider.getGasPrice(); info(`L2 gas price: ${gasPriceBid.toString()}`); - const callValue = maxSubmissionCost.add(gasPriceBid.mul(maxGas)); + info(`Sending greeting to L2 with ${deposit.toString()} callValue for L2 fees:`); const processCallsTransaction = await crossChainRelayerArbitrum.processCalls( - BigNumber.from(1), + relayCallsNonce, calls, deployer, - maxGas, + gasLimit, maxSubmissionCost, gasPriceBid, { - value: callValue, + value: deposit, }, ); @@ -133,7 +144,7 @@ export const relayCalls = task( processCallsTransactionReceipt.logs[2], ); - const [sender, nonce, ticketId] = processedCallsEventLogs.args; + const [nonce, sender, ticketId] = processedCallsEventLogs.args; const receipt = await l1Provider.getTransactionReceipt(processCallsTransaction.hash); const l1Receipt = new L1TransactionReceipt(receipt); From e7239868ca7e39f4e3feda8d7891e80fd51e3f11 Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Tue, 13 Dec 2022 16:10:48 -0600 Subject: [PATCH 03/12] chore(README): add new Arbitrum Goerli deployments --- README.md | 12 ++++++------ lib/contracts | 2 +- lib/forge-std | 2 +- lib/nitro | 2 +- lib/optimism | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) diff --git a/README.md b/README.md index 0f89600..c44d2c3 100644 --- a/README.md +++ b/README.md @@ -205,9 +205,9 @@ function _nonce() internal pure returns (uint256 _callDataNonce); | Network | Contract | Address | | --------------- | -------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------- | -| Ethereum Goerli | [EthereumToArbitrumRelayer.sol](./src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol) | [0x7460fDb4db23C7287c67122A31661b753081e80a](https://goerli.etherscan.io/address/0x7460fDb4db23C7287c67122A31661b753081e80a) | -| Arbitrum Goerli | [EthereumToArbitrumExecutor](./src/ethereum-arbitrum/EthereumToArbitrumExecutor.sol) | [0x18771cC0bbcA24d3B28C040669DCc7b5Ffba30FB](https://goerli.arbiscan.io/address/0x18771cC0bbcA24d3B28C040669DCc7b5Ffba30FB) | -| Arbitrum Goerli | [Greeter](./test/contracts/Greeter.sol) | [0xa1d913940B8dbb7bDB1F68D8E9C54484D575FefC](https://goerli.arbiscan.io/address/0xa1d913940B8dbb7bDB1F68D8E9C54484D575FefC) | +| Ethereum Goerli | [EthereumToArbitrumRelayer.sol](./src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol) | [0x961f05163dBB383EF39323D04e04aE5b7cc5A7b2](https://goerli.etherscan.io/address/0x961f05163dBB383EF39323D04e04aE5b7cc5A7b2) | +| Arbitrum Goerli | [EthereumToArbitrumExecutor](./src/ethereum-arbitrum/EthereumToArbitrumExecutor.sol) | [0x9c53fb1D0AE3b7EDd6da970Fa3dC70e8d2092723](https://goerli.arbiscan.io/address/0x9c53fb1D0AE3b7EDd6da970Fa3dC70e8d2092723) | +| Arbitrum Goerli | [Greeter](./test/contracts/Greeter.sol) | [0xcCD175Fe1f7389A06C40765eaf33180295216460](https://goerli.arbiscan.io/address/0xcCD175Fe1f7389A06C40765eaf33180295216460) | ### Ethereum Goerli -> Optimism Goerli @@ -342,9 +342,9 @@ It takes about 15 minutes for the message to be bridged to Arbitrum Goerli. | Network | Call | Transaction hash | | --------------- | ------------ | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| Ethereum Goerli | relayCalls | [0x102ae324d996fdeadf666d1f6f00db00c73be632c8b115cf6f2ab901fd1ca7f7](https://goerli.etherscan.io/tx/0x102ae324d996fdeadf666d1f6f00db00c73be632c8b115cf6f2ab901fd1ca7f7) | -| Ethereum Goerli | processCalls | [0x34876c7553b4618170e1c95aaa30daf79e4ddb6436cb7e317215ea0e3593dbcc](https://goerli.etherscan.io/tx/0x34876c7553b4618170e1c95aaa30daf79e4ddb6436cb7e317215ea0e3593dbcc) | -| Arbitrum Goerli | executeCalls | [0xc74b9570949b941ec1f1a020c1d988614448947e6f2de691e2031304bb76bd0c](https://goerli.arbiscan.io/tx/0xc74b9570949b941ec1f1a020c1d988614448947e6f2de691e2031304bb76bd0c) | +| Ethereum Goerli | relayCalls | [0x34c62a91cf035c19393b90027b26bd4883e68e079f0814854a396b183ae03c28](https://goerli.etherscan.io/tx/0x34c62a91cf035c19393b90027b26bd4883e68e079f0814854a396b183ae03c28) | +| Ethereum Goerli | processCalls | [0xf028e7415e2bd88c63419b9ba519913cd063651d4f6a5c5f3b4a2d3d7c6ccb04](https://goerli.etherscan.io/tx/0xf028e7415e2bd88c63419b9ba519913cd063651d4f6a5c5f3b4a2d3d7c6ccb04) | +| Arbitrum Goerli | executeCalls | [0xcb418fb130f95035ee1cb17aa0759557b12e083bb00d9893663497fd7be8c197](https://goerli.arbiscan.io/tx/0xcb418fb130f95035ee1cb17aa0759557b12e083bb00d9893663497fd7be8c197) | #### Ethereum Goerli to Optimism Goerli diff --git a/lib/contracts b/lib/contracts index dc41712..eb370b6 160000 --- a/lib/contracts +++ b/lib/contracts @@ -1 +1 @@ -Subproject commit dc41712b802a65a0cc2d00ec0833da741dd5ba7c +Subproject commit eb370b655923162c442077246855affe56809263 diff --git a/lib/forge-std b/lib/forge-std index 17656a2..cd7d533 160000 --- a/lib/forge-std +++ b/lib/forge-std @@ -1 +1 @@ -Subproject commit 17656a2fa5453f495d8c1302a0cedded912457eb +Subproject commit cd7d533f9a0ee0ec02ad81e0a8f262bc4203c653 diff --git a/lib/nitro b/lib/nitro index 1f32bec..48fda6c 160000 --- a/lib/nitro +++ b/lib/nitro @@ -1 +1 @@ -Subproject commit 1f32bec6b9b228bb2fab4bfa02867716f65d0c5c +Subproject commit 48fda6c75955c73a95b8a6ac394768d8dacefa91 diff --git a/lib/optimism b/lib/optimism index f7dbad0..764a927 160000 --- a/lib/optimism +++ b/lib/optimism @@ -1 +1 @@ -Subproject commit f7dbad0287fd97488e62a3bee53fb3353a6c56b9 +Subproject commit 764a9271eb68f70bf4c791662261ad0ba4e8e73f From dd715ae824b9d57e16cf4ced2aff2fd31de62ba3 Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Tue, 13 Dec 2022 17:27:30 -0600 Subject: [PATCH 04/12] fix(contracts): check executor address in relayCalls --- .../EthereumToArbitrumRelayer.sol | 5 ++++- .../EthereumToOptimismRelayer.sol | 5 ++++- .../EthereumToPolygonRelayer.sol | 2 ++ test/fork/EthereumToOptimismFork.t.sol | 17 +++++++++++++++++ test/fork/EthereumToPolygonFork.t.sol | 18 ++++++++++++++++++ .../EthereumToArbitrumRelayer.t.sol | 10 +++++++++- 6 files changed, 54 insertions(+), 3 deletions(-) diff --git a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol index a95df78..b8303e9 100644 --- a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol +++ b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol @@ -107,6 +107,9 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { ) external payable returns (uint256) { require(relayed[_getTxHash(_nonce, _calls, _sender, _gasLimit)], "Relayer/calls-not-relayed"); + address _executorAddress = address(executor); + require(_executorAddress != address(0), "Relayer/executor-not-set"); + bytes memory _data = abi.encodeWithSignature( "executeCalls(uint256,address,(address,bytes)[])", _nonce, @@ -115,7 +118,7 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { ); uint256 _ticketID = inbox.createRetryableTicket{ value: msg.value }( - address(executor), + _executorAddress, 0, _maxSubmissionCost, msg.sender, diff --git a/src/ethereum-optimism/EthereumToOptimismRelayer.sol b/src/ethereum-optimism/EthereumToOptimismRelayer.sol index 010e9fa..edd5623 100644 --- a/src/ethereum-optimism/EthereumToOptimismRelayer.sol +++ b/src/ethereum-optimism/EthereumToOptimismRelayer.sol @@ -56,12 +56,15 @@ contract CrossChainRelayerOptimism is ICrossChainRelayer { revert GasLimitTooHigh(_gasLimit, _maxGasLimit); } + address _executorAddress = address(executor); + require(_executorAddress != address(0), "Relayer/executor-not-set"); + nonce++; uint256 _nonce = nonce; crossDomainMessenger.sendMessage( - address(executor), + _executorAddress, abi.encodeWithSignature( "executeCalls(uint256,address,(address,bytes)[])", _nonce, diff --git a/src/ethereum-polygon/EthereumToPolygonRelayer.sol b/src/ethereum-polygon/EthereumToPolygonRelayer.sol index 4878ca8..4a25bbb 100644 --- a/src/ethereum-polygon/EthereumToPolygonRelayer.sol +++ b/src/ethereum-polygon/EthereumToPolygonRelayer.sol @@ -52,6 +52,8 @@ contract CrossChainRelayerPolygon is ICrossChainRelayer, FxBaseRootTunnel { revert GasLimitTooHigh(_gasLimit, _maxGasLimit); } + require(address(fxChildTunnel) != address(0), "Relayer/fxChildTunnel-not-set"); + nonce++; uint256 _nonce = nonce; diff --git a/test/fork/EthereumToOptimismFork.t.sol b/test/fork/EthereumToOptimismFork.t.sol index be84235..d75dd34 100644 --- a/test/fork/EthereumToOptimismFork.t.sol +++ b/test/fork/EthereumToOptimismFork.t.sol @@ -152,6 +152,23 @@ contract EthereumToOptimismForkTest is Test { assertEq(_nonce, nonce); } + function testExecutorNotSet() public { + deployAll(); + + vm.selectFork(mainnetFork); + + CallLib.Call[] memory _calls = new CallLib.Call[](1); + + _calls[0] = CallLib.Call({ + target: address(greeter), + data: abi.encodeWithSignature("setGreeting(string)", l1Greeting) + }); + + vm.expectRevert(bytes("Relayer/executor-not-set")); + + relayer.relayCalls(_calls, 200000); + } + /* ============ executeCalls ============ */ function testExecuteCalls() public { diff --git a/test/fork/EthereumToPolygonFork.t.sol b/test/fork/EthereumToPolygonFork.t.sol index 01913e6..15e6e9e 100644 --- a/test/fork/EthereumToPolygonFork.t.sol +++ b/test/fork/EthereumToPolygonFork.t.sol @@ -132,6 +132,7 @@ contract EthereumToPolygonForkTest is Test { function testRelayCalls() public { deployAll(); + setAll(); vm.selectFork(mainnetFork); @@ -151,6 +152,23 @@ contract EthereumToPolygonForkTest is Test { assertEq(_nonce, nonce); } + function testFxChildTunnelNotSet() public { + deployAll(); + + vm.selectFork(mainnetFork); + + CallLib.Call[] memory _calls = new CallLib.Call[](1); + + _calls[0] = CallLib.Call({ + target: address(greeter), + data: abi.encodeWithSignature("setGreeting(string)", l1Greeting) + }); + + vm.expectRevert(bytes("Relayer/fxChildTunnel-not-set")); + + relayer.relayCalls(_calls, 200000); + } + function testExecuteCalls() public { deployAll(); setAll(); diff --git a/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol b/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol index 9c0cb79..0b8d5cf 100644 --- a/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol +++ b/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol @@ -134,13 +134,21 @@ contract CrossChainRelayerArbitrumUnitTest is Test { assertEq(_ticketId, _randomNumber); } - function testProcessCallsFail() public { + function testCallsNotRelayed() public { setExecutor(); vm.expectRevert(bytes("Relayer/calls-not-relayed")); relayer.processCalls(nonce, calls, address(this), gasLimit, maxSubmissionCost, gasPriceBid); } + function testExecutorNotSet() public { + uint256 _nonce = relayer.relayCalls(calls, gasLimit); + + vm.expectRevert(bytes("Relayer/executor-not-set")); + + relayer.processCalls(_nonce, calls, address(this), gasLimit, maxSubmissionCost, gasPriceBid); + } + /* ============ Getters ============ */ function testGetTxHash() public { bytes32 txHash = relayer.getTxHash(nonce, calls, sender, gasLimit); From dc54e0de14d3c7e4ecdfb91b9d765c93052c1f81 Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Tue, 13 Dec 2022 18:01:24 -0600 Subject: [PATCH 05/12] fix(contracts): remove maxGasLimit --- Constants.ts | 7 ----- script/deploy/DeployToArbitrumGoerli.s.sol | 2 +- script/deploy/DeployToMumbai.s.sol | 2 +- script/deploy/DeployToOptimismGoerli.s.sol | 2 +- script/fork/deploy/DeployToArbitrum.ts | 9 ++---- .../EthereumToArbitrumRelayer.sol | 15 +-------- .../EthereumToOptimismRelayer.sol | 15 +-------- .../EthereumToPolygonRelayer.sol | 21 ++----------- test/fork/EthereumToOptimismFork.t.sol | 26 +--------------- test/fork/EthereumToPolygonFork.t.sol | 23 +------------- .../EthereumToArbitrumRelayer.t.sol | 31 ++----------------- 11 files changed, 15 insertions(+), 138 deletions(-) diff --git a/Constants.ts b/Constants.ts index c4f7ad8..50f0b9d 100644 --- a/Constants.ts +++ b/Constants.ts @@ -11,13 +11,6 @@ export const USDC_TOKEN_DECIMALS = 6; export const DELAYED_INBOX = '0x4Dbd4fc535Ac27206064B68FfCf827b0A60BAB3f'; -/** - * Retrieved by calling the `getGasAccountingParams` function on the `ArbGasInfo` contract - * Address: https://arbiscan.io/address/0x000000000000000000000000000000000000006C - * Contract: https://github.com/OffchainLabs/nitro/blob/master/contracts/src/precompiles/ArbGasInfo.sol#L80 - */ -export const ARBITRUM_MAX_TX_GAS_LIMIT = 32000000; - export const MAINNET_CHAIN_ID = 1; export const ARBITRUM_CHAIN_ID = 42161; export const OPTIMISM_CHAIN_ID = 10; diff --git a/script/deploy/DeployToArbitrumGoerli.s.sol b/script/deploy/DeployToArbitrumGoerli.s.sol index e5293ac..e9f01e2 100644 --- a/script/deploy/DeployToArbitrumGoerli.s.sol +++ b/script/deploy/DeployToArbitrumGoerli.s.sol @@ -16,7 +16,7 @@ contract DeployCrossChainRelayerToGoerli is Script { function run() public { vm.broadcast(); - new CrossChainRelayerArbitrum(IInbox(delayedInbox), 32000000); + new CrossChainRelayerArbitrum(IInbox(delayedInbox)); vm.stopBroadcast(); } diff --git a/script/deploy/DeployToMumbai.s.sol b/script/deploy/DeployToMumbai.s.sol index e1d1927..d360928 100644 --- a/script/deploy/DeployToMumbai.s.sol +++ b/script/deploy/DeployToMumbai.s.sol @@ -17,7 +17,7 @@ contract DeployCrossChainRelayerToGoerli is Script { function run() public { vm.broadcast(); - new CrossChainRelayerPolygon(checkpointManager, fxRoot, 30000000); + new CrossChainRelayerPolygon(checkpointManager, fxRoot); vm.stopBroadcast(); } diff --git a/script/deploy/DeployToOptimismGoerli.s.sol b/script/deploy/DeployToOptimismGoerli.s.sol index 50589f4..ed2c82a 100644 --- a/script/deploy/DeployToOptimismGoerli.s.sol +++ b/script/deploy/DeployToOptimismGoerli.s.sol @@ -16,7 +16,7 @@ contract DeployCrossChainRelayerToGoerli is Script { function run() public { vm.broadcast(); - new CrossChainRelayerOptimism(ICrossDomainMessenger(proxyOVML1CrossDomainMessenger), 1920000); + new CrossChainRelayerOptimism(ICrossDomainMessenger(proxyOVML1CrossDomainMessenger)); vm.stopBroadcast(); } diff --git a/script/fork/deploy/DeployToArbitrum.ts b/script/fork/deploy/DeployToArbitrum.ts index 1d34a66..db0f789 100644 --- a/script/fork/deploy/DeployToArbitrum.ts +++ b/script/fork/deploy/DeployToArbitrum.ts @@ -1,12 +1,7 @@ import { task } from 'hardhat/config'; import { HardhatRuntimeEnvironment } from 'hardhat/types'; -import { - ARBITRUM_CHAIN_ID, - MAINNET_CHAIN_ID, - DELAYED_INBOX, - ARBITRUM_MAX_TX_GAS_LIMIT, -} from '../../../Constants'; +import { ARBITRUM_CHAIN_ID, MAINNET_CHAIN_ID, DELAYED_INBOX } from '../../../Constants'; import { getContractAddress } from '../../../helpers/getContract'; import { action, info, success } from '../../../helpers/log'; import { CrossChainRelayerArbitrum, CrossChainExecutorArbitrum } from '../../../types'; @@ -28,7 +23,7 @@ export const deployRelayer = task( const { address } = await deploy('CrossChainRelayerArbitrum', { from: deployer, - args: [DELAYED_INBOX, ARBITRUM_MAX_TX_GAS_LIMIT], + args: [DELAYED_INBOX], }); success(`Arbitrum Relayer deployed on Ethereum at address: ${address}`); diff --git a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol index b8303e9..f3bda6d 100644 --- a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol +++ b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol @@ -33,9 +33,6 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { /// @notice Address of the executor contract on the Arbitrum chain. ICrossChainExecutor public executor; - /// @notice Gas limit provided for free on Arbitrum. - uint256 public immutable maxGasLimit; - /// @notice Nonce to uniquely idenfity each batch of calls. uint256 public nonce; @@ -51,14 +48,10 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { /** * @notice CrossChainRelayer constructor. * @param _inbox Address of the Arbitrum inbox on Ethereum - * @param _maxGasLimit Gas limit provided for free on Arbitrum */ - constructor(IInbox _inbox, uint256 _maxGasLimit) { + constructor(IInbox _inbox) { require(address(_inbox) != address(0), "Relayer/inbox-not-zero-address"); - require(_maxGasLimit > 0, "Relayer/max-gas-limit-gt-zero"); - inbox = _inbox; - maxGasLimit = _maxGasLimit; } /* ============ External Functions ============ */ @@ -68,12 +61,6 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { external returns (uint256) { - uint256 _maxGasLimit = maxGasLimit; - - if (_gasLimit > _maxGasLimit) { - revert GasLimitTooHigh(_gasLimit, _maxGasLimit); - } - nonce++; uint256 _nonce = nonce; diff --git a/src/ethereum-optimism/EthereumToOptimismRelayer.sol b/src/ethereum-optimism/EthereumToOptimismRelayer.sol index edd5623..0ddd4b8 100644 --- a/src/ethereum-optimism/EthereumToOptimismRelayer.sol +++ b/src/ethereum-optimism/EthereumToOptimismRelayer.sol @@ -22,9 +22,6 @@ contract CrossChainRelayerOptimism is ICrossChainRelayer { /// @notice Address of the executor contract on the Optimism chain. ICrossChainExecutor public executor; - /// @notice Gas limit provided for free on Optimism. - uint256 public immutable maxGasLimit; - /// @notice Nonce to uniquely idenfity each batch of calls. uint256 internal nonce; @@ -33,14 +30,10 @@ contract CrossChainRelayerOptimism is ICrossChainRelayer { /** * @notice CrossChainRelayerOptimism constructor. * @param _crossDomainMessenger Address of the Optimism cross domain messenger - * @param _maxGasLimit Gas limit provided for free on Optimism */ - constructor(ICrossDomainMessenger _crossDomainMessenger, uint256 _maxGasLimit) { + constructor(ICrossDomainMessenger _crossDomainMessenger) { require(address(_crossDomainMessenger) != address(0), "Relayer/CDM-not-zero-address"); - require(_maxGasLimit > 0, "Relayer/max-gas-limit-gt-zero"); - crossDomainMessenger = _crossDomainMessenger; - maxGasLimit = _maxGasLimit; } /* ============ External Functions ============ */ @@ -50,12 +43,6 @@ contract CrossChainRelayerOptimism is ICrossChainRelayer { external returns (uint256) { - uint256 _maxGasLimit = maxGasLimit; - - if (_gasLimit > _maxGasLimit) { - revert GasLimitTooHigh(_gasLimit, _maxGasLimit); - } - address _executorAddress = address(executor); require(_executorAddress != address(0), "Relayer/executor-not-set"); diff --git a/src/ethereum-polygon/EthereumToPolygonRelayer.sol b/src/ethereum-polygon/EthereumToPolygonRelayer.sol index 4a25bbb..b97ebd8 100644 --- a/src/ethereum-polygon/EthereumToPolygonRelayer.sol +++ b/src/ethereum-polygon/EthereumToPolygonRelayer.sol @@ -16,9 +16,6 @@ import "../libraries/CallLib.sol"; contract CrossChainRelayerPolygon is ICrossChainRelayer, FxBaseRootTunnel { /* ============ Variables ============ */ - /// @notice Gas limit provided for free on Polygon. - uint256 public immutable maxGasLimit; - /// @notice Nonce to uniquely idenfity each batch of calls. uint256 internal nonce; @@ -28,16 +25,10 @@ contract CrossChainRelayerPolygon is ICrossChainRelayer, FxBaseRootTunnel { * @notice CrossChainRelayerPolygon constructor. * @param _checkpointManager Address of the root chain manager contract on Ethereum * @param _fxRoot Address of the state sender contract on Ethereum - * @param _maxGasLimit Gas limit provided for free on Polygon */ - constructor( - address _checkpointManager, - address _fxRoot, - uint256 _maxGasLimit - ) FxBaseRootTunnel(_checkpointManager, _fxRoot) { - require(_maxGasLimit > 0, "Relayer/max-gas-limit-gt-zero"); - maxGasLimit = _maxGasLimit; - } + constructor(address _checkpointManager, address _fxRoot) + FxBaseRootTunnel(_checkpointManager, _fxRoot) + {} /* ============ External Functions ============ */ @@ -46,12 +37,6 @@ contract CrossChainRelayerPolygon is ICrossChainRelayer, FxBaseRootTunnel { external returns (uint256) { - uint256 _maxGasLimit = maxGasLimit; - - if (_gasLimit > _maxGasLimit) { - revert GasLimitTooHigh(_gasLimit, _maxGasLimit); - } - require(address(fxChildTunnel) != address(0), "Relayer/fxChildTunnel-not-set"); nonce++; diff --git a/test/fork/EthereumToOptimismFork.t.sol b/test/fork/EthereumToOptimismFork.t.sol index d75dd34..dd7b19a 100644 --- a/test/fork/EthereumToOptimismFork.t.sol +++ b/test/fork/EthereumToOptimismFork.t.sol @@ -31,7 +31,6 @@ contract EthereumToOptimismForkTest is Test { string public l1Greeting = "Hello from L1"; string public l2Greeting = "Hello from L2"; - uint256 public maxGasLimit = 1920000; uint256 public nonce = 1; /* ============ Events to test ============ */ @@ -57,10 +56,7 @@ contract EthereumToOptimismForkTest is Test { function deployRelayer() public { vm.selectFork(mainnetFork); - relayer = new CrossChainRelayerOptimism( - ICrossDomainMessenger(proxyOVML1CrossDomainMessenger), - maxGasLimit - ); + relayer = new CrossChainRelayerOptimism(ICrossDomainMessenger(proxyOVML1CrossDomainMessenger)); vm.makePersistent(address(relayer)); } @@ -211,26 +207,6 @@ contract EthereumToOptimismForkTest is Test { assertEq(greeter.greet(), l1Greeting); } - function testGasLimitTooHigh() public { - deployAll(); - setAll(); - - vm.selectFork(mainnetFork); - - CallLib.Call[] memory _calls = new CallLib.Call[](1); - - _calls[0] = CallLib.Call({ - target: address(greeter), - data: abi.encodeWithSignature("setGreeting(string)", l1Greeting) - }); - - vm.expectRevert( - abi.encodeWithSelector(ICrossChainRelayer.GasLimitTooHigh.selector, 2000000, maxGasLimit) - ); - - relayer.relayCalls(_calls, 2000000); - } - function testIsAuthorized() public { deployAll(); setAll(); diff --git a/test/fork/EthereumToPolygonFork.t.sol b/test/fork/EthereumToPolygonFork.t.sol index 15e6e9e..60a85fd 100644 --- a/test/fork/EthereumToPolygonFork.t.sol +++ b/test/fork/EthereumToPolygonFork.t.sol @@ -28,7 +28,6 @@ contract EthereumToPolygonForkTest is Test { string public l1Greeting = "Hello from L1"; string public l2Greeting = "Hello from L2"; - uint256 public maxGasLimit = 1920000; uint256 public nonce = 1; /* ============ Events to test ============ */ @@ -58,7 +57,7 @@ contract EthereumToPolygonForkTest is Test { function deployRelayer() public { vm.selectFork(mainnetFork); - relayer = new CrossChainRelayerPolygon(checkpointManager, fxRoot, maxGasLimit); + relayer = new CrossChainRelayerPolygon(checkpointManager, fxRoot); vm.makePersistent(address(relayer)); } @@ -109,7 +108,6 @@ contract EthereumToPolygonForkTest is Test { assertEq(address(relayer.checkpointManager()), checkpointManager); assertEq(address(relayer.fxRoot()), fxRoot); - assertEq(relayer.maxGasLimit(), maxGasLimit); assertEq(relayer.fxChildTunnel(), address(executor)); } @@ -197,25 +195,6 @@ contract EthereumToPolygonForkTest is Test { assertEq(greeter.greet(), l1Greeting); } - function testGasLimitTooHigh() public { - deployAll(); - - vm.selectFork(mainnetFork); - - CallLib.Call[] memory _calls = new CallLib.Call[](1); - - _calls[0] = CallLib.Call({ - target: address(greeter), - data: abi.encodeWithSignature("setGreeting(string)", l1Greeting) - }); - - vm.expectRevert( - abi.encodeWithSelector(ICrossChainRelayer.GasLimitTooHigh.selector, 2000000, maxGasLimit) - ); - - relayer.relayCalls(_calls, 2000000); - } - function testCallFailure() public { deployAll(); setAll(); diff --git a/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol b/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol index 0b8d5cf..c3d22f0 100644 --- a/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol +++ b/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol @@ -23,9 +23,8 @@ contract CrossChainRelayerArbitrumUnitTest is Test { address public sender = 0xa3a935315931A09A4e9B8A865517Cc18923497Ad; address public attacker = 0xdBdDa361Db11Adf8A51dab8a511a8ee89128E89A; - uint256 public maxGasLimit = 32000000; uint256 public gasLimit = 1000000; - uint256 public gasLimitGTMax = maxGasLimit + 1; + uint256 public maxSubmissionCost = 1 ether; uint256 public gasPriceBid = 500; uint256 public nonce = 1; @@ -47,7 +46,7 @@ contract CrossChainRelayerArbitrumUnitTest is Test { /* ============ Setup ============ */ function setUp() public { - relayer = new CrossChainRelayerArbitrum(inbox, maxGasLimit); + relayer = new CrossChainRelayerArbitrum(inbox); calls.push( CallLib.Call({ @@ -64,17 +63,11 @@ contract CrossChainRelayerArbitrumUnitTest is Test { /* ============ Constructor ============ */ function testConstructor() public { assertEq(address(relayer.inbox()), address(inbox)); - assertEq(relayer.maxGasLimit(), maxGasLimit); } function testConstructorInboxFail() public { vm.expectRevert(bytes("Relayer/inbox-not-zero-address")); - relayer = new CrossChainRelayerArbitrum(IInbox(address(0)), maxGasLimit); - } - - function testConstructorMaxGasLimitFail() public { - vm.expectRevert(bytes("Relayer/max-gas-limit-gt-zero")); - relayer = new CrossChainRelayerArbitrum(inbox, 0); + relayer = new CrossChainRelayerArbitrum(IInbox(address(0))); } /* ============ relayCalls ============ */ @@ -91,24 +84,6 @@ contract CrossChainRelayerArbitrumUnitTest is Test { assertTrue(relayer.relayed(txHash)); } - function testRelayCallsFail() public { - setExecutor(); - - vm.expectRevert( - abi.encodeWithSelector( - ICrossChainRelayer.GasLimitTooHigh.selector, - gasLimitGTMax, - maxGasLimit - ) - ); - - relayer.relayCalls(calls, gasLimitGTMax); - - bytes32 txHash = relayer.getTxHash(nonce, calls, address(this), gasLimit); - - assertTrue(!relayer.relayed(txHash)); - } - /* ============ processCalls ============ */ function testProcessCalls() public { From b4cc02e8245b660ed3528e84da128ce5c82f782b Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Tue, 13 Dec 2022 20:39:11 -0600 Subject: [PATCH 06/12] fix(contracts): add refundAddress to processCalls --- script/bridge/BridgeToArbitrumGoerli.ts | 1 + script/fork/bridge/BridgeToArbitrum.ts | 1 + .../EthereumToArbitrumRelayer.sol | 9 ++++- .../EthereumToArbitrumRelayer.t.sol | 39 ++++++++++++++++++- 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/script/bridge/BridgeToArbitrumGoerli.ts b/script/bridge/BridgeToArbitrumGoerli.ts index 99531cd..8120e84 100644 --- a/script/bridge/BridgeToArbitrumGoerli.ts +++ b/script/bridge/BridgeToArbitrumGoerli.ts @@ -118,6 +118,7 @@ const main = async () => { relayCallsNonce, calls, deployer, + deployer, gasLimit, maxSubmissionCost, gasPriceBid, diff --git a/script/fork/bridge/BridgeToArbitrum.ts b/script/fork/bridge/BridgeToArbitrum.ts index e55a618..874c1de 100644 --- a/script/fork/bridge/BridgeToArbitrum.ts +++ b/script/fork/bridge/BridgeToArbitrum.ts @@ -126,6 +126,7 @@ export const relayCalls = task( relayCallsNonce, calls, deployer, + deployer, gasLimit, maxSubmissionCost, gasPriceBid, diff --git a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol index f3bda6d..a067fbb 100644 --- a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol +++ b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol @@ -75,10 +75,12 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { /** * @notice Process calls that have been relayed. * @dev The transaction hash must match the one stored in the `relayed` mapping. + * @dev `_sender` is passed as `callValueRefundAddress` cause this address can cancel the retryably ticket. * @dev We store `_data` in memory to avoid a stack too deep error. * @param _nonce Nonce of the batch of calls to process * @param _calls Array of calls being processed * @param _sender Address who relayed the `_calls` + * @param _refundAddress Address that will receive the `excessFeeRefund` amount if any * @param _gasLimit Maximum amount of gas required for the `_calls` to be executed * @param _maxSubmissionCost Max gas deducted from user's L2 balance to cover base submission fee * @param _gasPriceBid Gas price bid for L2 execution @@ -88,6 +90,7 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { uint256 _nonce, CallLib.Call[] calldata _calls, address _sender, + address _refundAddress, uint256 _gasLimit, uint256 _maxSubmissionCost, uint256 _gasPriceBid @@ -97,6 +100,8 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { address _executorAddress = address(executor); require(_executorAddress != address(0), "Relayer/executor-not-set"); + require(_refundAddress != address(0), "Relayer/refund-address-not-zero"); + bytes memory _data = abi.encodeWithSignature( "executeCalls(uint256,address,(address,bytes)[])", _nonce, @@ -108,8 +113,8 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { _executorAddress, 0, _maxSubmissionCost, - msg.sender, - msg.sender, + _refundAddress, + _sender, _gasLimit, _gasPriceBid, _data diff --git a/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol b/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol index c3d22f0..0db4b73 100644 --- a/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol +++ b/test/unit/ethereum-arbitrum/EthereumToArbitrumRelayer.t.sol @@ -101,6 +101,7 @@ contract CrossChainRelayerArbitrumUnitTest is Test { _nonce, calls, address(this), + msg.sender, gasLimit, maxSubmissionCost, gasPriceBid @@ -113,7 +114,15 @@ contract CrossChainRelayerArbitrumUnitTest is Test { setExecutor(); vm.expectRevert(bytes("Relayer/calls-not-relayed")); - relayer.processCalls(nonce, calls, address(this), gasLimit, maxSubmissionCost, gasPriceBid); + relayer.processCalls( + nonce, + calls, + address(this), + msg.sender, + gasLimit, + maxSubmissionCost, + gasPriceBid + ); } function testExecutorNotSet() public { @@ -121,7 +130,33 @@ contract CrossChainRelayerArbitrumUnitTest is Test { vm.expectRevert(bytes("Relayer/executor-not-set")); - relayer.processCalls(_nonce, calls, address(this), gasLimit, maxSubmissionCost, gasPriceBid); + relayer.processCalls( + _nonce, + calls, + address(this), + msg.sender, + gasLimit, + maxSubmissionCost, + gasPriceBid + ); + } + + function testRefundAddressNotZero() public { + setExecutor(); + + uint256 _nonce = relayer.relayCalls(calls, gasLimit); + + vm.expectRevert(bytes("Relayer/refund-address-not-zero")); + + relayer.processCalls( + _nonce, + calls, + address(this), + address(0), + gasLimit, + maxSubmissionCost, + gasPriceBid + ); } /* ============ Getters ============ */ From ee4b1baa9487ddaf8547bf5f33589b6c07fdafe4 Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Wed, 14 Dec 2022 18:40:50 -0600 Subject: [PATCH 07/12] fix(CallLib): uncheck callIndex increment --- src/libraries/CallLib.sol | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/libraries/CallLib.sol b/src/libraries/CallLib.sol index 18d8cdc..025ccc7 100644 --- a/src/libraries/CallLib.sol +++ b/src/libraries/CallLib.sol @@ -58,7 +58,7 @@ library CallLib { uint256 _callsLength = _calls.length; - for (uint256 _callIndex; _callIndex < _callsLength; _callIndex++) { + for (uint256 _callIndex; _callIndex < _callsLength; ) { Call memory _call = _calls[_callIndex]; (bool _success, bytes memory _returnData) = _call.target.call( @@ -68,6 +68,10 @@ library CallLib { if (!_success) { revert CallFailure(_callIndex, _returnData); } + + unchecked { + _callIndex++; + } } } } From ef5d3f4bcca5af72a7b2ba96d9dad4dcf34f046d Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Wed, 14 Dec 2022 20:24:36 -0600 Subject: [PATCH 08/12] fix(CallLib): revert if target is not a contract --- src/libraries/CallLib.sol | 2 ++ test/fork/EthereumToPolygonFork.t.sol | 21 +++++++++++++++++++ .../EthereumToArbitrumExecutor.t.sol | 17 ++++++++++++++- 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/src/libraries/CallLib.sol b/src/libraries/CallLib.sol index 025ccc7..b707a5c 100644 --- a/src/libraries/CallLib.sol +++ b/src/libraries/CallLib.sol @@ -61,6 +61,8 @@ library CallLib { for (uint256 _callIndex; _callIndex < _callsLength; ) { Call memory _call = _calls[_callIndex]; + require(_call.target.code.length > 0, "CallLib/no-contract-at-target"); + (bool _success, bytes memory _returnData) = _call.target.call( abi.encodePacked(_call.data, _nonce, _sender) ); diff --git a/test/fork/EthereumToPolygonFork.t.sol b/test/fork/EthereumToPolygonFork.t.sol index 60a85fd..b9b574f 100644 --- a/test/fork/EthereumToPolygonFork.t.sol +++ b/test/fork/EthereumToPolygonFork.t.sol @@ -195,6 +195,27 @@ contract EthereumToPolygonForkTest is Test { assertEq(greeter.greet(), l1Greeting); } + function testExecuteCallsTargetNotZeroAddress() public { + deployAll(); + setAll(); + + vm.selectFork(polygonFork); + + assertEq(greeter.greet(), l2Greeting); + + CallLib.Call[] memory _calls = new CallLib.Call[](1); + + _calls[0] = CallLib.Call({ + target: address(0), + data: abi.encodeWithSignature("setGreeting(string)", l1Greeting) + }); + + vm.startPrank(fxChild); + + vm.expectRevert(bytes("CallLib/no-contract-at-target")); + executor.processMessageFromRoot(1, address(relayer), abi.encode(nonce, address(this), _calls)); + } + function testCallFailure() public { deployAll(); setAll(); diff --git a/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol b/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol index 942d92d..29c3812 100644 --- a/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol +++ b/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol @@ -42,10 +42,12 @@ contract CrossChainExecutorArbitrumUnitTest is Test { function setUp() public { executor = new CrossChainExecutorArbitrum(); greeter = new Greeter(address(executor), "Hello from L2"); + } + function pushCalls(address _target) public { calls.push( CallLib.Call({ - target: address(greeter), + target: _target, data: abi.encodeWithSignature("setGreeting(string)", l1Greeting) }) ); @@ -59,6 +61,7 @@ contract CrossChainExecutorArbitrumUnitTest is Test { function testExecuteCalls() public { setRelayer(); + pushCalls(address(greeter)); vm.startPrank(relayerAlias); @@ -75,6 +78,7 @@ contract CrossChainExecutorArbitrumUnitTest is Test { function testExecuteCallsAlreadyExecuted() public { setRelayer(); + pushCalls(address(greeter)); vm.startPrank(relayerAlias); executor.executeCalls(nonce, sender, calls); @@ -85,11 +89,22 @@ contract CrossChainExecutorArbitrumUnitTest is Test { function testExecuteCallsUnauthorized() public { setRelayer(); + pushCalls(address(greeter)); vm.expectRevert(bytes("Executor/sender-unauthorized")); executor.executeCalls(nonce, sender, calls); } + function testExecuteCallsTargetNotZeroAddress() public { + setRelayer(); + pushCalls(address(0)); + + vm.startPrank(relayerAlias); + + vm.expectRevert(bytes("CallLib/no-contract-at-target")); + executor.executeCalls(nonce, sender, calls); + } + /* ============ Setters ============ */ function testSetRelayer() public { From a62f08aa54e331e52b8a75fa20c14ea316801a88 Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Thu, 15 Dec 2022 18:12:31 -0600 Subject: [PATCH 09/12] fix(contracts): uncheck relayCalls nonce increment --- src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol | 4 +++- src/ethereum-optimism/EthereumToOptimismRelayer.sol | 4 +++- src/ethereum-polygon/EthereumToPolygonRelayer.sol | 4 +++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol index a067fbb..902605e 100644 --- a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol +++ b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol @@ -61,7 +61,9 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { external returns (uint256) { - nonce++; + unchecked { + nonce++; + } uint256 _nonce = nonce; diff --git a/src/ethereum-optimism/EthereumToOptimismRelayer.sol b/src/ethereum-optimism/EthereumToOptimismRelayer.sol index 0ddd4b8..35c6970 100644 --- a/src/ethereum-optimism/EthereumToOptimismRelayer.sol +++ b/src/ethereum-optimism/EthereumToOptimismRelayer.sol @@ -46,7 +46,9 @@ contract CrossChainRelayerOptimism is ICrossChainRelayer { address _executorAddress = address(executor); require(_executorAddress != address(0), "Relayer/executor-not-set"); - nonce++; + unchecked { + nonce++; + } uint256 _nonce = nonce; diff --git a/src/ethereum-polygon/EthereumToPolygonRelayer.sol b/src/ethereum-polygon/EthereumToPolygonRelayer.sol index b97ebd8..3677d63 100644 --- a/src/ethereum-polygon/EthereumToPolygonRelayer.sol +++ b/src/ethereum-polygon/EthereumToPolygonRelayer.sol @@ -39,7 +39,9 @@ contract CrossChainRelayerPolygon is ICrossChainRelayer, FxBaseRootTunnel { { require(address(fxChildTunnel) != address(0), "Relayer/fxChildTunnel-not-set"); - nonce++; + unchecked { + nonce++; + } uint256 _nonce = nonce; From 85d7a704484b4f02bd399e77e5b03505418911bd Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Thu, 15 Dec 2022 11:49:30 -0600 Subject: [PATCH 10/12] fix(CallLib): improve calls logging --- .../EthereumToArbitrumExecutor.sol | 15 ++++++++- .../EthereumToOptimismExecutor.sol | 15 ++++++++- .../EthereumToPolygonExecutor.sol | 15 ++++++++- src/interfaces/ICrossChainExecutor.sol | 1 + src/libraries/CallLib.sol | 31 +++++++++++++------ test/fork/EthereumToPolygonFork.t.sol | 8 ++++- .../EthereumToArbitrumExecutor.t.sol | 23 ++++++++++++++ 7 files changed, 95 insertions(+), 13 deletions(-) diff --git a/src/ethereum-arbitrum/EthereumToArbitrumExecutor.sol b/src/ethereum-arbitrum/EthereumToArbitrumExecutor.sol index bfee411..eb4b89f 100644 --- a/src/ethereum-arbitrum/EthereumToArbitrumExecutor.sol +++ b/src/ethereum-arbitrum/EthereumToArbitrumExecutor.sol @@ -13,6 +13,15 @@ import "../libraries/CallLib.sol"; * These calls are sent by the `CrossChainRelayerArbitrum` contract which lives on the Ethereum chain. */ contract CrossChainExecutorArbitrum is ICrossChainExecutor { + /* ============ Custom Errors ============ */ + + /** + * @notice Emitted when a batch of calls fails to execute. + * @param relayer Address of the contract that relayed the calls on the origin chain + * @param nonce Nonce to uniquely identify the batch of calls that failed to execute + */ + error ExecuteCallsFailed(ICrossChainRelayer relayer, uint256 nonce); + /* ============ Variables ============ */ /// @notice Address of the relayer contract on the Ethereum chain. @@ -39,7 +48,11 @@ contract CrossChainExecutorArbitrum is ICrossChainExecutor { bool _executedNonce = executed[_nonce]; executed[_nonce] = true; - CallLib.executeCalls(_nonce, _sender, _calls, _executedNonce); + bool _callsExecuted = CallLib.executeCalls(_nonce, _sender, _calls, _executedNonce); + + if (!_callsExecuted) { + revert ExecuteCallsFailed(_relayer, _nonce); + } emit ExecutedCalls(_relayer, _nonce); } diff --git a/src/ethereum-optimism/EthereumToOptimismExecutor.sol b/src/ethereum-optimism/EthereumToOptimismExecutor.sol index 1aba9c1..9a11cc3 100644 --- a/src/ethereum-optimism/EthereumToOptimismExecutor.sol +++ b/src/ethereum-optimism/EthereumToOptimismExecutor.sol @@ -13,6 +13,15 @@ import "../libraries/CallLib.sol"; * These calls are sent by the `CrossChainRelayerOptimism` contract which lives on the Ethereum chain. */ contract CrossChainExecutorOptimism is ICrossChainExecutor { + /* ============ Custom Errors ============ */ + + /** + * @notice Emitted when a batch of calls fails to execute. + * @param relayer Address of the contract that relayed the calls on the origin chain + * @param nonce Nonce to uniquely identify the batch of calls that failed to execute + */ + error ExecuteCallsFailed(ICrossChainRelayer relayer, uint256 nonce); + /* ============ Variables ============ */ /// @notice Address of the Optimism cross domain messenger on the Optimism chain. @@ -53,7 +62,11 @@ contract CrossChainExecutorOptimism is ICrossChainExecutor { bool _executedNonce = executed[_nonce]; executed[_nonce] = true; - CallLib.executeCalls(_nonce, _sender, _calls, _executedNonce); + bool _callsExecuted = CallLib.executeCalls(_nonce, _sender, _calls, _executedNonce); + + if (!_callsExecuted) { + revert ExecuteCallsFailed(_relayer, _nonce); + } emit ExecutedCalls(_relayer, _nonce); } diff --git a/src/ethereum-polygon/EthereumToPolygonExecutor.sol b/src/ethereum-polygon/EthereumToPolygonExecutor.sol index 29bc54f..d651327 100644 --- a/src/ethereum-polygon/EthereumToPolygonExecutor.sol +++ b/src/ethereum-polygon/EthereumToPolygonExecutor.sol @@ -12,6 +12,15 @@ import "../libraries/CallLib.sol"; * These calls are sent by the `CrossChainRelayerPolygon` contract which lives on the Ethereum chain. */ contract CrossChainExecutorPolygon is FxBaseChildTunnel { + /* ============ Custom Errors ============ */ + + /** + * @notice Emitted when a batch of calls fails to execute. + * @param relayer Address of the contract that relayed the calls on the origin chain + * @param nonce Nonce to uniquely identify the batch of calls that failed to execute + */ + error ExecuteCallsFailed(address relayer, uint256 nonce); + /* ============ Events ============ */ /** @@ -54,7 +63,11 @@ contract CrossChainExecutorPolygon is FxBaseChildTunnel { bool _executedNonce = executed[_nonce]; executed[_nonce] = true; - CallLib.executeCalls(_nonce, _callsSender, _calls, _executedNonce); + bool _callsExecuted = CallLib.executeCalls(_nonce, _callsSender, _calls, _executedNonce); + + if (!_callsExecuted) { + revert ExecuteCallsFailed(_sender, _nonce); + } emit ExecutedCalls(_sender, _nonce); } diff --git a/src/interfaces/ICrossChainExecutor.sol b/src/interfaces/ICrossChainExecutor.sol index 227f5cf..08cd1e9 100644 --- a/src/interfaces/ICrossChainExecutor.sol +++ b/src/interfaces/ICrossChainExecutor.sol @@ -21,6 +21,7 @@ interface ICrossChainExecutor { /** * @notice Execute calls from the origin chain. * @dev Should authenticate that the call has been performed by the bridge transport layer. + * @dev Must revert if a call fails. * @dev Must emit the `ExecutedCalls` event once calls have been executed. * @param nonce Nonce to uniquely idenfity the batch of calls * @param sender Address of the sender on the origin chain diff --git a/src/libraries/CallLib.sol b/src/libraries/CallLib.sol index b707a5c..a7e3683 100644 --- a/src/libraries/CallLib.sol +++ b/src/libraries/CallLib.sol @@ -19,14 +19,23 @@ library CallLib { bytes data; } - /* ============ Custom Errors ============ */ + /* ============ Events ============ */ + + /** + * @notice Emitted if a call to a target contract fails. + * @param callIndex Index of the call + * @param errorData Error data returned by the call + */ + event CallFailure(uint256 callIndex, bytes errorData); /** - * @notice Custom error emitted if a call to a target contract fails. - * @param callIndex Index of the failed call - * @param errorData Error data returned by the failed call + * @notice Emitted if a call to a target contract succeeds. + * @param callIndex Index of the call + * @param successData Error data returned by the call */ - error CallFailure(uint256 callIndex, bytes errorData); + event CallSuccess(uint256 callIndex, bytes successData); + + /* ============ Custom Errors ============ */ /** * @notice Emitted when a batch of calls has already been executed. @@ -39,19 +48,18 @@ library CallLib { /** * @notice Execute calls from the origin chain. * @dev Will revert if `_calls` have already been executed. - * @dev Will revert if a call fails. - * @dev Must emit the `ExecutedCalls` event once calls have been executed. * @param _nonce Nonce to uniquely idenfity the batch of calls * @param _sender Address of the sender on the origin chain * @param _calls Array of calls being executed * @param _executedNonce Whether `_calls` have already been executed or not + * @return bool Whether the batch of calls was executed successfully or not */ function executeCalls( uint256 _nonce, address _sender, Call[] memory _calls, bool _executedNonce - ) internal { + ) internal returns (bool) { if (_executedNonce) { revert CallsAlreadyExecuted(_nonce); } @@ -68,12 +76,17 @@ library CallLib { ); if (!_success) { - revert CallFailure(_callIndex, _returnData); + emit CallFailure(_callIndex, _returnData); + return false; } + emit CallSuccess(_callIndex, _returnData); + unchecked { _callIndex++; } } + + return true; } } diff --git a/test/fork/EthereumToPolygonFork.t.sol b/test/fork/EthereumToPolygonFork.t.sol index b9b574f..94fe803 100644 --- a/test/fork/EthereumToPolygonFork.t.sol +++ b/test/fork/EthereumToPolygonFork.t.sol @@ -231,7 +231,13 @@ contract EthereumToPolygonForkTest is Test { vm.startPrank(fxChild); - vm.expectRevert(abi.encodeWithSelector(CallLib.CallFailure.selector, 0, bytes(""))); + vm.expectRevert( + abi.encodeWithSelector( + CrossChainExecutorPolygon.ExecuteCallsFailed.selector, + address(relayer), + 1 + ) + ); executor.processMessageFromRoot(1, address(relayer), abi.encode(nonce, address(this), _calls)); } diff --git a/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol b/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol index 29c3812..ff01799 100644 --- a/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol +++ b/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol @@ -68,6 +68,9 @@ contract CrossChainExecutorArbitrumUnitTest is Test { vm.expectEmit(true, true, true, true, address(greeter)); emit SetGreeting(l1Greeting, nonce, sender, address(executor)); + vm.expectEmit(true, true, true, true, address(executor)); + emit CallLib.CallSuccess(0, bytes("")); + vm.expectEmit(true, true, true, true, address(executor)); emit ExecutedCalls(relayer, nonce); @@ -87,6 +90,26 @@ contract CrossChainExecutorArbitrumUnitTest is Test { executor.executeCalls(nonce, sender, calls); } + function testExecuteCallsFailed() public { + setRelayer(); + pushCalls(address(this)); + + vm.startPrank(relayerAlias); + + vm.expectEmit(true, true, true, true, address(executor)); + emit CallLib.CallFailure(0, bytes("")); + + vm.expectRevert( + abi.encodeWithSelector( + CrossChainExecutorArbitrum.ExecuteCallsFailed.selector, + address(relayer), + nonce + ) + ); + + executor.executeCalls(nonce, sender, calls); + } + function testExecuteCallsUnauthorized() public { setRelayer(); pushCalls(address(greeter)); From 39deff14dc40ebe14c3d9a417d6838335fdd4946 Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Mon, 19 Dec 2022 09:52:44 -0600 Subject: [PATCH 11/12] fix(CallLib): log nonce in CallFailure and Success --- src/libraries/CallLib.sol | 10 ++++++---- .../ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol | 4 ++-- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/src/libraries/CallLib.sol b/src/libraries/CallLib.sol index a7e3683..a364106 100644 --- a/src/libraries/CallLib.sol +++ b/src/libraries/CallLib.sol @@ -23,17 +23,19 @@ library CallLib { /** * @notice Emitted if a call to a target contract fails. + * @param nonce Nonce to uniquely idenfity the batch of calls * @param callIndex Index of the call * @param errorData Error data returned by the call */ - event CallFailure(uint256 callIndex, bytes errorData); + event CallFailure(uint256 nonce, uint256 callIndex, bytes errorData); /** * @notice Emitted if a call to a target contract succeeds. + * @param nonce Nonce to uniquely idenfity the batch of calls * @param callIndex Index of the call * @param successData Error data returned by the call */ - event CallSuccess(uint256 callIndex, bytes successData); + event CallSuccess(uint256 nonce, uint256 callIndex, bytes successData); /* ============ Custom Errors ============ */ @@ -76,11 +78,11 @@ library CallLib { ); if (!_success) { - emit CallFailure(_callIndex, _returnData); + emit CallFailure(_nonce, _callIndex, _returnData); return false; } - emit CallSuccess(_callIndex, _returnData); + emit CallSuccess(_nonce, _callIndex, _returnData); unchecked { _callIndex++; diff --git a/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol b/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol index ff01799..c192fe5 100644 --- a/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol +++ b/test/unit/ethereum-arbitrum/EthereumToArbitrumExecutor.t.sol @@ -69,7 +69,7 @@ contract CrossChainExecutorArbitrumUnitTest is Test { emit SetGreeting(l1Greeting, nonce, sender, address(executor)); vm.expectEmit(true, true, true, true, address(executor)); - emit CallLib.CallSuccess(0, bytes("")); + emit CallLib.CallSuccess(1, 0, bytes("")); vm.expectEmit(true, true, true, true, address(executor)); emit ExecutedCalls(relayer, nonce); @@ -97,7 +97,7 @@ contract CrossChainExecutorArbitrumUnitTest is Test { vm.startPrank(relayerAlias); vm.expectEmit(true, true, true, true, address(executor)); - emit CallLib.CallFailure(0, bytes("")); + emit CallLib.CallFailure(1, 0, bytes("")); vm.expectRevert( abi.encodeWithSelector( From 0aa13ce7a2292440088a3ef2984916b6168283b2 Mon Sep 17 00:00:00 2001 From: Pierrick Turelier Date: Wed, 14 Dec 2022 21:08:00 -0600 Subject: [PATCH 12/12] fix(contracts): use abi.encodeWithSelector --- src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol | 4 ++-- src/ethereum-optimism/EthereumToOptimismRelayer.sol | 7 +------ src/ethereum-polygon/EthereumToPolygonRelayer.sol | 1 - 3 files changed, 3 insertions(+), 9 deletions(-) diff --git a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol index 902605e..5326af6 100644 --- a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol +++ b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol @@ -104,8 +104,8 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer { require(_refundAddress != address(0), "Relayer/refund-address-not-zero"); - bytes memory _data = abi.encodeWithSignature( - "executeCalls(uint256,address,(address,bytes)[])", + bytes memory _data = abi.encodeWithSelector( + ICrossChainExecutor.executeCalls.selector, _nonce, _sender, _calls diff --git a/src/ethereum-optimism/EthereumToOptimismRelayer.sol b/src/ethereum-optimism/EthereumToOptimismRelayer.sol index 35c6970..2ea326c 100644 --- a/src/ethereum-optimism/EthereumToOptimismRelayer.sol +++ b/src/ethereum-optimism/EthereumToOptimismRelayer.sol @@ -54,12 +54,7 @@ contract CrossChainRelayerOptimism is ICrossChainRelayer { crossDomainMessenger.sendMessage( _executorAddress, - abi.encodeWithSignature( - "executeCalls(uint256,address,(address,bytes)[])", - _nonce, - msg.sender, - _calls - ), + abi.encodeWithSelector(ICrossChainExecutor.executeCalls.selector, _nonce, msg.sender, _calls), uint32(_gasLimit) ); diff --git a/src/ethereum-polygon/EthereumToPolygonRelayer.sol b/src/ethereum-polygon/EthereumToPolygonRelayer.sol index 3677d63..560e193 100644 --- a/src/ethereum-polygon/EthereumToPolygonRelayer.sol +++ b/src/ethereum-polygon/EthereumToPolygonRelayer.sol @@ -4,7 +4,6 @@ pragma solidity 0.8.16; import { FxBaseRootTunnel } from "@maticnetwork/fx-portal/contracts/tunnel/FxBaseRootTunnel.sol"; -import { ICrossChainExecutor } from "../interfaces/ICrossChainExecutor.sol"; import { ICrossChainRelayer } from "../interfaces/ICrossChainRelayer.sol"; import "../libraries/CallLib.sol";