[Bug]: set_spend_limit has none of record_spend's compromised-key defenses — a stolen key can just raise its own limit before draining

[ndii-dev]

Context

GlobeWallet::set_spend_limit and GlobeWallet::record_spend, contracts/globe-wallet/src/lib.rs.

record_spend's doc comment is unusually explicit about its threat model: "require_auth() on user authenticates the caller, not the value — in the compromised-key threat model this function exists to defend against, the attacker can already produce valid user signatures, so require_auth() alone provides no protection here." That analysis is correct, and it's why record_spend rejects amount <= 0 (to stop an attacker resetting spent_today before a legitimate-looking large spend).

Problem

set_spend_limit was never given the same analysis. It is public, requires only user.require_auth(), and has no upper bound, no rate limit, no timelock, and no second-party (guardian) involvement on increases:

pub fn set_spend_limit(env: Env, user: Address, asset_code: String, limit: i128) -> Result<(), WalletError> {
    user.require_auth();
    if limit < 0 { return Err(WalletError::InvalidSpendLimit); }
    // "retroactive enforcement" below only blocks LOWERING the limit below
    // today's already-recorded spend -- it does nothing to slow down RAISING it.
    ...
}

Under the exact threat model record_spend's own comment describes, the attacker holding the compromised user key doesn't need to fight record_spend's defenses at all. They call set_spend_limit(user, "USDC", i128::MAX) first, then spend freely. The entire feature — whose module-level doc comment states its purpose is "to limit loss on key compromise" — provides zero protection against the one attacker it exists to defend against, because the sole authority over the limit is the same single key being defended.

Reproduction steps

#[test]
fn test_spend_limit_is_self_escalatable_by_the_key_it_defends_against() {
    let (env, _cid, admin, client) = setup();
    let user = Address::generate(&env);
    client.add_asset(&user, &usdc(&env));

    // Owner sets a conservative daily cap.
    client.set_spend_limit(&user, &String::from_str(&env, "USDC"), &100);
    assert_eq!(client.get_spend_limit(&user, &String::from_str(&env, "USDC")), 100);

    // Attacker has `user`'s compromised key -- same signature capability
    // record_spend's own doc comment assumes. Nothing stops them from
    // raising the limit to whatever they want before spending.
    client.set_spend_limit(&user, &String::from_str(&env, "USDC"), &i128::MAX);
    assert_eq!(client.get_spend_limit(&user, &String::from_str(&env, "USDC")), i128::MAX);

    // The "protection" is now meaningless.
    client.record_spend(&user, &String::from_str(&env, "USDC"), &1_000_000_000);
    // Expected once fixed: raising a limit should require something the
    // attacker doesn't already have -- e.g. a guardian-approved change, or
    // a timelock long enough for the legitimate owner to notice and cancel,
    // the same pattern already used for admin recovery.
}

Impact

The core security promise of this whole subsystem is false as implemented. Anyone relying on "I set a $50/day cap, so a compromised key can only cost me $50" is wrong — a compromised key costs them everything, in one transaction, with zero additional friction. This is worse than having no spend-limit feature at all, because it creates false confidence.

Suggested fix

Split "lowering/removing a limit" (safe to do instantly — it can only ever restrict the attacker further) from "raising a limit" (must be slow and/or require a second factor). A minimal fix: raising a limit goes through the same propose/timelock pattern already built for admin transfer and recovery — propose_spend_limit_increase + a delay + either the same key confirming again after the delay, or (stronger) guardian co-signature, mirroring the existing RecoveryConfig/guardian infrastructure that already exists in this contract for exactly this class of problem.

Definition of done

• Root cause written out: why set_spend_limit needs asymmetric handling for increases vs. decreases, referencing record_spend's existing threat-model comment
• Limit decreases remain instant (no regression to the existing retroactive-enforcement behavior for lowering)
• Limit increases require either a timelock delay or guardian co-signature (design decision documented and justified in the PR, not silently chosen)
• Test proving a compromised-key holder cannot instantly raise their own limit and spend past the previously-configured cap in one transaction
• Test proving legitimate limit decreases are unaffected
• cargo test --workspace output pasted showing red→green for the new test(s)

[Zarmaijemimah]

Hi maintainer I’m a full stack developer. I have knowledge and interest in solving any tech issues and I would be glad to contribute my skills. I’d appreciate the opportunity to work on this issue. Thank you.