Infinite Recursion in `getMessageType()`

[code423n4]

Lines of code

https://github.com/skalenetwork/ima-c4-audit/blob/main/contracts/Messages.sol#L195-L202

Vulnerability details

Impact

The function getMessageType() may cause infinite recursion.

Consider the situation where data is 0x0000...00 (for 32 bytes). We will have the situation where

firstWord = abi.decode(0x00...00, (uint256)) => firstWord = 0

Thus, we hit the conditional statement

if (firstWord % 32 == 0) { since 0 % 32 == 0

So we will execute

return getMessageType(data[firstWord:]);

where data[firstWord:] = data[0:] = data and thus we have just call getMessageType(data) exactly as we did originally. After which the steps above will repeat and there will be another layer of recursion.

The impact of this vulnerability is that the function will eventually run out of memory and revert.

Proof of Concept

    function getMessageType(bytes calldata data) internal pure returns (MessageType) {
        uint256 firstWord = abi.decode(data, (uint256));
        if (firstWord % 32 == 0) {
            return getMessageType(data[firstWord:]);
        } else {
            return abi.decode(data, (Messages.MessageType));
        }
    }

Recommended Mitigation Steps

Consider checking if firstWord == 0 in which case the MessageType is EMPTY.

An alternative solution would be to revert if firstWord == 0 to save gas for the caller.

[cstrangedk]

Recursion is necessary to handle, for example, ERC1155 message structure, see: https://github.com/skalenetwork/ima-c4-audit/blob/11d6a6ae5bf16af552edd75183791375e501915f/contracts/Messages.sol#L186-L189

This is a complex struct of struct within struct. ABI of the struct will have shifts in the data, and one will need to pass the shift to get the MessageType. For this reason, recursion is implemented. This can cause an infinite recursion if one adds an index of MessageType that is % 32. But this is not possible, since message types are additive and for each 32 block an EMPTY message type will be added.

At the very least, an in-line doc comment describing this recursion requirement and MessageType may help clarify to external contributors. Suggest 0 (Non-critical)

[GalloDaSballo]

I believe that the warden finding has validity, however the impact is minimized by the fact that the worst case scenario is a revert (no state changes), additionally, per the sponsor comment, recursion is necessary to unpack some of the data.

I believe that there may be edge cases where this finding can cause problems, however in lack of a POC, I agree with the sponsor with a non-critical severity