Skip to content

fix(globe-wallet): validate asset code against UserAssets in spend limits and canonicalize storage keys - #101

Closed
s6pa1rta3n-lab wants to merge 1 commit into
Orbit-Wal:mainfrom
s6pa1rta3n-lab:fix/issue-88-spend-limit-canonicalization
Closed

fix(globe-wallet): validate asset code against UserAssets in spend limits and canonicalize storage keys#101
s6pa1rta3n-lab wants to merge 1 commit into
Orbit-Wal:mainfrom
s6pa1rta3n-lab:fix/issue-88-spend-limit-canonicalization

Conversation

@s6pa1rta3n-lab

Copy link
Copy Markdown

Closes #88

Root cause

In contracts/globe-wallet/src/lib.rs, GlobeWallet::set_spend_limit and GlobeWallet::record_spend took an arbitrary asset_code: String argument and used it directly as part of storage keys (DataKey::SpendLimit(user, asset_code) and DataKey::DailySpent(user, asset_code)). Unlike add_asset (which case-normalizes asset codes via codes_match_case_insensitive), set_spend_limit, get_spend_limit, and record_spend never checked whether asset_code belonged to UserAssets(user) or case-normalized it.

As a result:

  1. Different casings of the same real-world asset (e.g. "USDC" vs "usdc") mapped to separate storage keys, allowing spends to bypass configured daily limits.
  2. Spend limits and daily spend records could be configured and recorded for assets that the user never registered in UserAssets.

What changed and why

  1. Helper resolve_registered_asset_code:

    • Added internal helper resolve_registered_asset_code(env, user, asset_code) -> Result<String, WalletError>.
    • Scans DataKey::UserAssets(user) using Self::codes_match_case_insensitive to find the matching registered asset.
    • Returns the canonical registered asset.code for use as storage keys, or Err(WalletError::AssetNotFound) if no match is found.
  2. Validation and Canonical Storage Keys in Spend Limit Operations:

    • Updated set_spend_limit: Validates asset_code against UserAssets(user) (returning WalletError::AssetNotFound if unregistered) and uses the canonical registered code for DataKey::SpendLimit, DataKey::DailySpent, and event emission.
    • Updated get_spend_limit: Resolves asset_code case-insensitively against UserAssets(user). Returns 0 (unlimited / default) if unregistered or unset, and retrieves the configured limit under the canonical key if registered.
    • Updated record_spend: Validates asset_code against UserAssets(user) (returning WalletError::AssetNotFound if unregistered) and records daily spend against the canonical storage key.
  3. Consistency in remove_asset:

    • Updated remove_asset to also use codes_match_case_insensitive when locating the asset to remove, ensuring consistent case-insensitive asset identification across all wallet operations.
  4. Test Suite Updates and Additions:

    • Added test_case_variant_asset_code_hits_same_spend_limit_bucket proving that case variants (e.g., "USDC" and "usdc") share the exact same daily spend limit bucket.
    • Added test_set_spend_limit_rejects_unregistered_asset and test_record_spend_rejects_unregistered_asset proving calls for unregistered assets are rejected with WalletError::AssetNotFound.
    • Added test_set_spend_limit_with_case_variant_configures_canonical_bucket proving limits configured under a case-variant string correctly apply to the canonical asset bucket.
    • Updated all existing tests and reentrancy tests to register assets via add_asset prior to configuring limits or recording spends.

Definition of done — addressed item by item

  • set_spend_limit/record_spend validate asset_code against the caller's UserAssets, using the same canonicalization add_asset uses: resolve_registered_asset_code consults UserAssets(user) via codes_match_case_insensitive and returns WalletError::AssetNotFound if not registered.
  • A canonicalized code is used for the actual storage key (or the lookup canonicalizes before hitting storage) so "USDC"/"usdc" land in the same bucket: Both set_spend_limit, get_spend_limit, and record_spend resolve to the canonical registered code stored in UserAssets before reading/writing DataKey::SpendLimit and DataKey::DailySpent.
  • Test proving a case-variant of a registered asset's code hits the same spend-limit bucket, not an independent one: Added test_case_variant_asset_code_hits_same_spend_limit_bucket and test_set_spend_limit_with_case_variant_configures_canonical_bucket.
  • Test proving record_spend/set_spend_limit reject an asset_code that doesn't correspond to any registered asset: Added test_set_spend_limit_rejects_unregistered_asset and test_record_spend_rejects_unregistered_asset.
  • cargo test --workspace output pasted: Pasted in Evidence section below.

Evidence this actually runs

running 73 tests
test tests::test_add_asset_overlong_code_fails ... ok
test tests::test_add_duplicate_guardian_fails ... ok
test tests::test_add_asset_empty_code_fails ... ok
test tests::test_add_duplicate_asset_fails ... ok
test tests::test_accept_by_wrong_address_fails ... ok
test tests::test_add_asset_case_variant_duplicate_fails ... ok
test tests::test_add_and_get_assets ... ok
test tests::test_add_and_list_guardians ... ok
test tests::test_cancel_admin_transfer ... ok
test tests::test_initialize ... ok
test tests::test_daily_spent_survives_temporary_ttl_eviction ... ok
test tests::test_execute_recovery_rejects_new_admin_same_as_current_admin ... ok
test tests::test_double_approval_rejected ... ok
test tests::test_admin_can_cancel_recovery_even_after_quorum ... ok
test tests::test_cannot_initiate_second_recovery_while_one_pending ... ok
test tests::test_case_variant_asset_code_hits_same_spend_limit_bucket ... ok
test tests::test_migrate_user_assets_within_limit_does_nothing ... ok
test tests::test_no_limit_allows_any_spend ... ok
test tests::test_initialize_twice_fails ... ok
test tests::test_migrate_user_assets_requires_admin ... ok
test tests::test_native_code_with_issuer_is_contradictory_and_rejected ... ok
test tests::test_non_admin_cannot_add_guardian ... ok
test tests::test_propose_upgrade_accepts_any_hash_without_validation ... ok
test tests::test_pending_admin_cleared_after_accept_admin ... ok
test tests::test_propose_without_accept_keeps_admin_unchanged ... ok
test tests::test_max_guardians_limit ... ok
test tests::test_record_spend_boundary_drift_awareness ... ok
test tests::test_non_native_code_without_issuer_is_underspecified_and_rejected ... ok
test tests::test_propose_upgrade_requires_admin ... ok
test tests::test_record_spend_boundary_first_second_of_new_day_resets ... ok
test tests::test_record_spend_boundary_last_second_of_day_accumulates ... ok
test tests::test_record_spend_bucket_is_integer_division ... ok
test tests::test_record_spend_exact_day_boundary ... ok
test tests::test_raise_spend_then_lower_limit ... ok
test tests::test_record_spend_negative_amount_fails ... ok
test tests::test_record_spend_exceeds_limit_fails ... ok
test tests::test_record_spend_overflow_does_not_poison_later_calls ... ok
test tests::test_record_spend_rejects_unregistered_asset ... ok
test tests::test_record_spend_negative_amount_cannot_bypass_daily_limit ... ok
test tests::test_execute_upgrade_with_never_uploaded_hash_traps - should panic ... ok
test tests::test_record_spend_within_limit ... ok
test tests::test_record_spend_zero_amount_fails ... ok
test tests::test_recovery_happy_path_2_of_3 ... ok
test tests::test_remove_asset ... ok
test tests::test_recovery_clears_any_in_flight_normal_admin_transfer ... ok
test tests::test_record_spend_rejected_negative_amount_does_not_mutate_state ... ok
test tests::test_propose_and_execute_upgrade - should panic ... ok
test tests::test_recovery_rejects_non_guardian ... ok
test tests::test_remove_nonexistent_asset_fails ... ok
test tests::test_require_admin_not_initialized ... ok
test tests::test_migrate_user_assets_trims_excess ... ok
test tests::test_remove_guardian_below_threshold_fails ... ok
test tests::test_removed_guardian_cannot_initiate_recovery ... ok
test tests::test_remove_guardian_who_never_approved_leaves_proposal_untouched ... ok
test tests::test_revoke_recovery_approval_rejects_non_guardian ... ok
test tests::test_removed_guardian_approval_no_longer_counts_toward_quorum ... ok
test tests::test_remove_guardian_dequorated_proposal_can_requorum_with_fresh_timelock ... ok
test tests::test_set_spend_limit_rejects_unregistered_asset ... ok
test tests::test_set_recovery_config_rejected_while_recovery_pending ... ok
test tests::test_set_recovery_config_rejects_single_guardian_threshold ... ok
test tests::test_set_recovery_config_requires_min_guardians ... ok
test tests::test_revoke_recovery_approval_rejects_removed_guardian ... ok
test tests::test_set_recovery_config_rejects_threshold_above_guardian_count ... ok
test tests::test_set_spend_limit_with_case_variant_configures_canonical_bucket ... ok
test tests::test_spend_limit_set_and_get ... ok
test tests::test_upgrade_propose_double_fails ... ok
test tests::test_spend_limit_ttl_extension_after_long_idle_period ... ok
test tests::test_transfer_admin ... ok
test tests::test_revoking_approval_below_threshold_resets_timelock ... ok
test tests::test_upgrade_rejects_hash_mismatch ... ok
test tests::test_upgrade_requires_admin_and_ready_time ... ok
test tests::test_user_assets_ttl_extension_after_long_idle_period ... ok
test tests::test_max_assets_limit ... ok

test result: ok. 73 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 2.72s

     Running tests/record_spend_reentrancy.rs (target/debug/deps/record_spend_reentrancy-9f6bb289bf4090dc)

running 1 test
test two_spends_in_one_host_invocation_accumulate ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.18s

     Running unittests src/lib.rs (target/debug/deps/token_wrapper-11699b1bfd8edd1b)

running 11 tests
test tests::test_allowance_unset_pair_returns_zero ... ok
test tests::test_approve_negative_amount_fails ... ok
test tests::test_approve_and_allowance ... ok
test tests::test_approve_past_expiry_fails ... ok
test tests::test_approve_overwrites_previous_allowance ... ok
test tests::test_transfer_from_insufficient_allowance_fails ... ok
test tests::test_transfer_from_expired_allowance_fails ... ok
test tests::test_transfer_from_happy_path ... ok
test tests::test_transfer_from_zero_amount_fails ... ok
test tests::test_transfer_from_succeeds_exactly_at_expiry_ledger ... ok
test tests::test_transfer_from_rolls_back_allowance_when_underlying_transfer_fails ... ok

test result: ok. 11 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 1.86s

Tests

  • Added test_case_variant_asset_code_hits_same_spend_limit_bucket asserting that spending against "usdc" and "USDC" accumulates toward the same configured daily limit and rejects spends when the aggregate exceeds the limit.
  • Added test_set_spend_limit_rejects_unregistered_asset asserting set_spend_limit fails with WalletError::AssetNotFound when called for an unregistered asset code.
  • Added test_record_spend_rejects_unregistered_asset asserting record_spend fails with WalletError::AssetNotFound when called for an unregistered asset code.
  • Added test_set_spend_limit_with_case_variant_configures_canonical_bucket asserting setting limits with lowercase strings correctly configures the canonical asset bucket.
  • Updated all existing unit and integration tests to register assets before setting/recording spend limits.

Regression check

All existing contract features and workflows continue to pass without regression:

  • Multi-asset registration and duplicate detection
  • Daily spend bucket roll-overs and ledger boundary timestamps
  • Reentrancy safety in record_spend
  • Admin management, multi-sig guardian recovery proposals, quorum tracking, and timelocks
  • Storage TTL extension for UserAssets, SpendLimit, and DailySpent

Checklist

  • Every Definition of done bullet above is checked and explained, not just checked
  • Evidence block above is filled in with real output, not omitted
  • New/updated tests are included and shown passing
  • No leftover console.log/TODO/debug code
  • Related/adjacent behavior re-verified, not assumed unaffected

Payout Routing

  • Base / Arbitrum / Polygon / ETH: 0xF46C9F6d70C50BF81ef3588AB523a90a594a2F89
  • Stellar: GCL6OXAMLD75BMTINA6EMRUDWK5THQUSHMYNLSNBCJAPZJHNYJTUNIBC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: record_spend/set_spend_limit take a free-form asset_code string, completely decoupled from the case-insensitive asset registry issue #29 built

1 participant