Calldata check can not be added for adjacent parameters

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/c474b9480850d08514c100b415efcbc962608c62/src/Timelock.sol#L1028-L1155
https://github.com/code-423n4/2024-10-kleidi/blob/c474b9480850d08514c100b415efcbc962608c62/src/Timelock.sol#L1119-L1123

Vulnerability details

Explanation

The _addCalldataCheck function is used to enable certain addresses and selectors to be called by hot signers and run validation on their input data.
However, while checking the overlap of parameters, the validation is too restrictive, and results in making it impossible for two adjacent parameters to be added independently.

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

This happens because the startIndex is supposed to be inclusive, and the endIndex is exclusive. As an example, if the two first inputs of a function are of types address, the start and end index of the first one should be [4, 24), and the next variables should be at indexes [24, 44) of the calldata. And since the start index of the second variable is equal to the end index of the first variable, the check fails.

Impact

The comments in the code clearly mention that the separate checks should be possible. While this is still possible to do, the users need to merge all of the adjacent addresses they need together and add all of the accepted string separately. This will make the management of such calldata much harder than it should be.

As an example, for a function call signature(dataType, dataType, dataType, dataType, dataType), if for each of the first 4 fields there are 5 accepted strings, user should enter 4^5 = 1024 strings. And if at a later date they want to add a restriction on the 5th data type, they need to remove all the previous ones and add all the strings from the start. This problem increases exponentially with the number of inputs and their possible values, and can make it impossible for some situations to add the values. 10 parameters with 10 different values would take 10^10 restrictions in total which can be quite impossible for users to add.

Proof of Concept

The testWhitelistingBatchCalldataSucceeds test is slightly modified as below and can show that adjacent variables can not be added. The result fails with revert: CalldataList: Partial check overlap:

    function testWhitelistingBatchCalldataSucceeds() 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;

            uint16[] memory endIndexes = new uint16[](2);
            endIndexes[0] = 36;
            endIndexes[1] = 56;

            bytes[][] memory checkedCalldatas = new bytes[][](2);
            bytes[] memory checkedCalldata = new bytes[](1);
            checkedCalldata[0] = abi.encodePacked(address(timelock));
            checkedCalldatas[0] = checkedCalldata;
            checkedCalldatas[1] = checkedCalldata;

            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)
            });
        }

        address[] memory safeSigner = new address[](1);
        safeSigner[0] = address(this);

        safe.setOwners(safeSigner);

        address[] memory lendingAddresses = new address[](2);
        lendingAddresses[0] = address(lending);
        lendingAddresses[1] = address(lending);

        bytes[] memory lendingPayloads = new bytes[](2);
        lendingPayloads[0] = abi.encodeWithSelector(
            lending.deposit.selector, address(timelock), 100
        );
        lendingPayloads[1] = abi.encodeWithSelector(
            lending.withdraw.selector, address(timelock), 100
        );

        _executeWhitelistedBatch({
            caller: address(this),
            timelock: address(timelock),
            targets: lendingAddresses,
            values: new uint256[](2),
            payloads: lendingPayloads
        });

        Timelock.IndexData[] memory calldataChecks = timelock.getCalldataChecks(
            address(lending), MockLending.deposit.selector
        );

        assertEq(calldataChecks.length, 1, "calldata checks should exist");
        assertEq(calldataChecks[0].startIndex, 16, "startIndex should be 16");
        assertEq(calldataChecks[0].endIndex, 36, "startIndex should be 16");
        assertEq(
            calldataChecks[0].dataHashes[0],
            keccak256(abi.encodePacked(address(timelock))),
            "data should be correct"
        );

        return address(lending);
    }

Recommended Mitigation Steps

Change the code and use <= and >= instead of > and <.

                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