Integer Overflow

[code423n4]

Lines of code

https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol#L67-L87 https://github.com/pooltogether/ERC5164/blob/5647bd84f2a6d1a37f41394874d567e45a97bf48/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol#L101-L132

Vulnerability details

Impact

Potential integer overflow vulnerability in the relayCalls() function. The function does not check for the maximum value of the _gasLimit parameter, and if it is set to a value greater than the maxGasLimit, it will cause an integer overflow.

Integer overflow bug in the processCalls() function. Specifically, the _maxSubmissionCost parameter is being used as a multiplier and can therefore potentially overflow if a large _maxSubmissionCost is passed.

Proof of Concept

1. An attacker can call the relayCalls() function with a _gasLimit parameter greater than the maxGasLimit, causing an integer overflow. This would cause the function to revert and could lead to a denial of service attack.
2. A malicious actor could exploit this bug by passing an excessively large _maxSubmissionCost parameter to the processCalls function. This could result in an Integer overflow which could cause the entire transaction to fail.
3. In the following exploit, an attacker can send a value of 2^256 - 1 for the _maxSubmissionCost parameter. This will cause _maxSubmissionCost to overflow, resulting in a cost of 0:

   function exploit(address _inbox, uint256 _maxGasLimit) public {
   CrossChainRelayerArbitrum relayer = new CrossChainRelayerArbitrum(_inbox, _maxGasLimit);
   CallLib.Call[] memory _calls = new CallLib.Call[](1);
   _calls[0] = {destination: msg.sender, data: 0};
   relayer.processCalls(0, _calls, msg.sender, _maxGasLimit, 2**256 - 1, 0);
   }

Tools Used

Slither, Echidna, MythX, and Manticore.

Recommended Mitigation Steps

1. Add require(_gasLimit <= _maxGasLimit, "Relayer/GasLimitTooHigh"); statement at the beginning of relayCalls() function;
2. Add require(_maxSubmissionCost <= maxGasLimit, "Relayer/max-submission-cost-overflow"); statement at the beginning of processCalls() function;

function relayCalls(CallLib.Call[] calldata _calls, uint256 _gasLimit)
    external
    payable
    returns (uint256)
  {
    uint256 _maxGasLimit = maxGasLimit;
    require(_gasLimit <= _maxGasLimit, "Relayer/GasLimitTooHigh");

    if (_gasLimit > _maxGasLimit) {
      revert GasLimitTooHigh(_gasLimit, _maxGasLimit);
    }

    nonce++;

    uint256 _nonce = nonce;

    relayed[_getTxHash(_nonce, _calls, msg.sender, _gasLimit)] = true;

    emit RelayedCalls(_nonce, msg.sender, _calls, _gasLimit);

    return _nonce;
  }

function processCalls(
    uint256 _nonce,
    CallLib.Call[] calldata _calls,
    address _sender,
    uint256 _gasLimit,
    uint256 _maxSubmissionCost,
    uint256 _gasPriceBid
  ) external payable returns (uint256) {
    require(_maxSubmissionCost <= maxGasLimit, "Relayer/max-submission-cost-overflow");
    require(relayed[_getTxHash(_nonce, _calls, _sender, _gasLimit)], "Relayer/calls-not-relayed");

    bytes memory _data = abi.encodeWithSignature(
      "executeCalls(uint256,address,(address,bytes)[])",
      _nonce,
      _sender,
      _calls
    );

    uint256 _ticketID = inbox.createRetryableTicket{ value: msg.value }(
      address(executor),
      0,
      _maxSubmissionCost,
      msg.sender,
      msg.sender,
      _gasLimit,
      _gasPriceBid,
      _data
    );

    emit ProcessedCalls(_nonce, msg.sender, _ticketID);

    return _ticketID;
  }

[GalloDaSballo]

It's a self-inflicted issue that can be easily sidestepped. Could have accepted perhaps as QA. Am closing as overly inflated.

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory: Overinflated severity