Invalid Validation in _addCalldataCheck is causing dead bytes in payload

[howlbot-integration]

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#L50-L55

Vulnerability details

Vulnerability Details

In Kleidi protocol, whitelisting a calldata feature is used for restricting the parameters of the specific functions at specific external contracts. Hot signers only can use whitelisted parameters. The calldata indexes are defined as bytes and the overlapping between the checks are not allowed for security considerations. This check is applied in following line:

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

This check is prevent overlapping between the calldata checks. But this check doesn't correctly handle the edge case for the endIndex. Let say we have a function which takes two parameter as follows:

test(uint256 parameter1, bytes32 parameter2)

The calldata payload for this function will be 4 bytes selector + 32 bytes parameter1 + 32 bytes parameter2. As we mentioned before it's bytes type parameter and it slice the bytes data using indexes as follows in BytesHelper.sol contract:

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

        for (uint256 i = 0; i < length; i++) {
            sliced[i] = toSlice[i + start];
        }

Let's continue with our example, our parameter1's startIndex should be 4 because selector is represented in [0,3] (both inclusive) range. Our length for uint256 parameter will be 32 and the endIndex for parameter1 should be 4 + 32 = 36. The range of parameter will be [4,36). The index 36 is exclusive in here because if we make both 4 and 36 inclusive the length will be 33. Basicly, index 36 is not used in here by our first calldata check. When user want to restrict the bytes32 parameter the index range for parameter2 should be [36,68). But we can't use index 36 in here because it is the endIndex of our first calldata check.

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

This vulnerability will cause dead bytes between the parameters. If we have n amount of parameters and we want to restrict the parameters for all of them the dead bytes amount will be n - 1.

Impact

Those dead bytes will cause wildcarded parameters because if a selector in a contract is restricted by only 1 parameter the all the other parameters is wildcarded by default which is a design choice for the protocol. We can understand that behaviour in following calldata check:

        require(
            calldataChecks.length > 0, "CalldataList: No calldata checks found"
        );

        for (uint256 i = 0; i < calldataChecks.length; i++) {
            Index storage calldataCheck = calldataChecks[i];

            if (calldataCheck.startIndex == calldataCheck.endIndex) {
                return;
            }

            require(
                calldataCheck.dataHashes.contains(
                    data.getSlicedBytesHash(
                        calldataCheck.startIndex, calldataCheck.endIndex
                    )
                ),
                "CalldataList: Calldata does not match expected value"
            );
        }

If a function has at least 1 calldata check and if it passes then the payload is whitelisted in here.

Deep dive to the impact

Let say in a function we have 5 parameters and we want to restrict the parameters for the hotsigners and all the parameters are uint256 type. The only way to restrict it is using following ranges [4,36) [37, 68) [69, 100) [101, 132), [133, 164). Following indexes are dead right now: 36, 68, 100, 132 and they are automatically wildcarded. Normally, hot signers can only use that function with that specific parameters but now due to dead bytes ( there are 4 dead bytes ). They can call many other calldata payload by changing the first 8 bits of the parameters. If any hot signer is compromised, the problem becomes much bigger. Multisig wallet can face with huge loses even though only safe calldatas are added to the system.

The following statement in EDGECASES.md file will be wrong. It doesn't guarantee a match the whitelisted calldata and the protocol's main purpose is being ensure about safety even if the user signed a malicious transaction off-chain but it's not possible in current implementation.

The timelock calldata whitelisting feature acts as an onchain policy engine, enforcing that transactions the hot signers send are guaranteed to match the whitelisted calldata. This is a powerful feature that can be used to enforce that the hot signers can never lose funds or sign messages that can drain the wallet. This speeds the process of interacting with DeFi protocols for power users who want the ability to quickly interact with DeFi protocols without the cognitive overhead of checking calldata byte-by-byte.

Coded PoC

The following add calldata test will fail. In this PoC we restrict both address of deposit and deposit amount by the calldata check. You can add this to ./test/unit/Timelock.t.sol and call it with forge test --match-test testWhitelistingBatchCalldataSucceeds2 -vvv command to see where the error occur.

    function testWhitelistingBatchCalldataSucceeds2() public returns (address) {
        address[] memory targets = new address[](1);
        targets[0] = address(timelock);

        uint256[] memory values = new uint256[](1);
        values[0] = 0;

        {
            bytes[] memory payloads = new bytes[](1);
            payloads[0] = abi.encodeWithSelector(
                timelock.grantRole.selector,
                timelock.HOT_SIGNER_ROLE(),
                address(this)
            );

            _scheduleBatch({
                caller: address(safe),
                timelock: address(timelock),
                targets: targets,
                values: values,
                payloads: payloads,
                salt: bytes32(0),
                delay: MINIMUM_DELAY
            });

            vm.warp(block.timestamp + MINIMUM_DELAY);

            _executeBatch({
                caller: address(this),
                timelock: address(timelock),
                targets: targets,
                values: values,
                payloads: payloads,
                salt: bytes32(0)
            });
        }

        MockLending lending = new MockLending();

        {
            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] = 16;
            startIndexes[1] = 36; // Start index for deposit amount

            uint16[] memory endIndexes = new uint16[](2);
            endIndexes[0] = 36;
            endIndexes[1] = 68; // End index for deposit amount

            bytes[][] memory checkedCalldatas = new bytes[][](2);
            bytes[] memory checkedCalldata = new bytes[](1);
            bytes[] memory checkedCalldata2 = new bytes[](1);
            checkedCalldata[0] = abi.encodePacked(address(timelock));
            checkedCalldatas[0] = checkedCalldata;
            checkedCalldata2[0] = abi.encodePacked(uint256(100 ether)); // Allow only 100 ether calls
            checkedCalldatas[1] = checkedCalldata2;

            bytes[] memory datas = new bytes[](1);
            datas[0] = abi.encodeWithSelector(
                timelock.addCalldataChecks.selector,
                targetAddresses,
                selectors,
                startIndexes,
                endIndexes,
                checkedCalldatas
            );

            _scheduleBatch({
                caller: address(safe),
                timelock: address(timelock),
                targets: targets,
                values: values,
                payloads: datas,
                salt: bytes32(0),
                delay: MINIMUM_DELAY
            });

            bytes32 id =
                timelock.hashOperationBatch(targets, values, datas, bytes32(0));

            assertEq(
                timelock.timestamps(id),
                block.timestamp + MINIMUM_DELAY,
                "operation should be scheduled"
            );
            assertTrue(timelock.isOperation(id), "operation should be present");
            assertFalse(
                timelock.isOperationReady(id), "operation should not be ready"
            );
            assertFalse(
                timelock.isOperationDone(id), "operation should not be done"
            );

            assertEq(
                timelock.getAllProposals()[0],
                id,
                "proposal should be in proposals"
            );
            assertEq(
                timelock.getAllProposals().length,
                1,
                "proposal length incorrect"
            );

            vm.warp(block.timestamp + MINIMUM_DELAY);

            _executeBatch({
                caller: address(this),
                timelock: address(timelock),
                targets: targets,
                values: values,
                payloads: datas,
                salt: bytes32(0)
            });
        }
    }

Recommended Mitigation

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

The endIndex of previous parameter should be able to equal to the next parameter's startIndex and also the next parameter's startIndex should be able to equal to previous parameters's endIndex

Assessed type

Invalid Validation

[c4-judge]

GalloDaSballo marked the issue as satisfactory

[DemoreXTess]

After second consideration, I believe it should be a high severity problem. Because in current implementation, if there are at least 1 calldata check for a specific parameter in a selector, it means all the other parameters are wildcarded. This behaviour is design choice, we don’t have problem until here.

Actually, we can still solve revert problem for every parameter which has lower than 248 bits because only first 8 bits can't be used due to dead index. So, we can actually restrict the data just ignoring the first byte of the parameter but dead bytes cannot be restricted in current circumstances and this situation makes dead bytes wildcarded again. If any hot signer is compromised then attacker can directly execute harmful actions using this bug without waiting for the execution delay and it may cause direct loss of funds.

High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

I believe this situation makes the bug high severity.

[GalloDaSballo]

I'm not convinced this is different from #2 and the reliance on additional conditions doesn't fundamentally change the underlying problem, nor it's impact
Not changing the severity nor the uniqueness of the finding

[DemoreXTess]

@GalloDaSballo

There is misunderstanding. I would like to explain myself. I am not stating it's different than #2. I am stating the severity for both #2, duplicates and #54 should be high. #2 doesn't fully express the impact which is why the impact looks like medium severity.

My argument is dead bytes is wildcarded due to this bug and after applying the restrictions to the calldatas, hot signers will be able to call the functions with many other parameters. If hot signers are compromised, then it may cause direct loss of funds because hot signers can directly call without timelock delay. I believe it should be high severity because of direct loss of funds.