Allarious - relayMessage may call the target functions with less gas than was anticipated by its sender

[github-actions]

Allarious

low

relayMessage may call the target functions with less gas than was anticipated by its sender

Summary

The relayMessage inside CrossDomainMessenger does not check the minGasLimit specified by the user correctly and can call the function with less gas than specified by the user.

Vulnerability Detail

Security check for _minGasLimit inside the relaymessage() inside the CrossDomainMessenger contract is implemented such as:

require(
            gasleft() >= _minGasLimit + RELAY_GAS_REQUIRED,
            "CrossDomainMessenger: insufficient gas to relay message"
        );

However, while the baseGas used by the sendMessage function does calculate the 1/64 of the gas that is reserved through internal calls according to EIP-150, is not considered by the implemented security check by relayMessage. It should be mentioned that the user has to pay for it on the other chain, where baseGas calculates this amount.

Related LoCs:
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L318-L321
https://github.com/sherlock-audit/2023-01-optimism/blob/main/optimism/packages/contracts-bedrock/contracts/L1/OptimismPortal.sol#L313-L316

Impact

While a user can provide the correct amount of _minGasLimit to the protocol, the target function can be called with less gas and can fail by running out of gas. The transaction should then be replayed later with a greater gas value as it gets stored in failedMessages.

This issue is highlighted when the amount of _minGasLimit is increased. If it is more than RELAY_GAS_REQUIRED * 64 = 45000 * 64 = 2880000, the problem will show itself. In the below PoC, target function is called with more than 6000 gas less than _minGasLimit, this problem increases with the amount of _minGasLimit.

Code Snippet

// SPDX-License-Identifier: MIT
pragma solidity 0.8.15;

import "../contracts/universal/CrossDomainMessenger.sol";

import "forge-std/Test.sol";

contract PoC is Test {

    CrossDomainMessenger cdm;
    TargetContract target;
    uint256 minGas = 3000000;

    function setUp() public {
        cdm = new CDM();
        target = new TargetContract(minGas);
    }

    function testMinGas() public {
        console.log("Starting the test");

        cdm.relayMessage{gas: 3065000}(
            cdm.messageNonce(), // It is ok to use the same messanger nonce here, does not matter for our case
            address(1),
            address(target),
            0,
            minGas,
            abi.encodeWithSignature("targetFunction()")
        );
    }

}

contract CDM is CrossDomainMessenger {
    constructor() CrossDomainMessenger(address(0)) initializer {
        __CrossDomainMessenger_init(); // We do not care if it is behind a proxy for this test!
    }
    function _sendMessage(
        address _to,
        uint64 _gasLimit,
        uint256 _value,
        bytes memory _data
    ) internal override {}

    function _isOtherMessenger() internal view override returns (bool){ 
        return true;
    }
    function _isUnsafeTarget(address _target) internal view override returns (bool){
        return false;
    }
}

contract TargetContract {
    uint256 immutable public minGas;
    constructor (uint256 _minGas) {
        minGas = _minGas;
    }
    function targetFunction() public returns(uint256 gasLeft){
        gasLeft = gasleft();
        console.log(gasLeft); // 2993969, it is less than 3000000 specified for the function call!
        return gasLeft;
    }
}

Tool used

Manual Review

Recommendation

The mitigation is as easy as checking the amount of gasleft() to be more than 64/63 of the current checked value for _minGasLimit:

@@ -315,15 +317,22
         require(
-            gasleft() >= _minGasLimit + RELAY_GAS_REQUIRED,
+            gasleft() >= ((_minGasLimit) * 64 / 63) + RELAY_GAS_REQUIRED,

Where the final check should look like:

        require(
            gasleft() >= ((_minGasLimit) * 64 / 63) + RELAY_GAS_REQUIRED,
            "CrossDomainMessenger: insufficient gas to relay message"
        );

Duplicate of #96

[rcstanciu]

Comment from Optimism

---

Dupe of 096