Koolex - `finalizeWithdrawalTransaction` transaction will not be processed if the minimum gas is set too high

[sherlock-admin]

Koolex

high

finalizeWithdrawalTransaction transaction will not be processed if the minimum gas is set too high

Summary

if the minimum gas is set too high then finalizeWithdrawalTransaction transaction will not be processed. Eventually, causing loss of funds for withdrawers.

Vulnerability Detail

In the new Optimism update, callWithMinGas was introduced to make sure the minimum gas limit specified by the user is guaranteed. Basically the following is checked

gasleft() >= ((_minGas + 200) * 64) / 63

Please note that 200 is added to minimum gas limit. However, 51 will be used between GAS opcode and CALLopcode For a clearer picture, check this link Safe Call with minimum gas

Let's deduct 51 from 200, we have 149 gas. This means: minimum gas limit + 149 is guaranteed to be passed to the sub-call L1CrossDomainMessenger.relayMessage. However, there is no check for the maximum. Therefore, if the gas provided is too high exceeding the block gas limit then finalizeWithdrawalTransaction transaction will not be processed. Eventually, causing loss of funds since finalizeWithdrawalTransaction is reverting if you provide less gas.

Imagine the following scenario:

1. A DeFi protocol initiated a withdrawal on L2
2. The withdrawal has gasLimit set to 30M (which is the block gas limit on Ethereum. Therefore, the protocol assumes that it should work)
3. Now according to callWithMinGas, the relayer has to provide gas as follows:

   gas = ((_minGas + 200) * 64) / 63
   gas = ((30M + 200) * 64) / 63
   gas = 1920012800 / 63
   gas = 30476393

As noticed, the gas provided already exceeded the block gas limit. So it won't be processed. if the relayer provides less than that, the finalizeWithdrawalTransaction method will revert.

Please note that this calculation is just right before callWithMinGas call. We still need to count the gas used before and after it. So it even gets bigger than 30476393.

Impact

Withdrawals with too high gas limit can not be finalized. causing loss of funds for the withdrawer.

Code Snippet

    if lt(gas(), div(mul(64, add(_minGas, 200)), 63)) {
        // Store the "Error(string)" selector in scratch space.
        mstore(0, 0x08c379a0)
        // Store the pointer to the string length in scratch space.
        mstore(32, 32)
        // Store the string.
        //
        // SAFETY:
        // - We pad the beginning of the string with two zero bytes as well as the
        // length (24) to ensure that we override the free memory pointer at offset
        // 0x40. This is necessary because the free memory pointer is likely to
        // be greater than 1 byte when this function is called, but it is incredibly
        // unlikely that it will be greater than 3 bytes. As for the data within
        // 0x60, it is ensured that it is 0 due to 0x60 being the zero offset.
        // - It's fine to clobber the free memory pointer, we're reverting.
        mstore(88, 0x0000185361666543616c6c3a204e6f7420656e6f75676820676173)

        // Revert with 'Error("SafeCall: Not enough gas")'
        revert(28, 100)
    }

https://github.com/sherlock-audit/2023-03-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/libraries/SafeCall.sol#L64

Tool used

Manual Review

Recommendation

1. Before ensuring that

   gasleft() >= ((_minGas + 200) * 64) / 63

   check if _minGas is too high then set it for example to 29M as a max.

2. You could also apply the following restriction on L2.
   • On withdrawal initiation, don't allow gaslimit to be more than 29M for example. Make it configurable for future in case block gas limit changes on Ethereum.

[GalloDaSballo]

See #85

[hrishibhat]

Sponsor comment: This report is true, but it is a duplicate of #115 from the first challenge and not a concern of the team. To avoid confusion and inform users of the contracts, we have a ticket open to document this caveat. In addition, there was already a known maximum value for the minimum gas limit that is < the L1 block gas limit due to the gas consumed around the external call performed by the OptimismPortal or XDM.

[GalloDaSballo]

https://github.com/sherlock-audit/2023-01-optimism-judging/issues/115

[GalloDaSballo]

User Mistake, agree with Low