joshuajee - Return status from call in the `finalizeWithdrawalTransactionExternalProof` is not properly checked meaning the `call` can fail, without being reverted

[sherlock-admin2]

joshuajee

medium

Return status from call in the finalizeWithdrawalTransactionExternalProof is not properly checked meaning the call can fail, without being reverted

Summary

In solidity the call operation returns a bool and a byte of data, the bool is true when the transaction is successful and false when it is not.

Vulnerability Detail

In the OptimismPortal2.sol:finalizeWithdrawalTransactionExternalProof function a low level call operation was made bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, _tx.data);, this low level call returns the transaction status, for a successful transaction bool success would be true and for a failed transaction it would be false.

    function finalizeWithdrawalTransactionExternalProof(
        Types.WithdrawalTransaction memory _tx,
        address _proofSubmitter
    )
        public
        whenNotPaused
    {

        ...
        // Mark the withdrawal as finalized so it can't be replayed.
@->     finalizedWithdrawals[withdrawalHash] = true;

        // Set the l2Sender so contracts know who triggered this withdrawal on L2.
        l2Sender = _tx.sender;

        // Trigger the call to the target contract. We use a custom low level method
        // SafeCall.callWithMinGas to ensure two key properties
        //   1. Target contracts cannot force this call to run out of gas by returning a very large
        //      amount of data (and this is OK because we don't care about the returndata here).
        //   2. The amount of gas provided to the execution context of the target is at least the
        //      gas limit specified by the user. If there is not enough gas in the current context
        //      to accomplish this, `callWithMinGas` will revert.
@->     bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _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 && tx.origin == Constants.ESTIMATION_ADDRESS) {
            revert("OptimismPortal: withdrawal failed");
        }
    }

The issue here is that the transaction status is not adequately checked. The if condition that checks for it requires that the transaction should fail and the transaction should originate from a specified address. So if the transaction failed and tx.origin != Constants.ESTIMATION_ADDRESS the transaction won't revert.

   if (!success && tx.origin == Constants.ESTIMATION_ADDRESS) {
            revert("OptimismPortal: withdrawal failed");
    }

Impact

Loss of funds.

Code Snippet

https://github.com/sherlock-audit/2024-02-optimism-2024/blob/main/optimism/packages/contracts-bedrock/src/L1/OptimismPortal2.sol#L3a81

Tool used

Manual Analysis

Recommendation

Check if the call was successful

-   if (!success && tx.origin == Constants.ESTIMATION_ADDRESS) {
+   if (!success) {
        revert("OptimismPortal: withdrawal failed");
    }

Duplicate of #145