sherlock-audit / 2023-01-optimism-judging

22 stars 8 forks source link

unforgiven - [Medium] hard-coded gas amount specified in CrossDomainMessenger and it can cause cross domain messages to revert if Ethereum gas cost changes or contracts in other chain get updated #304

Closed github-actions[bot] closed 1 year ago

github-actions[bot] commented 1 year ago

unforgiven

medium

[Medium] hard-coded gas amount specified in CrossDomainMessenger and it can cause cross domain messages to revert if Ethereum gas cost changes or contracts in other chain get updated

Summary

Function sendMessage() in CrossDomainMessanger Sends a message to some target address on the other chain and to make sure that message has enough gas that user specified contract add a constant gas to user specified gas. but as other chain gas cost schema can change or the bridge contract can be updated and gas usage can be change in the future the hard coded value won't be enough in the future and it would cause users withdrawal or deposits to fail and users would lose funds even so they are using CrossDomainMessanger.

Vulnerability Detail

This is sendMessage() code:

    /**
     * @notice Sends a message to some target address on the other chain. Note that if the call
     *         always reverts, then the message will be unrelayable, and any ETH sent will be
     *         permanently locked. The same will occur if the target on the other chain is
     *         considered unsafe (see the _isUnsafeTarget() function).
     *
     * @param _target      Target contract or wallet address.
     * @param _message     Message to trigger the target address with.
     * @param _minGasLimit Minimum gas limit that the message can be executed with.
     */
    function sendMessage(
        address _target,
        bytes calldata _message,
        uint32 _minGasLimit
    ) external payable {
        // 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.
        _sendMessage(
            OTHER_MESSENGER,
            baseGas(_message, _minGasLimit),
            msg.value,
            abi.encodeWithSelector(
                this.relayMessage.selector,
                messageNonce(),
                msg.sender,
                _target,
                msg.value,
                _minGasLimit,
                _message
            )
        );

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

        unchecked {
            ++msgNonce;
        }
    }

As you can see to specify the minimum gas limit for other chain transaction code use function baseGas(_message, _minGasLimit) and this function code is:

    /**
     * @notice Computes the amount of gas required to guarantee that a given message will be
     *         received on the other chain without running out of gas. Guaranteeing that a message
     *         will not run out of gas is important because this ensures that a message can always
     *         be replayed on the other chain if it fails to execute completely.
     *
     * @param _message     Message to compute the amount of required gas for.
     * @param _minGasLimit Minimum desired gas limit when message goes to target.
     *
     * @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) +
            // Calldata overhead
            (uint64(_message.length) * MIN_GAS_CALLDATA_OVERHEAD) +
            // Constant overhead
            MIN_GAS_CONSTANT_OVERHEAD;
    }

to make sure that transaction in the other chain have enough gas to process bridge contracts code, MIN_GAS_CONSTANT_OVERHEAD is added to required gas in the other chain. but this value is constant and this can cause issue in the future where:

  1. Ethereum gas cost schema has changed.
  2. The bridge contracts in other chain(Like OptimisimPortal or CrossDomainMessanger) has been updated and use more gas. and in those cases the message in other chain would fail because of the low gas and message won't be received by other chain CroosDomainMessanger.

Impact

withdraw or deposit messages between CrossDomainMessangers would be broken and users funds would be lost.

Code Snippet

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L372-L395

https://github.com/ethereum-optimism/optimism/blob/3f4b3c328153a8aa03611158b6984d624b17c1d9/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L202-L234

Tool used

Manual Review

Recommendation

give admin access to change increase the MIN_GAS_CONSTANT_OVERHEAD.

rcstanciu commented 1 year ago

Comment from Optimism


Description: hard-coded gas amount specified in CrossDomainMessenger

Reason: This is untrue and the current implementation is a design decision. The minGasLimit specified by the user is considered for a remote tx gasleft() is provided to the subsequent call.