PoolTogether contest - csanuragjain's results

Lines of code

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

Vulnerability details

Impact

It seems there is no expiry or cancelling facility for processing relay calls. This could become a problem where an old failed transaction could be run at any point of time in future without any deadline

Proof of Concept

1. User relays Calls C1 using relayCalls function

function relayCalls(CallLib.Call[] calldata _calls, uint256 _gasLimit)
    external
    payable
    returns (uint256)
  {
    ...

    nonce++;

    uint256 _nonce = nonce;

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

    ...
  }

2. Now User process this transaction using processCalls function

function processCalls(
    uint256 _nonce,
    CallLib.Call[] calldata _calls,
    address _sender,
    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)[])",
      _nonce,
      _sender,
      _calls
    );
...
}

3. Assume that executeCalls fails in which case transaction will revert

function executeCalls(
    uint256 _nonce,
    address _sender,
    Call[] memory _calls,
    bool _executedNonce
  ) internal {
    if (_executedNonce) {
      revert CallsAlreadyExecuted(_nonce);
    }

    ...

      (bool _success, bytes memory _returnData) = _call.target.call(
        abi.encodePacked(_call.data, _nonce, _sender)
      );

      if (!_success) {
        revert CallFailure(_callIndex, _returnData);
      }
    }
  }

4. User understands that current condition does not allow the transaction to complete as of now (say contract is locked). But now User has no way to cancel this relay transaction. This means anyone can execute this transaction in future even if it is not required

Recommended Mitigation Steps

Allow only X amount of duration post relayCalls and if call is not processed during that time then it expires Or Add a new function which allows to reset relay calls ie relayed[_getTxHash(_nonce, _calls, msg.sender, _gasLimit)]