Recovery Failure Due to Ablity to Set Contracts as RecoverySpell Owners

[howlbot-integration]

Lines of code

https://github.com/solidity-labs-io/kleidi/blob/0d72b6cb5725c1380212dc76257da96fcfacf22f/src/RecoverySpellFactory.sol#L37-L78

Vulnerability details

Description

The createRecoverySpell() function in the RecoverySpellFactory contract, allows setting contracts as owners of a RecoverySpell. When initiating a recovery, owners need to sign a message, but since contracts cannot sign messages like EOAs, this will cause the recovery of safe to be impossible. If the recovery cannot be initiated, the safe and timelock will become unusable and leaving the safe and timelock unmanageable forever.

Impact

Setting contracts as owners in a RecoverySpell will cause the recovery process to fail because signatures are required for initiating recovery. this makes it impossible to recover the safe and as result timelock funds will be locked, since safe is the one that has ablity to manage timelock funds.

Proof Of Concept

On #L58-L71, we do owner validation, but we dont make sure the owners are EOA's, capable of signing messages, so they can initiate successful recovery of safe:

    function createRecoverySpell(
        bytes32 salt,
        address[] memory owners,
        address safe,
        uint256 threshold,
        uint256 recoveryThreshold,
        uint256 delay
    ) external returns (RecoverySpell recovery) {
        
        // ignore extra code above these for loop...

        for (uint256 i = 0; i < owners.length; i++) {
            address owner = owners[i];
            bool found;
            assembly ("memory-safe") {
                found := tload(owner)
                if eq(found, 0) { tstore(owner, 1) }
            }

            require(!found, "RecoverySpell: Duplicate owner");
        }

        // ignore rest of the code for now

    }

Recommended Mitigation

add a check in the createRecoverySpell() function to ensure that all owners are externally owned accounts (EOAs) and not contracts.

This can be done by implementing a helper function isContract() inside the RecoverySpellFactory that checks if an address is a contract or not:

    function isContract(address account) internal view returns (bool) {
        return account.code.length > 0;
    }   

Add this check during the owner validation loop to prevent contracts from being set as owners of RecoverySpell:

        function createRecoverySpell(
        bytes32 salt,
        address[] memory owners,
        address safe,
        uint256 threshold,
        uint256 recoveryThreshold,
        uint256 delay
    ) external returns (RecoverySpell recovery) {

        _paramChecks(owners, threshold, recoveryThreshold, delay);
        require(safe.code.length != 0, "RecoverySpell: safe non-existent");

        for (uint256 i = 0; i < owners.length; i++) {
            address owner = owners[i];
            bool found;
            assembly ("memory-safe") {
                found := tload(owner)
                if eq(found, 0) { tstore(owner, 1) }
            }

+           require(!isContract(owners[i]), "RecoverySpellFactory: Contract owners not allowed");
            require(!found, "RecoverySpell: Duplicate owner");
        }

        recovery = new RecoverySpell{salt: salt}(
            owners, safe, threshold, recoveryThreshold, delay
        );

        emit RecoverySpellCreated(address(recovery), address(safe));
    }

This will ensure that all owners are EOAs, capable of signing necessary message, and as result be able to have successful recovery of safe.

Assessed type

Other

[ElliotFriedman]

good catch!

[ElliotFriedman]

On further thought, if we add this check, it would block creation of a recovery spell if the owner was a contract, even if the recovery spell could reach quorum, so I think this would introduce a new issue.

It does make sense that this check is added, but just to the calculateAddress function and not in a way that blocks the creation of a recovery spell.

This way, counterfactual deployments are allowed inside of recovery spells and can't block a pre-existing recovery spell.

[GalloDaSballo]

I'm not convinced this is a valid medium, the finding asserts that the owner cannot be a contract:

1. We cannot know since we could be in owner constructor (to bypass the check), or the contract could be yet to be deployed
2. Technically speaking a Contract and EOA can have a clash (see the usual Create2 debate), so technically a contract can sign the tx (because a clash was found)
3. I believe this would fall as misconfiguration either way
4. Having a X/X threshold is also another level of misconfiguration, since having an invalid owner is not necessarily a condition for revert on signature check (just don't pass that account as one of the signing owners)

Overall seems like a QA report, happy to hear from the Warden on this one

[c4-judge]

GalloDaSballo changed the severity to QA (Quality Assurance)

[GalloDaSballo]

I think this is a borderline self-rekt
As the intended usage is for signers that can sign, setting up a "dead-weight" signer is something people can do, but at their own risk

[c4-judge]

GalloDaSballo marked the issue as grade-c