Unable to set to check consecutive parameters

[c4-bot-10]

Lines of code

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

Vulnerability details

Impact

Unable to check hot signer call parameters precisely.

Proof of Concept

When checking call parameters, the [start, end) of the call parameters, that is, from index [start] to [end-1], are parsed and checked.

For example, let's say start is 4 and end is 6. sliceBytes parses 2 bytes, and the parsed bytes are data[4] and data[5].

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 (uint256 i = 0; i < length; i++) {
@>      sliced[i] = toSlice[i + start];
    }

    return sliced;
}

Therefore, when verifying parameters of a function with multiple parameters, the startIndex of the next parameter should be set to be the same as the endIndex of the previous parameter(When parameters are contiguous).

For example, let's say we allow a function deposit(uint16, uint16) and want to check both parameters. To check this, the call parameters are parsed as follows:

• function signature: data[0]~data[3]
• first parameter: data[4]~[19]
• second parameter: data[20]~[35]

To parse this, the Index should be set as follows:

• Index for checking the first parameter(_calldataList[contractAddress][selector][0])
  • startIndex: 4, endIndex: 20
• Index for checking the second parameter(_calldataList[contractAddress][selector][1])
  • startIndex: 20, endIndex: 36

However, when registering the Index, it is restricted from setting the next startIndex to be the same as the previous endIndex. Therefore, it is not possible to check consecutive parameters.

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

This is the PoC:

1. First, we demonstrate that sliceBytes parses [start]~[end-1]. Add this test at BytesHelper.t.sol file to run it.

   function testPoCSliceBytes() public {
       bytes memory toSlice = hex"00010203040506070809"; // [0,1,2,3,4,5,6,7,8,9]
       
       bytes memory parsed = toSlice.sliceBytes(4, 6); // toSlice[4]~toSlice[5] = 0x0405
   
       require(parsed[0] == 0x04);
       require(parsed[1] == 0x05);
   
       parsed = toSlice.sliceBytes(6, 8); // toSlice[6]~toSlice[7] = 0x0607
       require(parsed[0] == 0x06);
       require(parsed[1] == 0x07);
   }

2. This demonstrates that in addCalldataCheck, it's not possible to set the next startIndex to be the same as the previous endIndex, making it impossible to set up checking for consecutive parameters. Add this test at CalldataList.t.sol file to run it.

   function testPoCAddCalldataCheckFails() public {
       bytes[] memory datas = new bytes[](1);
       datas[0] = hex"0405";
   
       // first param 2 byte
       vm.prank(address(timelock));
       timelock.addCalldataCheck(
           address(lending), MockLending.deposit.selector, 4, 6, datas
       ); // this is for calldata[4]calldata[5]
   
       // second param 2 byte
       datas = new bytes[](1);
       datas[0] = hex"0607";
   
       vm.prank(address(timelock));
       vm.expectRevert("CalldataList: Partial check overlap");
       timelock.addCalldataCheck(
           address(lending), MockLending.deposit.selector, 6, 8, datas
       ); // this is for calldata[6]calldata[7]
   }

Tools Used

Manual Review

Recommended Mitigation Steps

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

Assessed type

Other

[ElliotFriedman]

This is a good finding, and it makes sense that it is a medium issue.

[GalloDaSballo]

This seems to be valid, will check POC and mitigation as well

[GalloDaSballo]

Great catch, thank you!

[GalloDaSballo]

It seems like this and #17 are dups / tighly related

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

Leaving 2 and 17 as separate for now, I think mitigation will require both, but they seem to be 2 instances of 2 different off by one errors

[ElliotFriedman]

fix is in here: solidity-labs-io/kleidi#51

[DemoreXTess]

I am requesting re-considering the selected report. This report doesn't correctly states the problem. The PoC is using uint16 data type but in low-level .call() usage all the parameters should be padded to 32 bytes by default in EVM.
For instance, if we have test(uint16 a, uint16 b) function, the payload will be:
4 bytes selector + 30 bytes zero padding + 2 bytes parameter a + 30 bytes zero padding + 2 bytes parameter b

So, the demonstrated calldata is completely wrong in the selected report. Uint16 is not affected in here because EVM will already revert if we send only 2 bytes data for uint16, instead we should send 32 bytes with padded zeros and we don’t need to restrict first 8 bits of the parameter because if it’s not zero, EVM will revert for us. In order to see the effect of the bug we can only look at the data types which has 32 bytes.
In #54, I covered many impacts which is related with the issue:

1. Dead bytes will be wildcarded by default. ( The design choice will affect the severity of the bug )

2. It contradicts against EDGECASES.md

   In conclusion, the issue doesn't affect any parameter which has lower than 32 bytes. You can investigate the issue with much more details in Invalid Validation in _addCalldataCheck is causing dead bytes in payload #54 submission. I covered all the impacts and situations in here. I believe this submission is not worthy to 30% selected report bonus. I believe Invalid Validation in _addCalldataCheck is causing dead bytes in payload #54 ‘s coverage about the issue is better than this report, please reconsider it.

[c4-judge]

GalloDaSballo marked the issue as not selected for report

[c4-judge]

GalloDaSballo marked issue #17 as primary and marked this issue as a duplicate of 17

[c4-judge]

GalloDaSballo marked the issue as satisfactory