addCalldataCheck() will always revert when more than 1 parameter is whitelisted for the callDatas.

[howlbot-integration]

Lines of code

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

Vulnerability details

Proof of Concept

Due to an invalid check of startIndex and endIndex, parameters for the functionSelectors cannot be added to the callDataCheck.dataHash correctly.

lets say , safe wants to whiteList mul2numbers(bytes2 x,bytes2 y) from a contract Multiply so that hot signers can execute mul operations from timelock.

safe will schedule an operation to add this callDatacheck so that when the operation is ready to execute, timeLock can execute addCallDataCheck (a user will be callling execute() to execute this operation).

timelock.addCalldataCheck(
        contractAddress, selector, startIndex, endIndex, datas
);

Now to whitelist a bytes2 value (hex("1234")) for the parameter x , the inputs datas passed would be

    contractAddress : address(`Multiply`)
    selector : timelock.addCalldataCheck.selector,
    startIndex:4
    endIndex:6
    datas:hex("1234")

and then safe decided to whitelist another bytes2 value (hex("1234")) for the parameter y , here the inputs datas passed would be

    contractAddress : address(`Multiply`)
    selector : timelock.addCalldataCheck.selector,
    startIndex:6
    endIndex:8
    datas:hex("3456")

This should efficiently store both these parameters inside the callDataChecks since

hex(1234) will occupy the callDatas's 4th index and 5th index (total occupying 2 bytes spaces) and
hex(3456) will occupy the callDatas's 6th index and 8th index (total occupying 2 bytes spaces)

There shouldnot not be any overlap.

But,
due to the check in the _addCalldataCheck()

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

It will always revert.

startIndex can equal to endIndex of newly added callDataCheck since while adding a callData value into a callDataCheck , it will only occupy from startIndex to endIndex - 1 of it.

POC

forge test --match-test testDCantAddDatastoCallDataCheck -vvv

    function testDCantAddDatastoCallDataCheck() public {
        bytes[] memory datasX = new bytes[](1);
        datasX[0] = hex"1234"; //it is a 2 bytes long data

        vm.prank(address(timelock));
        timelock.addCalldataCheck(
            address(10000), timelock.addCalldataCheck.selector, 4, 6, datasX
        ); //occupy 4th and 5th index

        bytes[] memory datasY = new bytes[](1);
        datasY[0] = hex"3456"; //it is a 2 bytes long data
        vm.prank(address(timelock));
        vm.expectRevert("CalldataList: Partial check overlap"); // It will revert with this message 
        timelock.addCalldataCheck(
            address(10000), timelock.addCalldataCheck.selector, 6, 8, datasY
        ); //should occupy 6th and 7th index but it will revert
    }

when adding the datasY , the addCalldataCheck will revert.

Impact

The safe cannot whitelist callDatachecks for functions with more than 1 parameters.

Recommended Mitigation Steps

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

Assessed type

Other

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)