code423n4 commented 2 years ago

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L112-L123 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L308-L312 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L158 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L306-L308

Vulnerability details

Impact

When tokens are transferred from one schain to another schain, the outgoing messages are not transmitted to the mainnet receiver. The amount of tokens on the receiving schain will increase but when exiting on mainnet with the full amount, it will cause underflow in DepositBoxERC20.postMessage when calling

    function _removeTransferredAmount(bytes32 schainHash, address erc20Token, uint256 amount) private {
        transferredAmount[schainHash][erc20Token] -= amount;
    }

because transferredAmount[receivingSchain][token] on mainnet is not updated with the transfer, thus having less than the full amount.

This will trap the tokens on the receiving schain unless it is willing to be transferred back to the previous schain to exit to mainnet.

Proof of Concept (ERC20)

When ERC20 token is transferred from schain1 to schain2, the record in DepositBoxERC20 in transferredAmount[schain1][token] and transferredAmount[schain2][token] do not change. This is because calling

    function transferToSchainERC20(
        string calldata targetSchainName,
        address contractOnMainnet,
        uint256 amount
    )
        external
        override
        rightTransaction(targetSchainName, msg.sender)
    {
        bytes32 targetSchainHash = keccak256(abi.encodePacked(targetSchainName));
        _exit(targetSchainHash, tokenManagers[targetSchainHash], contractOnMainnet, msg.sender, amount);
    }

triggers only one outgoing message to the target schain in _exit function

        messageProxy.postOutgoingMessage(
            chainHash,
            messageReceiver,
            data
        );

When the user intends to withdraw the increased amount to mainnet, it won't succeed because in postMessage in DepositBoxERC20,

   require(ERC20Upgradeable(message.token).balanceOf(address(this)) >= message.amount, "Not enough money");

will pass but the next line

  _removeTransferredAmount(schainHash, message.token, message.amount);

will revert due to underflow as transferredAmount[schain2][token]< amount.

Tools Used

Recommended Mitigation Steps

When transferring tokens from one schain to another, one can either go through exit to mainnet en route or post Outgoing message to both the target schain as well as mainnet receiver.

cstrangedk commented 2 years ago

This is not possible to transfer a token from mainnet, to schain 1 to schain2. IMA is restricted to one cross-chain transfer only (tokens are only able to be transferred from an "origin Chain" to a "destination chain" and no further). This was covered in docs in the audit readme, here: https://docs.skale.network/ima/1.2.x/getting-started#_tokens_move_by_one_chain

GalloDaSballo commented 2 years ago

Per the sponsors provided documentation, the POC is invalidated, making the finding invalid

code-423n4 / 2022-02-skale-findings