Underpaying Optimism l2gas(_minGasLimit) may lead to loss of funds

[code423n4]

Lines of code

https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/L1/L1StandardBridge.sol#L188-L205

Vulnerability details

Impact

The contract L1StandardBridge.sol is susceptible to a vulnerability where underpaying the l2Gas(here in all contract, it used as "_minGasLimit") value provided by users can result in a potential loss of funds. This vulnerability exists in the depositERC20() and depositERC20To() functions, which allow users to initiate deposits by specifying the l2Gas(_minGasLimit) value. If the provided l2Gas(_minGasLimit) value is insufficient/underpaid, it may cause the finalizeBridgeERC20 function in the L2 bridge contract to fail, resulting in a loss of the deposited funds.

This report focuses on deposit functionality, the same issue should be considered for withdraw() andwithdrawTo() in L2 contract.

In L1StandardBridge.sol,depositERC20 and depositERC20To() functions are used to transfer tokens on L2 to self and to other addresses. These two function has used internal function _initiateERC20Deposit and this internal function has used _initiateBridgeERC20 which is given as below,

File: contracts/L1/L1StandardBridge.sol

402    function _initiateBridgeERC20(
403        address _localToken,
404        address _remoteToken,
405        address _from,
406        address _to,
407        uint256 _amount,
408        uint32 _minGasLimit,
409        bytes memory _extraData
410    ) internal {
411        if (_isOptimismMintableERC20(_localToken)) {
412            require(
413                _isCorrectTokenPair(_localToken, _remoteToken),
414                "StandardBridge: wrong remote token for Optimism Mintable ERC20 local token"
415            );
416
417            OptimismMintableERC20(_localToken).burn(_from, _amount);
418        } else {
419            IERC20(_localToken).safeTransferFrom(_from, address(this), _amount);
420            deposits[_localToken][_remoteToken] = deposits[_localToken][_remoteToken] + _amount;
421        }
422
423        // Emit the correct events. By default this will be ERC20BridgeInitiated, but child
424        // contracts may override this function in order to emit legacy events as well.
425        _emitERC20BridgeInitiated(_localToken, _remoteToken, _from, _to, _amount, _extraData);
426
427        MESSENGER.sendMessage(
428            address(OTHER_BRIDGE),
429            abi.encodeWithSelector(
430                this.finalizeBridgeERC20.selector,
431                // Because this call will be executed on the remote chain, we reverse the order of
432                // the remote and local token addresses relative to their order in the
433                // finalizeBridgeERC20 function.
434                _remoteToken,
435                _localToken,
436                _from,
437                _to,
438                _amount,
439                _extraData
440            ),
441            _minGasLimit
442        );
443    }

This optimism standard token bridge makes the cross-chain deposit by sending a cross-chain message via MESSENGER(i.e CrossDomainMessenger.sol) to L2Bridge or standard bridge here.

The problem here is the _minGasLimit i.e l2Gas.

If users provide an insufficient l2Gas(_minGasLimit) value when initiating a deposit, it can result in the failure of the finalizeBridgeERC20 function in the standard bridge contract. Consequently, the deposited funds may be lost, leading to potential financial losses for the affected users.

Proof of Concept

L1StandardBridge.depositERC20(),

https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/L1/L1StandardBridge.sol#L157-L173

L1StandardBridge.depositERC20To(),

https://github.com/ethereum-optimism/optimism/blob/382d38b7d45bcbf73cb5e1e3f28cbd45d24e8a59/packages/contracts-bedrock/contracts/L1/L1StandardBridge.sol#L188-L205

Similar Medium severity finding at Li.Fi audit at Spearbit- https://solodit.xyz/issues/7049

Tools Used

Manual review

Recommended Mitigation Steps

To mitigate the risk of losing user funds, it is crucial to address the issue and provide clear guidance to users. The following recommendations are suggested:

Enhance Documentation: Update the user documentation, developer guides, and communication channels to emphasize the importance of providing an adequate l2Gas/l1Gas value when initiating deposits. Clearly explain the potential consequences of underpaying the l2Gas/l1Gas value and provide specific guidance on determining an appropriate value.

Input Validation: Implement input validation mechanisms to ensure that users provide a sufficient l2Gas/l1Gas value when initiating deposits. This can include setting minimum gas limits and providing warnings when users provide values that are below the recommended thresholds.

Assessed type

Other

[c4-judge]

0xleastwood marked the issue as primary issue

[c4-judge]

0xleastwood marked the issue as satisfactory

[c4-judge]

0xleastwood marked the issue as selected for report

[anupsv]

Please provide end to end foundry fork test PoC

[c4-sponsor]

anupsv marked the issue as sponsor disputed

[itsmetechjay]

@mohammedrizwann123 please provide a POC per the sponsor's request.

[JeeberC4]

@mohammedrizwann123 please provide POC within 24 hours.

[0xRizwan]

I think the issue is misunderstood here. The _minGasLimit required for deposit and withdraw(multiple functions including ETH and ERC20 deposit and withdraw functions) has not validated, specified and documented clearly. Therefore Underpaying _minGasLimit may lead to loss of funds

I have gone through this documentation There is no specific mention of _minGasLimit. Like in case of default gas limit for receive() function is hardcoded in smart contract. and the same default gas limit is used test which is passed test

However the test with _minGasLimit seems to be tested with some Magic numbers like 100, 1000, 10000 etc. Refer below test links. Reference1 Reference2 Reference3 Reference4 Reference5 Reference6 Reference7

If the gas limit is decided to be as _minGasLimit then the minGasLimit has to be documented so that the user at front end should be aware that if i pass this gasLimit then my transaction will be successful. I think when _minGasLimit term is coined then it had to be thought with some gas limit or it would have named simple like, "gasLimit".

POC:

Since there is no _minGasLimit is specified in functions like deposit or withdraw. In that case a _minGasLimit with 0 can be passed and the transaction will be reverted. Basically tests <_minGasLimit will always fail. But before that _minGasLimit has to be clearly documented.

Please check similar issue found by spearbit team where such minGasLimit is documented for user information- Reference link- https://solodit.xyz/issues/7049

@anupsv Considering the report and above details, the major issue here is about input _minGasLimit validation that user will pass and the user is not aware of how much gas has to be passed since the documentation is missing with this information to user. Please check the above spearbit similar findings found in Lifi audit. Therefore the following mitigation should be considered.

1) Enhance Documentation: Update the user documentation, developer guides, and communication channels to emphasize the importance of providing an adequate l2Gas/l1Gas(here _minGasLimit) value when initiating deposits. Clearly explain the potential consequences of underpaying the l2Gas/l1Gas value and provide specific guidance on determining an appropriate value.

2) Input Validation: Implement input validation mechanisms to ensure that users provide a sufficient l2Gas/l1Gas(here _minGasLimit) value when initiating deposits. This can include setting minimum gas limits and providing warnings when users provide values that are below the recommended thresholds.

[0xleastwood]

Leaving open for the base protocol team to verify the POC.

[anupsv]

Reading through, this isn't an issue with the protocol since the user would have to go around normal workflows, including gas estimations etc., to specify values for the scenario to apply.

[0xleastwood]

This seems like a typical input validation issue which seems to be of low severity.

[c4-judge]

0xleastwood marked the issue as not selected for report

[c4-judge]

0xleastwood changed the severity to QA (Quality Assurance)

[0xleastwood]

Similar issues found in: https://github.com/sherlock-audit/2023-03-optimism-judging/issues/100 https://github.com/sherlock-audit/2023-03-optimism-judging/issues/7 https://github.com/sherlock-audit/2023-01-optimism-judging/issues/304

As this has already been noted in previous audit contests as low risk. I'll make this as out-of-scope because it is already a known issue.

[c4-judge]

This previously downgraded issue has been upgraded by 0xleastwood

[c4-judge]

0xleastwood marked the issue as unsatisfactory: Out of scope