SKALE contest - kirk-baird's results

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/CommunityPool.sol#L82-L112 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/MessageProxyForMainnet.sol#L235-L250

Vulnerability details

Impact

The function refundGasByUser() can be exploited by the message sender to drain nodes and SChain owners of their balances when processing incoming messages.

When a node collates a set of exits from an SChain to Ethereum, they are submitted on-chain via MessageProxyForMainnet.sol. For each message to a registered contract the user is required to pay for the refund via CommunityPool.refundGasByUser().

The issue occurs in CommunityPool.refundGasByUser() as the amount to be refunded is calculated as uint amount = tx.gasprice * gas;, where gas is the gas used by the message. Since tx.gasprice is set by the node and there is no upper bounds on the price. Since EIP1559 the gas price is BaseFee + Tip and although Base is predetermined Tip is any arbitrary non-zero integer.

The attack is for a node to set an excessively high tx.gasprice which will be refunded out of the balance of the user who initiated the outgoing transaction or if that user has insufficient balance then from the SChain owner. Since the node submitting the transaction is refunded for their gas they do not lose from setting a higher gas price.

The impact of the attack is that the user requesting the exit and/or the SChain owner may have their ETH balances depleted to refund the submitter. The impact is worsened as if the user has insufficient balance a message will be sent to the SChain preventing them from making further exits until they have sufficient balance.

Note a similar issue may be seen in IWallets.refundGasBySchain() depending on how the gas calculations are performed (they are not in scope but the TestWallet also uses tx.gasprice in the same manner).

Proof of Concept

Processing incoming messages in MessageProxyForMainnet.sol

        for (uint256 i = 0; i < messages.length; i++) {
            gasTotal = gasleft();
            if (isContractRegistered(bytes32(0), messages[i].destinationContract)) {
                address receiver = _getGasPayer(fromSchainHash, messages[i], startingCounter + i);
                _callReceiverContract(fromSchainHash, messages[i], startingCounter + i);
                notReimbursedGas += communityPool.refundGasByUser(
                    fromSchainHash,
                    payable(msg.sender),
                    receiver,
                    gasTotal - gasleft() + additionalGasPerMessage
                );
            } else {
                _callReceiverContract(fromSchainHash, messages[i], startingCounter + i);
                notReimbursedGas += gasTotal - gasleft() + additionalGasPerMessage;
            }
        }

Refunding gas in CommunityPool.sol

    function refundGasByUser(
        bytes32 schainHash,
        address payable node,
        address user,
        uint gas
    )
        external
        override
        onlyMessageProxy
        returns (uint)
    {
        require(node != address(0), "Node address must be set");
        if (!activeUsers[user][schainHash]) {
            return gas;
        }
        uint amount = tx.gasprice * gas;
        if (amount > _userWallets[user][schainHash]) {
            amount = _userWallets[user][schainHash];
        }
        _userWallets[user][schainHash] = _userWallets[user][schainHash] - amount;
        if (!_balanceIsSufficient(schainHash, user, 0)) {
            activeUsers[user][schainHash] = false;
            messageProxy.postOutgoingMessage(
                schainHash,
                schainLinks[schainHash],
                Messages.encodeLockUserMessage(user)
            );
        }
        node.sendValue(amount);
        return (tx.gasprice * gas - amount) / tx.gasprice;
    }

Recommended Mitigation Steps

One solution to avoid excessive over refunding of gas fees is to use a gas price oracle rather than tx.gasprice.

An alternate solution is to set a maximum gas price and have some incentives for the node submitting at a gas price below the maximum.

Lines of code

https://github.com/skalenetwork/skale-manager/blob/6827f3c8918424641647f20700232713626be828/contracts/SchainsInternal.sol#L206-L234 https://github.com/skalenetwork/skale-manager/blob/6827f3c8918424641647f20700232713626be828/contracts/SchainsInternal.sol#L522-L524 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/SkaleManagerClient.sol#L70-L74 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L196-L209

Vulnerability details

Impact

If the SChain is removed before all the funds are withdrawn from the bridge they will be permanently locked in the bridge.

When a SChain is removed in by the SKALE protocol via the function SchainsInternal.removeSchain() the data including the owner will be deleted. As a result the function SchainsInternal.getSchainOwner() will always return address(0).

Since getFunds() in each of the deposit boxes can only be called by the SChain owner (i.e. has the modifier onlySchainOwner) and the owner address is now address(0) getFunds() will always revert. The impact is that any funds associated with this SChain are now locked in the bridge.

Proof of Concept

SchainsInternal.sol#L206-L234

    function removeSchain(bytes32 schainHash, address from)
        external
        override
        allow("Schains")
        schainExists(schainHash)
    {
        isSchainActive[schainHash] = false;
        uint length = schainIndexes[from].length;
        uint index = schains[schainHash].indexInOwnerList;
        if (index != length - 1) {
            bytes32 lastSchainHash = schainIndexes[from][length - 1];
            schains[lastSchainHash].indexInOwnerList = index;
            schainIndexes[from][index] = lastSchainHash;
        }
        schainIndexes[from].pop();

        // TODO:
        // optimize
        for (uint i = 0; i + 1 < schainsAtSystem.length; i++) {
            if (schainsAtSystem[i] == schainHash) {
                schainsAtSystem[i] = schainsAtSystem[schainsAtSystem.length - 1];
                break;
            }
        }
        schainsAtSystem.pop();

        delete schains[schainHash];
        numberOfSchains--;
    }

SchainsInternal.sol#L522-L524

    function getSchainOwner(bytes32 schainHash) external view override schainExists(schainHash) returns (address) {
        return schains[schainHash].owner;
    }

SkaleManagerClient.sol#L70-L74

    function isSchainOwner(address sender, bytes32 schainHash) public view override returns (bool) {
        address skaleChainsInternal = contractManagerOfSkaleManager.getContract("SchainsInternal");
        return ISchainsInternal(skaleChainsInternal).isOwnerAddress(sender, schainHash);
    }
}

DepositBoxERC20.sol#L196-L209

    function getFunds(string calldata schainName, address erc20OnMainnet, address receiver, uint amount)
        external
        override
        onlySchainOwner(schainName)
        whenKilled(keccak256(abi.encodePacked(schainName)))
    {
        bytes32 schainHash = keccak256(abi.encodePacked(schainName));
        require(transferredAmount[schainHash][erc20OnMainnet] >= amount, "Incorrect amount");
        _removeTransferredAmount(schainHash, erc20OnMainnet, amount);
        require(
            ERC20Upgradeable(erc20OnMainnet).transfer(receiver, amount),
            "Transfer was failed"
        );
    }

Recommended Mitigation Steps

Consider allowing a fallback option, such as the DEFAULT_ADMIN_ROLE of the boxes to be allowed to call getFunds() if SchainsInternal.getSchainOwner() returns address(0) and there is still balance in the deposit box for this SChain.

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/schain/TokenManagers/TokenManagerEth.sol#L45-L49

Vulnerability details

Impact

There is a Centralisation risk of the bridge where the DEFAULT_ADMIN_ROLE of TokenManagerEth.sol is able to modify the ERC20 token on the SChain to any arbitrary address. This would allow the admin role to change the address to one where they have infinite supply, they could then call exitToMain(amount) equal to the balance of the DepositBox in the main Ethereum chain. After the message is process on the main Ethereum chain they will receive the entire Eth balance of that contract.

The rug pull attack occurs because there is a DEFAULT_ADMIN_ROLE which is set in the intiialisation to the msg.sender as seen in initializeTokenManager() below.

The DEFAULT_ADMIN_ROLE may then call setEthErc20Address(IEthErc20 newEthErc20Address) setting newEthErc20Address to any arbitrary contract they control.

Proof of Concept

    function setEthErc20Address(IEthErc20 newEthErc20Address) external override {
        require(hasRole(DEFAULT_ADMIN_ROLE, msg.sender), "Not authorized caller");
        require(ethErc20 != newEthErc20Address, "Must be new address");
        ethErc20 = newEthErc20Address;
    }

    function initializeTokenManager(
        string memory newSchainName,
        IMessageProxyForSchain newMessageProxy,
        ITokenManagerLinker newIMALinker,
        ICommunityLocker newCommunityLocker,
        address newDepositBox
    )
        public
        virtual
        initializer
    {
        require(newDepositBox != address(0), "DepositBox address has to be set");

        AccessControlEnumerableUpgradeable.__AccessControlEnumerable_init();
        _setupRole(DEFAULT_ADMIN_ROLE, msg.sender);
        _setupRole(AUTOMATIC_DEPLOY_ROLE, msg.sender);
        _setupRole(TOKEN_REGISTRAR_ROLE, msg.sender);

        schainHash = keccak256(abi.encodePacked(newSchainName));
        messageProxy = newMessageProxy;
        tokenManagerLinker = newIMALinker;
        communityLocker = newCommunityLocker;        
        depositBox = newDepositBox;

        emit DepositBoxWasChanged(address(0), newDepositBox);
    }

Recommended Mitigation Steps

Consider removing the function setEthErc20Address() as ethErc20 is set in the initialize() function and does not need to be changed.

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/DepositBoxes/DepositBoxERC20.sol#L95-L132

Vulnerability details

Impact

The function DepositBoxERC20.depositERC20() does not account for FoT (Fee on Transfer) tokens. FoT tokens charge a fee when transfer() or transferFrom() is called and it is subtracted from amount so the receiving address will receive less than amount of tokens.

This occurs in depositERC20() since the original amount is transferred through the bridge to the ERC20 token on the SChain. However, this amount may have fees deducted from it during the following

            ERC20Upgradeable(erc20OnMainnet).transferFrom(msg.sender,
                address(this),
                amount
            )

The impact is that the bridge will end up absorbing all of the fees for deposits.

Proof of Concept

    function depositERC20(
        string calldata schainName,
        address erc20OnMainnet,
        uint256 amount
    )
        external
        override
        rightTransaction(schainName, msg.sender)
        whenNotKilled(keccak256(abi.encodePacked(schainName)))
    {
        bytes32 schainHash = keccak256(abi.encodePacked(schainName));
        address contractReceiver = schainLinks[schainHash];
        require(contractReceiver != address(0), "Unconnected chain");
        require(
            ERC20Upgradeable(erc20OnMainnet).allowance(msg.sender, address(this)) >= amount,
            "DepositBox was not approved for ERC20 token"
        );
        bytes memory data = _receiveERC20(
            schainName,
            erc20OnMainnet,
            msg.sender,
            amount
        );
        _saveTransferredAmount(schainHash, erc20OnMainnet, amount);
        require(
            ERC20Upgradeable(erc20OnMainnet).transferFrom(
                msg.sender,
                address(this),
                amount
            ),
            "Transfer was failed"
        );
        messageProxy.postOutgoingMessage(
            schainHash,
            contractReceiver,
            data
        );
    }

Recommended Mitigation Steps

Consider preventing fee on transfer tokens from being used in the system. This must be done either by only allowing whitelisted addresses.

Alternatively, this can be done by ensuring the balance of the Deposit Box increases by exactly amount. For example.

        uint256 balanceBefore = ERC20Upgradeable(erc20OnMainnet).balanceOf(address(this));
        require(
            ERC20Upgradeable(erc20OnMainnet).transferFrom(
                msg.sender,
                address(this),
                amount
            ),
        require(ERC20Upgradeable(erc20OnMainnet).balanceOf(address(this)) == balanceBefore + amount)

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/MessageProxyForMainnet.sol#L205-L253 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/schain/MessageProxyForSchain.sol#L200-L227 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L415-L449

Vulnerability details

Impact

Message passing through the bridge occur in a two step process.

The first step is transferring the tokens or funds to the bridge for the chain we are currently on (e.g. if we are doing Eth->Schain the bridge smart contracts on the Eth chain will receive the tokens). In this was the ownership of the tokens of funds is transferred to the bridge (either a deposit box or a token manager).

The second step is undertaken by one of the validators for the SChain. One of the validators calls postIncomingMessages() on the other side of the bridge (e.g. Eth->SChain then we call postIncomingMessages() on the SChain). During postIncomingMessages() a batch of messages are processed individually, transferring the required assets to their destination.

An issue arises in the second step because any individual message that fails will be ignored. This is due to the try-catch statement in the function _callReceiverContract() when processing a message. This try-catch statement is necessary to prevent one failed message causing the entire transaction to revert and that would mean the bridge is stuck and can't process any messages for that SChain.

The funds are considered locked because in the case described above where step two fails due to the postMessage() reverting, the message has passed through step one, transferring the tokens into the bridge on one side. However, since the postMessage() has failed the tokens/funds are not transferred to the end receiver. There is no way to recover these stuck tokens and so they are lost in the bridge.

Proof of Concept

MessageProxyForMainnet.sol

    function postIncomingMessages(
        string calldata fromSchainName,
        uint256 startingCounter,
        Message[] calldata messages,
        Signature calldata sign
    )
        external
        override(IMessageProxy, MessageProxy)
        messageInProgressLocker
    {
        uint256 gasTotal = gasleft();
        bytes32 fromSchainHash = keccak256(abi.encodePacked(fromSchainName));
        require(_checkSchainBalance(fromSchainHash), "Schain wallet has not enough funds");
        require(connectedChains[fromSchainHash].inited, "Chain is not initialized");
        require(messages.length <= MESSAGES_LENGTH, "Too many messages");
        require(
            startingCounter == connectedChains[fromSchainHash].incomingMessageCounter,
            "Starting counter is not equal to incoming message counter");

        require(
            _verifyMessages(
                fromSchainName,
                _hashedArray(messages, startingCounter, fromSchainName),
                sign
            ),
            "Signature is not verified"
        );
        uint additionalGasPerMessage = 
            (gasTotal - gasleft() + headerMessageGasCost + messages.length * messageGasCost) / messages.length;
        uint notReimbursedGas = 0;
        for (uint256 i = 0; i < messages.length; i++) {
            gasTotal = gasleft();
            if (isContractRegistered(bytes32(0), messages[i].destinationContract)) {
                address receiver = _getGasPayer(fromSchainHash, messages[i], startingCounter + i);
                _callReceiverContract(fromSchainHash, messages[i], startingCounter + i);
                notReimbursedGas += communityPool.refundGasByUser(
                    fromSchainHash,
                    payable(msg.sender),
                    receiver,
                    gasTotal - gasleft() + additionalGasPerMessage
                );
            } else {
                _callReceiverContract(fromSchainHash, messages[i], startingCounter + i);
                notReimbursedGas += gasTotal - gasleft() + additionalGasPerMessage;
            }
        }
        connectedChains[fromSchainHash].incomingMessageCounter += messages.length;
        communityPool.refundGasBySchainWallet(fromSchainHash, payable(msg.sender), notReimbursedGas);
    }

MessageProxyForSchain.sol

    function postIncomingMessages(
        string calldata fromChainName,
        uint256 startingCounter,
        Message[] calldata messages,
        Signature calldata signature 
    )
        external
        override(IMessageProxy, MessageProxy)
    {
        bytes32 fromChainHash = keccak256(abi.encodePacked(fromChainName));
        require(connectedChains[fromChainHash].inited, "Chain is not initialized");
        require(messages.length <= MESSAGES_LENGTH, "Too many messages");
        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");
        for (uint256 i = 0; i < messages.length; i++) {
            _callReceiverContract(fromChainHash, messages[i], startingCounter + 1);
        }
        connectedChains[fromChainHash].incomingMessageCounter += messages.length;
        _topUpBalance();
    }

MessageProxy.sol

    function _callReceiverContract(
        bytes32 schainHash,
        Message calldata message,
        uint counter
    )
        internal
        returns (address)
    {
        if (!message.destinationContract.isContract()) {
            emit PostMessageError(
                counter,
                "Destination contract is not a contract"
            );
            return address(0);
        }
        try IMessageReceiver(message.destinationContract).postMessage{gas: gasLimit}(
            schainHash,
            message.sender,
            message.data
        ) returns (address receiver) {
            return receiver;
        } catch Error(string memory reason) {
            emit PostMessageError(
                counter,
                bytes(reason)
            );
            return address(0);
        } catch (bytes memory revertData) {
            emit PostMessageError(
                counter,
                revertData
            );
            return address(0);
        }
    }

Recommended Mitigation Steps

One solution is to store failed message in the MessageProxy.sol and allowing the receiver of the tokens or funds to replay the message, potentially with slightly different parameters.

Another option is to allow validators to vote on failed messages and have them returned to the owners on the original chain, though there is no guarantee the owner will be able to use the returned tokens (e.g. a smart contract whose accounting does not handle direct transfers or if this address is owned by a centralised exchange wallet).