HE1M - Causing users lose their fund during finalizing withdrawal transaction

[github-actions]

HE1M

high

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

        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:

1. Bob creates a malicious contract on L1 called AttackContract.
2. Bob sends a message from L2 to L1 to call the function AttackContract.attack on L1.
3. On L1 side, after the challenge period is passed, the function AttackContract.attack will be called.
4. Message relay on L1 will be unsuccessful, because the function AttackContract.attack reverts. So, Bob's message will be flagged as failed message.
5. Bob sets AttackContract.donotRevert to true.
6. Bob waits for an innocent user to request withdrawal transaction.
7. Bob waits for the innocent user's withdrawal transaction to be proved.
8. Bob sets meta data in his malicious contract based on the innocent user's withdrawal transaction.
9. Bob waits for the challenge period to be passed.
10. After the challenge period is elapsed, Bob retries to relay his failed message again.
11. 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.
12. After finalizing the innocent user's withdrawal transaction, Bob's message will be flagged as successful.
13. 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");
            }
        }

[rcstanciu]

Comment from Optimism

---

Description: Causing users lose their fund during finalizing withdrawal transaction

Reason: This is a very creative exploit that takes advantage of the L1CrossDomainMessenger's reentrancy guard on relayMessage. By creating an attack contract that reverts the first time a message is relayed by the OptimismPortal to the L1CrossDomainMessenger, but can be toggled to call finalizeWithdrawalTransaction for a different withdrawal transaction when the attacker replays it via the L1CrossDomainMessenger, the attacker can force the honest withdrawal's call to the L1CrossDomainMessenger to revert, effectively bricking it. PoC: here

Action: If a withdrawal transaction fails, we should check to see if the _target of the WithdrawalTransaction is the L1CrossDomainMessenger. If so, check if the call's revert data is the reentrancy guard message. If it is, we should revert the finalizeWithdrawalTransaction message and NOT mark the withdrawal as finalized.