possible to mint max uint amount of bridgeERC20 tokens via FxERC20ChildTunnel.deposit()

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/governance/contracts/bridges/FxERC20ChildTunnel.sol#L96-L106 https://github.com/code-423n4/2023-12-autonolas/blob/2a095eb1f8359be349d23af67089795fb0be4ed1/governance/contracts/bridges/FxERC20RootTunnel.sol#L74-L77

Vulnerability details

Impact

if an erc20 token like cUSDCV3 or with special behaviour similar to cUSDCV3 is added to the FxERC20ChildTunnel bridge, it will be possible to trick the bridge to send a message to FxERC20RootTunnel to mint max uint amount of bridged tokens to the user address. This behaviour happens because tokens like cUSDCV3 send the balance of the holder if the amount to be transferred is specified as type(uint).max instead of reverting. It is always best to check the balance of the user against the amount specified in parameter when intgerating erc20 tokens.

Tokens like this, cUSDCV3 for example have good representation on polygon and ethereum mainnet and tokens with these behaviour have been documented to exist - https://github.com/d-xo/weird-erc20?tab=readme-ov-file#transfer-of-less-than-amount It is better to design

Proof of Concept

• the cUSDCV3 token code snippet below, showing how the code checks if amount to be transferred is type(uint).max and changes the amount to be transferred to the token holder's total balance if true https://github.com/compound-finance/comet/blob/22cf923b6263177555272dde8b0791703895517d/contracts/Comet.sol#L893C1-L896C6

  function transferFrom(address src, address dst, uint amount) override external returns (bool) {
      transferInternal(msg.sender, src, dst, baseToken, amount);
      return true;
  }

https://github.com/compound-finance/comet/blob/22cf923b6263177555272dde8b0791703895517d/contracts/Comet.sol#L929

    function transferInternal(address operator, address src, address dst, address asset, uint amount) internal {
        if (isTransferPaused()) revert Paused();
        if (!hasPermission(src, operator)) revert Unauthorized();
        if (src == dst) revert NoSelfTransfer();

        if (asset == baseToken) {
            if (amount == type(uint256).max) {
                amount = balanceOf(src);
            }
            return transferBase(src, dst, amount);
        } else {
            return transferCollateral(src, dst, asset, safe128(amount));
        }
    }

• if this cUSDCV3 token or token with similar behavior is added to the L2 token management contract FxERC20ChildTunnel, we can trick the FxERC20ChildTunnel to send a message to the FxERC20RootTunnel counterpart on L1 to mint max uint amount of tokens to the our address when we call FxERC20ChildTunnel.deposit() . amount is specified as max uint, _sendMessageToRoot() is called, message is emitted and cUSDCV3.transferfrom() is called. cUSDCV3.transferfrom() is sucessful and the caller's total cUSDCV3 balance is transferred to the bridge contract while _sendMessageToRoot() emits a message with amount as max uint. Code snippet below.

    function deposit(uint256 amount) external {
        _deposit(msg.sender, amount);
    }

    function _deposit(address to, uint256 amount) internal {
        // Check for the non-zero amount
        if (amount == 0) {
            revert ZeroValue();
        }

        // Encode message for root: (address, address, uint256)
        bytes memory message = abi.encode(msg.sender, to, amount);
        // Send message to root
        _sendMessageToRoot(message);

        // Deposit tokens on an L2 bridge contract (lock)
        bool success = IERC20(childToken).transferFrom(msg.sender, address(this), amount);
        if (!success) {
            revert TransferFailed(childToken, msg.sender, address(this), amount);
        }

        emit FxDepositERC20(childToken, rootToken, msg.sender, to, amount);
    }

    function _sendMessageToRoot(bytes memory message) internal {
        emit MessageSent(message);
    }

• after the FxERC20ChildTunnel on L2 is tricked and the message is sent, FxERC20RootTunnel.receiveMessage() can be called. This will call _processMessageFromChild() which will mint the max uint bridged ERC20 token amount to the user on L1. The bridgedERC20 tokens have no total supply specified so i expect it can be possible to mint up to max uint. Also this attack is very possible in the event where the FxERC20RootTunnel and its bridged ERC20 root token on L1 is newly launched, there will be zero supply at this point, a max uint value mint can be sucessfull and overflow error will be avoided. The attacker can try to be the first user and execute this attack from L2 to get a max uint bridged token balance on L1.

    function _processMessageFromChild(bytes memory message) internal override {
        // Decode incoming message from child: (address, address, uint256)
        (address from, address to, uint256 amount) = abi.decode(message, (address, address, uint256));

        // Mints bridged amount of tokens to a specified address
        IERC20(rootToken).mint(to, amount);

        emit FxDepositERC20(childToken, rootToken, from, to, amount);
    }

I wrote a POC to illutrate this vuln.

• Please check the gist link here -> https://gist.github.com/adeolu98/00cb7375590f96269d725e15e61d9902
• copy code, create new file in /governance/test folder and paste.
• navigate cmd to governance folder and run forge init --force to initialise folder for forge tests
• while cmd is in governance run run code with : forge test --mc BridgeTest -vvvvvv

Tools Used

MANUAL REVIEW, FOUNDRY

Recommended Mitigation Steps

• check the balance of the user against the amount supplied in FxERC20ChildTunnel.deposit() like below

    function _deposit(address to, uint256 amount) internal {
        // Check for the non-zero amount
        if (amount == 0) {
            revert ZeroValue();
        }

    if ( IERC20(childToken).balanceOf(msg.sender) < amount) {
          revert InvalidAmount();
    }

        // Encode message for root: (address, address, uint256)
        bytes memory message = abi.encode(msg.sender, to, amount);
        // Send message to root
        _sendMessageToRoot(message);

        // Deposit tokens on an L2 bridge contract (lock)
        bool success = IERC20(childToken).transferFrom(msg.sender, address(this), amount);
        if (!success) {
            revert TransferFailed(childToken, msg.sender, address(this), amount);
        }

        emit FxDepositERC20(childToken, rootToken, msg.sender, to, amount);
    }

if user token balance is less than amount to be transferred, revert function.

Assessed type

ERC20

[c4-pre-sort]

alex-ppg marked the issue as duplicate of #152

[c4-pre-sort]

alex-ppg marked the issue as insufficient quality report

[alex-ppg]

Describes full balance transfer w/ max input relating to non-standard EIP-20 tokens for the FX portal, similar to how #152 describes deflationary / rebasing token incompatibility.

[dmvt]

https://docs.code4rena.com/awarding/judging-criteria/supreme-court-decisions-fall-2023#verdict-unsupported-fee-on-transfer-tokens-and-broader-erc20-discussion

[c4-judge]

dmvt marked the issue as unsatisfactory: Invalid