RecoverySpellFactory::_paramChecks doesn't check for SENTINEL_OWNERS 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

Decsription

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 SENTINEL_OWNERS which refers to the head of the linked list, 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_NotValidatingSENTINELOwner:

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

function test_NotValidatingSENTINELOwner() 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 SENTINEL
    address[] memory recoveryOwners = new address[](2);
    recoveryOwners[0] = owner2;
    recoveryOwners[1] = SENTINEL;

    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 address(1) as a recovery owner.

function _paramChecks(
    address[] memory owners,
    uint256 threshold,
    uint256 recoveryThreshold,
    uint256 delay
) private pure {
    ...

    for (uint256 i = 0; i < owners.length; i++) {
        require(owners[i] != address(0), "RecoverySpell: Owner cannot be 0");
+       require(owners[i] != address(1), "RecoverySpell: Owner cannot be SENTINEL");
    }
}

Assessed type

Invalid Validation

[ElliotFriedman]

This is a low severity issue. The owner is by default not going to be malicious when they set up their own system. If they are malicious they will just immediately hand control over to the attacker, they wouldn't make a recovery spell that doesn't work when it can escalate privileges

[c4-sponsor]

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

[GalloDaSballo]

This is also a way for the user to self-rekt, I think this should be considered invalid

[c4-judge]

GalloDaSballo marked the issue as nullified