obront - When messenger is paused, all matured withdrawals can be forever invalidated

[github-actions[bot]]

obront

high

When messenger is paused, all matured withdrawals can be forever invalidated

Summary

In the Bedrock withdrawal implementation, withdrawals mature 7 days after submission. At this point they can be delivered using OptimismPortal's finalizeWithdrawalTransaction() by anyone, as long as they provide enough gas.

Using the Cross Domain Messenger guarantees replayability of transactions. However, if the L1CrossDomainMessenger is paused when finalizeWithdrawalTransaction() is called, the transaction will revert in a way that is non-replayable and the funds will be locked permanently.

Vulnerability Detail

The Cross Domain Messenger's relayMessage() function has the whenNotPaused modifier, which will revert when paused. Since OptimismPortal has no replayability, the call will fail and the withdrawal would be lost forever.

bool success = SafeCall.call(
    _tx.target,
    gasleft() - FINALIZE_GAS_BUFFER,
    _tx.value,
    _tx.data
);
// Reset the l2Sender back to the default value.
l2Sender = Constants.DEFAULT_L2_SENDER;
// All withdrawals are immediately finalized. Replayability can
// be achieved through contracts built on top of this contract
emit WithdrawalFinalized(withdrawalHash, success);
// Reverting here is useful for determining the exact gas cost to successfully execute the
// sub call to the target contract if the minimum gas limit specified by the user would not
// be sufficient to execute the sub call.
if (success == false && tx.origin == Constants.ESTIMATION_ADDRESS) {
    revert("OptimismPortal: withdrawal failed");
}

The impact is extremely severe, because finalizeWithdrawalTransaction() can be called by anyone. The moment the messenger is paused, a malicious user, such as an Optimism competitor, could finalize all matured withdrawals, leading to massive user losses.

For this reason, severity of High is definitely justified, whereas if finalize is only callable by user, Medium severity would be accurate.

Impact

When the Cross Domain Messenger is paused, all matured withdrawals can be forever invalidated by any user.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/407f97b9d13448b766624995ec824d3059d4d4f6/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L324

Tool used

Manual Review

Recommendation

Add protection logic in finalizeWithdrawalTransaction(). If call target is the messenger and the messenger is paused, revert the transaction.

[rcstanciu]

Comment from Optimism

---

Description: When messenger is paused, all matured withdrawals can be forever invalidated

Reason: This issue was listed as #3 under the "Known issues" section of the contest.

[zobront]

Escalate for 250 USDC

This report should be marked as a duplicate of #57.

We have two issues with the judging for this.

First, it appears that the following Known Issue was a part of the repo:

3.If the L1CrossDomainMessenger is paused, withdrawals sent to it will fail and not be replayable.

However, we pulled the repo locally on the first day of the contest, and this issue was not there. It appears it was added at a later date, with no communication to inform us not to include it.

Secondly, the accepted issues seemed to be accepted because of the clarification that this same vulnerability exists L1 => L2 as L2 => L1.

However, if you review our report, you will see that there is no mention of "direction" because it was clear that both directions were equivalent and vulnerable to the same issue.

Our report seems to have been rejected simply because we wrote an issue about L1 AND L2, instead of just L2 — but this came only from the fact that we were not notified to not include L1.

In either case, our report addresses the L2 issue covered in #57 and should be included.

[sherlock-admin]

Escalate for 250 USDC

This report should be marked as a duplicate of #57.

We have two issues with the judging for this.

First, it appears that the following Known Issue was a part of the repo:

3.If the L1CrossDomainMessenger is paused, withdrawals sent to it will fail and not be replayable.

However, we pulled the repo locally on the first day of the contest, and this issue was not there. It appears it was added at a later date, with no communication to inform us not to include it.

Secondly, the accepted issues seemed to be accepted because of the clarification that this same vulnerability exists L1 => L2 as L2 => L1.

However, if you review our report, you will see that there is no mention of "direction" because it was clear that both directions were equivalent and vulnerable to the same issue.

Our report seems to have been rejected simply because we wrote an issue about L1 AND L2, instead of just L2 — but this came only from the fact that we were not notified to not include L1.

In either case, our report addresses the L2 issue covered in #57 and should be included.

You've created a valid escalation for 250 USDC!

To remove the escalation from consideration: Delete your comment. To change the amount you've staked on this escalation: Edit your comment (do not create a new comment).

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[Evert0x]

Escalation rejected as there is no specific mention of L2CrossDomainMessenger being paused, the issue is describing the effect on L1CrossDomainMessenger which is a separate contract and was labeled as a known issue.

[sherlock-admin]

Escalation rejected as there is no specific mention of L2CrossDomainMessenger being paused, the issue is describing the effect on L1CrossDomainMessenger which is a separate contract and was labeled as a known issue.

This issue's escalations have been rejected!

Watsons who escalated this issue will have their escalation amount deducted from their next payout.