sherlock-audit / 2023-03-optimism-judging

6 stars 0 forks source link

Koolex - Initiating withdrawals on L2 is always open (not pausable) which has a non-trivial impact #70

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Koolex

medium

Initiating withdrawals on L2 is always open (not pausable) which has a non-trivial impact

Summary

In case of emergency, OptimismPortal prove and finalize methods could be paused. However, initiating withdrawals on L2 is always open which has a non-trivial impact.

Vulnerability Detail

proveWithdrawalTransaction and finalizeWithdrawalTransaction method of OptimismPortal are now pausable. In case of an emergency, the guardian can pause both methods. However, L2CrossDomainMessenger.sendMessage is not pausable. That means, users can still initiate withdrawals on L2 even though they won't be able to finalize it on L1 via OptimismPortal. This leads to unnecessarily funds locked till the emergency situation gets cleared. Although this might seem somewhat acceptable for some users, it impacts protocols with big withdrawals. Please note that protocols might not be aware immediately of an emergency situation in Optimism. Thus, their users are still interacting with the protocol that initiate withdrawals on L2. This has a bigger impact in case the emergency period is long.

Moreever, withdrawers (users who use EOAs and not aware of OptimismPortal being paused) could have used a third party bridge if they have known that it's not possible to complete the withdrawal till the emergency situation is finished.

Impact

Withdrawals can still be initiated even though OptimismPortal's prove and finalize methods are paused for emergency. Thus, new initiated withdrawals have to wait till the emergency situation gets cleared to complete their withdrawal. This has impact on both users and protocols. However, It has a bigger impact on protocols with big withdrawals especially If the emergency period is long.

Code Snippet

function pause() external {
    require(msg.sender == GUARDIAN, "OptimismPortal: only guardian can pause");
    paused = true;
    emit Paused(msg.sender);
}

https://github.com/sherlock-audit/2023-03-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L175

Tool used

Manual Review

Recommendation

Make L2ToL1MessagePasser.initiateWithdrawal pausable. So, it could be paused when OptimismPortal's methods are paused.

hrishibhat commented 1 year ago

Sponsor comment: This is a design decision and these withdrawals are still valid, only the OptimismPortal withdrawal functions may be paused.

GalloDaSballo commented 1 year ago

Inconsistent pausing is a valid QA concern in general

We cannot conclude that this is a vulnerability though

koolexcrypto commented 1 year ago

Escalate for 10 USDC.

With all due respect, according to the end to end flow of initiating and finalizing a withdrawal described here.

5.Once the challenge period has passed, a relayer submits a withdrawal finalizing transaction to the OptimismPortal contract. The relayer doesn't need to be the same entity that initiated the withdrawal on L2. 6.The OptimismPortal contract receives the withdrawal transaction data and verifies that the withdrawal has both been proven and passed the challenge period. 7.If the requirements are not met, the call reverts. Otherwise the call is forwarded, and the hash is recorded to prevent it from being replayed.

So, if the requirements are met, the call should be forwarded.

If a user follows this end to end flow, it shouldn't be paused in the middle. However, pausing only OptimismPortal withdrawal functions is pausing the flow in the middle which makes the user's funds stuck till OptimismPortal withdrawal functions are unpaused. This also means Optimism's guardian can censor withdrawals finalization which is a valid security concern for users.

According to the Optimism protocol, it should be censorship-resistant.

Secure: This is self-evident. User’s assets are at stake. Every component of the system must be incredibly secure. Decentralizable: Optimism must be designed to avail itself of the security and censorship-resistant guarantees achieved by a decentralized system

sherlock-admin commented 1 year ago

Escalate for 10 USDC.

With all due respect, according to the end to end flow of initiating and finalizing a withdrawal described here.

5.Once the challenge period has passed, a relayer submits a withdrawal finalizing transaction to the OptimismPortal contract. The relayer doesn't need to be the same entity that initiated the withdrawal on L2. 6.The OptimismPortal contract receives the withdrawal transaction data and verifies that the withdrawal has both been proven and passed the challenge period. 7.If the requirements are not met, the call reverts. Otherwise the call is forwarded, and the hash is recorded to prevent it from being replayed.

So, if the requirements are met, the call should be forwarded.

If a user follows this end to end flow, it shouldn't be paused in the middle. However, pausing only OptimismPortal withdrawal functions is pausing the flow in the middle which makes the user's funds stuck till OptimismPortal withdrawal functions are unpaused. This also means Optimism's guardian can censor withdrawals finalization which is a valid security concern for users.

According to the Optimism protocol, it should be censorship-resistant.

Secure: This is self-evident. User’s assets are at stake. Every component of the system must be incredibly secure. Decentralizable: Optimism must be designed to avail itself of the security and censorship-resistant guarantees achieved by a decentralized system

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

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

hrishibhat commented 1 year ago

Escalation rejected

Valid low Given that this is a Spec-related inconsistency since this is a design choice & the withdrawals are still valid and Optimism Guardian is trusted. The impact of this can be considered low

sherlock-admin commented 1 year ago

Escalation rejected

Valid low Given that this is a Spec-related inconsistency since this is a design choice & the withdrawals are still valid and Optimism Guardian is trusted. The impact of this can be considered low

This issue's escalations have been rejected!

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