Even if the users use batch operations, removing calldata with indexes can cause unexpected behaviours

[howlbot-integration]

Lines of code

https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/Timelock.sol#L1193-L1243

Vulnerability details

Proof of Concept

In the timelock contract, removing calldata can be done by using indexes. Each calldata is stored in a list in the contract states and it can be removed from the timelock using indexes including the delay. The delay is important thing here because in timelock we cannot create calldata for timelock itself, therefore, any removing calldata action should be scheduled by Gnosis safe. The delay can be determined while scheduling but it should be higher than min delay. The problem in this timelock design is indexes. They can cause unexpected behaviours while execution timestamp even though they're accurate while scheduling due to delay mechanism in the timelock.

Actually, it's also reported by Alex in Recon report but in mitigation part, they decided to use batch scheduling mechanism in order to be sure about execution order of tasks.

Q-2
Mitigation
It's important to document the risks of not using batch operations to end users
As long as end users batch their operations, no meaningful risk should be present

But even if the users use batch operations for executions removing the calldata with index is not a safe method due to high delays between the operations.

function _removeCalldataCheck(
        address contractAddress,
        bytes4 selector,
        uint256 index
    ) private {
        Index[] storage calldataChecks =
            _calldataList[contractAddress][selector];
        /// if no calldata checks are found, this check will fail because
        /// calldataChecks.length will be 0, and no uint value can be lt 0
        require(
            index < calldataChecks.length,
            "CalldataList: Calldata index out of bounds"
        );

        /// index check to remove by overwriting with ending list element
        Index storage indexCheck = calldataChecks[index];

        uint16 removedStartIndex = indexCheck.startIndex;
        uint16 removedEndIndex = indexCheck.endIndex;
        bytes32[] memory removedDataHashes = indexCheck.dataHashes.values();

        for (uint256 i = 0; i < removedDataHashes.length; i++) {
            assert(indexCheck.dataHashes.remove(removedDataHashes[i]));
        }

        /// pop the index without swap if index is same as last index
        if (calldataChecks.length > 1) {
            /// index check to overwrite the specified index check with
            Index storage lastIndexCheck =
                calldataChecks[calldataChecks.length - 1];

            indexCheck.startIndex = lastIndexCheck.startIndex;
            indexCheck.endIndex = lastIndexCheck.endIndex;
            bytes32[] memory dataHashes = lastIndexCheck.dataHashes.values();

            for (uint256 i = 0; i < dataHashes.length; i++) {
                assert(indexCheck.dataHashes.add(dataHashes[i]));
                assert(lastIndexCheck.dataHashes.remove(dataHashes[i]));
            }
        }

        calldataChecks.pop();

        emit CalldataRemoved(
            contractAddress,
            selector,
            removedStartIndex,
            removedEndIndex,
            removedDataHashes
        );
    }

Let's examine the following scenario:

Note: Assume multisig is used by single user for simplicity

1. Our user's minimum timelock delay is 2 days and the timelock has 4 calldata whitelistings.
2. He wants to remove the last calldata whitelisting after 7 days.
3. He scheduled that action using index 3 which is correct and valid action.
4. This action can be done after 7 days.
5. After 2 days, he decided to delete first calldata from the timelock with 2 days delay.
6. After 2 days again, he removed the first calldata from the timelock.
7. Now, our last calldata swapped with the deleted calldata and our first execution won't remove the correct calldata at that moment.
8. He decided to use a new calldata and scheduled it for after 2 days.
9. 2 days later, he executed that operation and now index 3 is holding a different calldata.
10. 1 days later, our first remove action can be executed but it will remove wrong calldata.

In this scenario, we can't use batch operations because our user's decisions has time gaps and this is why this is still a problem here.

Impact

Actually, I believe this issue is between Medium/Low. Because user can always cancel the wrong proposals. But in reality, it can be disaster for the real users because they should always check whether the new scheduled action affect the existing live proposals and executions can be done by anyone after the delay period, it should also be considered in this situation. I believe judge can decide a correct severity for this report but I am marking it as medium for now.

Recommended Mitigation Steps

In the mitigation part, my professional advice is using nonce for the scheduled executions and while removing them using this nonce is better than the current execution flow.

Assessed type

Timing

[ElliotFriedman]

This logic will need to be enshrined on the frontend.

My guess is that destructive actions that remove checks will execute on a fairly infrequent basis, and actions that add checks will happen much more frequently.

You are right that it is hard to reason about this kind of thing on the frontend.

What we can do is say, if a user tries to modify an index they already have a timelocked operation for, do not allow them to make modifications to that same index as it is too hard to reason about.

Will leave severity to @GalloDaSballo as this may have been already covered in his report before the c4 competition.

[GalloDaSballo]

I think the only 2 rational ways to look at this are:

1. Invalid as known, since I have sent this exact "undefined execution order" finding
2. Valid QA; since I sent it, but there's some extra nuance that should be considered

I do not believe we can consider the finding as valid, because the pre-condition is a set of owners that don't cancel the proposals, meaning their behaviour creates undefined execution because they themselves allow it, while it being known (from my report)

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[DemoreXTess]

After second consideration I believe this issue is a medium risk issue. I want to add my comment based on #20 (comment)

For the first part ( Invalid Known Issue ), it shouldn't be classified as known issue because in your Recon report for this issue marked as acknowledged based on the following comment:

Q-2
Mitigation
It's important to document the risks of not using batch operations to end users
As long as end users batch their operations, no meaningful risk should be present

In my report, I demonstrated that a meaningful risk still exists even if users batch their operations in this scenario. Due to time gaps, it’s not always feasible to mitigate this risk effectively with batch operations alone. So, it shouldn't be classified as known issue.

Second part ( Valid QA ): Initially, when I encountered this issue in the codebase, I was uncertain about its severity, considering it to fall between Medium and Low. However, upon further analysis, I now believe it should be classified as Medium severity. This is a multi-signature wallet used by multiple users, so there is a greater likelihood of this scenario occurring. Users may not be able to track or execute actions in the correct order.

If, at timestamp X, a user decides to remove calldata A with a delay t, this scheduled execution should reliably remove calldata A after timestamp X + t regardless of any subsequent actions. However, due to index-based calldata removal, this may not happen as expected, highlighting a flaw in the current design. There is no user mistake but user can still face with the bug.

Med: Assets not at direct risk, but the function of the protocol or its availability could be impacted, or leak value with a hypothetical attack path with stated assumptions, but external requirements.

As the definition of medium severity issue in C4, it should be a medium severity issue.

My proposed mitigation, which uses a nonce for tracking, provides a more accurate and reliable method for removing the correct calldata.

[GalloDaSballo]

Per the Supreme Court verdict we have to assume users performing reasoned operations
I have sent a known finding about how things can get out of order and that can cause issues
Using batches ensures that for a specific check the behaviour is deterministic, if we assume that the people have read the finding and they execute the batch before performing other operations

For this reason, I think QA is the most appropriate severity

[DemoreXTess]

@GalloDaSballo

I am assuming users performing reasoned operations. There are time gaps between the executions. Timelock can accept up to 30 days delay. Users can't use batch operations with this time gaps. Can you please reconsider the PoC that the issue reported because I used only reasoned operations in PoC.