SKALE contest - kyliek's results

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.

Low Severity

[L01] Outdated version of inherited contracts and upgrade library

@openzeppelin/contracts-upgradeable   4.3.2  -> 4.5.1 
@openzeppelin/hardhat-upgrades        1.10.0 -> 1.14.0   

[L02] Not calling inherited function directly

For instance https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/SkaleManagerClient.sol#L62

AccessControlEnumerableUpgradeable.__AccessControlEnumerable_init();

is not necessary. Just use __AccessControlEnumerable_init();. It compiles and all tests passed.

[L03] Require messages not indicative of the contracts

Almost all require message has no indication of which contract it is coming from. It is a good practice for debugging and monitoring purposes to mark the contract that reverted.

For example, https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L188 the revert message could be "MessageProxy: Extra contract is not registered". Many other instances are also present in almost all contracts by globally search require.

[L04] Use initializer instead of onlyInitializing

In OpenZeppline-contracts-upgradeable v4.5 initializer has a new modifier https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/20380ee699406211ae88c4edd11b0af87c8abdd6/contracts/proxy/utils/Initializable.sol#L72

    modifier onlyInitializing() {
        require(_initializing, "Initializable: contract is not initializing");
        _;
    }

to mark and protect the initializing functions that should not be called directly called.

Instances such as

• https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/SkaleManagerClient.sol#L54-L60 , which is inherited by Twin.sol

• https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/Twin.sol#L95-L101

• https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBox.sol#L99-L107

[L05] Inconsistent use of schainHash or schainName when referencing a schain

There is inconsistency in referring to a schain, sometimes by bytes32 hash, sometimes by string name in a single contract.

For example, in SkaleManagerClient,

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/SkaleManagerClient.sol#L70-L73 uses schainNash and https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/SkaleManagerClient.sol#L42-L48 uses schainName.

Bytes32 should be preferred over string for gas-saving reasons.

[L06] Schain owner cannot add ERC20 token manually as specified in the doc

The specification in the doc is inconsistent with the code. For example in https://docs.skale.network/ima/1.2.x/managing-erc20 and the diagram https://docs.skale.network/ima/1.2.x/flows#_manual_setup_steps is not consistent with the code.

Minter role in ERC20 is granted automatically either to TokenManagerERC20 or to the deployer in the constructor.

[L07] Outdated Solidity Version

Solidity version 0.8.6 is used throughout. Many updates have been applied to the compiler. A more recent version such as 0.8.10 would work better. The latest version is 0.8.11.

[L08] Use OnlyRole() modifier from inherited contract AccessControlUpgradeable

Many instances can be simplified using the OnlyRole modifier.

https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/20380ee699406211ae88c4edd11b0af87c8abdd6/contracts/access/AccessControlUpgradeable.sol#L75-L78

For instance, https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/MessageProxyForSchain.sol#L237 and https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/tokens/ERC20OnChain.sol#L60-L61 and many other role-based require statements.

Informational

[N01] NatSpec on Events and Functions

Many functions and events do not have @param and @return labelled in Natspec, for instance

• https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/MessageProxyForSchain.sol#L245-L251
• https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/MessageProxyForSchain.sol#L261-L265

and many others.

[N02] initialize functions positioned too much down the contract

initialize function is a special feature in upgradeability patterns and often important and prone to grave consequences too. It is a good practice to put them at the beginning of the contract, like those in the OpenZeppelin-contracts-upgradeable library.

[N03] Repeated code

Repeated unimplemented function postIncomingMessages() in both

• https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L152-L160

and "@skalenetwork/ima-interfaces/IMessageProxy.sol" .

One can remove the one in MessageProxy.sol without affecting any test results.

Gas Saving

[G01] public functions can be external

Here are two example instances

• https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L262

• https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/MessageProxy.sol#L313

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyForMainnet.sol#L232-L233

Vulnerability details

Impact

Division by zero will cause EVM to revert in calculating additionalGasPerMessage in MessageProxyForMainnet.postIncomingMessages.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

See https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyForMainnet.sol#L232-L233

Unit test shows

  Error: VM Exception while processing transaction: reverted with panic code 0x12 (Division or modulo division by zero)

Tools Used

Recommended Mitigation Steps

validate above https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/MessageProxyForMainnet.sol#L232-L233 by

require(messages.length > 0,  "MsgProxyForMainnet:zero message length");