Calldata checks require an unvalidated byte in between calldata segments which leads to flawed validation

[howlbot-integration]

Lines of code

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

Vulnerability details

Vulnerability Details

Impact

When a call is whitelisted, either during the creation of the system instance or when a proposal to addCalldataCheck() or addCalldataChecks() is executed, the required data needs to be provided.

This includes, among other things, the startIndex and endIndex of each calldata segment.

During the whitelisting process, _addCalldataCheck() in Timelock.sol is called. This function checks that the startIndex and endIndex of the calldata do not partially overlap with other indexes.

However, this check incorrectly interprets the calldatas, which could result in the execution to be completely different. For example, an amount that needs to be transferred could be larger than the user intended.

Proof of Concept

The following test needs to be added to Timelock.t.sol:

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

    bytes4[] memory selectors = new bytes4[](2);
    selectors[0] = MockLending.deposit.selector;
    selectors[1] = MockLending.deposit.selector;

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

    uint16[] memory endIndexes = new uint16[](2);
    endIndexes[0] = 5;
    endIndexes[1] = 6;

    bytes[][] memory checkedCalldatas = new bytes[][](2);
    /* @audit
    We want to add a check for the fith byte which is "1" (start index 4),
    and a check for the sixth byte which is "2" (start index 5).
    But it's not possible due to the wrong "partial overlap" check.
    To add a check for the fith byte, the end index needs to be 5 but
    this is the same as the start index for the sixth byte.
    */
    checkedCalldatas[0] = new bytes[](1);
    checkedCalldatas[0][0] = bytes("1");
    checkedCalldatas[1] = new bytes[](1);
    checkedCalldatas[1][0] = bytes("2");

    vm.prank(address(timelock));
    vm.expectRevert("CalldataList: Partial check overlap");
    timelock.addCalldataChecks(
        targetAddresses,
        selectors,
        startIndexes,
        endIndexes,
        checkedCalldatas
    );      
}

The test first sets the necessary variables and then calls addCalldataChecks() as the timelock.

The important variables are the start and endIndexes, which are set as follows:

startIndexes[0] = 4;
startIndexes[1] = 5;

endIndexes[0] = 5;
endIndexes[1] = 6;

Currently, due to a bug in the code, it is not possible to add these two calldata segments because one unvalidated byte is required between them.

Further details are described in the comments within the test.

The cold signers would provide calldata checks in such a way that there is no revert while also expecting that there is no unvalidated byte. As a result the validation that is expected by the cold signers deviates from the validation that is actually performed and unintented calldata can slip through and be executed by the hot signers.

Recommended Mitigation Steps

The following changes should be made in Timelock._addCalldataCheck():

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

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

Assessed type

Invalid Validation

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)