Causing users lose their fund during finalizing withdrawal transaction

Summary

A malicious user can make users lose their fund during finalizing their withdrawal. This is possible due to presence of reentrancy guard on the function relayMessage.

Vulnerability Detail

Bob (a malicious user) creates a contract (called AttackContract) on L1.
```
// SPDX-License-Identifier: MIT
pragma solidity 0.8.0;
```

struct WithdrawalTransaction { uint256 nonce; address sender; address target; uint256 value; uint256 gasLimit; bytes data; }

interface IOptimismPortal { function finalizeWithdrawalTransaction(WithdrawalTransaction memory _tx) external; }

contract AttackContract { bool public donotRevert; bytes metaData; address optimismPortalAddress;

constructor(address _optimismPortal) {
    optimismPortalAddress = _optimismPortal;
}

function enableRevert() public {
    donotRevert = true;
}

function setMetaData(WithdrawalTransaction memory _tx) public {
    metaData = abi.encodeWithSelector(
        IOptimismPortal.finalizeWithdrawalTransaction.selector,
        _tx
    );
}

function attack() public {
    if (!donotRevert) {
        revert();
    } else {
        optimismPortalAddress.call(metaData);
    }
}

}

 - Bob sends a message from L2 to L1 by calling the function `sendMessage` with the following parameters. He intends to call the function `attack()` through relaying the message from L2 to L1.
   - `_target` = address of `AttackContract` on L1 
   - `_message` = abi.encodeWithSignature("attack()")
   - `_minGasLimit` = just big enough so that the transaction can be executed
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L212
 - On the L1 side, after challenge period and validation elapsed, the function `attack()` on contract `AttackContract` will be called during relaying the message.
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L324
 -  But, since `donotRevert` is `false` in the contract `AttackContract`, the relayed message will be unsuccessful. So, we will have `failedMessages[versionedHash] = true`. It means that it is possible again to retry relaying the message later.
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L331
```solidity
        if (!donotRevert) {
            revert();
        }

Then, Bob calls the function enableRevert to set donotRevert to true. So that if later the function attack() is called again, it will not revert.
```
function enableRevert() public {
    donotRevert = true;
}
```
Then, Bob notices that Alice is withdrawing large amount of fund from L2 to L1. Her withdrawal transaction is proved but she is waiting for the challenge period to be finished to finalize it.
Then, Bob calls the function setMetaData on the contract AttackContract with the following parameter:
- _tx = Alice's withdrawal transaction
- By doing so, the metaData will be equal to finalizeWithdrawalTransaction.selector + Alice's withdrawal transaction.
```
function setMetaData(WithdrawalTransaction memory _tx) public {
metaData = abi.encodeWithSelector(
    IOptimismPortal.finalizeWithdrawalTransaction.selector,
    _tx
);
}
```
- Now, after the challenge period is passed, and before the function finalizeWithdrawalTransaction is called by anyone (Alice), Bob calls the function relayMessage with the required data to retry his previous failed message again.
- This time, since donotRevert is true, the call to function attack() will not revert, instead the body of else clause will be executed.
```
else {
    optimismPortalAddress.call(metaData);
}
```
- In the else clause, it calls the function finalizeWithdrawalTransaction with Alice's withdrawal transaction as the parameter, to finalize Alice's transaction. https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L243
During finalizing Alice's withdrawal transaction, everything goes smoothly (as the challenge period is passed, and everything is valid) until the call to the function relayMessage in the contract CrossDomainMessanger. https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L324
Due to the reentrancy guard, the call will be unsuccessful. https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L263
Please note that the flow is as follows: Bob ==> CrossDomainMessenger.relayMessage ==> AttackContract.attack ==> OptimismPortal.finalizeWithdrawalTransaction => CrossDomainMessenger.relayMessage
Since, the failed call is not handled during finalizing the message, the transaction will be finished without any error.
Then, Bob's relayed message transaction will be finished successfully.
By doing so, Alice's withdrawal transaction is flagged as finalized, but in reality it was not because of reentrancy guard. So, Alice loses her fund. https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L308

In summary the attack is as follows:

Bob creates a malicious contract on L1 called AttackContract.
Bob sends a message from L2 to L1 to call the function AttackContract.attack on L1.
On L1 side, after the challenge period is passed, the function AttackContract.attack will be called.
Message relay on L1 will be unsuccessful, because the function AttackContract.attack reverts. So, Bob's message will be flagged as failed message.
Bob sets AttackContract.donotRevert to true.
Bob waits for an innocent user to request withdrawal transaction.
Bob waits for the innocent user's withdrawal transaction to be proved.
Bob sets meta data in his malicious contract based on the innocent user's withdrawal transaction.
Bob waits for the challenge period to be passed.
After the challenge period is elapsed, Bob retries to relay his failed message again.
CrossDomainMessenger.relayMessage will call the AttackContract.attack, then it calls OptimismPortal.finalizeWithdrawalTransaction to finalize innocent user's withdrawal transaction. Then, it calls CrossDomainMessenger.relayMessage, but it will be unsuccessful because of reentrancy guard.
After finalizing the innocent user's withdrawal transaction, Bob's message will be flagged as successful.
So, innocent user's withdrawal transaction is flagged as finalized, while it is not.

Impact

By doing this attack it is possible to prevent users from withdrawing their fund. Moreover, they lose their fund because withdrawal is flagged as finalized, but the withdrawal sent to L1CrossDomainMessanger was not successful.

Code Snippet

Tool used

Manual Review

Recommendation

Maybe it is better to use the following code instead of: https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L324-L329

        try IL1CrossDomainMessanger.relayMessage(...) {} catch Error(string memory reason) {
            if (
                keccak256(abi.encodePacked(reason)) ==
                keccak256(abi.encodePacked("ReentrancyGuard: reentrant call"))
            ) {
                revert("finalizing should be reverted");
            }
        }

sherlock-audit / 2023-01-optimism-judging

HE1M - Causing users lose their fund during finalizing withdrawal transaction #87