Malicious `destinationContract` Could Cause Bridge To Become Stucl

[code423n4]

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/MessageProxy.sol#L415-L449 https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/mainnet/MessageProxyForMainnet.sol#L235-L250

Vulnerability details

Impact

In the function _callReceiverContract() if a malicious destinationContract is set which does not return an address then the transaction will revert.

The try-catch statement seen in the code snippet below for the function _callReceiverContract() has returns (address receiver). Solidity will then perform abi.decode(returndata, (address)) on the return data of postMessage() assuming the function does not revert (i.e. is successful).

However, this poses a threat to the system as abi.decode() will revert if there are insufficient bytes to decode into an address i.e. if returndata is less than 20 bytes. Since a malicious destinationContract has control over the size of the returndata they could send less than 20 bytes which would cause _callReceiverContract() to revert.

The impact of _callReceiverContract() reverting is the postIncomingMessages() would revert for that SChain. Therefore the bridge will still since no incoming messages could be processed.

Proof of Concept

        try IMessageReceiver(message.destinationContract).postMessage{gas: gasLimit}(
            schainHash,
            message.sender,
            message.data
        ) returns (address receiver) {
            return receiver;
        } catch Error(string memory reason) {

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

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

Recommended Mitigation Steps

Consider removing the else statement in MessageProxyForMainnet.sol seen in the snippet below. This would prevent processing messages where the desitantionContract is not registered by the protocol.

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

[DimaStebaev]

In this case only single SKALE chain <> Ethereum connection will be affected. Other chains still will be able to send messages. In addition to this not everyone can send messages via IMA. SKALE chain owner explicitly whitelist entities that are allowed to send messages and it can ensure that a receiver is not a malicious smart contract. By default only TokenManagers and DepositBoxs smart contracts are used and they cover needs of majority of developers. Obviously extending of IMA functionality is more dangerous than using out of the box functionality.

[GalloDaSballo]

While I agree with the warden that a griefing destinationContract could be set to cause reverts, and that those reverts would be try/catched, I don't believe that any meaningful disruption has been proven in this finding.

Ultimately if the 16*2/3 validators sign a message that will cause this, they would ultimately achieve their self-determined goal of having some transaction revert.

I believe a different argument for the bridge being custodial (which has been made) can be brought up, however this is not the case for this finding.

I believe the finding to be valid but it's impact to be minimal, I think Low Severity to be more appropriate