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

1 stars 0 forks source link

GGONE - Revert on Insufficient Gas in CrossDomainMessenger May Lead to Irrecoverable Fund Loss #32

Open sherlock-admin3 opened 3 weeks ago

sherlock-admin3 commented 3 weeks ago

GGONE

Medium

Revert on Insufficient Gas in CrossDomainMessenger May Lead to Irrecoverable Fund Loss

Summary

Reference link

https://github.com/sherlock-audit/2023-03-optimism-judging/issues/27

The vulnerability arises in the CrossDomainMessenger contract when handling messages with insufficient gas. Specifically, when the gas provided for relaying a message is insufficient, the contract improperly uses revert instead of returning false. This design flaw prevents the message from being flagged as failed, making it impossible for the user to retry or replay the transaction, potentially leading to permanent loss of funds.

Vulnerability Detail

When relaying a message from L2 to L1, the gas estimate is calculated using the baseGas function. If external factors, like an EIP introducing new gas costs, increase the actual gas needed in L1CrossDomainMessenger.relayMessage, the function may revert due to insufficient gas. This causes the entire transaction to revert, preventing the contract from marking the message as failed and leaving the user unable to replay the transaction, resulting in potential fund loss.

Additionally, gas checks occur twice: first in OptimismPortal2.finalizeWithdrawalTransaction and then in L1CrossDomainMessenger.relayMessage. If the first check passes but the second fails, the user's withdrawal is finalized but not marked as failed, preventing them from replaying the transaction and risking fund loss.

1.Assume Alice (an honest user) intends to send a message from L2 to L1 by calling sendMessage: Link.

    function sendMessage(address _target, bytes calldata _message, uint32 _minGasLimit) external payable {

2.The required gas amount is calculated in the baseGas function: Link.

            _gasLimit: baseGas(_message, _minGasLimit),

3.Assume that over time, due to changes like the introduction of a new Elastic IP (this is just an example that can lead to changes in gas consumption), the gas costs for certain opcodes change. During this time, Alice's withdrawal transaction has not yet executed on L1.

4.Bob (an attacker) proves Alice's withdrawal transaction and, after the challenge period, calls finalizeWithdrawalTransaction to complete Alice's withdrawal: Link.

    function finalizeWithdrawalTransaction(Types.WithdrawalTransaction memory _tx) external whenNotPaused {
        finalizeWithdrawalTransactionExternalProof(_tx, msg.sender);
    }

5.Bob provides the required gas calculated by the baseGas function on L2. This amount of gas passes the check in OptimismPortal, and thus the finalizedWithdrawals mapping is set to true: Link.

        bool success = SafeCall.callWithMinGas(_tx.target, _tx.gasLimit, _tx.value, _tx.data);

Link

            if iszero(_hasMinGas) {

[Link]https://github.com/sherlock-audit/2024-08-tokamak-network/blob/main/tokamak-thanos/packages/contracts-bedrock/src/L1/OptimismPortal2.sol#L360

        finalizedWithdrawals[withdrawalHash] = true;

6.The remaining gas is forwarded to the L1CrossDomainMessenger calling relayMessage: Link.

            function relayMessage(

7.For any reason (such as the above example: the introduction of an EIP that changes gas costs), the gas consumption in relayMessage exceeds the expected amount, causing it to fail the gas condition: Link.

bool success = SafeCall.call(_target, gasleft() - RELAY_RESERVED_GAS, _value, _message);

[Link]https://github.com/sherlock-audit/2024-08-tokamak-network/blob/main/tokamak-thanos/packages/contracts-bedrock/src/libraries/SafeCall.sol#L101-L122

8.Since it does not pass the gas condition: Link.

        assembly {
            // Assertion: gasleft() >= (_minGas * 64) / 63 + 40_000
            if iszero(_hasMinGas) {
                // Store the "Error(string)" selector in scratch space.
                mstore(0, 0x08c379a0)
                // Store the pointer to the string length in scratch space.
                mstore(32, 32)
                // Store the string.
                //
                // SAFETY:
                // - We pad the beginning of the string with two zero bytes as well as the
                // length (24) to ensure that we override the free memory pointer at offset
                // 0x40. This is necessary because the free memory pointer is likely to
                // be greater than 1 byte when this function is called, but it is incredibly
                // unlikely that it will be greater than 3 bytes. As for the data within
                // 0x60, it is ensured that it is 0 due to 0x60 being the zero offset.
                // - It's fine to clobber the free memory pointer, we're reverting.
                mstore(88, 0x0000185361666543616c6c3a204e6f7420656e6f75676820676173)

                // Revert with 'Error("SafeCall: Not enough gas")'
                revert(28, 100)
            }

9.However, the revert in L1CrossDomainMessenger.relayMessage is incorrect. The entire transaction of relayMessage will be reverted, and thus it will not set the flag failedMessages[versionedHash] to true: Link.

            failedMessages[versionedHash] = true;

Due to the withdrawal transaction being marked as finalized in OptimismPortal but not as failed in L1CrossDomainMessenger, Alice is unable to replay her transaction and loses her funds.

Impact

Causing users lose fund

Code Snippet

Tool used

Manual Review

Emphasized Points

In the future, the preconditions for this vulnerability may not arise, as they depend on some future gas schedule changes. However, the assumption regarding EIPs serves as a better example for understanding the issue. In other words, this project aims to accurately estimate gas consumption when relaying messages from L2 to L1, so that the correct amount of gas can be forwarded to the target. If anything unexpected occurs in between, there are reentrancy mechanisms in place to ensure that users can retry their withdrawals. However, due to the improper use of revert, all these efforts could be undermined, and users may lose their funds.

Recommendation

Reference Link

https://solodit.xyz/issues/m-4-usage-of-revert-in-case-of-low-gas-in-l1crossdomainmessenger-can-result-in-loss-of-fund-sherlock-none-optimism-update-git

If the required gas conditions are not met, the call should return false instead of using revert: [Link]https://github.com/sherlock-audit/2024-08-tokamak-network/blob/main/tokamak-thanos/packages/contracts-bedrock/src/universal/CrossDomainMessenger.sol#L287C8-L287C97

        bool success = SafeCall.call(_target, gasleft() - RELAY_RESERVED_GAS, _value, _message);