Spomaria - The `StandardBridge::finalizeBridgeERC20` function reverts for tokens that are not optimism mintable leading to loss of user funds

[sherlock-admin4]

Spomaria

High

The StandardBridge::finalizeBridgeERC20 function reverts for tokens that are not optimism mintable leading to loss of user funds

Summary

The StandardBridge::finalizeBridgeERC20 function is designed to finalize the bridging of an ERC20 token allowing the L2StandardBridge to send the required token to the recipient address on Layer 2 after the sender must have deposited the necessary token on Layer 1. However, in the event that the ERC20 token is not optimism mintable, the transaction will revert locking the deposited token on the L1StandardBridge contract without the recipient address receiving the L2 token.

Root Cause

The vulnerability lies in the fact that the contract attempts to subtract the amount to be sent to the recipient address i.e. _amount from the deposits variable. To understand better how this is an issue, recall that StandardBridge is an abstract contract that is inherited by both L1StandardBridge and L2StandardBridge.

If a user attempts to bridge their ERC20 token that is not optimism mintable, they will first deposit their ERC20 token on L1 by calling the L1StandardBridge::depositERC20 function which in turn calls the StandardBridge::_initiateERC20Deposit function which transfers the ERC20 token from the sender to the L1StandardBridge contract and then increments the deposits variable by the amount deposited. See https://github.com/sherlock-audit/2024-08-tokamak-network/blob/main/tokamak-thanos/packages/contracts-bedrock/src/universal/StandardBridge.sol#L347-L392 Note however that the state variable deposits is only updated on the L1StandardBridge contract alone.

Now, to finalize the bridging of the ERC20 token, the L1CrossDomainMessenger contract calls the L2StandardBridge::finalizeBridgeERC20 function. Since the ERC20 token is not optimism mintable, the L2StandardBridge::finalizeBridgeERC20 function attempts to decrement the deposits variable by the amount to be sent to the recipient address, see https://github.com/sherlock-audit/2024-08-tokamak-network/blob/main/tokamak-thanos/packages/contracts-bedrock/src/universal/StandardBridge.sol#L278-L305

Note that this will revert due to an arithmetic underflow since the state variable deposits was only initialized on L2StandardBridge and never updated, this will attempt to subtract a uint256 from zero causing a revert.

    function finalizeBridgeERC20(
        address _localToken,
        address _remoteToken,
        address _from,
        address _to,
        uint256 _amount,
        bytes calldata _extraData
    )
        public
        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);
    }

Internal pre-conditions

1. deposits[_localToken][_remoteToken] = 0 or deposits[_localToken][_remoteToken] < _amount
2. _localToken and _remoteToken are not optimism mintable tokens

External pre-conditions

Likely to happen irrespective of any external pre-conditions

Attack Path

User attempts to bridge their ERC20 token that is not optimism mintable as follows user deposits ERC20 > messenger calls the otherBridge to finalize bridging > the otherBridge fails to finalize bridging due to arithmetic underflow or overflow

Impact

Due to the arithmetic underflow or overflow when L2StandardBridge::finalizeBridgeERC20 is called in relation to ERC20 tokens that are not optimism mintable, the call reverts and the recipient address does not receive the ERC20 on Layer 2 as expected.

In fact, this also happens the other way round when an ERC20 token that is not optimism mintable is deposited on L2 such that the corresponding ERC20 token is to be received on Layer 1. Here also, the L1StandardBridge::finalizeERC20Withdrawal still calls the StandardBridge::finalizeBridgeERC20 function which is likely to revert due to arithmetic underflow or overflow. In both cases, the user stands the change of losing their funds.

PoC

Place the following code into L1StandardBridge.t.sol.

contract L1StandardBridge_FinalizeBridgeERC20_Test is Bridge_Initializer {
    using stdStorage for StdStorage;

    function test_finalizeBridgeERC20_Fails_Due_To_UnderFlow() external {
        uint256 nonce = l1CrossDomainMessenger.messageNonce();
        uint256 version = 0; // Internal constant in the OptimismPortal: DEPOSIT_VERSION
        address l1MessengerAliased = AddressAliasHelper.applyL1ToL2Alias(address(l1CrossDomainMessenger));

        // Deal Alice's ERC20 State
        deal(address(L1Token), alice, 100000, true);
        vm.prank(alice);
        L1Token.approve(address(l1StandardBridge), type(uint256).max);

        // The l1StandardBridge should transfer alice's tokens to itself
        vm.expectCall(
            address(L1Token), abi.encodeWithSelector(ERC20.transferFrom.selector, alice, address(l1StandardBridge), 100)
        );

        bytes memory message = abi.encodeWithSelector(
            StandardBridge.finalizeBridgeERC20.selector, address(NativeL2Token), address(L1Token), alice, alice, 100, hex""
        );

        // the L1 bridge should call L1CrossDomainMessenger.sendMessage
        vm.expectCall(
            address(l1CrossDomainMessenger),
            abi.encodeWithSelector(CrossDomainMessenger.sendMessage.selector, address(l2StandardBridge), message, 10000)
        );

        bytes memory innerMessage = abi.encodeWithSelector(
            CrossDomainMessenger.relayMessage.selector,
            nonce,
            address(l1StandardBridge),
            address(l2StandardBridge),
            0,
            10000,
            message
        );

        uint64 baseGas = l1CrossDomainMessenger.baseGas(message, 10000);
        vm.expectCall(
            address(optimismPortal),
            abi.encodeWithSelector(
                OptimismPortal.depositTransaction.selector,
                address(l2CrossDomainMessenger),
                0,
                baseGas,
                false,
                innerMessage
            )
        );

        bytes memory opaqueData = abi.encodePacked(uint256(0), uint256(0), baseGas, false, innerMessage);

        // Should emit both the bedrock and legacy events
        vm.expectEmit(address(l1StandardBridge));
        emit ERC20DepositInitiated(address(L1Token), address(NativeL2Token), alice, alice, 100, hex"");

        vm.expectEmit(address(l1StandardBridge));
        emit ERC20BridgeInitiated(address(L1Token), address(NativeL2Token), alice, alice, 100, hex"");

        // OptimismPortal emits a TransactionDeposited event on `depositTransaction` call
        vm.expectEmit(address(optimismPortal));
        emit TransactionDeposited(l1MessengerAliased, address(l2CrossDomainMessenger), version, opaqueData);

        // SentMessage event emitted by the CrossDomainMessenger
        vm.expectEmit(address(l1CrossDomainMessenger));
        emit SentMessage(address(l2StandardBridge), address(l1StandardBridge), message, nonce, 10000);

        // SentMessageExtension1 event emitted by the CrossDomainMessenger
        vm.expectEmit(address(l1CrossDomainMessenger));
        emit SentMessageExtension1(address(l1StandardBridge), 0);

        vm.prank(alice);
        l1StandardBridge.depositERC20(address(L1Token), address(NativeL2Token), 100, 10000, hex"");
        // deposits(address(L1Token), address(NativeL2Token)) is updated on L1
        assertEq(l1StandardBridge.deposits(address(L1Token), address(NativeL2Token)), 100);
        // deposits(address(L1Token), address(NativeL2Token)) is NOT updated on L2
        assertEq(l2StandardBridge.deposits(address(L1Token), address(NativeL2Token)), 0);

        assertEq(ERC20(address(L1Token)).balanceOf(address(l1StandardBridge)), 100);

        deal(address(NativeL2Token), address(l2StandardBridge), 100000, true);

        // Now finalize the bridge on l2

        address messenger = address(l2StandardBridge.messenger());
        vm.mockCall(
            messenger,
            abi.encodeWithSelector(CrossDomainMessenger.xDomainMessageSender.selector),
            abi.encode(address(l2StandardBridge.OTHER_BRIDGE()))
        );
        vm.prank(messenger);

        l2StandardBridge.finalizeBridgeERC20(address(NativeL2Token), address(L1Token), alice, alice, 100, hex"" );
    }
}

Now run forge test --match-test test_finalizeBridgeERC20_Fails_Due_To_UnderFlow -vvvv

Output:

.
.
.
875853a7734B70Fd209924], ERC20: [0xB112d79D8e3E5a830d353f61F4905BA67Fb1EFDD], alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], alice: [0x328809Bc894f92807417D2dAD6b7C998c1aFdac6], 100, 0x) [delegatecall]
    │   │   ├─ [0] L2CrossDomainMessenger::xDomainMessageSender() [staticcall]
    │   │   │   └─ ← [Return] L1StandardBridgeProxy: [0x96d3F6c20EEd2697647F543fE6C08bC2Fbf39758]
    │   │   ├─ [172] ERC20::supportsInterface(0x01ffc9a700000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   │   └─ ← [Revert] EvmError: Revert
    │   │   ├─ [172] ERC20::supportsInterface(0x01ffc9a700000000000000000000000000000000000000000000000000000000) [staticcall]
    │   │   │   └─ ← [Revert] EvmError: Revert
    │   │   └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
    │   └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)
    └─ ← [Revert] panic: arithmetic underflow or overflow (0x11)

Suite result: FAILED. 0 passed; 1 failed; 0 skipped; finished in 260.56ms (5.46ms CPU time)

Ran 1 test suite in 6.64s (260.56ms CPU time): 0 tests passed, 1 failed, 0 skipped (1 total tests)

Failing tests:
Encountered 1 failing test in test/L1/L1StandardBridge.t.sol:L1StandardBridge_FinalizeBridgeERC20_Test
[FAIL. Reason: panic: arithmetic underflow or overflow (0x11)] test_finalizeBridgeERC20_Fails_Due_To_UnderFlow() (gas: 1344490)

Encountered a total of 1 failing tests, 0 tests succeeded

Mitigation

Consider other possible ways of updating the state variable deposits in a central location where both sides of the bridge can access and modify instead of each layer of the bridge updating separately from the other side.