Guardian can be DoSed by submitting many transactions at once

[c4-bot-6]

Lines of code

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

Vulnerability details

Impact

The Guardian role is supposed to pause the timelock in case of compromised cold signer keys, as explained in the docs. The pause() function will pause the contract and remove all current live proposals, allowing time for a Recovery spell to rotate to a new set of signers. However, the pause() function does an unbounded loop over the current live proposals, an array that the attackers can add any number of entries to. The attackers can flood the array with entries, essentially making calls to function revert due to running out of gas, thus DoSing the Guardian and essentially rendering the role useless.

If the team does not notice the malicios activity on time and the timelock is not deployed/executed before its too late, the attackers can do serious damage, like draining of funds/complete takeover of the system. Since the guardian role is rendered useless, the protocol will be in a race with the clock to save itself.

Proof of Concept

We can see that the pause function will load the _liveProposals array into memory and iterate over all of its entries, removing each of them from the unbounded array:

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

    function pause() public override {
        /// check that msg.sender is the pause guardian, pause the contract
        super.pause();

        bytes32[] memory proposals = _liveProposals.values();
        for (uint256 i = 0; i < proposals.length; i++) {
            bytes32 id = proposals[i];

            delete timestamps[id];
            assert(_liveProposals.remove(id));

            emit Cancelled(id);
        }
    }

Consider the following attack scenario:

• A malicios entity manages to compromise the cold singer keys, enough of them to sign transactions - say 2 out of 3 keys.
• The malicios entity begins submitting massive amounts of malicios transactions to drain the timelock.
• The guardian attempts to pause the contract, but due to the large amount of entries in the liveProposals array, the transaction reverts.
• As a result the guardian is rendered useless by the attackers.

Also, note that the malicios signers can frontrun calls to Timelock::cancel since Safe::execTransaction uses a nonce to check validity. By increasing the nonce in a frontrunning transaction they will make honest transactions revert. So, honest signers can be blocked from manually canceling the malicios transactions.

The protocol team will have the last option of rotating out the signers using a recovery spell. However, if this action is not taken before the time left for the malicios transcation to become valid is less than the recovery spell delay, the malicios transactions will go through.

Furthermore, while only the Safe can schedule tranasactions, anyone can execute them. This means that even if the protocol manages to execute the recovery spell on time, they will still be in a race with the clock to cancel all pending malicios transactions. There is no function to batch cancel, only the cancel function to cancel individual transactions, while the attackers have the option to batch schedule. They can submit any number of transactions before the recovery spell comes into effect. As a result the honest signers may not have enough time to cancel all transactions before the timelock delay exprises and a malicios tranasction is executed taking over the protocol.

Recommended Mitigation Steps

Move the logic to remove entries from the live proposals contract into a separate function that is callable only when the contract is paused - say emergencyBatchCancel(). The function must have the functionality to remove a specifiable number of entries, so that it is not DoSable. Also, consider adding a normal batchCancel function for easier cleanup in general.

Assessed type

DoS

[ElliotFriedman]

Batch scheduled operations resolve to a single proposal id, so it is the same amount of transactions to cancel as it is to propose.

How many proposals would a malicious user have to propose in order to DoS the pause function?

Also, since this is deleting values in storage, it is eligible for a gas refund, so unless you could make so many entries that it would be impossible to loop over all of them due to the block gas limit, I would think the refund cancels out the majority of the cost, meaning it only costs a few hundred gas per transaction cancelled.

[GalloDaSballo]

I think the unbounded pause is a valid concern, I'll crunch the numbers on what it will take to do vs cancel
Overall this looks like something that may need to be mitigated in the case in which pause would actually be Dossable

Considering that some chains have different max gas (Need to check scope), this could be a bigger concern for chains that have a cap below 30MLN (IIRC OP is capped at 15 MLN)

Overall this is a legitimate concern

[ElliotFriedman]

@GalloDaSballo lmk what you find on the PoC side.

[GalloDaSballo]

I believe this finding to be valid, as a Medium Severity bug

I will follow up with math, but fundamentally:

• The cost to attack is the same as to defend (gas limit wise)
• The gas limit doesn't account for refunds, refunds are applied after
• This leads me to believe that as long as you setup enough calls, the pause will break

I believe this can be elegantly mitigated by using the time at which the pause ends, and making any proposal that is created or altered before that marked as invalid.

Please double check my logic @ElliotFriedman

[c4-judge]

GalloDaSballo changed the severity to 2 (Med Risk)

[GalloDaSballo]

I think the argument for Med is:

• The guardian can prevent this in most cases

The argument for High would be:

• The cost of mitigating the attack is as high as the cost of performing it, meaning a proper setup of the attack makes it unavoidable

Will need to write the code for this specifically:

• Cap 30MLN gas
• Attack (create as many malicious proposals)
• Defend (pause)

If Defend consumes less gas than that means that attack requires more than one block, meaning it could be prevented

Whereas if they consume similar amounts of gas the attack could have not been prevented via pausing

[c4-judge]

GalloDaSballo marked issue #24 as primary and marked this issue as a duplicate of 24

[ElliotFriedman]

mitigated solidity-labs-io/kleidi#53

[c4-judge]

GalloDaSballo marked the issue as partial-75