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

1 stars 0 forks source link

10ap17 - Bridge finalization failure due to incorrect deposit accounting #71

Open sherlock-admin4 opened 3 weeks ago

sherlock-admin4 commented 3 weeks ago

10ap17

High

Bridge finalization failure due to incorrect deposit accounting

Vulnerability Details

The vulnerability occurs in the finalizeBridgeERC20 function of the standard bridge contract, which is responsible for finalizing withdrawals of ERC20 tokens from Layer 1 (L1) to Layer 2 (L2). During the finalization process, the contract includes the following code:

deposits[_localToken][_remoteToken] = deposits[_localToken][_remoteToken] - _amount;

When attempting to finalize a withdrawal, this line subtracts the amount _amount from the deposits mapping for the respective token pair (_localToken and _remoteToken). This subtraction will result in an underflow error, causing the transaction to revert.

StandardBridge.sol#L356

Impact

This vulnerability results in users' funds becoming stuck in the bridge, as the deposit balance on L2 is not correctly updated when bridging ERC20 tokens from L1. During finalization, when the contract tries to subtract the deposit amount from the tracked balance, it attempts to underflow the deposit balance (which remains 0), causing the transaction to revert. As a result, users are unable to complete their withdrawals on L2, leading to potential loss of access to their funds and disruption in cross-chain operations.

Proof of Concept

This vulnerability occurs when bridging an ERC20 token from Layer 1 (L1) to Layer 2 (L2) using the Standard Bridge contract. The core issue lies in the incorrect management of the deposits mapping when attempting to finalize the bridge on L2.

The process flow is as follows:

  1. Step 1: Initiating the ERC20 Bridge on L1
    • A user initiates a bridge for an ERC20 token that is not mintable.
deposits[_localToken][_remoteToken] = deposits[_localToken][_remoteToken] + _amount;
  1. Step 2: Relaying the Message from L1 to L2

    • The message is picked up by the Optimism node, which processes it and relays the information to the L2 Standard Bridge contract.
    • The L2 Standard Bridge receives the message and prepares to finalize the bridge.
  2. Step 3: Finalizing the ERC20 Bridge on L2

    • The L2 bridge attempts to execute the finalizeBridgeERC20 function, which handles the completion of the bridging process by transferring the tokens to the recipient.

However, when the finalizeBridgeERC20 function tries to update the deposits mapping by subtracting the bridged amount, it uses the following code:

deposits[_localToken][_remoteToken] = deposits[_localToken][_remoteToken] - _amount;

The problem occurs here because, when processing on L2:

The deposits[_localToken][_remoteToken] mapping for non-mintable tokens on L2 has not been initialized or updated when tokens were sent from L1. When the L2 contract tries to subtract the bridged amount from the deposits balance, it finds that the current value in the mapping is zero. This subtraction causes an underflow since the contract tries to subtract _amount from zero. This leads to a revert in the transaction, which stops the bridging process, leaving the user’s funds in a stuck state.

Code Snippet

function finalizeBridgeERC20(
        address _localToken,
        address _remoteToken,
        address _from,
        address _to,
        uint256 _amount,
        bytes calldata _extraData
    )
        public
        virtual
        onlyOtherBridge
    {
        require(paused() == false, "StandardBridge: paused");
        if (_isOptimismMintableERC20(_localToken)) {
            require(
                _isCorrectTokenPair(_localToken, _remoteToken),
                "StandardBridge: wrong remote token for Optimism Mintable ERC20 local token"
            );

            OptimismMintableERC20(_localToken).mint(_to, _amount);
        } else {
            deposits[_localToken][_remoteToken] = deposits[_localToken][_remoteToken] - _amount;
            IERC20(_localToken).safeTransfer(_to, _amount);
        }

        // Emit the correct events. By default this will be ERC20BridgeFinalized, but child
        // contracts may override this function in order to emit legacy events as well.
        _emitERC20BridgeFinalized(_localToken, _remoteToken, _from, _to, _amount, _extraData);
    }

Tool used

Recommendation

Consider removing the deposits mapping from the initiateBridgeERC20 and finalizeBridgeERC20 functions. Since users need to have enough tokens to transfer them to the bridge for these functions to be executed, eliminating the deposits mapping would improve both the clarity of the code and the overall security of the protocol ( since the functions will no longer revert ).