Merge pull request #6 from pooltogether/fix/audit-13

fix(contracts): remove payable from relayCalls

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;

README.md

@@ -13,7 +13,7 @@ To use ERC-5164 to send messages your contract code will need to:
 - On the sending chain, send a batch of calls to the CrossChainRelayer `relayCalls` function
 - Listen for calls from the corresponding CrossChainExecutor(s) on the receiving chain.
 
-*The listener will need to be able to unpack the original sender address (it's appended to calldata). We recommend inheriting from the included [`ExecutorAware.sol`](./src/abstract/ExecutorAware.sol) contract.*
+_The listener will need to be able to unpack the original sender address (it's appended to calldata). We recommend inheriting from the included [`ExecutorAware.sol`](./src/abstract/ExecutorAware.sol) contract._
 
 **Note**
 
@@ -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
 

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,22 @@ 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,
+    deployer,
+    gasLimit,
     maxSubmissionCost,
     gasPriceBid,
     {
-      value: callValue,
+      value: deposit,
     },
   );
 

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();
   }

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();
   }

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();
   }

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,70 @@ 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,
+    deployer,
+    gasLimit,
     maxSubmissionCost,
     gasPriceBid,
     {
-      value: callValue,
+      value: deposit,
     },
   );
 
@@ -133,7 +145,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);

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}`);

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);
   }