RecoverySpellFactory::_paramChecks doesn't check if an owner is the same as the safe making it possible to create unusable recovery spells

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/main/src/RecoverySpellFactory.sol#L118-L138

Vulnerability details

Description

Recovery spells are custom recovery mechanisms that can be used to recover funds in case of lost keys. They can be used to rotate the signers of a multisig, or to create other custom recovery logic. They are created after the wallet/safe creation, but their addresses are predicted and added beforehand to the wallet/safe creation, this logic is handled in RecoverySpellFactory.

When a recovery spell is executed, it removes/swaps/adds the safe owners, this logic is done in the Owner module, and some checks are done upon owners' rotation, one of them is that the new owner shouldn't be the safe itself, else the rotation will revert.

require(owner != address(0) && owner != SENTINEL_OWNERS && owner != address(this), "GS203");

However, this check is not available in the RecoverySpellFactory, this allows a malicious owner to construct, inject, and deploy a recovery spell that can never be used.

Proof of Concept

Add the following test in test/integration/RecoverySpells.t.sol, and run it using forge test -vv --fork-url "https://mainnet.infura.io/v3/PROJECT_ID" --fork-block-number 20515328 --mt test_NotValidatingSafeOwner:

address SENTINEL = address(0x1);
address PREDICTED_WALLET_ADDRESS =
    0xF5BFbBb117CecEB3911084D47d047b380a7E4Eba;

function test_NotValidatingSafeOwner() public {
    NewInstance memory instance;

    address owner = vm.addr(11);
    address owner2 = vm.addr(12);

    // Wallet owners
    address[] memory owners = new address[](1);
    owners[0] = owner;

    // Recovery spell owners, contains PREDICTED_WALLET_ADDRESS
    address[] memory recoveryOwners = new address[](2);
    recoveryOwners[0] = owner2;
    recoveryOwners[1] = PREDICTED_WALLET_ADDRESS;

    address[] memory hotSigners = new address[](1);
    hotSigners[0] = HOT_SIGNER_ONE;

    // Prepare recovery spells
    address[] memory recoverySpells = new address[](1);
    recoverySpells[0] = address(
        recoveryFactory.calculateAddress(
            bytes32(0),
            recoveryOwners,
            PREDICTED_WALLET_ADDRESS,
            1,
            1,
            1
        )
    );

    // Deploy wallet
    vm.prank(HOT_SIGNER_ONE);
    SystemInstance memory wallet = deployer.createSystemInstance(
        NewInstance({
            owners: owners,
            threshold: 1,
            recoverySpells: recoverySpells,
            timelockParams: DeploymentParams(
                MIN_DELAY,
                EXPIRATION_PERIOD,
                guardian,
                PAUSE_DURATION,
                hotSigners,
                new address[](0),
                new bytes4[](0),
                new uint16[](0),
                new uint16[](0),
                new bytes[][](0),
                bytes32(0)
            )
        })
    );

    // Wallet's safe has the recovery spell as a module
    assertTrue(
        Safe(payable(address(wallet.safe))).isModuleEnabled(
            recoverySpells[0]
        )
    );
    // Wallet's safe address matches the predicted address
    assertEq(address(wallet.safe), PREDICTED_WALLET_ADDRESS);

    // Deploy recovery spell
    RecoverySpell spell = recoveryFactory.createRecoverySpell(
        bytes32(0),
        recoveryOwners,
        address(wallet.safe),
        1,
        1,
        1
    );

    // Deployed spell address matches the predicted address
    assertEq(address(spell), recoverySpells[0]);

    vm.warp(block.timestamp + spell.delay() + 1);

    {
        bytes32[] memory r = new bytes32[](1);
        bytes32[] memory s = new bytes32[](1);
        uint8[] memory v = new uint8[](1);
        bytes32 digest = spell.getDigest();
        (v[0], r[0], s[0]) = vm.sign(12, digest);

        // Execute recovery spell, reverts
        vm.expectRevert();
        spell.executeRecovery(SENTINEL, v, r, s);
    }
}

Recommended Mitigation Steps

Don't allow users to pass the safe's address as a recovery owner.

function _paramChecks(
    address[] memory owners,
    uint256 threshold,
    uint256 recoveryThreshold,
-   uint256 delay
+   uint256 delay,
+   address safe
) private pure {
    require(
        threshold <= owners.length,
        "RecoverySpell: Threshold must be lte number of owners"
    );
    require(
        recoveryThreshold <= owners.length,
        "RecoverySpell: Recovery threshold must be lte number of owners"
    );

    require(threshold != 0, "RecoverySpell: Threshold must be gt 0");
    require(delay <= 365 days, "RecoverySpell: Delay must be lte a year");
    for (uint256 i = 0; i < owners.length; i++) {
        require(owners[i] != address(0), "RecoverySpell: Owner cannot be 0");
+       require(owners[i] != safe, "RecoverySpell: Owner cannot be the safe");        
    }
}

and pass the safe's address to the above function from both createRecoverySpell and calculateAddress.

Assessed type

Invalid Validation

[ElliotFriedman]

Come on, we're not going to have 50 checks in the recovery spell, this is a dup of #19

I don't want to pay out for this behavior.

[c4-sponsor]

@ElliotFriedman Sponsors can only use these labels: sponsor confirmed, sponsor disputed, sponsor acknowledged, critical.

[GalloDaSballo]

This seems to be a misconfiguration by the user, I don't believe this is valid

[c4-judge]

GalloDaSballo marked the issue as nullified