[Bug]: Neither timelock (upgrade proposals, guardian recovery) enforces a minimum delay — both can be set to 0, defeating their entire purpose

[ndii-dev]

Context

GlobeWallet::propose_upgrade (delay_in_ledgers: u32) and GlobeWallet::set_recovery_config (delay_in_ledgers: u32), contracts/globe-wallet/src/lib.rs.

Both timelocks exist for exactly one documented reason. The upgrade path: "Wait for the delay (in ledgers) to elapse" before execute_upgrade becomes callable. The recovery path, verbatim from the code comment: "Gives the legitimate admin a window to notice and cancel a malicious or mistaken recovery."

Problem

Neither function validates a minimum for delay_in_ledgers:

pub fn propose_upgrade(env: Env, proposer: Address, wasm_hash: BytesN<32>, delay_in_ledgers: u32) -> Result<(), WalletError> {
    ...
    let ready_at = env.ledger().sequence().saturating_add(delay_in_ledgers); // delay_in_ledgers = 0 is accepted
    ...
}

pub fn set_recovery_config(env: Env, admin: Address, threshold: u32, delay_in_ledgers: u32) -> Result<(), WalletError> {
    ...
    // threshold has a floor (`threshold <= 1` rejected). delay_in_ledgers has none.
    let config = RecoveryConfig { threshold, delay_in_ledgers };
    ...
}

delay_in_ledgers = 0 means ready_at == env.ledger().sequence() at the moment quorum/proposal is reached — execute_upgrade/execute_recovery's < ready_at check passes immediately. Both timelocks can be configured to provide exactly zero reaction time, silently, by whoever calls propose_upgrade or set_recovery_config (the current admin, in both cases).

Reproduction steps

#[test]
fn test_zero_delay_upgrade_is_immediately_executable() {
    let (env, _cid, admin, client) = setup();
    let wasm_hash = BytesN::from_array(&env, &[7u8; 32]);
    client.propose_upgrade(&admin, &wasm_hash, &0); // no minimum enforced
    // No time passes at all -- this is not "wait for the delay to elapse",
    // it's instant, because the delay was configured to be zero.
    let result = client.try_execute_upgrade(&admin, &wasm_hash);
    // Expected once fixed: propose_upgrade should reject delay_in_ledgers
    // below some documented MIN_UPGRADE_DELAY, so a "timelocked" upgrade
    // can never actually mean "immediate."
}

#[test]
fn test_zero_delay_recovery_gives_admin_no_reaction_window() {
    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, &0); // no minimum enforced on delay
    let new_admin = Address::generate(&env);
    client.initiate_recovery(&g[0], &new_admin);
    client.approve_recovery(&g[1]); // quorum reached -> ready_at = current sequence (delay=0)
    // The admin has had zero ledgers -- effectively zero time -- to notice and
    // call cancel_recovery. execute_recovery is callable right now.
    client.execute_recovery();
    // Expected once fixed: set_recovery_config should reject delay_in_ledgers
    // below some documented MIN_RECOVERY_DELAY.
}

Impact

Both of this contract's timelock-based defenses are opt-out by design, invisibly. An admin (or an attacker who has already compromised the admin key and wants to entrench that compromise before the legitimate owner can react via guardian recovery) can configure propose_upgrade's delay to 0 and swap the contract's code in the same breath as proposing it — no external observer watching for "an upgrade was proposed, do I have time to object?" gets any real window. Symmetrically, a set_recovery_config call with delay_in_ledgers = 0 (which itself could be set by a compromised admin key, since set_recovery_config is admin-only) means once quorum is reached, cancel_recovery has to race the very same transaction/next call — not a realistic defense at all, despite the doc comment promising one.

Suggested fix

Add a MIN_UPGRADE_DELAY_LEDGERS and MIN_RECOVERY_DELAY_LEDGERS constant (mirroring how MIN_GUARDIANS_FOR_RECOVERY and MAX_GUARDIANS already document their reasoning), pick a value with a real justification (e.g. enough ledgers to plausibly correspond to at least a day, giving a human time to notice an event before it becomes executable), and reject delay_in_ledgers below it in both propose_upgrade and set_recovery_config.

Definition of done

• MIN_UPGRADE_DELAY_LEDGERS and MIN_RECOVERY_DELAY_LEDGERS constants added, with a documented rationale for the chosen values (same rigor as MAX_GUARDIANS's existing doc comment)
• propose_upgrade rejects delay_in_ledgers below the minimum with a new/reused typed error
• set_recovery_config rejects delay_in_ledgers below the minimum with a new/reused typed error
• Tests proving both zero-delay and below-minimum-delay calls are now rejected
• Test proving a delay at or above the minimum still works exactly as before (no regression to the happy path)
• cargo test --workspace output pasted

[christabel888]

Gm, please can i get this??

[grantfox-oss]

🦊 GrantFox — @christabel888 has been assigned to this issue as part of the Third Campaign campaign!

Next steps:

1. Open a Pull Request referencing this issue (e.g., Closes #84)
2. Your PR will be reviewed by the Orbit-Wal maintainers

Good luck! Track your progress on GrantFox.

[grantfox-oss]

🎉 This issue has been marked as completed on GrantFox as part of the Third Campaign campaign!

@christabel888's PR #106 was approved and merged by @ndii-dev.

🏆 @christabel888: You earned 35 FoxPoints for this contribution! Your current tier: Explorer (780 total points). Track your full progress on GrantFox.

👏 Great work, @christabel888! Keep contributing to Orbit-Wal.