It's impossible to add a calldata check for neighboring byte portions (and more implications)

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/Timelock.sol#L1119-L1122,https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/Timelock.sol#L496-L501

Vulnerability details

Summary

_addCalldataCheck and checkCalldata are not fine tuned to work correctly when checking neighboring byte portions - impossible to add two calldata checks which are neighboring.

Vulnerability details

When adding a calllData check it does not allow to add check whose startIndex or endIndex are overlapping or EQUAL.

require(
    startIndex > indexes[i].endIndex // <-- any new addition CAN'T have startIndex equal endIndex
      || endIndex < indexes[i].startIndex, 
    "CalldataList: Partial check overlap"
);

This means the closest checks possible will look like this (example using 4 byte portions):
{ startIndex: 4, endIndex: 8, ... } and { startIndex: 9, endIndex: 13 ... }

And on the surface this may look fine, but when we reach checkCalldata, it is calling getSliceBytesHash -> which uses sliceBytes function underneath:

require(
    calldataCheck.dataHashes.contains(
        data.getSlicedBytesHash(  // <-- is used to slice and hash
            calldataCheck.startIndex, calldataCheck.endIndex
        )
    ),
    "CalldataList: Calldata does not match expected value"
);

And if we look at sliceBytes function:

function sliceBytes(bytes memory toSlice, uint256 start, uint256 end) {
...
        uint256 length = end - start;
        bytes memory sliced = new bytes(length);

        for (uint256 i = 0; i < length; i++) {
            sliced[i] = toSlice[i + start]; // <-- slice a portion from start NOT including endIndex [start; end)
        }
...
}

It counts bytes from start index (i starts at 0) and keeps slicing (end - start -> 8 - 4 = 4) times. This means 4th,5th,6th and 7th bytes are sliced. This also means that the endIndex itself (8) is NOT going to be included in the slice.

From here we see that there is a discrepancy:

• when adding new checks -> you CAN'T set the startIndex (9) to equal any existing endIndex (8)
• when checking -> endIndex (8) is always SKIPPED in slicing

Which creates a 1 byte gap any time more than 1 check (with a different byte portion) is added to a single calldata.

Impact

Can't add neighboring calldata checks, because the checked indexes are applied on wrong pieces of calldata bytes. Also because the slicing is applied incorrectly, the calldata check will fail if calldata is the exact size as the checks (due to 1 byte shift, needs to have an extra byte at the end).

Tools used

Manual review + foundry tests

Proof of Concept

Lets use an example calldata with portions as follows:
0x42424242111111112222222233

arg1 and arg2 are 4 bytes each
arg3 is a single byte

[selector] [arg1 ] [arg2 ] [arg 3]
0x42424242 11111111 22222222 33

foundry test

   function testAddArbitraryCheckSucceeds() public {
        address[] memory targetAddresses = new address[](2);
        targetAddresses[0] = address(0x1);
        targetAddresses[1] = address(0x1);

        bytes4[] memory selectors = new bytes4[](2);
        selectors[0] = bytes4(0x42424242);
        selectors[1] = bytes4(0x42424242);

        /// compare first 20 bytes
        uint16[] memory startIndexes = new uint16[](2);
        startIndexes[0] = 4;
        startIndexes[1] = 9;

        uint16[] memory endIndexes = new uint16[](2);
        endIndexes[0] = 8;
        endIndexes[1] = 13;
        uint256 numberOfMatches = 1;

        bytes[][] memory checkedCalldatas = new bytes[][](2);
        checkedCalldatas[0] = new bytes[](numberOfMatches);
        checkedCalldatas[1] = new bytes[](numberOfMatches);

        checkedCalldatas[0][0] = abi.encodePacked(bytes4(0x11111111));
        checkedCalldatas[1][0] = abi.encodePacked(bytes4(0x22222222));

        vm.prank(address(timelock));
        timelock.addCalldataChecks(
            targetAddresses,
            selectors,
            startIndexes,
            endIndexes,
            checkedCalldatas
        );


        timelock.checkCalldata(
            targetAddresses[0],
            abi.encodePacked(
                bytes4(0x42424242), // selector
                bytes4(0x11111111), // arg 1
                bytes4(0x22222222), // arg 2
                bytes1(0x33)        // arg 3
            )
        );
    }

This test will fail with "CalldataList: Calldata does not match expected value"

I've replaced the getSlicedBytesHash with explicit keccak256(sliceBytes(toSlice, start, end)), to get the output of sliceBytes in logs and on the failed test case they are as follows:

    ├─ [16540] Timelock::checkCalldata(ECRecover: [0x0000000000000000000000000000000000000001], 0x42424242111111112222222233)
    │   ├─ [824] BytesHelper::getFunctionSignature(0x42424242111111112222222233) [delegatecall]
    │   │   └─ ← [Return] 0x42424242
    │   ├─ [2176] BytesHelper::sliceBytes(0x42424242111111112222222233, 4, 8) [delegatecall]
    │   │   └─ ← [Return] 0x11111111 // <-- first slice is correct
    │   ├─ emit Log(data: 0)
    │   ├─ [2176] BytesHelper::sliceBytes(0x42424242111111112222222233, 9, 13) [delegatecall]
    │   │   └─ ← [Return] 0x22222233 // <-- includes the last byte - it should't
    │   └─ ← [Revert] revert: CalldataList: Calldata does not match expected value
    └─ ← [Revert] revert: CalldataList: Calldata does not match expected value

This also means that calldata of exact size (without the 0x33) at the end will outright revert due to:
End index is greater than the length of the byte string. Which adds more trouble to the same problem

Recommended Mitigation Steps

Fine tunning of checks in adding and checking is required - two approaches come to mind:

• Allow _addCalldataCheck to add different portion checks with matching startIndex and endIndex
• Update the slicing in checkCalldata to include the endIndex

Both of these would need another re-check if it doesn't break code elsewhere.

Assessed type

Invalid Validation

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)