Koolex - Possible loss of funds if the minimum gas limit is set too high on deposit

[sherlock-admin]

Koolex

high

Possible loss of funds if the minimum gas limit is set too high on deposit

Summary

if the minimum gas is set too high on deposit then L2CrossDomainMessenger.relayMessage transaction will not be processed. Eventually, causing loss of funds for depositors.

Vulnerability Detail

in L2CrossDomainMessenger.relayMessage method, callWithMinGas was introduced to make sure the minimum gas limit specified by the user is guaranteed.
Basically the following is checked at Safe Call with minimum gas

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

As there is no check for the maximum gas limit in depositTransaction, if the minimum gas limit provided is too high exceeding the L2 block gas limit then L2CrossDomainMessenger.relayMessage transaction will not be processed. Eventually, causing loss of funds since relayMessage is reverting if you provided gas is less.

A possible scenario:

1. A protocol initiated a deposit on L1.
2. The protocol checks for L2 block gas limit by reading gasLimit from SystemConfig contract. Let's say it's 40M.
3. The protocol set the minimum gas limit for the deposit to 40M (The protcol assumes it should work since it didn't exceed the L2 block gas limit).
4. Now according to callWithMinGas, the relayer has to provide gas as follows:

gas = ((_minGas + 200) * 64) / 63
gas = ((40M + 200) * 64) / 63
gas = 2560012800 / 63
gas = 40635123

The gas provided already exceeded the block gas limit 40M. So it won't be processed. if the relayer provides less than that, the L2CrossDomainMessenger.relayMessage will revert. Thus, resulting in loss of funds.

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 40635123.

Impact

Deposits with too high L2 gas limit can not be relayed on L2. causing loss of funds for the depositors.

Code Snippet

• from relayMessage method

	xDomainMsgSender = _sender;
	bool success = SafeCall.callWithMinGas(_target, _minGasLimit, _value, _message);
	xDomainMsgSender = Constants.DEFAULT_L2_SENDER;

https://github.com/sherlock-audit/2023-03-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L360-L362

• from SafeCall.callWithMinGas method

	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

On depositTransaction, check if the gaslimit is too high (e.g. gasLimit > SystemConfig.gasLimit-1M) then revert.

[GalloDaSballo]

Maybe looked as:

• Cannot handle tx with too high gas (perhaps Med)

Or

• User can set a tx that will always revert (QA as self-caused)

Interested in sponsor thoughts, also see: #71

[hrishibhat]

Sponsor comment:
While this is a valid concern, this issue can only be caused by user error. By setting a minimum gas limit that is higher than an achievable value, the user is effectively locking their funds in the contract due to the fact that it is impossible to supply enough gas to satisfy the check.

[GalloDaSballo]

User Mistake, agree with Low

[koolexcrypto]

Escalate for 10 USDC.

I agree that an issue caused by a user error is low. However, this issue above is not caused by a user error. Protocols that have advanced on-chain computation such as Jumbo or dHEDGE platform need to set the gas limit to the block gas limit (or close to it) to ensure the transactions succeed.
When such a protocol sets the gas limit to OP block gas limit 30M (which what will be in production) or even lower (e.g. 29.6M), the transaction will not be processed. Since the protocols are adhering to OP block gas limit, it is not an error caused by them.

Have a look at this discussion where Jumbo transactions failed when the block gas limit was 15M. This is just to show that some protocols might need to use up to the block gas limit.

[sherlock-admin]

Escalate for 10 USDC.

I agree that an issue caused by a user error is low. However, this issue above is not caused by a user error. Protocols that have advanced on-chain computation such as Jumbo or dHEDGE platform need to set the gas limit to the block gas limit (or close to it) to ensure the transactions succeed.
When such a protocol sets the gas limit to OP block gas limit 30M (which what will be in production) or even lower (e.g. 29.6M), the transaction will not be processed. Since the protocols are adhering to OP block gas limit, it is not an error caused by them.

Have a look at this discussion where Jumbo transactions failed when the block gas limit was 15M. This is just to show that some protocols might need to use up to the block gas limit.

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]

Escalation rejected

Valid low
The original observation on the issue stands:

By setting a minimum gas limit that is higher than an achievable value, the user is effectively locking their funds in the contract due to the fact that it is impossible to supply enough gas to satisfy the check.

[sherlock-admin]

Escalation rejected

Valid low
The original observation on the issue stands:

By setting a minimum gas limit that is higher than an achievable value, the user is effectively locking their funds in the contract due to the fact that it is impossible to supply enough gas to satisfy the check.

This issue's escalations have been rejected!

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