PoolTogether contest - hihen's results

Lines of code

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

Vulnerability details

Impact

Cross chain execution may be replayed by anyone at anytime in the future. The sender of the origin cross-chain-tx or some related protocols on arbitrum may suffer fund loss or any unforeseen problems.

Proof of Concept

Suppose user A start an ethereum-to-arbitrum execution as follows:

1. A calls relayer.relayCalls(calls1) on ethereum. Let's label the request as nonce1.calls1.
2. A calls relayer.processCalls(nonce1, calls1, A, ...) on ethereum.
3. Arbitrum bridge invokes executor.executeCalls(nonce1, A, calls1) on arbitrum. This execution will call all the calls1.

Suppose the execution in step 3 reverted for target.call - CallFailure.

User A sees this result (CallFailure error) and may take one of the following actions:

1. Start a new ethereum-to-arbitrum execution from step 1, if A found something is wrong in the first execution nonce1.calls1(invalid parameters in calls1, or states changed on arbitrum, etc).
2. Just ignore the error and do nothing more. Because the time or state is changed on arbitrum, and A don't want to or need to call the calls1 anymore.

Anyway, user A doesn't care about the nonce1.calls1 anymore.

But, malicious users will always care about nonce1.calls1 and all the other failed ethereum-to-arbitrum executions. Because these failed executions may succeed in the future with the passage of time and change of states on arbitrum.

That is to say, if anyone finds it profitable to invode calls1 with sender = A on arbitrums at any time in the future, he/she can immediately start an attack by calling relayer.processCalls(nonce1, calls1, A, ...) on ethereum to replay execution nonce1.calls1 on arbitrum.

The replay attack will succeed, because:

• The relayer.processCalls() can be called repeatedly by anyone for the same nonce.
• The executed flag in executor is not set when tx is reverted for CallFailure, which lead to the failed execution can be tried again and again (until succ).

In fact, even if user A knows about the risk after step 3, he/she probably can't do anything about it if the target contracts of calls1 are not controlled by A or can not be modified.

Tools Used

VS Code

Recommended Mitigation Steps

Add a function invalidateCalls() to EthereumToArbitrumRelayer.sol like follows:

diff --git a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol
index f88674d..2cab94e 100644
--- a/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol
+++ b/src/ethereum-arbitrum/EthereumToArbitrumRelayer.sol
@@ -131,6 +131,16 @@ contract CrossChainRelayerArbitrum is ICrossChainRelayer {
     return _ticketID;
   }

+  function invalidateCalls(
+    uint256 _nonce,
+    CallLib.Call[] calldata _calls,
+    uint256 _gasLimit
+  ) external returns (uint256) {
+    bytes32 txHash = _getTxHash(_nonce, _calls, msg.sender, _gasLimit);
+    require(relayed[txHash], "Relayer/calls-not-relayed");
+    delete relayed[txHash];
+  }
+
   /**
    * @notice Set executor contract address.
    * @dev Will revert if it has already been set.

Now, senders can invalidate their calls when needed, noone can replay them anymore.

Or, you could mark the nonce/txHash as processed at the end of relayer.processCalls(nonce1, calls1, A, ...) to prevent it from being processed agian.