Intended configuration of recoveryDelay < timelockDelay allows for malicious takeover by recovery spells

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/RecoverySpell.sol#L95-L127

Vulnerability details

Vulnerability Details

Impact

RecoverySpell.sol states that it is of CRITICAL importance for the deployer to have recovery spells with the delay for the recoverySpell to become executable (hereafter referred to as recoveryDelay) shorter than the delay for a scheduled proposal to become executable in Timelock.sol (hereafter referred to as timelockDelay)

Even though it is only a comment, it says that the condition should be met by default and is basically a rule that should be followed, but that it is not possible to enforce that in the smart contracts.

This will make deployers to follow the comment's instructions. However, if the recovery spell is malicious, they will lose their safe.

Proof of Concept

1. The attacker creates a recovery spell with recoveryDelay = 10 days.

2. The deployer creates a new system instance with the malicious recovery spell and a timelockDelay of 20 days. He chooses this delay because of the comment that says it is of critical importance for the following to be true: recoveryDelay < timelockDelay.

3. The recoveryDelay runs out and the malicious recovery spell gets executed.

Recommended Mitigation Steps

The timing constraints recommended to be used by the protocol are unsafe and the protocol needs to assess which timing constraints are not exploitable, if such timing constraints exist.

It should also be assessed whether it is possible to enforce the timing constraints in the smart contracts.

Assessed type

Timing

[ElliotFriedman]

This is a known issue, already covered in the docs, see guardian scenarios https://github.com/code-423n4/2024-10-kleidi/blob/main/docs/EDGECASES.md#guardian

[ElliotFriedman]

all of these parameters will be enforced from the frontend as there is no way to enforce them onchain without revealing the recovery spells

[GalloDaSballo]

I don't believe this is valid

There is no one intended configuration, the EDGECASES and Readme make it clear that it "depends on who you trust"

This seems like a known feature of the system

[c4-judge]

GalloDaSballo marked the issue as nullified