Missing New Recovery Spell Replacement in the Recovery Process, To Ensure Safe Can Be Saved Again

[howlbot-integration]

Lines of code

https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/RecoverySpell.sol#L154-L317

Vulnerability details

Description

The RecoverySpell::executeRecovery() function in the RecoverySpell contract is designed to recover the safe by replacing recovery spell owners with safe owners that lost keys.

However, once this recovery process is executed, the recovery spell is effectively a one-time-use operation, we will leaving no backup recovery mechanism in place for the safe.

If new owners of safe lose their keys again after recovery and safe has no other recovery spells enabled as module(backup plan), owners of safe will be unable to recover their safe again.

This lack of a fallback could result in new safe owners being unable to use safe forever and interact with Timelock and most likely losing funds stored inside the Timelock, if the RecoverySpell used to recover the safe, was their last recovery plan.

Impact

In a situations where single RecoverySpell contract is the last recovery mechanism set in place for the safe, if the keys are lost again after a successful recovery, the new owners will have no way to regain access to safe again.

This creates a critical situation where we have inablity to restore our funds from Timelock since We can't access Safe to Schedule Operations.

Proof Of Concept

Lets say our safe has only single Recovery Method Available. after using it and recovering our safe, we will leaving our safe with no backup plan, meaning if we forget to set new RecoverySpell as safe module, there's no way to recover our safe if new owners lost private keys again.

so for that reason it is critical to enforce owners of RecoverySpell, to add replacement as recovery method, in safe recovery process.

Recommended Mitigation

1. add the following function inside to RecoverySpell.sol:

    /*
    @notice this function is gonna be used to ensure the `newRecoverySpell`
            is valid contract address, not eq to ADDRESS_ZERO and is actually a
            Recovery Spell contract.
    */
    function isRecoverySpell() public pure returns(bool) {
        return true
    }

2. add a new address parameter called newRecoverySpell to the executeRecovery() function. This parameter should take the address of a new recovery spell, which will be added as a safe module during the recovery process. This will ensure that a replacement recovery mechanism is set up when the current spell is used. The modified process could look like this:

    function executeRecovery(
        address previousModule,
+       address newRecoverySpell
        uint8[] calldata v,
        bytes32[] calldata r,
        bytes32[] calldata s
    ) external {
        
+       require(newRecoverySpell != address(this) && newRecoverySpell != address(0)); // ensure new Recovery Spell is not the same contract we are initiating recovery and is valid address
+       require(isRecoverySpell(newRecoverySpell)); // ensure new Recovery Spell is contract and is actually a Recovery Spell
+       require(!safe.isModuleEnabled(newRecoverySpell)); // ensure new Recovery Spell is not safe module already

        // rest of the code...

+       IMulticall3.Call3[] memory calls3 = new IMulticall3.Call3[](owners.length + existingOwnersLength + 2); // + 2 for setting `newRecoverySpell` as new safe module and remove this contract as safe module.

        uint256 index = 0;

        for (uint256 i = 0; i < existingOwnersLength - 1; i++) {
            calls3[index++].callData = abi.encodeWithSelector(
                OwnerManager.removeOwner.selector,
                SENTINEL,
                existingOwners[i],
                1
            );
        }

        calls3[index++].callData = abi.encodeWithSelector(
            OwnerManager.swapOwner.selector,
            SENTINEL,
            existingOwners[existingOwnersLength - 1],
            owners[0]
        );

        for (uint256 i = 1; i < owners.length; i++) {
            calls3[index++].callData = abi.encodeWithSelector(
                OwnerManager.addOwnerWithThreshold.selector, owners[i], 1
            );
        }

        calls3[index++].callData = abi.encodeWithSelector(
            OwnerManager.changeThreshold.selector, threshold
        );

+       calls3[index++].callData = abi.encodeWithSelector(
+           ModuleManager.enableModule.selector, newRecoverySpell
+       );        

        calls3[index].callData = abi.encodeWithSelector(
            ModuleManager.disableModule.selector, previousModule, address(this)
        );

        // rest of the code...
    }

Assessed type

Other

[c4-judge]

GalloDaSballo marked the issue as nullified