CrossDomainCheck `RELAY_GAS_CHECK_BUFFER` should be increased to more than 7100 gas

[code423n4]

Lines of code

https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L374-L377

Vulnerability details

Impact

CrossDomainMessenger has a added check to ensure that the _minGasLimit is enforced at the time of the check

The math is sound as it ensures that there's enough gas to ensure execution in-spite of EIP-150

During the relaying of a Message, an important additional check causes an overhead that seems to not have been considered.

The check below: https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L374-L377

        if (
            !SafeCall.hasMinGas(_minGasLimit, RELAY_RESERVED_GAS + RELAY_GAS_CHECK_BUFFER) || // Has the gas since the checks passes
            xDomainMsgSender != Constants.DEFAULT_L2_SENDER // 2.1k + minor extra overheads
        ) {

Comprises of two checks

The first one ensure that there's enough gas for relaying the TX and adds a RELAY_GAS_CHECK_BUFFER of 5000 gas

The second check is to avoid griefing and reentrant calls, that extra check will have a cost of at least 2.1k gas (Cold Slot SLOAD + some overhead)

That's not accounted for in the math

Proof Of Concept

First of all the SafeCall.call is performed using call rather than callWithMinGas meaning that if we were able to pass less gas than intended, via EIP-150, we could have a scenario of a tx reverting while being recorded as working.

This POC doesn't get to this but shows how an additional overhead makes the math slightly shaky

This check passes: as false !SafeCall.hasMinGas(_minGasLimit, RELAY_RESERVED_GAS + RELAY_GAS_CHECK_BUFFER)

When the check is performed, we have the gas to perform the check + 64/63*_minGasLimit + RELAY_RESERVED_GAS + RELAY_GAS_CHECK_BUFFER + EXTRA

The extra buffer is 40k gas and is accounting for additional costs of a CALL + a buffer of 5.7k

The RELAY_GAS_CHECK_BUFFER is 5k, meant to ensure that the processing before the call is covered.

However, the additional checks before SafeCall.call cost more than 5k

Gas Breakdown of extra costs

The following is a commented chunk of code from: https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L374-L395

        if (
            !SafeCall.hasMinGas(_minGasLimit, RELAY_RESERVED_GAS + RELAY_GAS_CHECK_BUFFER) || // Has the gas since the checks passes
            xDomainMsgSender != Constants.DEFAULT_L2_SENDER // 2.1k + minor extra overheads
        ) {
            failedMessages[versionedHash] = true;
            emit FailedRelayedMessage(versionedHash);

            // Revert in this case if the transaction was triggered by the estimation address. This
            // should only be possible during gas estimation or we have bigger problems. Reverting
            // here will make the behavior of gas estimation change such that the gas limit
            // computed will be the amount required to relay the message, even if that amount is
            // greater than the minimum gas limit specified by the user.
            if (tx.origin == Constants.ESTIMATION_ADDRESS) {
                revert("CrossDomainMessenger: failed to relay message");
            }

            return;
        } // Jump

        xDomainMsgSender = _sender; // 5k + overhead
        // Minor overhead to compute memory values for call
        bool success = SafeCall.call(_target, gasleft() - RELAY_RESERVED_GAS, _value, _message); // 3k = 46 gas
        xDomainMsgSender = Constants.DEFAULT_L2_SENDER;

Meaning that RELAY_GAS_CHECK_BUFFER should be closer to 7.1k (probably 8k to be safe), than 5k which is used in the first SSTORE

Tools used

Manual review

Recommended Mitigation Steps

Change RELAY_GAS_CHECK_BUFFER to be between 7.1k and 8k gas

Assessed type

Invalid Validation

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood marked the issue as selected for report

[anupsv]

Could you demonstrate impact with a end to end PoC

[c4-sponsor]

anupsv marked the issue as sponsor disputed

[itsmetechjay]

@GalloDaSballo please provide a PoC per the sponsor's request.

[JeeberC4]

@GalloDaSballo please provide POC within 24 hours.

[GalloDaSballo]

The buffer for hasMinGas is 3.4k gas due to it being a static 40k

And CALL having a maximum additional gas costs of: address_access_cost = 2.6k positive_value_cost = 9k value_to_empty_account_cost = 25k

Source: https://www.evm.codes/

---

The overhead in RELAY_GAS_CHECK_BUFFER is 5k, but the extra costs are closer to 7.1k+ (but definitely less than 8k)

---

This means that the code in scope will not run out of gas, nor can be exploited to burn other users tx

However, the way in which the code is safe is because hasMinGas has added an extra buffer, which is preventing the inaccurate math from having a dramatic impact

[0xleastwood]

Considering this is not directly vulnerable but definitely worth being aware of, I'll downgrade the severity to low risk.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)