transferredAmount on mainnet can be drained if a malicious account can mint more tokens on Schain

[code423n4]

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L45 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/tokens/ERC20OnChain.sol#L49-L50 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L95 https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/tokens/ERC20OnChain.sol#L60-L63

Vulnerability details

Impact

Anyone on Schain that is able to mint more tokens, other than the mint action from postMessage in tokenManagerERC20 by bridging tokens over, can potentially drain the locked tokens in transferredAmount in depositBoxERC20 on mainnet by calling exit with the same amount of tokens in transferredAmount[schainHash][token].

This will trap other users' funds on sChain and lost those funds on mainnet to the malicious attacker.

An example of proof of concept using ERC20OnChian is given below. This case may seem to be special as the deployer of the clone contract is malicious. However, this is a potential risk that generalises to other custom contracts with any mint functionality.

Proof of Concept (with ERC20)

• Malicious account deploys an ERC20 clone ERC20OnChain on Schain.

• By deployment, the malicious account is assigned the MINTER_ROLE on ERC20OnChain in the constructor.

    constructor(
        string memory contractName,
        string memory contractSymbol
    ) initializer
    {
        AccessControlEnumerableUpgradeable.__AccessControlEnumerable_init();
        ERC20Upgradeable.__ERC20_init(contractName, contractSymbol);
        ERC20BurnableUpgradeable.__ERC20Burnable_init();
        _setRoleAdmin(MINTER_ROLE, MINTER_ROLE);
        _setupRole(MINTER_ROLE, _msgSender());
    }

• This ERC20 clone get registered on tokenMangagerERC20.sol by function addERC20TokenByOwner.

  function addERC20TokenByOwner(
      string calldata targetChainName,
      address erc20OnMainChain,
      address erc20OnSchain
   )

• The malicious account wait for more users to deposit tokens on depositBoxERC20.depositERC20, which will increase the amount in transferredAmount[schainHash][token]

    function depositERC20(
        string calldata schainName,
        address erc20OnMainnet,
        uint256 amount
    )

• Malicious account mint to his account the amount equal to transferredAmount[schainHash][token] on ERC20OnChain

    function mint(address account, uint256 value) external override {
        require(hasRole(MINTER_ROLE, _msgSender()), "Sender is not a Minter");
        _mint(account, value);
    }

• Malicious account calls exitToMainERC20 in TokenManagerERC20.

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

• transferredAmount[schainHash][token] is drained to the malicious account on mainnet and victims' tokens get stranded on schain.

Tools Used

Recommended Mitigation Steps

Disable minting function to be called directly in ERC20OnChain. Only allow minting when bridging tokens over.

[cstrangedk]

Similar disputed issue of #17 and #39, here applies to ERC20OnChain token clone.

In production with automatic deploy mode enabled, ERC20OnChain is deployed when token is received in DepositBox and subsequent postMessage()._sendERC20() method is executed... MINTER_ROLE and burner address is set to TokenManagerERC20, which is responsible for minting and burning token clones in response to Deposits in DepositBoxes. When automatic deploy is not abled, the SKALE chain owner is responsible for manually adding tokens and assigning the appropriate MINT and burn addresses.

Here TokenManagerERC20 mints tokens in response to a deposit https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L231

and here burns the token in response to an exit: https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/TokenManagers/TokenManagerERC20.sol#L306

[GalloDaSballo]

I believe the sponsor reply sheds further light into how the system will be used in 99% of cases.

That said we have to judge the codebase for how it could be exploited and used by malicious actors, and I do agree with he warden that if the MINTER_ROLE is granted to someone (let's say the admin) then they could use it to drain funds from the mainnet side of the bridge.

Because this is contingent on setup and Admin Privilege, I believe Medium Severity to be more appropriate.

Switching from a role based system to an immutable Minter Address (the bridge contract) can be used as mitigation, alternatively sChain end users will have to monitor for these types of changes and act accordingly