Unable to create a calldata check immediately adjacent to an existing one

[howlbot-integration]

Lines of code

https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/Timelock.sol#L1120-L1121

Vulnerability details

Impact

This vulnerability will prevent the creation of a calldata check map as intended.

Proof of Concept

The Timelock::checkCalldata() function verifies the whitelisted status of calldata by invoking BytesHelper.getSlicedBytesHash() which internally calls BytesHelper.sliceBytes().

src/Timelock.sol#L495-L502

            require(
                calldataCheck.dataHashes.contains(
                    data.getSlicedBytesHash(
                        calldataCheck.startIndex, calldataCheck.endIndex
                    )
                ),
                "CalldataList: Calldata does not match expected value"
            );

The sliceBytes() function grabs a slice of bytes by start and end indexes.

    function sliceBytes(bytes memory toSlice, uint256 start, uint256 end)
        public
        pure
        returns (bytes memory)
    {
        ... ...

        uint256 length = end - start;
        bytes memory sliced = new bytes(length);

        for (uint256 i = 0; i < length; i++) {
            sliced[i] = toSlice[i + start];
        }

        return sliced;
    }

From the above code snippet, we can see that the function takes a slice by the range [start, end - 1].

However, when creating a calldata check map by using the Timelock::_addCalldataCheck()function, the function checks the overlapping status between new range and existing ones by the following lines:

    function _addCalldataCheck(
        address contractAddress,
        bytes4 selector,
        uint16 startIndex,
        uint16 endIndex,
        bytes[] memory data
    ) private {
        ... ...
        uint256 targetIndex = indexes.length;
        {
            bool found;
            for (uint256 i = 0; i < indexes.length; i++) {
                ... ...
                require(
                    startIndex > indexes[i].endIndex
                        || endIndex < indexes[i].startIndex,
                    "CalldataList: Partial check overlap"
                );
            }
            ... ...
        }
        ... ...
    }

Let's assume S1, E1 is an existing range. Then, the new range to be added can be put into either within [E1+1, +Inf) or [4, S1-1].

But, as sliceBytes() takes a slice from S1 to E1 - 1, the index E1 cannot be occupied by calldata checks.

If there are several ranges, their end indexes will not be included as calldata check region.

Tools Used

Manual Review

Recommended Mitigation Steps

There are 2 alternative mitigation ways:

1. Use GTE and LTE comparison operators rather than GT and LT like the below:

    function _addCalldataCheck(
        address contractAddress,
        bytes4 selector,
        uint16 startIndex,
        uint16 endIndex,
        bytes[] memory data
    ) private {
        ... ...
        uint256 targetIndex = indexes.length;
        {
            bool found;
            for (uint256 i = 0; i < indexes.length; i++) {
                ... ...
                require(
-                   startIndex > indexes[i].endIndex
-                       || endIndex < indexes[i].startIndex,
+                   startIndex >= indexes[i].endIndex
+                       || endIndex <= indexes[i].startIndex,
                    "CalldataList: Partial check overlap"
                );
            }
            ... ...
        }
        ... ...
    }

2. Or, the sliceBytes() function takes a slice of bytes from start to end, not to end - 1.

Assessed type

Invalid Validation

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[GalloDaSballo]

@ElliotFriedman pls check the comment here, which seems to tie #2 to #17

Lmk if you mitigate them differently