Jeiwan - Gas usage of cross-chain messages is undercounted, causing discrepancy between L1 and L2 and impacting intrinsic gas calculation

[sherlock-admin]

Jeiwan

medium

Gas usage of cross-chain messages is undercounted, causing discrepancy between L1 and L2 and impacting intrinsic gas calculation

Summary

Gas consumption of messages sent via CrossDomainMessenger (including both L1CrossDomainMessenger and L2CrossDomainMessenger) is calculated incorrectly: the gas usage of the "relayMessage" wrapper is not counted. As a result, the actual gas consumption of sending a message will be higher than expected. Users will pay less for gas on L1, and L2 blocks may be filled earlier than expected. This will also affect gas metering via ResourceMetering: metered gas will be lower than actual consumed gas, and the EIP-1559-like gas pricing mechanism won't reflect the actual demand for gas.

Vulnerability Detail

The CrossDomainMessenger.sendMessage function is used to send cross-chain messages. Users are required to set the _minGasLimit argument, which is the expected amount of gas that the message will consume on the other chain. The function also computes the amount of gas required to pass the message to the other chain: this is done in the baseGas function, which computes the byte-wise cost of the message. CrossDomainMessenger also allows users to replay their messages on the destination chain if they failed: to allow this, the contract wraps user messages in relayMessage calls. This increases the size of messages, but the baseGas call above counts gas usage of only the original, not wrapped in the relayMessage call, message.

This contradicts the intrinsic gas calculation in op-geth, which calculates gas of an entire message data:

dataLen := uint64(len(data))
// Bump the required gas by the amount of transactional data
if dataLen > 0 {
    ...
}

Thus, there's a discrepancy between the contract and the node, which will result in the node consuming more gas than users paid for.

This behaviour also disagrees with how the migration process works:

1. when migrating pre-Bedrock withdrawals, data is the entire messages, including the relayMessage calldata;
2. the gas limit of migrated messages is computed on the entire data.

Taking into account the logic of paying cross-chain messages' gas consumption on L1, I think the implementation in the migration code is correct and the implementation in CrossDomainMessenger is wrong: users should pay for sending the entire cross-chain message, not just the calldata that will be execute on the recipient on the other chain.

Impact

Since the CrossDomainMessenger contract is recommended to be used as the main cross-chain messaging contract and since it's used by both L1 and L2 bridges (when bridging ETH or ERC20 tokens), the undercounted gas will have a broad impact on the system. It'll create a discrepancy in gas usage and payment on L1 and L2: on L1, users will pay for less gas than actually will be consumed by cross-chain messages.

Also, since messages sent from L1 to L2 (via OptimismPortal.depositTransaction) are priced using an EIP-1559-like mechanism (via ResourceMetering._metered), the mechanism will fail to detect the actual demand for gas and will generally set lower gas prices, while actual gas consumption will be higher.

The following bytes are excluded from gas usage counting:

1. the 4 bytes of the relayMessage selector;
2. the 32 bytes of the message nonce;
3. the address of the sender (20 bytes);
4. the address of the recipient (20 bytes);
5. the amount of ETH sent with the message (32 bytes);
6. the minimal gas limit of the nested message (32 bytes).

Thus, every cross-chain message sent via the bridge or the messenger will contain 140 bytes that won't be paid by users. The bytes will however be processed by the node and accounted in the gas consumption.

Code Snippet

1. CrossDomainMessenger.sendMessage sends cross-chain messages: https://github.com/ethereum-optimism/optimism/blob/9b9f78c6613c6ee53b93ca43c71bb74479f4b975/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L247
2. CrossDomainMessenger.sendMessage wraps cross-chain messages in relayMessage calls: https://github.com/ethereum-optimism/optimism/blob/9b9f78c6613c6ee53b93ca43c71bb74479f4b975/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L260-L268
3. The gas limit counting of cross-chain messages includes only the length of the nested message and doesn't include the relayMessage wrapping: https://github.com/ethereum-optimism/optimism/blob/9b9f78c6613c6ee53b93ca43c71bb74479f4b975/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L258

4. When pre-Bedrock withdrawals are migrated, gas limit calculation does include the relayMessage wrapping: https://github.com/ethereum-optimism/optimism/blob/9b9f78c6613c6ee53b93ca43c71bb74479f4b975/op-chain-ops/crossdomain/migrate.go#L73-L86

   Tool used

   Manual Review

   Recommendation

   When counting gas limit in the CrossDomainMessenger.sendMessage function, consider counting the entire message, including the relayMessage calldata wrapping. Consider a change like that:

   diff --git a/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol b/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol
   index f67021010..5239feefd 100644
   --- a/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol
   +++ b/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol
   @@ -253,19 +253,20 @@ abstract contract CrossDomainMessenger is
        // message is the amount of gas requested by the user PLUS the base gas value. We want to
        // guarantee the property that the call to the target contract will always have at least
        // the minimum gas limit specified by the user.
   +        bytes memory wrappedMessage = abi.encodeWithSelector(
   +            this.relayMessage.selector,
   +            messageNonce(),
   +            msg.sender,
   +            _target,
   +            msg.value,
   +            _minGasLimit,
   +            _message
   +        );
        _sendMessage(
            OTHER_MESSENGER,
   -            baseGas(_message, _minGasLimit),
   +            baseGas(wrappedMessage, _minGasLimit),
            msg.value,
   -            abi.encodeWithSelector(
   -                this.relayMessage.selector,
   -                messageNonce(),
   -                msg.sender,
   -                _target,
   -                msg.value,
   -                _minGasLimit,
   -                _message
   -            )
   +            wrappedMessage
        );
   
        emit SentMessage(_target, msg.sender, _message, messageNonce(), _minGasLimit);

[hrishibhat]

Sponsor comment: This should improve gas estimation but is low in severity since it does not affect usage or impact the intended functionality.

[GalloDaSballo]

Would judge in the same way as #77 the incorrect math can lead to an issue

[GalloDaSballo]

I see this as logically equivalent to #77 so I believe it should be awarded as Med

[GalloDaSballo]

This can also be quantified as 140 * 16 = 2240 All l1 -> l2 tx are underpriced by that amount (roughly 10% of fixed base cost)

[GalloDaSballo]

Can see this being escalated against because it's "only" 10% incorrect, but find hard to argue against the math not being correct