Merge pull request #8 from pooltogether/fix/audit-219

fix(contracts): check executor address in 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;

script/bridge/BridgeToArbitrumGoerli.ts

@@ -118,6 +118,7 @@ const main = async () => {
     relayCallsNonce,
     calls,
     deployer,
+    deployer,
     gasLimit,
     maxSubmissionCost,
     gasPriceBid,

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

@@ -126,6 +126,7 @@ export const relayCalls = task(
     relayCallsNonce,
     calls,
     deployer,
+    deployer,
     gasLimit,
     maxSubmissionCost,
     gasPriceBid,

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

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,14 +61,10 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer {
     external
     returns (uint256)
   {
-    uint256 _maxGasLimit = maxGasLimit;
-
-    if (_gasLimit > _maxGasLimit) {
-      revert GasLimitTooHigh(_gasLimit, _maxGasLimit);
+    unchecked {
+      nonce++;
     }
 
-    nonce++;
-
     uint256 _nonce = nonce;
 
     relayed[_getTxHash(_nonce, _calls, msg.sender, _gasLimit)] = true;
@@ -88,10 +77,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
@@ -101,25 +92,31 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer {
     uint256 _nonce,
     CallLib.Call[] calldata _calls,
     address _sender,
+    address _refundAddress,
     uint256 _gasLimit,
     uint256 _maxSubmissionCost,
     uint256 _gasPriceBid
   ) external payable returns (uint256) {
     require(relayed[_getTxHash(_nonce, _calls, _sender, _gasLimit)], "Relayer/calls-not-relayed");
 
-    bytes memory _data = abi.encodeWithSignature(
-      "executeCalls(uint256,address,(address,bytes)[])",
+    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.encodeWithSelector(
+      ICrossChainExecutor.executeCalls.selector,
       _nonce,
       _sender,
       _calls
     );
 
     uint256 _ticketID = inbox.createRetryableTicket{ value: msg.value }(
-      address(executor),
+      _executorAddress,
       0,
       _maxSubmissionCost,
-      msg.sender,
-      msg.sender,
+      _refundAddress,
+      _sender,
       _gasLimit,
       _gasPriceBid,
       _data

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

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,24 +43,18 @@ contract CrossChainRelayerOptimism is ICrossChainRelayer {
     external
     returns (uint256)
   {
-    uint256 _maxGasLimit = maxGasLimit;
+    address _executorAddress = address(executor);
+    require(_executorAddress != address(0), "Relayer/executor-not-set");
 
-    if (_gasLimit > _maxGasLimit) {
-      revert GasLimitTooHigh(_gasLimit, _maxGasLimit);
+    unchecked {
+      nonce++;
     }
 
-    nonce++;
-
     uint256 _nonce = nonce;
 
     crossDomainMessenger.sendMessage(
-      address(executor),
-      abi.encodeWithSignature(
-        "executeCalls(uint256,address,(address,bytes)[])",
-        _nonce,
-        msg.sender,
-        _calls
-      ),
+      _executorAddress,
+      abi.encodeWithSelector(ICrossChainExecutor.executeCalls.selector, _nonce, msg.sender, _calls),
       uint32(_gasLimit)
     );
 

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

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";
 
@@ -16,9 +15,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 +24,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,14 +36,12 @@ contract CrossChainRelayerPolygon is ICrossChainRelayer, FxBaseRootTunnel {
     external
     returns (uint256)
   {
-    uint256 _maxGasLimit = maxGasLimit;
+    require(address(fxChildTunnel) != address(0), "Relayer/fxChildTunnel-not-set");
 
-    if (_gasLimit > _maxGasLimit) {
-      revert GasLimitTooHigh(_gasLimit, _maxGasLimit);
+    unchecked {
+      nonce++;
     }
 
-    nonce++;
-
     uint256 _nonce = nonce;
 
     _sendMessageToChild(abi.encode(_nonce, msg.sender, _calls));

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