QA Report

[code423n4]

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).

[DimaStebaev]

It is a known issue. Some particular examples are listed here. We put attention of developers on it in our documentation.

[GalloDaSballo]

Dup of #23

[JeeberC4]

Creating QA Report for warden as judge downgraded. Preserving original title: Failed Message in postIncomingMessage() Result in Locked Fund