Some bytes may be emitted when we check the call data

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/BytesHelper.sol#L35-L58

Vulnerability details

Proof of Concept

In Timelock contract, we can assign hot assign's valid behaviour via addCalldataChecks. The hot signers can execute the whitelist calldata transaction.
The problem is that some bytes in the whole calldata will be missed in the calldata check. This will decrease the calldata check's safety.

When we try to add several index, we have one check to make sure there will be any overlap between two different index.
For example:
The first index is from 4 to 8. (startIndex=4, endIndex=8)
If we want to add one second index, the minimum startIndex should be 9. Imagine the second index is from 9 to 12.(startIndex=9, endIndex=12)
The problem is that when we want to check the first index, we will get one sliced bytes via sliceBytes(). The byte index 4,5,6,7 will be included into the return value sliced. The endIndex 8 for the first index is not included into the sliced.
When we go on checking the second index, we will get another sliced bytes, the byte index 9,10,11 will be included the second sliced.
From the above example, we will miss the byte index 8 in the calldata checking even if we don't leave any gap between the first index and the second index.

The impact for this is serious. Because that some input parameters cannot be checked via checkCalldata.

    function _addCalldataCheck(
        address contractAddress,
        bytes4 selector,
        uint16 startIndex,
        uint16 endIndex,
        bytes[] memory data // Here the data is bytes[], 
    ) private {
        ...
                require(
                    startIndex > indexes[i].endIndex
                        || endIndex < indexes[i].startIndex,
                    "CalldataList: Partial check overlap"
                );
        ...
}

    function sliceBytes(bytes memory toSlice, uint256 start, uint256 end)
        public
        pure
        returns (bytes memory)
    {
        require(
            start < toSlice.length,
            "Start index is greater than the length of the byte string"
        );
        require(
            end <= toSlice.length,
            "End index is greater than the length of the byte string"
        );
        require(start < end, "Start index not less than end index");

        uint256 length = end - start;
        bytes memory sliced = new bytes(length);
        // For example, start = 4, end = 8
        // The first byte is toSlice[start] = toSlice[4], the last byte is toSlice[7]
        for (uint256 i = 0; i < length; i++) {
            sliced[i] = toSlice[i + start];
        }

        return sliced;
    }

Recommended Mitigation Steps

If we keep current sliceBytes's implementation, it means that when we check one index(startIndex, endIndex), the startIndex will be included into this index calldata check and endIndex will be not included into this calldata check.
Based on above implementation, when we create one second index, the second index's startIndex can equal to the previous index's endIndex.

                require(
                    startIndex > indexes[i].endIndex
                        || endIndex < indexes[i].startIndex,
                    "CalldataList: Partial check overlap"
                );

Assessed type

Context

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)