search

sherlock-audit / 2023-05-ecoprotocol-judging

1 stars 0 forks source link

stopthecap - STOPTHECAP - Loss of user funds due to _gasLimit set to zero on L1ECOBridge #103

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

stopthecap

high

STOPTHECAP - Loss of user funds due to _gasLimit set to zero on L1ECOBridge

Summary

Loss of user funds due to _gasLimit set to zero on L1ECOBridge

Vulnerability Detail

In L1ECOBridge.finalizeERC20Withdrawal, there exists an issue related to the gas limit setting in case of a withdraw refund.

The function L1ECOBridge.finalizeERC20Withdrawal is designed to finalize an ERC20 token withdrawal from L2 to L1. In the event of a failed transfer, it attempts to create a return transaction to refund the user on L2.

However, in case the transfer reverts or returns false, it will attempt to call sendCrossDomainMessage with _gasLimit set to 0. Depending on the size of _data, the gas cost on L2 can prevent the refund from happening, leading to potential user loss of funds.

As the Optimism documentation states (1, 2, 3):

In order to prevent the Optimism network from being DOSed via forced L1 to L2 transactions that bypass the Sequencer, a fee adjustment schedule to all L1→L2 transactions that closely mimics EIP1559 is included with Bedrock. Like in the current network, deposit fees are paid by burning some amount of L1 gas proportional to your deposit's L2 gas limit. Unfortunately, this means that you may have cases where you estimate how much gas an L1→L2 deposit will cost, and deposit fees increase by the time your transaction gets included in a block and executed, causing your deposit to run out of gas and revert. This is why we recommend adding a 50% buffer to your gasLimit to ensure your deposit will not run out of gas.

The majority of the cost of an L1 to L2 transaction comes from sending a transaction on Ethereum. You send a transaction to the L1 CrossDomainMessenger contract, which then sends a call to the CanonicalTransactionChain. This cost is ultimately determined by gas prices on Ethereum when you're sending the cross-chain transaction. An L1 to L2 message is expected to trigger contract execution on L2, and that contract execution costs gas. The first 1.92 million gas on L2 is free. The vast majority of L1 to L2 calls spend less than the 1.92 million, so nothing further is required.

The process of sending a transaction on Optimism is identical to the process of sending a transaction on Ethereum. When sending a transaction, you should provide a gas price greater than or equal to the current L2 gas price. Like on Ethereum, you can query this gas price with the eth_gasPrice RPC method. Similarly, you should set your transaction gas limit in the same way that you would set your transaction gas limit on Ethereum (e.g. via eth_estimateGas).

Impact

User loss of funds in the event a withdraw fails from L2 to L1

Code Snippet

https://github.com/sherlock-audit/2023-05-ecoprotocol/blob/main/op-eco/contracts/bridge/L1ECOBridge.sol#L280

Tool used

Manual Review

Recommendation

Correctly estimate the gas limit for the refund and add a 50% buffer to the estimated gas limit returned by estimateGas to ensure that your transaction will not run out of gas.

ctf-sec commented 1 year ago

It is difficult to set accurate gas limit on l2 for sure

but l2gas should not be set to 0

given that the L1 finalizeERC20Withdrawal normal transfer only revert in edge case such as when token is paused and

sendCrossDomainMessage(l2TokenBridge, 0, message);

only triggers in edge case

a medium severity is well deserved!

albertnbrown commented 1 year ago

Yeah, I was chatting a bit with our Optimism liaison on best ways to calculate this. This was a weird one because it came from mirroring a code pattern from pre-bedrock contracts, but this functionality worked on Goerli. This is definitely a real issue, though.

shahrukhtrao commented 1 year ago

This issue has been fixed in this PR: https://github.com/eco-association/op-eco/pull/39

sherlock-admin commented 1 year ago

Escalate for 10 USDC I don't think this is a valid medium because optimism-bedrock has built-in baseGas protection to ensure messages have enough gas to be replayed on the other chain. Even though '0' is input in sendCrossDomainMessage, '0' will be overwritten and the transaction will still succeed.

In L1ECOBridge.sol sendCrossDomainMessage() calls sendMessage() on CrossDomainMessenger.sol. This will pass baseGas(_message,_minGasLimit) to _sendMessage(). In baseGas, even if _minGasLimit is 0, it does a calculation of the required based gas based on the size of _message, plus adding a buffer MIN_GAS_CONSTANT_OVERHEAD which is 200000. 

//CrossDomainMessenger.sol
    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
            )
        );
//CrossDomainMessenger.sol
    /**
     * @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;
    }

Because baseGas() calculated the gas based on the size of the message relayed, plus a buffer for overhead, this should ensure L1ECOBridge's finalizeDeposit() message succeeds. When relayMessage() on CrossDomainMessenger.sol on L2 is called, the overwritten gas value would be passed instead of '0'.

See a test as POC. 

  L1ECOBridgeWithdraw
    L1WithdrawSucceed
L1ERC20.address 0x5FbDB2315678afecb367f032d93F642f64180aa3
optimismPortal.address 0xe7f1725E7734CE288F8367e1Bb143E90bb3F0512
Fake_SystemConfig.address 0x90F79bf6EB2c4f870365E785982E1f101E93b906
l1CrossDomainMessenger.address 0x9fE46736679d2D9a65F0992F2272dE9f3c7fa6e0
L1ERC20Bridge.address 0xCf7Ed3AccA5a467e9e704C703E8D87F634fB0Fc9
      ✔ should succeed when FinalizedWithdrawal is called (89ms)

  1 passing (5s)

In all, the transaction relay will pass even with 0 hardcoded gas limit. And the size of the message is already taken into account in CrossDomainMessenger.sol, which is contrary to the claim in the issue citing concerns with the size of the data and the buffer unaccounted.

You've deleted an escalation for this issue.
albertnbrown commented 1 year ago

This escalation is a good assessment of the technical side of Optimism's. However, the team is still worried that the baseGas protection could some time in the future be changed or the bridge might change to surpass it. Optimism's support of this issue in their documentation is lacking and it is not difficult to see that it might change.

albertnbrown commented 1 year ago

In general, though, we acknowledged this issue because we hadn't looked into the issue and it pointed out an oversight from our side.

0xdeadbeef0x commented 1 year ago

Escalate for 10 USDC

I want to add some more reasons why this issue is incorrect and low severity.

  1. Optimism has taken great efforts makes sure that even if 0 l2gas is set there is more then enough gas to successfully bridge the message from L1 to L2 (going through the entire flow of contracts including the l2 CrossDomainMessenger). It is extremely unlikely that they will break this architecture - it is their promise that messages will reach the L2CrossDomainMessenger as can be seen in the baseGas function in the CrossDomainMessenger: https://github.com/ethereum-optimism/optimism/blob/0638daf505ee5fe0ccda418b4f85a7611a9fdd8d/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L443 
     * @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.
  2. l2Gas parameter is not treated as a maximum limit, it is treated as a minimum limit - if the message for some reason fails on L2 (For example an L2 bridge has an update that triggers an edge case) it will still be stored in failedMessages on L2CrossDomainMessenger and can be executed with a higher gas amount. Therefore the claim that this issue will cause loss of user funds is incorrect.

https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L365-L395

        // 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 (
            !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.call(_target, gasleft() - RELAY_RESERVED_GAS, _value, _message);
        xDomainMsgSender = Constants.DEFAULT_L2_SENDER;
sherlock-admin commented 1 year ago

Escalate for 10 USDC

I want to add some more reasons why this issue is incorrect and low severity.

  1. Optimism has taken great efforts makes sure that even if 0 l2gas is set there is more then enough gas to successfully bridge the message from L1 to L2 (going through the entire flow of contracts including the l2 CrossDomainMessenger). It is extremely unlikely that they will break this architecture - it is their promise that messages will reach the L2CrossDomainMessenger as can be seen in the baseGas function in the CrossDomainMessenger: https://github.com/ethereum-optimism/optimism/blob/0638daf505ee5fe0ccda418b4f85a7611a9fdd8d/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L443 
     * @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.
  2. l2Gas parameter is not treated as a maximum limit, it is treated as a minimum limit - if the message for some reason fails on L2 (For example an L2 bridge has an update that triggers an edge case) it will still be stored in failedMessages on L2CrossDomainMessenger and can be executed with a higher gas amount. Therefore the claim that this issue will cause loss of user funds is incorrect.

https://github.com/ethereum-optimism/optimism/blob/develop/packages/contracts-bedrock/contracts/universal/CrossDomainMessenger.sol#L365-L395

        // 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 (
            !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.call(_target, gasleft() - RELAY_RESERVED_GAS, _value, _message);
        xDomainMsgSender = Constants.DEFAULT_L2_SENDER;

You've created a valid escalation for 10 USDC!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

ctf-sec commented 1 year ago

Can maintain the medium severity.

Medium: There is a viable scenario (even if unlikely) that could cause the protocol to enter a state where a material amount of funds can be lost. The attack path is possible with assumptions that either mimic on-chain conditions or reflect conditions that have a reasonable chance of becoming true in the future. The more expensive the attack is for an attacker, the less likely it will be included as a Medium (holding all other factors constant). The vulnerability must be something that is not considered an acceptable risk by a reasonable protocol team.

albertnbrown commented 1 year ago

I have tested this today and the value passed into sendCrossDomainMessage is additive to the baseGas value optimisms requires for the function execution. So with sending 0 you get approximately 200,000 gas to complete execution on L2 (our bridge finalization requires about 130,000 gas). With setting the parameter to 200,000, you require 400,000 gas for this part of the function execution.

albertnbrown commented 1 year ago

To me this makes it clear that the original code was the correct way to code this.

albertnbrown commented 1 year ago

revert triggering "u-turn" conditional in finalize withdrawal tx when 0 is the parameter: https://goerli.etherscan.io/tx/0x320762efb39ed830725d3c211042819d4a85473c9e77c2bdf9a79309d2251807 revert triggering "u-turn" conditional in finalize withdrawal tx when 200000 is the parameter: https://goerli.etherscan.io/tx/0xbfae538cef55b87dfbbf6417a95899ef0b5def462777a1ca80da0f2a5550b717

hrishibhat commented 1 year ago

Escalation accepted

Invalid issue In the current implementation, there is no issue with the code and there is no loss of funds with setting _gasLimit to zero.

sherlock-admin commented 1 year ago

Escalation accepted

Invalid issue In the current implementation, there is no issue with the code and there is no loss of funds with setting _gasLimit to zero. 

This issue's escalations have been accepted!

Contestants' payouts and scores will be updated according to the changes made on this issue.