search

sherlock-audit / 2023-03-optimism-judging

7 stars 0 forks source link

Koolex - Stuck funds can not be recovered although possible loss of funds is likely especially for a custom developed messengers #78

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

Koolex

medium

Stuck funds can not be recovered although possible loss of funds is likely especially for a custom developed messengers

Summary

Stuck funds can not be recovered although possible loss of funds is likely especially for a custom developed messengers.

Vulnerability Detail

When sending messages from L2 to L1, users are supposed to call L2CrossDomainMessenger.sendMessage. This would trigger a message to the other messenger (i.e. L1CrossDomainMessenger). Upon finlaization, the L1CrossDomainMessenger.relayMessage is called. The flow basically as follows:

Withdrawal on L2

flowchart TD;
        subgraph L2CrossDomainMessenger
    sendMessage;
    end
        subgraph L2ToL1MessagePasser
    initiateWithdrawal;
    end
        L2CrossDomainMessenger--L1CrossDomainMessenger.relayMessage.selector-->L2ToL1MessagePasser

Finlaize on L1

flowchart TD;
        subgraph OptimismPortal
proveWithdrawalTransaction --7 days-->finalizeWithdrawalTransaction;        
        end
        subgraph L1CrossDomainMessenger
relayMessage;       
        end
        OptimismPortal--This must never fail. Otherwise funds are lost-->L1CrossDomainMessenger

However, some protocols or bridges might develop a custom messanger. In this case, they should use L2ToL1MessagePasser directly. Let's assume L1CustomMessanger and L2CustomMessanger are custom messengers developed by a third party, the flow then would be as follows:

Withdrawal on L2

flowchart TD;
        subgraph L2CustomMessanger
    sendMessageToL1;
    end
        subgraph L2ToL1MessagePasser
    initiateWithdrawal;
    end
        L2CustomMessanger--L1CustomMessanger.relayMessageOnL1.selector-->L2ToL1MessagePasser

Finlaize on L1

flowchart TD;
        subgraph OptimismPortal
proveWithdrawalTransaction --7 days-->finalizeWithdrawalTransaction;        
        end
        subgraph L1CustomMessanger
relayMessageOnL1;       
        end
        OptimismPortal--This must never fail. Otherwise funds are lost-->L1CustomMessanger

As you know L1CustomMessanger.relayMessage must not fail under any circumstances when it is called from OptimismPortal.finalizeWithdrawalTransaction because finalizeWithdrawalTransaction is designed strictly to run only once regardless of relayMessage success or failure. Otherwise, funds will be stuck in OptimismPortal.

This put the responsibility on the potocol (i.e. L1CustomMessanger developer) to develop the method L1CustomMessanger.relayMessage in a way that never reverts if it is called by OptimismPortal. However, if the issue happens (which is a possiblity) then stuck ether can not be rescued from OptimismPortal contract. Now the bigger the withdrawal's amount, the higher the impact. Although the lost funds not caused by a bug, I believe Optimism team should have a way to rescue lost ether. This could be also used as a last resort for L1CrossDomainMessenger.relayMessage failure in case of undiscovered issue that could cause it to fail.

Impact

Code Snippet

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

Tool used

Manual Review

Recommendation

This could be easily mitigated by applying the following:

  1. In OptimismPortal.finalizeWithdrawalTransaction, if the subcall to relayMessage fails, then track the lost value (e.g. add it to a state variable).
  2. Add a method to withdraw ether from OptimismPortal with the condition not allowing to withdraw more than what was lost.

This way, in case of a withdrawal fails, Optimism team can still take action and recover it. Especially if the withdrawal is big and it is for a protocol that has high TVL.

GalloDaSballo commented 1 year ago

Counter argument is that they would need to use the Bridge

Portal has no replay protection on purpose

I think this boils down to lack of sweep which should be QA / Invalid

hrishibhat commented 1 year ago

Sponsor comment: This is out of the scope of the audit. It is up to custom messenger implementations to correctly adhere to the system design. This is essentially a form of user error.