Merge pull request #5508 from ethereum-optimism/clabby/ctb/xdm-min-gas-fix

fix(ctb): Remove `minGas` revert in `relayMessage`

packages/contracts-bedrock/contracts/L1/L1CrossDomainMessenger.sol

@@ -20,12 +20,12 @@ contract L1CrossDomainMessenger is CrossDomainMessenger, Semver {
     OptimismPortal public immutable PORTAL;
 
     /**
-     * @custom:semver 1.2.0
+     * @custom:semver 1.3.0
      *
      * @param _portal Address of the OptimismPortal contract on this network.
      */
     constructor(OptimismPortal _portal)
-        Semver(1, 2, 0)
+        Semver(1, 3, 0)
         CrossDomainMessenger(Predeploys.L2_CROSS_DOMAIN_MESSENGER)
     {
         PORTAL = _portal;

packages/contracts-bedrock/contracts/L2/L2CrossDomainMessenger.sol

@@ -17,12 +17,12 @@ import { L2ToL1MessagePasser } from "./L2ToL1MessagePasser.sol";
  */
 contract L2CrossDomainMessenger is CrossDomainMessenger, Semver {
     /**
-     * @custom:semver 1.2.0
+     * @custom:semver 1.3.0
      *
      * @param _l1CrossDomainMessenger Address of the L1CrossDomainMessenger contract.
      */
     constructor(address _l1CrossDomainMessenger)
-        Semver(1, 2, 0)
+        Semver(1, 3, 0)
         CrossDomainMessenger(_l1CrossDomainMessenger)
     {
         initialize();

packages/contracts-bedrock/contracts/test/invariants/CrossDomainMessenger.t.sol

@@ -7,6 +7,7 @@ import { L1CrossDomainMessenger } from "../../L1/L1CrossDomainMessenger.sol";
 import { Messenger_Initializer } from "../CommonTest.t.sol";
 import { Types } from "../../libraries/Types.sol";
 import { Predeploys } from "../../libraries/Predeploys.sol";
+import { Constants } from "../../libraries/Constants.sol";
 import { Encoding } from "../../libraries/Encoding.sol";
 import { Hashing } from "../../libraries/Hashing.sol";
 
@@ -21,38 +22,53 @@ contract RelayActor is StdUtils {
     OptimismPortal op;
     L1CrossDomainMessenger xdm;
     Vm vm;
+    bool doFail;
 
     constructor(
         OptimismPortal _op,
         L1CrossDomainMessenger _xdm,
-        Vm _vm
+        Vm _vm,
+        bool _doFail
     ) {
         op = _op;
         xdm = _xdm;
         vm = _vm;
+        doFail = _doFail;
     }
 
     /**
-     * Relays a message to the `L1CrossDomainMessenger` with a random `version`, `_minGasLimit`
-     * and `_message`.
+     * Relays a message to the `L1CrossDomainMessenger` with a random `version`, and `_message`.
      */
     function relay(
-        uint16 _version,
-        uint32 _minGasLimit,
+        uint8 _version,
+        uint8 _value,
         bytes memory _message
     ) external {
         address target = address(0x04); // ID precompile
         address sender = Predeploys.L2_CROSS_DOMAIN_MESSENGER;
 
+        // Set the minimum gas limit to the cost of the identity precompile's execution for
+        // the given message.
+        // ID Precompile cost can be determined by calculating: 15 + 3 * data_word_length
+        uint32 minGasLimit = uint32(15 + 3 * ((_message.length + 31) / 32));
+
         // set the value of op.l2Sender() to be the L2 Cross Domain Messenger.
         vm.store(address(op), bytes32(senderSlotIndex), bytes32(abi.encode(sender)));
 
-        // Restrict `_minGasLimit` to a number in the range of the block gas limit.
-        _minGasLimit = uint32(bound(_minGasLimit, 0, block.gaslimit));
-
         // Restrict version to the range of [0, 1]
         _version = _version % 2;
 
+        // Restrict the value to the range of [0, 1]
+        // This is just so we get variance of calls with and without value. The ID precompile
+        // will not reject value being sent to it.
+        _value = _value % 2;
+
+        // If the message should succeed, supply it `baseGas`. If not, supply it an amount of
+        // gas that is too low to complete the call.
+        uint256 gas = doFail
+            ? bound(minGasLimit, 60_000, 80_000)
+            : xdm.baseGas(_message, minGasLimit);
+
         // Compute the cross domain message hash and store it in `hashes`.
         // The `relayMessage` function will always encode the message as a version 1
         // message after checking that the V0 hash has not already been relayed.
@@ -60,22 +76,29 @@ contract RelayActor is StdUtils {
             Encoding.encodeVersionedNonce(0, _version),
             sender,
             target,
-            0, // value
-            _minGasLimit,
+            _value,
+            minGasLimit,
             _message
         );
+        hashes.push(_hash);
+        numHashes += 1;
+
+        // Make sure we've got a fresh message.
+        vm.assume(xdm.successfulMessages(_hash) == false && xdm.failedMessages(_hash) == false);
 
         // Act as the optimism portal and call `relayMessage` on the `L1CrossDomainMessenger` with
         // the outer min gas limit.
         vm.startPrank(address(op));
-        vm.expectCall(target, _message);
+        if (!doFail) {
+            vm.expectCallMinGas(address(0x04), _value, minGasLimit, _message);
+        }
         try
-            xdm.relayMessage{ gas: xdm.baseGas(_message, _minGasLimit) }(
+            xdm.relayMessage{ gas: gas, value: _value }(
                 Encoding.encodeVersionedNonce(0, _version),
                 sender,
                 target,
-                0, // value
-                _minGasLimit,
+                _value,
+                minGasLimit,
                 _message
             )
         {} catch {
@@ -85,34 +108,81 @@ contract RelayActor is StdUtils {
             reverted = true;
         }
         vm.stopPrank();
-
-        hashes.push(_hash);
-        numHashes += 1;
     }
 }
 
 contract XDM_MinGasLimits is Messenger_Initializer {
     RelayActor actor;
 
-    function setUp() public virtual override {
+    function init(bool doFail) public virtual {
         // Set up the `L1CrossDomainMessenger` and `OptimismPortal` contracts.
         super.setUp();
 
         // Deploy a relay actor
-        actor = new RelayActor(op, L1Messenger, vm);
+        actor = new RelayActor(op, L1Messenger, vm, doFail);
+
+        // Give the portal some ether to send to `relayMessage`
+        vm.deal(address(op), type(uint128).max);
 
         // Target the `RelayActor` contract
         targetContract(address(actor));
 
+        // Don't allow the estimation address to be the sender
+        excludeSender(Constants.ESTIMATION_ADDRESS);
+
         // Target the actor's `relay` function
         bytes4[] memory selectors = new bytes4[](1);
         selectors[0] = actor.relay.selector;
         targetSelector(FuzzSelector({ addr: address(actor), selectors: selectors }));
     }
+}
+
+contract XDM_MinGasLimits_Succeeds is XDM_MinGasLimits {
+    function setUp() public override {
+        // Don't fail
+        super.init(false);
+    }
+
+    /**
+     * @custom:invariant A call to `relayMessage` should succeed if at least the minimum gas limit
+     *                   can be supplied to the target context, there is enough gas to complete
+     *                   execution of `relayMessage` after the target context's execution is
+     *                   finished, and the target context did not revert.
+     *
+     * There are two minimum gas limits here:
+     *
+     * - The outer min gas limit is for the call from the `OptimismPortal` to the
+     * `L1CrossDomainMessenger`,  and it can be retrieved by calling the xdm's `baseGas` function
+     * with the `message` and inner limit.
+     *
+     * - The inner min gas limit is for the call from the `L1CrossDomainMessenger` to the target
+     * contract.
+     */
+    function invariant_minGasLimits() external {
+        uint256 length = actor.numHashes();
+        for (uint256 i = 0; i < length; ++i) {
+            bytes32 hash = actor.hashes(i);
+            // The message hash is set in the successfulMessages mapping
+            assertTrue(L1Messenger.successfulMessages(hash));
+            // The message hash is not set in the failedMessages mapping
+            assertFalse(L1Messenger.failedMessages(hash));
+        }
+        assertFalse(actor.reverted());
+    }
+}
+
+contract XDM_MinGasLimits_Reverts is XDM_MinGasLimits {
+    function setUp() public override {
+        // Do fail
+        super.init(true);
+    }
 
     /**
-     * @custom:invariant A call to `relayMessage` should never revert if at least the proper minimum
-     * gas limits are supplied.
+     * @custom:invariant A call to `relayMessage` should assign the message hash to the
+     *                   `failedMessages` mapping if not enough gas is supplied to forward
+     *                   `minGasLimit` to the target context or if there is not enough gas to
+     *                   complete execution of `relayMessage` after the target context's execution
+     *                   is finished.
      *
      * There are two minimum gas limits here:
      *
@@ -123,19 +193,15 @@ contract XDM_MinGasLimits is Messenger_Initializer {
      * - The inner min gas limit is for the call from the `L1CrossDomainMessenger` to the target
      * contract.
      */
-    function invariant_minGasLimits() public {
-        ///////////////////////////////////////////////////////////////////
-        //                            ~ DEV ~                            //
-        // This test is temporarily disabled, it is being fixed in #5470 //
-        ///////////////////////////////////////////////////////////////////
-        // uint256 length = actor.numHashes();
-        // for (uint256 i = 0; i < length; ++i) {
-        //     bytes32 hash = actor.hashes(i);
-        //     // the message hash is in the successfulMessages mapping
-        //     assertTrue(L1Messenger.successfulMessages(hash));
-        //     // it is not in the received messages mapping
-        //     assertFalse(L1Messenger.failedMessages(hash));
-        // }
-        // assertFalse(actor.reverted());
+    function invariant_minGasLimits() external {
+        uint256 length = actor.numHashes();
+        for (uint256 i = 0; i < length; ++i) {
+            bytes32 hash = actor.hashes(i);
+            // The message hash is not set in the successfulMessages mapping
+            assertFalse(L1Messenger.successfulMessages(hash));
+            // The message hash is set in the failedMessages mapping
+            assertTrue(L1Messenger.failedMessages(hash));
+        }
+        assertFalse(actor.reverted());
     }
 }

packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol

@@ -124,23 +124,39 @@ abstract contract CrossDomainMessenger is
     /**
      * @notice Constant overhead added to the base gas for a message.
      */
-    uint64 public constant MIN_GAS_CONSTANT_OVERHEAD = 200_000;
+    uint64 public constant RELAY_CONSTANT_OVERHEAD = 200_000;
 
     /**
      * @notice Numerator for dynamic overhead added to the base gas for a message.
      */
-    uint64 public constant MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR = 1016;
+    uint64 public constant MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR = 64;
 
     /**
      * @notice Denominator for dynamic overhead added to the base gas for a message.
      */
-    uint64 public constant MIN_GAS_DYNAMIC_OVERHEAD_DENOMINATOR = 1000;
+    uint64 public constant MIN_GAS_DYNAMIC_OVERHEAD_DENOMINATOR = 63;
 
     /**
      * @notice Extra gas added to base gas for each byte of calldata in a message.
      */
     uint64 public constant MIN_GAS_CALLDATA_OVERHEAD = 16;
 
+    /**
+     * @notice Gas reserved for performing the external call in `relayMessage`.
+     */
+    uint64 public constant RELAY_CALL_OVERHEAD = 40_000;
+
+    /**
+     * @notice Gas reserved for finalizing the execution of `relayMessage` after the safe call.
+     */
+    uint64 public constant RELAY_RESERVED_GAS = 40_000;
+
+    /**
+     * @notice Gas reserved for the execution between the `hasMinGas` check and the external
+     *         call in `relayMessage`.
+     */
+    uint64 public constant RELAY_GAS_CHECK_BUFFER = 5_000;
+
     /**
      * @notice Address of the paired CrossDomainMessenger contract on the other chain.
      */
@@ -345,17 +361,36 @@ abstract contract CrossDomainMessenger is
             "CrossDomainMessenger: message has already been relayed"
         );
 
+        // If there is not enough gas left to perform the external call and finish the execution,
+        // return early and assign the message to the failedMessages mapping.
+        // We are asserting that we have enough gas to:
+        // 1. Call the target contract (_minGasLimit + RELAY_CALL_OVERHEAD + RELAY_GAS_CHECK_BUFFER)
+        //   1.a. The RELAY_CALL_OVERHEAD is included in `hasMinGas`.
+        // 2. Finish the execution after the external call (RELAY_RESERVED_GAS).
+        //
         // If `xDomainMsgSender` is not the default L2 sender, this function
-        // is being re-entered. This marks the message as failed to allow it
-        // to be replayed.
-        if (xDomainMsgSender != Constants.DEFAULT_L2_SENDER) {
+        // is being re-entered. This marks the message as failed to allow it to be replayed.
+        if (
+            !SafeCall.hasMinGas(_minGasLimit, RELAY_RESERVED_GAS + RELAY_GAS_CHECK_BUFFER) ||
+            xDomainMsgSender != Constants.DEFAULT_L2_SENDER
+        ) {
             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;
         }
 
         xDomainMsgSender = _sender;
-        bool success = SafeCall.callWithMinGas(_target, _minGasLimit, _value, _message);
+        bool success = SafeCall.call(_target, gasleft() - RELAY_RESERVED_GAS, _value, _message);
         xDomainMsgSender = Constants.DEFAULT_L2_SENDER;
 
         if (success) {
@@ -415,17 +450,23 @@ abstract contract CrossDomainMessenger is
      * @return Amount of gas required to guarantee message receipt.
      */
     function baseGas(bytes calldata _message, uint32 _minGasLimit) public pure returns (uint64) {
-        // We peform the following math on uint64s to avoid overflow errors. Multiplying the
-        // by MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR would otherwise limit the _minGasLimit to
-        // type(uint32).max / MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR ~= 4.2m.
         return
-            // Dynamic overhead
-            ((uint64(_minGasLimit) * MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR) /
-                MIN_GAS_DYNAMIC_OVERHEAD_DENOMINATOR) +
+            // Constant overhead
+            RELAY_CONSTANT_OVERHEAD +
             // Calldata overhead
             (uint64(_message.length) * MIN_GAS_CALLDATA_OVERHEAD) +
-            // Constant overhead
-            MIN_GAS_CONSTANT_OVERHEAD;
+            // Dynamic overhead (EIP-150)
+            ((_minGasLimit * MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR) /
+                MIN_GAS_DYNAMIC_OVERHEAD_DENOMINATOR) +
+            // Gas reserved for the worst-case cost of 3/5 of the `CALL` opcode's dynamic gas
+            // factors. (Conservative)
+            RELAY_CALL_OVERHEAD +
+            // Relay reserved gas (to ensure execution of `relayMessage` completes after the
+            // subcontext finishes executing) (Conservative)
+            RELAY_RESERVED_GAS +
+            // Gas reserved for the execution between the `hasMinGas` check and the `CALL`
+            // opcode. (Conservative)
+            RELAY_GAS_CHECK_BUFFER;
     }
 
     /**

packages/contracts-bedrock/invariant-docs/CrossDomainMessenger.md

 # `CrossDomainMessenger` Invariants
 
-## A call to `relayMessage` should never revert if at least the proper minimum gas limits are supplied.
-**Test:** [`CrossDomainMessenger.t.sol#L126`](../contracts/test/invariants/CrossDomainMessenger.t.sol#L126)
+## A call to `relayMessage` should succeed if at least the minimum gas limit can be supplied to the target context, there is enough gas to complete execution of `relayMessage` after the target context's execution is finished, and the target context did not revert.
+**Test:** [`CrossDomainMessenger.t.sol#L161`](../contracts/test/invariants/CrossDomainMessenger.t.sol#L161)
+
+There are two minimum gas limits here: 
+- The outer min gas limit is for the call from the `OptimismPortal` to the `L1CrossDomainMessenger`,  and it can be retrieved by calling the xdm's `baseGas` function with the `message` and inner limit. 
+- The inner min gas limit is for the call from the `L1CrossDomainMessenger` to the target contract. 
+
+
+## A call to `relayMessage` should assign the message hash to the `failedMessages` mapping if not enough gas is supplied to forward `minGasLimit` to the target context or if there is not enough gas to complete execution of `relayMessage` after the target context's execution is finished.
+**Test:** [`CrossDomainMessenger.t.sol#L196`](../contracts/test/invariants/CrossDomainMessenger.t.sol#L196)
 
 There are two minimum gas limits here: 
 - The outer min gas limit is for the call from the `OptimismPortal` to the `L1CrossDomainMessenger`,  and it can be retrieved by calling the xdm's `baseGas` function with the `message` and inner limit. 

packages/contracts-bedrock/tasks/check-l2.ts

@@ -245,7 +245,7 @@ const check = {
     await assertSemver(
       L2CrossDomainMessenger,
       'L2CrossDomainMessenger',
-      '1.2.0'
+      '1.3.0'
     )
 
     const xDomainMessageSenderSlot = await signer.provider.getStorageAt(
@@ -274,9 +274,9 @@ const check = {
     const MIN_GAS_CALLDATA_OVERHEAD =
       await L2CrossDomainMessenger.MIN_GAS_CALLDATA_OVERHEAD()
     console.log(`  - MIN_GAS_CALLDATA_OVERHEAD: ${MIN_GAS_CALLDATA_OVERHEAD}`)
-    const MIN_GAS_CONSTANT_OVERHEAD =
-      await L2CrossDomainMessenger.MIN_GAS_CONSTANT_OVERHEAD()
-    console.log(`  - MIN_GAS_CONSTANT_OVERHEAD: ${MIN_GAS_CONSTANT_OVERHEAD}`)
+    const RELAY_CONSTANT_OVERHEAD =
+      await L2CrossDomainMessenger.RELAY_CONSTANT_OVERHEAD()
+    console.log(`  - RELAY_CONSTANT_OVERHEAD: ${RELAY_CONSTANT_OVERHEAD}`)
     const MIN_GAS_DYNAMIC_OVERHEAD_DENOMINATOR =
       await L2CrossDomainMessenger.MIN_GAS_DYNAMIC_OVERHEAD_DENOMINATOR()
     console.log(
@@ -287,6 +287,14 @@ const check = {
     console.log(
       `  - MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR: ${MIN_GAS_DYNAMIC_OVERHEAD_NUMERATOR}`
     )
+    const RELAY_CALL_OVERHEAD =
+      await L2CrossDomainMessenger.RELAY_CALL_OVERHEAD()
+    console.log(`  - RELAY_CALL_OVERHEAD: ${RELAY_CALL_OVERHEAD}`)
+    const RELAY_RESERVED_GAS = await L2CrossDomainMessenger.RELAY_RESERVED_GAS()
+    console.log(`  - RELAY_RESERVED_GAS: ${RELAY_RESERVED_GAS}`)
+    const RELAY_GAS_CHECK_BUFFER =
+      await L2CrossDomainMessenger.RELAY_GAS_CHECK_BUFFER()
+    console.log(`  - RELAY_GAS_CHECK_BUFFER: ${RELAY_GAS_CHECK_BUFFER}`)
 
     const slot = await signer.provider.getStorageAt(
       predeploys.L2CrossDomainMessenger,