Wrong handling of call data check indices, forcing it sometimes to revert

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/main/src/Timelock.sol#L1136
https://github.com/code-423n4/2024-10-kleidi/blob/main/src/BytesHelper.sol#L50

Vulnerability details

Description

Cold signers can add call data checks as whitelisted checks that hot signers could execute without timelocks, the call data checks depend on the indices of the encoded call. However, the protocol invalidly handles these indices in 2 separate places:

1. https://github.com/code-423n4/2024-10-kleidi/blob/main/src/Timelock.sol#L1136
2. https://github.com/code-423n4/2024-10-kleidi/blob/main/src/BytesHelper.sol#L50

Where length is computed as end index—start index, which is usually wrong as index subtraction needs +1 to be translated to a length. For most of the scenario, this is okay, however, if a parameter that is being checked filled all of its bytes then this would be an issue (PoC is an example), for example, a uint256 filling all of its 32 bytes.
NB: This is not caught in the unit tests because there isn't any test that checks this edge case, where a parameter that fills all its bytes is being checked.

This forces the whitelisted call to revert.

Proof of Concept

The following PoC shows a scenario where an infinite approval call is being whitelisted, we don't want to allow fewer approvals (only uint256 max), so the encoding of the call:

abi.encodeWithSelector(IERC20.approve.selector, owner, amount)

results in the following bytes string:

0x095ea7b30000000000000000000000001eff47bc3a10a45d4b230b5d10e37751fe6aa718ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff

To have an infinite approval call whitelisted we need to add conditions on both the spender and the amount:

1. Spender: 0x1efF47bc3a10a45D4B230B5d10E37751FE6AA718
2. Amount: 115792089237316195423570985008687907853269984665640564039457584007913129639935

For the spender part, it's straightforward where we need to check from index 16 to 35 (1eff47bc3a10a45d4b230b5d10e37751fe6aa718 from the encoded bytes), however, passing 16 and 35 will cause the TX to revert with CalldataList: Data length mismatch, this is where the issue starts, we pass 16 to 36, but now 36 is the start of the unit max. And we pass 37 to 69, to have the whole 32 bytes included (unit max fills all 32 bytes), passing the end index less than 69 reverts.

Now, when the whitelisted call is triggered with the above params, the TX will revert with End index is greater than the length of the byte string, and this is because the amount's byte length is 68 while the end index is 69.

As a result: wrong index/length handling => forcing to pass incorrect params.

Coded POC:

Add the following test in test/integration/System.t.sol, and run it using forge test -vv --fork-url "https://mainnet.infura.io/v3/PROJECT_ID" --fork-block-number 20515328 --mt test_DaiTransfer_withoutPlus1:

function test_DaiTransfer_withoutPlus1() public {
    address owner = vm.addr(pk1);

    address[] memory owners = new address[](1);
    owners[0] = owner;

    address[] memory hotSigners = new address[](1);
    hotSigners[0] = HOT_SIGNER_ONE;

    vm.prank(HOT_SIGNER_ONE);
    SystemInstance memory wallet = deployer.createSystemInstance(
        NewInstance({
            owners: owners,
            threshold: 1,
            recoverySpells: new address[](0),
            timelockParams: DeploymentParams(
                MIN_DELAY,
                EXPIRATION_PERIOD,
                guardian,
                PAUSE_DURATION,
                hotSigners,
                new address[](0),
                new bytes4[](0),
                new uint16[](0),
                new uint16[](0),
                new bytes[][](0),
                bytes32(0)
            )
        })
    );

    Timelock timelock = wallet.timelock;

    uint256 amount = type(uint256).max;
    bytes4 selector = IERC20.approve.selector;

    console.logBytes(abi.encodeWithSelector(selector, owner, amount));

    uint16 startIdx = 16;
    uint16 endIdx = 36;
    bytes[] memory data = new bytes[](1);
    data[0] = abi.encodePacked(owner);

    vm.prank(address(timelock));
    timelock.addCalldataCheck(dai, selector, startIdx, endIdx, data);

    startIdx = 37;
    endIdx = 69;
    data = new bytes[](1);
    data[0] = abi.encodePacked(amount);

    vm.prank(address(timelock));
    timelock.addCalldataCheck(dai, selector, startIdx, endIdx, data);

    assertEq(IERC20(dai).allowance(address(timelock), owner), 0);

    vm.prank(HOT_SIGNER_ONE);
    vm.expectRevert(
        bytes("End index is greater than the length of the byte string")
    );
    timelock.executeWhitelisted(
        address(dai),
        0,
        abi.encodeWithSelector(selector, owner, amount)
    );
}

Correct test with the mitigation implemented:

function test_DaiTransfer_withPlus1() public {
    address owner = vm.addr(pk1);

    address[] memory owners = new address[](1);
    owners[0] = owner;

    address[] memory hotSigners = new address[](1);
    hotSigners[0] = HOT_SIGNER_ONE;

    vm.prank(HOT_SIGNER_ONE);
    SystemInstance memory wallet = deployer.createSystemInstance(
        NewInstance({
            owners: owners,
            threshold: 1,
            recoverySpells: new address[](0),
            timelockParams: DeploymentParams(
                MIN_DELAY,
                EXPIRATION_PERIOD,
                guardian,
                PAUSE_DURATION,
                hotSigners,
                new address[](0),
                new bytes4[](0),
                new uint16[](0),
                new uint16[](0),
                new bytes[][](0),
                bytes32(0)
            )
        })
    );

    Timelock timelock = wallet.timelock;

    uint256 amount = type(uint256).max;
    bytes4 selector = IERC20.approve.selector;

    console.logBytes(abi.encodeWithSelector(selector, owner, amount));

    uint16 startIdx = 16;
    uint16 endIdx = 35;
    bytes[] memory data = new bytes[](1);
    data[0] = abi.encodePacked(owner);

    vm.prank(address(timelock));
    timelock.addCalldataCheck(dai, selector, startIdx, endIdx, data);

    startIdx = 36;
    endIdx = 67;
    data = new bytes[](1);
    data[0] = abi.encodePacked(amount);

    vm.prank(address(timelock));
    timelock.addCalldataCheck(dai, selector, startIdx, endIdx, data);

    assertEq(IERC20(dai).allowance(address(timelock), owner), 0);

    vm.prank(HOT_SIGNER_ONE);
    timelock.executeWhitelisted(
        address(dai),
        0,
        abi.encodeWithSelector(selector, owner, amount)
    );

    assertEq(IERC20(dai).allowance(address(timelock), owner), amount);
}

Recommended Mitigation Steps

In BytesHelper.sol:

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

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

    ...
}

In Timelock.sol:

function _addCalldataCheck(
    address contractAddress,
    bytes4 selector,
    uint16 startIndex,
    uint16 endIndex,
    bytes[] memory data
) private {
    ...

    for (uint256 i = 0; i < data.length; i++) {
        /// data length must equal delta index
        require(
-           data[i].length == endIndex - startIndex,
+           data[i].length == endIndex - startIndex + 1,
            "CalldataList: Data length mismatch"
        );
        bytes32 dataHash = keccak256(data[i]);

        /// make require instead of assert to have clear error messages
        require(
            indexes[targetIndex].dataHashes.add(dataHash),
            "CalldataList: Duplicate data"
        );
    }

    ...
}

Assessed type

Math

[ElliotFriedman]

Good finding, valid medium!

[GalloDaSballo]

Seems to be very closely related to #2

I'd be careful about mitigating these, prob best to use both cases for tests and then mitigate in one go

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

I'm not fully confident this bug is not different from the addCalldataChecks, checking in with the Sponsor to see how the bugs are mitigated

[DemoreXTess]

This submission is duplicate of #54. Let me explain the details:
The issues are completely same the only difference is mitigation. The problem in here is dead byte while calldata adding to timelock. Basicly endIndex is not included in both current parameter and the next parameter. This situation makes that bytes is wildcarded by default and if we try to add those calldatas using correct indexes, it will revert. All the details are covered in #54 but I want to explain some of my other investigations here.

test(bytes32 a, bytes32 b)

Let say we want to create a calldata check for both parameters as above

If we mitigate the issue as #54 did, addCalldataCheck will allow endIndex as a startIndex of the next parameter and indexes should be following format:

Notation will be in (x,y) format x is startIndex and y is endIndex.

(0,4) is selector, (4,36) is first parameter, (36, 68) is second parameter
Right now there is no revert problem in adjacent parameters as #17 mentioned.
But if we mitigate the issue as #17 did, it should be in following format:
(0,4) is selector, (4,35) is first parameter, (36, 67) is second parameter

By the way selector index is handled differently and overlapping of index 4 doesn't cause any problem.

In conclusion, it only affects the frontend, the UI should use the above parameters for correct usage. There are no separate two issues.

In addition, if we apply both mitigation in same time, the problem becomes much bigger than this. #54 ‘ mitigation is allowing endIndex to be used by the next parameter. #17 doesn’t allow this, instead it makes the endIndex inclusive for current calldata. If we apply both mitigation it will cause overlapping between the parameters because endIndex will be available storage index for both parameters.

[royalsalute]

@DemoreXTess 's comment is correct. This finding originates from the same validation with #2 and other dups of #2.

And the protocol team already fixed the issue as indicated in the recommended mitigation of #2. (solidity-labs-io/kleidi#51)

[0xAlix2]

@GalloDaSballo - Thank you for the quick review! I’m the author of this issue, and while I agree that it is a duplicate of the other "consecutive parameters" issues, I believe this one highlights the problem with the most logical solution. Let me explain.

We are dealing with indices, and it doesn’t make sense to select a parameter from index 0 to 6 by passing 0 to 7. Consider the simplest example of selecting a subset of an array (0 to 6); it’s illogical to specify 0 -> 7. The following code feels intuitively incorrect, and I believe the wardens would concur that it appears to be off by one:

data[i].length == endIndex - startIndex

If these were named by positions, it might make more sense. However, as it stands, it feels wrong.

According to the documentation:

All of these AND checks will have non-overlapping start index to end index range amongst each other.

Sharing the end index somewhat undermines this principle.

Could this be addressed on the frontend? Certainly, but we also need to consider users interacting with this at the contract level, where indices will simply be passed as "known indices" (start index -> end index) rather than (start index -> end index + 1). This means that to pass a consecutive parameter, one would have to use an index that’s already been utilized, despite the documentation stating that this is not allowed. 🤔

In conclusion, I recognize that this issue duplicates the others mentioned, but I firmly believe it is the only one that presents the "correct solution" rather than just the "simplest solution" and it should be the selected report for this issue. It would be great if we could get the sponsors to give their thoughts on this.

Thank you!

[GalloDaSballo]

@ElliotFriedman can you please confirm if you fixed this issue separately, or if you fixed it by fixing the finding from #2 ?

[c4-judge]

GalloDaSballo marked the issue as duplicate of #2

[GalloDaSballo]

As discussed am making a duplicate of the rest of the reports tied to indices

I will re-view one last time to identify the best report

@ElliotFriedman would appreciate if can re-link all fixes tied to indices as to ensure the issues and gotchas were fixed

[ElliotFriedman]

Mitigated with this PR https://github.com/solidity-labs-io/kleidi/pull/54/files

[ElliotFriedman]

All other changes were backed out as we realized the previous ways of fixing things were incomplete.

[c4-judge]

GalloDaSballo marked the issue as not selected for report

[c4-judge]

GalloDaSballo marked the issue as selected for report

[GalloDaSballo]

Marking best per the selected fix, as discussed above the finding highlights the same issue as it's duplicate

However, the mitigation the sponsor took matches the one provided in this report

Given that, and the fact that a coded POC is provided, I agree with making this the selected report

[thebrittfactor]

C4 staff have added the satisfactory label for awarding needs.