Reentrancy in Cross Domain Messenger can cause permanent loss of funds

Summary

Withdrawals using the guaranteed safe CrossDomainMessenger can be forever stuck when the to address interacts with external addresses, which is a key feature for composability and expected to be heavily used.

Vulnerability Detail

Following the previous contest, CrossDomainMessenger (XDM) is now re-enterable, so long as the message to be relayed is new. It will deliver the message in the snippet below:

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

The threat introduced here is that when a user's message is delivered and the target calls another address for any reason, the callee can perform a relayMessage themselves, on a previous failed TX. It will reach the same snippet above and, after executing, set xDomainMsgSender = Constants.DEFAULT_L2_SENDER.

When returning to original (victim) execution, this state variable is now incorrect, as the correct sender should be _sender. In fact, when the user's contract will use Optimism's API for getting the sender, it will cause a revert.

function xDomainMessageSender() external view returns (address) {
    require(
        xDomainMsgSender != Constants.DEFAULT_L2_SENDER,
        "CrossDomainMessenger: xDomainMessageSender is not set"
    );
    return xDomainMsgSender;
}

It is extremely likely that contracts which are delivered messages on L1 will (a) need to interact with untrusted addresses and (c) need to fetch the L2 sender. These are the only conditions for stuck funds to occur.

These are both extremely common patterns (for example, ERC721 safeTransfers trigger callbacks on the recipient, and transferring of funds with call also passes over control flow).

A silly vulnerable example is attached below, but any user-defined or widely used ecosystem contract could be used:

function tipAndWithdrawRest(address tipped, uint256 amount) external payable {
    tipped.call{value:amount}("");
    uint256 leftovers = address(this).balance();
    ICrossDomainMessenger(cdm).xDomainMessageSender().call{value:leftovers}("");
}

While it can be argued that such transactions will then be replayable, the problematic flow that results in a reverted transaction can be maintained indefinitely by the attacker. For example, in the example above, the tipped address can be set up to perform the reentrancy. Since the victim cannot change the calldata for their withdrawal, they will run into the same issue any time they try to withdraw, and their funds will become permanently stuck.

Impact

Any withdrawls that interacts with external addresses and then uses the xDomainMessageSender() variable can be exploited.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/9b9f78c6613c6ee53b93ca43c71bb74479f4b975/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L386-L400

Tool used

Manual Review

Recommendation

In every delivery of withdrawal, push the new sender to the top of the stack and then pop it after the call.

Duplicate of #4

sherlock-audit / 2023-03-optimism-judging

obront - Reentrancy in Cross Domain Messenger can cause permanent loss of funds #99