The same address in original owner set and recovery owner set can lead to permanent DoS

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-10-kleidi/blob/ab89bcb443249e1524496b694ddb19e298dca799/src/RecoverySpell.sol#L268

Vulnerability details

Describtion

The contract RecoverySpell is deployed with an array of new owners. This array is used when recovery is executed and new owners are set. However there is an edge case scenario which causes revert of the function executeRecovery.

Example scenario:

We have 3/4 multisig. And one of the signers is another multisig.
This wallet is considered safe and thats why we include the wallet to the recovery owners array. Other addresses will be new.

Threshold is set to 3/4 and 2 signer keys are lost. So we have the following list of Safe owners

# L - Lost key
# M - multisig
og = [A_L, B_L, C, D_M]

and a set of new recovery owners:

new = [D_M, E, F, G]

And we execute recovery process.

Now if we follow the code

• remove all but final existing owner, set owner threshold to 1

state after OwnerManager.removeOwner calls

og = [D_M]

• swap final existing owner for the first new owner

and here is the problem. Swapping [existingOwners[existingOwnersLength - 1] for owners[0]
Aka Swapping D_M for D_M.

We can see in Safe OwnerManager contract:

    function swapOwner(address prevOwner, address oldOwner, address newOwner) public authorized {
        // Owner address cannot be null, the sentinel or the Safe itself.
        require(newOwner != address(0) && newOwner != SENTINEL_OWNERS && newOwner != address(this), "GS203");
        // No duplicate owners allowed.
        require(owners[newOwner] == address(0), "GS204");
        // Validate oldOwner address and check that it corresponds to owner index.
        require(oldOwner != address(0) && oldOwner != SENTINEL_OWNERS, "GS203");
        require(owners[prevOwner] == oldOwner, "GS205");
        owners[newOwner] = owners[oldOwner];
        owners[prevOwner] = newOwner;
        owners[oldOwner] = address(0);
        emit RemovedOwner(oldOwner);
        emit AddedOwner(newOwner);
    }

More specifically:

require(owners[newOwner] == address(0), "GS204");

The address D_M was not removed before, and it is the last address remaining. It does point to SENTINEL_OWNERS.

It is an edge scenario, but the impact is very dangerous as it will make the recovery permanently DoSed. The system should be robust enough for all different use-cases, including the mentioned one.

PoC

See Safe test - same address swap fail:
https://github.com/safe-global/safe-smart-account/blob/6fde75d29c8b52d5ac0c93a6fb7631d434b64119/test/core/Safe.OwnerManager.spec.ts#L313C1-L322C12

Recommended Mitigation Steps

In case any address will be re-used for a new owner set, make sure it does not occupy the zero index of recovery owners

Assessed type

Invalid Validation

[c4-judge]

GalloDaSballo marked the issue as nullified