search

sherlock-audit / 2024-08-tokamak-network-judging

1 stars 0 forks source link

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

Open sherlock-admin2 opened 3 weeks ago

sherlock-admin2 commented 3 weeks ago

ChainPatrol

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 CrossDomainMesenger (including both L1CrossDomainMesenger and L2CrossDomainMesenger 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.

Vulnerability Detail

The CrossDomaingMesenger::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. 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 behaviour also disagrees with how the migration process works:

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

The following bytes are excluded from gas usage counting: link

Code Snippet

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:

function sendMessage(address _target, bytes calldata _message, uint32 _minGasLimit) external payable {
        if (isCustomGasToken()) {
            require(msg.value == 0, "CrossDomainMessenger: cannot send value with custom gas token");
        }

        // Triggers a message to the other messenger. Note that the amount of gas provided to the
        // 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({
            _to: address(otherMessenger),
            _gasLimit: baseGas(wrappedMessage , _minGasLimit),
            _value: msg.value,
            wrappedMessage 
        });

        emit SentMessage(_target, msg.sender, _message, messageNonce(), _minGasLimit);
        emit SentMessageExtension1(msg.sender, msg.value);

        unchecked {
            ++msgNonce;
        }
    }