[Bug]: MIN_GUARDIANS_FOR_RECOVERY is enforced only when set_recovery_config is called — remove_guardian can silently degrade below it

[ndii-dev]

Context

GlobeWallet::MIN_GUARDIANS_FOR_RECOVERY (= 3) and GlobeWallet::remove_guardian, contracts/globe-wallet/src/lib.rs.

The constant's doc comment: "Minimum number of guardians a wallet must have before a recovery threshold can be configured. Below this, 'M-of-N social recovery' degenerates into 'one or two people can unilaterally seize the wallet.'"

Problem

MIN_GUARDIANS_FOR_RECOVERY is checked in exactly one place: set_recovery_config. remove_guardian's only floor is the configured threshold itself, not the 3-guardian minimum the threshold was originally validated against:

pub fn remove_guardian(env: Env, admin: Address, guardian: Address) -> Result<(), WalletError> {
    ...
    let recovery_config = Self::recovery_config(env.clone());
    if let Some(config) = &recovery_config {
        if new_guardians.len() < config.threshold { // checks threshold, never MIN_GUARDIANS_FOR_RECOVERY
            return Err(WalletError::NotEnoughGuardians);
        }
    }
    ...
}

A wallet configured with 3 guardians and threshold = 2 (a valid, set_recovery_config-accepted state — 2 <= 3) can have a guardian removed down to 2 guardians total, since 2 < threshold(2) is false. The wallet now has fewer guardians than MIN_GUARDIANS_FOR_RECOVERY ever allowed configuring recovery with in the first place, but recovery remains fully configured and functional at the reduced size — the invariant set_recovery_config enforces at configuration time is not maintained afterward.

Reproduction steps

#[test]
fn test_remove_guardian_degrades_below_min_guardians_for_recovery() {
    let (env, _cid, admin, client) = setup();
    let g = [Address::generate(&env), Address::generate(&env), Address::generate(&env)];
    for guardian in &g { client.add_guardian(&admin, guardian); }
    client.set_recovery_config(&admin, &2, &10); // valid: 3 guardians >= MIN_GUARDIANS_FOR_RECOVERY, threshold 2 <= 3

    client.remove_guardian(&admin, &g[2]); // succeeds: 2 remaining >= threshold(2)

    assert_eq!(client.guardians().len(), 2); // below MIN_GUARDIANS_FOR_RECOVERY(3), never re-checked
    // set_recovery_config would reject configuring recovery from scratch at
    // this guardian count (2 < 3), but remove_guardian never re-validates
    // the same floor against an *already-configured* recovery setup.
}

Impact

This particular reproduction still requires 2-of-2 unanimous agreement, so it isn't an immediate "one person can unilaterally seize the wallet" outcome — but it demonstrates the code doesn't uphold its own documented security invariant once you're past initial configuration, and the drift compounds: repeated guardian removal can walk a wallet down to 2 guardians (the practical floor, since threshold itself can't go below 2), permanently below the number the design explicitly says is needed to avoid recovery degenerating toward "too few independent parties," with no mechanism ever flagging or blocking the drift.

Suggested fix

remove_guardian should re-validate against MIN_GUARDIANS_FOR_RECOVERY (not just the configured threshold) whenever a RecoveryConfig exists — i.e., removal should be rejected if it would take the guardian count below MIN_GUARDIANS_FOR_RECOVERY while recovery is configured, the same way it's already rejected if it would take the count below threshold.

Definition of done

• remove_guardian rejects removal that would drop the guardian count below MIN_GUARDIANS_FOR_RECOVERY whenever RecoveryConfig is set, in addition to the existing threshold check
• Test proving the reproduction case above is now rejected
• Test proving guardian removal still works normally when no RecoveryConfig exists (no regression)
• Test proving guardian removal down to exactly MIN_GUARDIANS_FOR_RECOVERY still succeeds (boundary correct)
• cargo test --workspace output pasted

[Chidi-Dev1]

Hi maintainer I will love to work on this issue.
Please assign it to me.
Thank you.