[WP-H2] When transferring tokens native on SKALE to Ethereum with `TokenManagerERC20.exitToMainERC20()`, the tokens on the schain will be frozen on `TokenManagerERC20`, but they will not receive tokens on Ethereum

[code423n4]

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L95-L104

Vulnerability details

In the current implementation of TokenManagerERC20, it allows exitToMainERC20(tokenOnSchain, amount).

At L277 of TokenManagerERC20.sol in exitToMainERC20(), if tokenOnSchain is minted on SKALE schain natively, there are no such require statement that prevents the target chain being mainnet, eg: require(chainHash != MAINNET_HASH, "...")

Therefore, a user can set mainnet as the target chain, and at L298 of TokenManagerERC20.sol, the tokens will be transferred to the contract, and at L308, send message to Ethereum mainnet.

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L95-L104

    function exitToMainERC20(
        address contractOnMainnet,
        uint256 amount
    )
        external
        override
    {
        communityLocker.checkAllowedToSendMessage(msg.sender);
        _exit(MAINNET_HASH, depositBox, contractOnMainnet, msg.sender, amount);
    }

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L264-L313

    function _exit(
        bytes32 chainHash,
        address messageReceiver,
        address contractOnMainChain,
        address to,
        uint256 amount
    )
        private
    {
        bool isMainChainToken;
        ERC20BurnableUpgradeable contractOnSchain = clonesErc20[chainHash][contractOnMainChain];
        if (address(contractOnSchain) == address(0)) {
            contractOnSchain = ERC20BurnableUpgradeable(contractOnMainChain);
            isMainChainToken = true;
        }
        require(address(contractOnSchain).isContract(), "No token clone on schain");
        require(contractOnSchain.balanceOf(msg.sender) >= amount, "Insufficient funds");
        require(
            contractOnSchain.allowance(
                msg.sender,
                address(this)
            ) >= amount,
            "Transfer is not approved by token holder"
        );
        bytes memory data = Messages.encodeTransferErc20Message(address(contractOnMainChain), to, amount);
        if (isMainChainToken) {
            data = _receiveERC20(
                chainHash,
                address(contractOnSchain),
                msg.sender,
                amount
            );
            _saveTransferredAmount(chainHash, address(contractOnSchain), amount);
            require(
                contractOnSchain.transferFrom(msg.sender, address(this), amount),
                "Transfer was failed"
            );
        } else {
            require(
                contractOnSchain.transferFrom(msg.sender, address(this), amount),
                "Transfer was failed"
            );
            contractOnSchain.burn(amount);
        }
        messageProxy.postOutgoingMessage(
            chainHash,
            messageReceiver,
            data
        );
    }

However, the DepositBoxERC20 contract on Ethereum mainnet does not support such message from TokenManagerERC20 on the schain:

The type of the message from schain TokenManagerERC20 is TRANSFER_ERC20_AND_TOKEN_INFO (see L354 of TokenManagerERC20.sol) or TRANSFER_ERC20_AND_TOTAL_SUPPLY (see L362 of TokenManagerERC20.sol).

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L339-L370

    function _receiveERC20(
        bytes32 chainHash,
        address erc20OnMainChain,
        address to,
        uint256 amount
    )
        private
        returns (bytes memory data)
    {
        ERC20BurnableUpgradeable erc20 = ERC20BurnableUpgradeable(erc20OnMainChain);
        uint256 totalSupply = erc20.totalSupply();
        require(amount <= totalSupply, "Amount is incorrect");
        bool isERC20AddedToSchain = _schainToERC20[chainHash].contains(erc20OnMainChain);
        if (!isERC20AddedToSchain) {
            _addERC20ForSchain(chainHash, erc20OnMainChain);
            data = Messages.encodeTransferErc20AndTokenInfoMessage(
                erc20OnMainChain,
                to,
                amount,
                _getErc20TotalSupply(erc20),
                _getErc20TokenInfo(erc20)
            );
        } else {
            data = Messages.encodeTransferErc20AndTotalSupplyMessage(
                erc20OnMainChain,
                to,
                amount,
                _getErc20TotalSupply(erc20)
            );
        }
        emit ERC20TokenReady(chainHash, erc20OnMainChain, amount);
    }

DepositBoxERC20 on Ethereum MAINNET can only process TRANSFER_ERC20. (see DepositBoxERC20.sol L155 and Messages.sol L270)

When getting a message with the type of TRANSFER_ERC20_AND_TOKEN_INFO or TRANSFER_ERC20_AND_TOTAL_SUPPLY from schain TokenManagerERC20, it will revert at L270 of Messages.sol.

As a result, the schain tokens will be frozen on TokenManagerERC20, but they will not receive tokens on Ethereum.

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L143-L164

    function postMessage(
        bytes32 schainHash,
        address sender,
        bytes calldata data
    )
        external
        override
        onlyMessageProxy
        whenNotKilled(schainHash)
        checkReceiverChain(schainHash, sender)
        returns (address)
    {
        Messages.TransferErc20Message memory message = Messages.decodeTransferErc20Message(data);
        require(message.token.isContract(), "Given address is not a contract");
        require(ERC20Upgradeable(message.token).balanceOf(address(this)) >= message.amount, "Not enough money");
        _removeTransferredAmount(schainHash, message.token, message.amount);
        require(
            ERC20Upgradeable(message.token).transfer(message.receiver, message.amount),
            "Transfer was failed"
        );
        return message.receiver;
    }

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/Messages.sol#L267-L272

    function decodeTransferErc20Message(
        bytes calldata data
    ) internal pure returns (TransferErc20Message memory) {
        require(getMessageType(data) == MessageType.TRANSFER_ERC20, "Message type is not ERC20 transfer");
        return abi.decode(data, (TransferErc20Message));
    }

Recommendation

Consider preventing moving schain native tokens to Ethereum MAINNET, for example: Add require(chainHash != MAINNET_HASH, "...") near L277 of TokenManagerERC20.sol.

[cstrangedk]

Valid issue, however disagree with severity as issue relates to function incorrect as to spec. Suggest low severity.

[GalloDaSballo]

Let me reason through the finding: -> The warden has shown a way for funds to be lost as long as a user uses a sChainNativeToken and burns them to bridge to mainnet (Potentially High)

-> This is contingent on the configuration of the depositBoxErc20 (Potentially Med)

I disagree with the sponsor argument in that while the code may not be as to spec, the functionality impaired as a medium to high impact.

At this time I can rationalize the severity either as: -> It should be High because the given codebase "default" functionality has this flaw -> it should be Med because this is contingent on a configuration parameter

Given the pre-context that the sChain could be set up by the admin to allow the misconfiguration to happen, at this time, I believe Medium Severity to be more appropriate as the impact is solely dependent upon the configuration of the chain which as explained in the readmes is mostly dependent on the admin

[cstrangedk]

Mitigated in https://github.com/skalenetwork/IMA/pull/1031/