PoolTogether contest - 0x52's results

Lines of code

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

Vulnerability details

Impact

Arbitrum relaying and transaction execution can be abused

Proof of Concept

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

When EthereumToArbitrumRelayer#processCalls is called it creates a retryable ticket with msg.sender as the beneficiary of the L2 message. Arbitrum allows the beneficiary to cancel a retryable ticket at anytime. In EthereumToArbitrumExecutor there is no way to cancel a relayed call, only offering replay protection. Additionally EthereumToArbitrumRelayer#processCalls doesn't offer any replay protection, allowing the same transaction to be queue up by whoever, whenever and as many times as desired. If the transaction is executed then there is nothing to worry about because of the replay protection provided by EthereumToArbitrumExecutor but can create issues when the call doesn't succeed.

EthereumToArbitrumRelayer use msg.sender as beneficiary so whoever processes the calls will be the only party able to cancel the message. This is problematic because EthereumToArbitrumRelayer#processCalls doesn't implement any replay protection.

Example: Assume you have two users, User A and User B. User A relays a transaction then processes the call. They realize they have made a mistake to so they cancel their message to prevent execution, which they are allowed to do since they are beneficiary. Now at any point User B can process User A's calls. This time around User B is beneficiary and User A can't cancel the message, allowing it to be executed.

The consequences of this can be extremely damaging. For example, if User A cancels the message then relays another similar transaction it may result in an effective replay attack because they wrongly assume their transaction is permanently canceled. Another example would be a set of calls that fails when executed. Imagine User A relays a call that fails when executed today. Months later conditions have changed and execution of that call will now succeed. Their call from months ago can now be re-processed and executed.

Tools Used

Manual Review

Recommended Mitigation Steps

Create a method in CrossChainExecutorArbitrum that allows a sender to cancel their call. I also recommend requiring a expiration timestamp by which a relayed call must be processed. I do not recommend implementing replay protection on CrossChainRelayerArbitrum. It can lead to DOS'ing, since msg.sender is the beneficiary. Any user would be able to queue a transaction then immediately cancel the message to DOS the request.