SKALE contest - WatchPug's results

Lines of code

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

Vulnerability details

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

function removeConnectedChain(string memory schainName) public virtual override onlyChainConnector {
    bytes32 schainHash = keccak256(abi.encodePacked(schainName));
    require(connectedChains[schainHash].inited, "Chain is not initialized");
    delete connectedChains[schainHash];
}

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

function _addConnectedChain(bytes32 schainHash) internal onlyChainConnector {
    require(!connectedChains[schainHash].inited,"Chain is already connected");
    connectedChains[schainHash] = ConnectedChainInfo({
        incomingMessageCounter: 0,
        outgoingMessageCounter: 0,
        inited: true
    });
}

In the current implementation, when a connected chain is removed, the incomingMessageCounter and outgoingMessageCounter will be deleted.

And if it's reconnected again, the incomingMessageCounter and outgoingMessageCounter will be reset to 0.

However, since the contract is using connectedChains[fromChainHash].incomingMessageCounter and signature to ensure that the message can only be processed once.

Impact

1. Once the incomingMessageCounter resets to 0, all the past messages (transactions) can be replayed with the old signatures.

2. Another impact is that, for the particular reconnected schain, both inbound and outbound messages may not be able to be processed properly, as the counter is now out of sync with the remote schain.

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/MessageProxyForSchain.sol#L212-L221

require(
    _verifyMessages(
        _hashedArray(messages, startingCounter, fromChainName),
        signature
    ),
    "Signature is not verified"
);
require(
    startingCounter == connectedChains[fromChainHash].incomingMessageCounter,
    "Starting counter is not qual to incoming message counter");

Recommendation

Recommendation

When removing and connecting a schain, instead of delete/reset the couter, consider leaving the counter as it is when removeConnectedChain, _addConnectedChain also should not reset the counter:

function removeConnectedChain(string memory schainName) public virtual override onlyChainConnector {
    bytes32 schainHash = keccak256(abi.encodePacked(schainName));
    require(connectedChains[schainHash].inited, "Chain is not initialized");
    connectedChains[targetChainHash].inited = false;
}

function _addConnectedChain(bytes32 schainHash) internal onlyChainConnector {
    require(!connectedChains[schainHash].inited,"Chain is already connected");
    connectedChains[schainHash].inited = true;
}

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.

Lines of code

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

Vulnerability details

When moving tokens that are native on the origin schain, to another schain, TokenManagerERC20.sol#transferToSchainERC20() will be called, which calls _exit() -> _receiveERC20():

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

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"
    );
}

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

bool isERC20AddedToSchain = _schainToERC20[chainHash].contains(erc20OnMainChain);
if (!isERC20AddedToSchain) {
    _addERC20ForSchain(chainHash, erc20OnMainChain);
    data = Messages.encodeTransferErc20AndTokenInfoMessage(
        erc20OnMainChain,
        to,
        amount,
        _getErc20TotalSupply(erc20),
        _getErc20TokenInfo(erc20)
    );
}

However, on the target schain, while handling the inbound message with postMessage() -> _sendERC20(), when contractOnSchain is false, The transaction will fail with "Automatic deploy is disabled" when automaticDeploy == false:

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

 contractOnSchain = clonesErc20[fromChainHash][token];

if (address(contractOnSchain) == address(0)) {
    require(automaticDeploy, "Automatic deploy is disabled");
    contractOnSchain = new ERC20OnChain(message.tokenInfo.name, message.tokenInfo.symbol);
    clonesErc20[fromChainHash][token] = contractOnSchain;
    addedClones[contractOnSchain] = true;
    emit ERC20TokenCreated(fromChainHash, token, address(contractOnSchain));
}

As a result, any tokens that are locked in the origin schain by the user will be frozen in the contract.

Recommendation

Consider adding a mapping storage to cache whether automaticDeploy is enabled on a certain schain, the cache should be updated once the automaticDeploy is updated.

And only allows S2S transfer when automaticDeploy is enabled on the target schain.

To further avoid the edge case of: right after the user submitted the S2S transfer tx on the from schain, the target schain disabled automaticDeploy and the user's tokens can be frozen in the from schain. We can introduce a 24 hrs timelock for disabling automaticDeploy.

[S]: Suggested optimation, save a decent amount of gas without compromising readability;

[M]: Minor optimation, the amount of gas saved is minor, change when you see fit;

[N]: Non-preferred, the amount of gas saved is at cost of readability, only apply when gas saving is a top priority.

[S] Cache storage variables in the stack can save gas

For the storage variables that will be accessed multiple times, cache them in the stack can save ~100 gas from each extra read (SLOAD after Berlin).

For example:

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

• MessageProxyForMainnet.sol#postIncomingMessages() messages.length can be cached as it will be read for more than 5 times;

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

    function postOutgoingMessage(
        bytes32 targetChainHash,
        address targetContract,
        bytes memory data
    )
        public
        override
        virtual
    {
        require(connectedChains[targetChainHash].inited, "Destination chain is not initialized");
        _authorizeOutgoingMessageSender(targetChainHash);
        
        emit OutgoingMessage(
            targetChainHash,
            connectedChains[targetChainHash].outgoingMessageCounter,
            msg.sender,
            targetContract,
            data
        );

        connectedChains[targetChainHash].outgoingMessageCounter += 1;
    }

connectedChains[targetChainHash].outgoingMessageCounter at L296 can be cached to be reused at L302.

[S] Cache array length in for loops can save gas

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack.

Caching the array length in the stack saves around 3 gas per iteration.

Instances include:

• MessageProxyForMainnet.sol#initializeAllRegisteredContracts()

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

[S] Remove redundant contains check for SE can save gas

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

        if (_deprecatedSchainToERC20[schainHash][tokens[i]] && !_schainToERC20[schainHash].contains(tokens[i])) {
                _schainToERC20[schainHash].add(tokens[i]);
                delete _deprecatedSchainToERC20[schainHash][tokens[i]];

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/node_modules/@openzeppelin/contracts-upgradeable/utils/structs/EnumerableSetUpgradeable.sol#L53-L63

    function _add(Set storage set, bytes32 value) private returns (bool) {
        if (!_contains(set, value)) {
            set._values.push(value);
            // The value is stored at length-1, but we add 1 to all indexes
            // and use 0 as a sentinel value
            set._indexes[value] = set._values.length;
            return true;
        } else {
            return false;
        }
    }

The check of !_schainToERC20[schainHash].contains(tokens[i]) is already included in function add().

Therefore, !_schainToERC20[schainHash].contains(tokens[i]) is redundant.

Removing it will make the code simpler and save some gas.

Recommendation

        if (_deprecatedSchainToERC20[schainHash][tokens[i]]) {
                _schainToERC20[schainHash].add(tokens[i]);
                delete _deprecatedSchainToERC20[schainHash][tokens[i]];

The same issue also exists in:

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

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

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

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC721.sol#L76-L81

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

[M] Adding unchecked directive can save gas

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

For example:

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/CommunityPool.sol#L98-L101

        if (amount > _userWallets[user][schainHash]) {
            amount = _userWallets[user][schainHash];
        }
        _userWallets[user][schainHash] = _userWallets[user][schainHash] - amount;

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/CommunityPool.sol#L171-L173

        require(amount <= _userWallets[msg.sender][schainHash], "Balance is too low");
        require(!messageProxy.messageInProgress(), "Message is in progress");
        _userWallets[msg.sender][schainHash] = _userWallets[msg.sender][schainHash] - amount;

_userWallets[user][schainHash] - amount will never underflow.

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/schain/MessageProxyForSchain.sol#L400-L404

        if (address(etherbase).isContract()
            && etherbase.hasRole(etherbase.ETHER_MANAGER_ROLE(), address(this)) 
            && balance < MINIMUM_BALANCE
        ) {
            uint missingAmount = MINIMUM_BALANCE - balance;

MINIMUM_BALANCE - balance will never underflow.

[M] Use short reason strings can save gas

Every reason string takes at least 32 bytes.

Use short reason strings that fits in 32 bytes or it will become more expensive.

Instances include:

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

        require(
            startingCounter == connectedChains[fromSchainHash].incomingMessageCounter,
            "Starting counter is not equal to incoming message counter");

[M] ++i is more efficient than i++

Using ++i is more gas efficient than i++, especially in for loops.

For example:

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

    {
        require(
            from < to && to - from <= 10 && to <= _getRegistryContracts()[schainHash].length(),
            "Range is incorrect"
        );
        contractsInRange = new address[](to - from);
        for (uint256 i = from; i < to; i++) {
            contractsInRange[i - from] = _getRegistryContracts()[schainHash].at(i);
        }
    }

Change to:

    {
        require(
            from < to && to - from <= 10 && to <= _getRegistryContracts()[schainHash].length(),
            "Range is incorrect"
        );
        contractsInRange = new address[](to - from);
        for (uint256 i = from; i < to; ++i) {
            contractsInRange[i - from] = _getRegistryContracts()[schainHash].at(i);
        }
    }

Other examples:

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

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

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

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/Linker.sol#L149-L151

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

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC721.sol#L260-L262

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC1155.sol#L79-L87

[M] Changing bool to uint256 can save gas

Booleans are more expensive than uint256 or any type that takes up a full word because each write operation emits an extra SLOAD to first read the slot's contents, replace the bits taken up by the boolean, and then write back. This is the compiler's defense against contract upgrades and pointer aliasing, and it cannot be disabled.

See: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/security/ReentrancyGuard.sol#L23-L27

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

    modifier messageInProgressLocker() {
        require(!messageInProgress, "Message is in progress");
        messageInProgress = true;
        _;
        messageInProgress = false;
    }

Recommendation

    modifier messageInProgressLocker() {
        require(messageInProgress == 1, "Message is in progress");
        messageInProgress = 0;
        _;
        messageInProgress = 1;
    }

[M] Setting uint256 variables to 0 is redundant

Setting uint256 variables to 0 is redundant as they default to 0.

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

        uint notReimbursedGas = 0;

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

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

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/Linker.sol#L149-L151

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

https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/mainnet/DepositBoxes/DepositBoxERC1155.sol#L79-L87

[N] Unnecessary checked arithmetic in for loops

There is no risk of overflow caused by increamenting the iteration index in for loops (the ++i in for for (uint256 i; i < count; ++i)).

Increments perform overflow checks that are not necessary in this case.

Recommendation

Surround the increment expressions with an unchecked { ... } block to avoid the default overflow checks. For example, change the for loop: