0xdeadbeef - finalizeWithdrawalTransaction in OptimismPortal marks the withdrawal as finalized even if the transaction fails

[github-actions]

0xdeadbeef

high

finalizeWithdrawalTransaction in OptimismPortal marks the withdrawal as finalized even if the transaction fails

Summary

Transactions from L2 to L1 are executed in OptimismPortal finalizeWithdrawalTransaction function after a 7 day validation period.

The withdrawal sent from L2 to L1 is marked as finalized even if the transaction fails. It cannot be replayed later.

Consider the following scenario:
Alice has funds some excessive on L2 and wants to deposit ETH to protocol X on L1 to gain yield.
Alice uses the more direct and gas efficient (no waste gas on relayer) L2ToL1MessagePasser to pass a message to L1 in order to deposit her funds to X protocol "depositFor" function.
After the 7 day validation period Alice (or anyone else) calls finalizeWithdrawalTransaction to finalize Alice's withdrawal and deposit into X protocol.
Unfortunately, X protocol had to do maintenance and paused deposits for a few blocks.
Because the deposit to protocol X reverted, Alice lost her funds (permanently locked in OptimisimPortal).

Alice does not have a way to replay the transaction (which will work in a few blocks)

Vulnerability Detail

finalizeWithdrawalTransaction sets finalizedWithdrawals to true if even if the call to _tx.target fails, therefore it cannot be called again with the same withdrawHash.

Replaying the transaction could be possible if the function would revert on failure or set finalizedWithdrawals[withdrawalHash] = true only if the transaction succeeded.

Impact

Loss of funds

Code Snippet

The following code snippet of finalizeWithdrawalTransaction shows:

1. The check if finalizedWithdrawals[withdrawalHash] == false`
2. The function does not revert on fail
3. marks the withdrawal as finalized before testing if it fails

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

Foundry POC:

There is already an implemented POC that demonstrates that an event is created on fail. As can be seen when executing the test, the transaction does not revert and event is created with success=false:
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/test/OptimismPortal.t.sol#L631-L650

Tool used

Manual Review

Recommendation

Consider reverting if the transaction failed or setting the finalizedWithdrawals[withdrawalHash] = true only if success==true