[Bug]: Issue #8's fix landed in the wrong contract — the SpendLimit/DailySpent code/issuer collision it describes is still live on main

[ndii-dev]

Context

globe-wallet's DataKey::SpendLimit(Address, String) and DataKey::DailySpent(Address, String), and the set_spend_limit/record_spend functions that use them.

Issue #8 described exactly this: spend-limit and daily-spend storage is keyed by asset code alone, so two different assets sharing a code (e.g. a scam token minted with code "USDC" under a different issuer) share the same budget as the legitimate asset. #8 was closed via PR #11, and the contributor was paid out via GrantFox for it.

Problem

PR #11's diff touches exactly one file: contracts/token-wrapper/src/lib.rs (694 additions, 69 deletions). token-wrapper has no SpendLimit, DailySpent, set_spend_limit, or record_spend — all of that lives in contracts/globe-wallet/src/lib.rs, a file PR #11 never modified. Whatever PR #11 actually added to token-wrapper is gone from the current tree — contracts/token-wrapper/src/lib.rs is 373 lines today and contains only approve/allowance/transfer_from, nothing resembling PR #11's description.

Net result: the current main branch's globe-wallet still has, verbatim, exactly what #8 described:

pub enum DataKey {
    ...
    SpendLimit(Address, String),
    DailySpent(Address, String),
    ...
}

No issuer component anywhere. The vulnerability #8 documented was never actually fixed — it was closed on the strength of a PR that modified the wrong contract.

Reproduction steps

// In contracts/globe-wallet/src/lib.rs's test module:
#[test]
fn test_same_code_different_issuer_share_a_budget() {
    let (env, _cid, admin, client) = setup();
    let user = Address::generate(&env);
    let real_issuer = Address::generate(&env);
    let scam_issuer = Address::generate(&env);

    let real_usdc = AssetInfo { code: String::from_str(&env, "USDC"), issuer: Some(real_issuer) };
    // A "USDC" from a different issuer is a DIFFERENT asset on Stellar (identity = code + issuer),
    // but add_asset's own duplicate check is also code-only, so this second registration
    // is itself rejected -- confirming code is being treated as the sole identity everywhere.
    client.add_asset(&user, &real_usdc);

    client.set_spend_limit(&user, &String::from_str(&env, "USDC"), &1000);
    client.record_spend(&user, &String::from_str(&env, "USDC"), &900);

    // A completely unrelated caller can record spend against the SAME bucket using
    // nothing but the string "USDC" -- no add_asset, no ownership of the real asset,
    // no relationship to real_issuer required, since record_spend never consults
    // UserAssets or AssetInfo.issuer at all.
    let attacker_amount = 200;
    let result = client.try_record_spend(&user, &String::from_str(&env, "USDC"), &attacker_amount);
    // Expected once fixed: this should be a DIFFERENT budget from a DIFFERENT
    // (code, issuer) pair and/or should require the asset_code to correspond to
    // a registered UserAssets entry. Today it silently succeeds against the same
    // bucket as the real asset's spend, and the 1000-limit is now oversubscribed
    // by an amount nothing about "USDC" (real issuer) authorized.
}

Impact

The spend-limit feature's entire value proposition ("per-asset daily caps to limit loss on key compromise") is undermined for any asset code that collides with another issuer's use of the same ticker — which on Stellar is trivial to engineer (anyone can issue an asset with code "USDC"). A malicious integration, a confused caller, or a scam-token interaction can consume/pollute the daily budget meant for a legitimate asset, or vice versa. This is the second time this exact class of gap has been reported (see also the new issue on record_spend/set_spend_limit never consulting UserAssets at all) — a real fix needs to close both, not just rename a storage key.

Suggested fix

Change DataKey::SpendLimit/DataKey::DailySpent to key on (Address, String /* code */, Address /* issuer, use a sentinel for native XLM */), or on a deterministic hash of the full AssetInfo. set_spend_limit/record_spend/get_spend_limit need to take the full AssetInfo (or an equivalent issuer parameter) instead of a bare asset_code: String, and should validate that the given AssetInfo matches a UserAssets-registered entry for that user (closing the separate "asset_code is a free-form string decoupled from the registry" gap at the same time). Include a migration path for any limits already set under the old, ambiguous key — the same requirement #8's original Definition of done specified and which the merged PR also never delivered.

Definition of done

• DataKey::SpendLimit / DataKey::DailySpent disambiguate by issuer (or full asset identity), implemented in contracts/globe-wallet/src/lib.rs specifically — not a parallel, disconnected implementation in token-wrapper
• set_spend_limit, get_spend_limit, record_spend take a full asset identity (not a bare String), and reject an asset_code/AssetInfo that isn't present in the caller's UserAssets
• Migration path for any SpendLimit/DailySpent entries already persisted under the old (Address, String) key
• Test proving two same-code, different-issuer assets get independent budgets, added to contracts/globe-wallet/src/lib.rs's own test module (not token-wrapper's)
• A one-line note in the PR explaining why the fix lives in globe-wallet and confirming token-wrapper was not touched by mistake this time
• cargo test --workspace output pasted showing the new test passing