Lack of Validation for Recovery Spell Addresses in InstanceDeployer To Ensure They're actually Recovery Spell Contracts, This May Lead to Enabling Non-functional or Malicious Contract/EOA as Safe Modules

[howlbot-integration]

Lines of code

https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/InstanceDeployer.sol#L274-L291

Vulnerability details

Description

In the InstanceDeployer::createSystemInstance() function, there is no validation to ensure that the addresses provided in the recoverySpells array are valid contracts and support the RecoverySpell model. This could result in the deployment of a safe with enabling non-functional or even Malicious modules, causing harm to entire system.

Impact

If EOA or contracts that are not actually RecoverySpell contracts, are passed as recovery spells when deploying the safe and Timelock, there's high possiblity that instanceDeployer will enable non-functional or malicious modules. This could lead to not having a backup plan if safe owners lost keys since no recovery mechanism is set for safe, or in the case of a malicious module, result in actions such as adding/removing owners and modules from the safe, scheduling operations on the timelock, which can even lead to stealing funds, or even removing the hot signer role from a legitimate hot signer.

Proof Of Concept

we need to ensure the recovery spell addresses on #L274-L291 are actually Recovery Spell contracts, but such validation is missing.

    function createSystemInstance(NewInstance memory instance) external returns (SystemInstance memory walletInstance) {

        // ignore above codes...

            for (uint256 i = 0; i < instance.recoverySpells.length; i++) {

                calls3[index++].callData = abi.encodeWithSelector(
                    ModuleManager.enableModule.selector,
@>                  instance.recoverySpells[i] // there's no check to ensure `recoverySpells[i]` is a contract and is actually a Recovery Spell contract.
                );
            }

        // rest of the code...

Recommended Mitigation

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

    @notice this function is gonna be used to ensure `recoverySpell` address
            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 validation to ensure that each address in recoverySpells array is a valid contract and supports the RecoverySpell model inside the InstanceDeployer::createSystemInstance() function:

    function createSystemInstance(NewInstance memory instance) external returns (SystemInstance memory walletInstance) {

        /*
            Rest of the Code...
        */


            for (uint256 i = 0; i < instance.recoverySpells.length; i++) {
+               require(IRecoverySpell(instance.recoverySpells[i]).isRecoverySpell(), "Invalid RecoverySpell contract"); 
                calls3[index++].callData = abi.encodeWithSelector( ModuleManager.enableModule.selector, instance.recoverySpells[i] );
            }


         /*
            Rest of the Code...
         */
    }

Assessed type

Other

[ElliotFriedman]

You misunderstand how the system works and it's clear you did not read the docs.

The recovery spell is hidden and not deployed until it is summoned.

There will never be validation in instance deployer that the recovery spell is a contract, because recovery spells are not deployed until they are used, which would be after the system has already been deployed.

[GalloDaSballo]

This is a valid comment, that I had already surfaced

The lack of validation is not a bug as the spell MUST not be revealed (else it would be harmed)

Seems invalid

[GalloDaSballo]

This was flagged here:
https://github.com/GalloDaSballo/kleidi-notes?tab=readme-ov-file#q-24-original-hot-signer-that-becomes-compromised-or-malicious-can-take-over-all-undeployed-systems

[c4-judge]

GalloDaSballo marked the issue as unsatisfactory:
Out of scope