A recalled ERC20 bridge transfer can lock tokens in the bridge

[c4-bot-7]

Lines of code

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/ERC20Vault.sol#L320-L335 https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/adapters/USDCAdapter.sol#L43-L45 https://github.com/circlefin/stablecoin-evm/blob/0828084aec860712531e8d79fde478927a34b3f4/contracts/v1/FiatTokenV1.sol#L133-L136

Vulnerability details

Impact

A recalling ERC20 bridge transfer can lock funds in the bridge if the call to mint tokens fail on the source chain. Depending on the Native token logic this could either be a permanent lock or a lock of an unknown period of time.

Example of how this can happen with the provided USDCAdapter:

USDC limits the amount of USDC that can be minted on each chain by giving each minter a minting allowance. If the minting allowance is reach minting will revert. If this happens in a recalled message the tokens together with the ETH value is locked.

Proof of Concept

USDC limits the amount of USDC that can be minted on each chain by giving each minter a minting allowance.

If _amount <= mintingAllowedAmount is reached for the USDCAdapter tokens can not be minted but since this is a recalled message the funds are stuck.

Both onMessageIncovation() and onMessageRecalled() call _transferToken() to either mint or release tokens

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/ERC20Vault.sol#L320-L335

    function _transferTokens(
        CanonicalERC20 memory _ctoken,
        address _to,
        uint256 _amount
    )
        private
        returns (address token_)
    {
        if (_ctoken.chainId == block.chainid) {
            token_ = _ctoken.addr;
            IERC20(token_).safeTransfer(_to, _amount);
        } else {
            token_ = _getOrDeployBridgedToken(_ctoken);
            IBridgedERC20(token_).mint(_to, _amount);
        }
    }

A recalled message to bridge USDC L1->L2 will revert when we attempt to mint through the USDCAdapter

https://github.com/code-423n4/2024-03-taiko/blob/f58384f44dbf4c6535264a472322322705133b11/packages/protocol/contracts/tokenvault/adapters/USDCAdapter.sol#L43-L45

    function _mintToken(address _account, uint256 _amount) internal override {
        usdc.mint(_account, _amount);

On the following condition in the native USDC contract

https://github.com/circlefin/stablecoin-evm/blob/0828084aec860712531e8d79fde478927a34b3f4/contracts/v1/FiatTokenV1.sol#L133-L136

        require(
            _amount <= mintingAllowedAmount,
            "FiatToken: mint amount exceeds minterAllowance"
        );

Course of events that ends in locked funds:

1. User bridges USDC from L2->L1
2. The message is recalled from L1
3. The USDCAdapter has reached the mintingAllowedAmount
4. The recalled message is stuck because minting reverts. The USDC and ETH passed in are both locked.

   Tools Used

   foundry, vscode

   Recommended Mitigation Steps

Add new functionality in the vault that allows users to send a new message to the destination chain again with new message data if onMessageRecalls() can not mint tokens. We give users the ability to redeem for canonical tokens instead of being stuck.

Assessed type

Other

[c4-pre-sort]

minhquanym marked the issue as sufficient quality report

[c4-pre-sort]

minhquanym marked the issue as primary issue

[dantaik]

Thank you for your feedback. In your example, if the user cannot mint tokens in USDCAdapter by using recallMessage, the user can wait and call recallMessage again. That's why recallMessage is retriable.

There is no perfect solution here, and I personally don't want to make the bridge too complicated by introducing this re-send-another-message feature.

Adding a warning on the bridge UI to show a warning message might be a good solution, something like "USDC on Taiko has reached 95% of its max supply cap, bridging USDC to Taiko may end up your fund becoming unavailable for some time until others bridge USDC away from Taiko".

[c4-sponsor]

dantaik (sponsor) acknowledged

[c4-judge]

0xean marked the issue as satisfactory

[c4-judge]

0xean marked the issue as selected for report

[t0x1cC0de]

Hey @0xean, flagging this for your review. Based on sponsor's comments above as well as those made here and given the prerequisite + impact, should it (and #271) still be a Med?

Thanks

[0xean]

Their comments simply show their isn't a great solution to the problem, it still represents a loss of user funds (if it goes on forever) or a denial of service and a risk that users should be aware of.

@dontonka / @adaki2004 any last comments here?

[0xmonrel]

I submitted two related issues, this and #271. I believe #271 could be more sever since it could lock a larger portion of old USDC into a state where they can not be redeemed. I would like to confirm that the sponsor is aware of #271 and that it wasn't missed since it is tagged as a duplicate to this issue.

I am not sure if there is ground for de-duplication but either way I would like to confirm that the sponsor has seen #271.

[adaki2004]

Their comments simply show their isn't a great solution to the problem, it still represents a loss of user funds (if it goes on forever) or a denial of service and a risk that users should be aware of.

@dontonka / @adaki2004 any last comments here?

We are OK with med, no issue. Please proceed accordinlgy - as we dont have:

1. the perfect solution to the problem
2. the intention to fix it ATM - since we wont be using any native tokens anytime soon.

But please proceed the way which is suitable for the wardens better, we appreciate their efforts. (So not questioning the severity)

[dontonka]

I've been tagged by mistake here, I assume that was for @dantaik .

[0xean]

leaving as valid M and with #271 duped.