Skip to content

fix(globe-wallet): add cancel_upgrade and clear pending upgrade on recovery (Closes #87) - #95

Closed
s6pa1rta3n-lab wants to merge 1 commit into
Orbit-Wal:mainfrom
s6pa1rta3n-lab:fix-issue-87
Closed

fix(globe-wallet): add cancel_upgrade and clear pending upgrade on recovery (Closes #87)#95
s6pa1rta3n-lab wants to merge 1 commit into
Orbit-Wal:mainfrom
s6pa1rta3n-lab:fix-issue-87

Conversation

@s6pa1rta3n-lab

Copy link
Copy Markdown

Closes #87

Context & Problem

In GlobeWallet (contracts/globe-wallet/src/lib.rs), while the admin transfer and guardian recovery subsystems had cancellation mechanisms (cancel_admin_transfer and cancel_recovery), the upgrade subsystem lacked an equivalent cancel_upgrade method. Furthermore, execute_recovery did not clear PendingUpgrade.

In a compromised admin scenario where an attacker proposed a malicious upgrade proposal before guardians executed recovery, the PendingUpgrade proposal remained stuck in instance storage. This caused two critical issues:

  1. The malicious upgrade proposal remained live in storage.
  2. Because propose_upgrade rejects new proposals when PendingUpgrade is present with UpgradeAlreadyPending, the new legitimate admin was permanently locked out from proposing upgrades.

What Changed

  1. Added cancel_upgrade:

    • Admin-authorized method that clears DataKey::PendingUpgrade and emits (Symbol::new(&env, "upgrade_cancelled"),), admin.
    • Returns WalletError::UpgradeNotPending if called when no upgrade proposal exists.
    • Symmetrically mirrors cancel_admin_transfer and cancel_recovery.
  2. Updated execute_recovery:

    • Clears DataKey::PendingUpgrade upon successful recovery, ensuring any malicious or unresolved upgrade proposal from the compromised outgoing admin is cleared automatically and documented as an explicit design decision.
  3. Added Comprehensive Unit Tests:

    • test_cancel_upgrade: Verifies proposing, cancelling, verifying removal, rejecting repeat cancellations with UpgradeNotPending, and verifying that proposing a new upgrade succeeds.
    • test_cancel_upgrade_requires_admin: Verifies non-admin authorization rejection (WalletError::Unauthorized).
    • test_cancel_upgrade_not_pending_fails: Verifies UpgradeNotPending error when no proposal is active.
    • test_stale_upgrade_cleared_by_new_admin_via_cancel_upgrade: Verifies a new admin can clear a stale upgrade proposal via cancel_upgrade and propose a new valid upgrade.
    • test_recovery_clears_pending_upgrade_and_unblocks_future_proposals: End-to-end test proving that execute_recovery automatically clears PendingUpgrade and unblocks the new admin from proposing legitimate upgrades.

Definition of Done Checklist

  • cancel_upgrade function added, admin-authorized, symmetric with cancel_admin_transfer/cancel_recovery
  • execute_recovery clears any pending PendingUpgrade as part of a successful recovery (documented as an explicit design decision, same as its existing PendingAdmin cleanup)
  • Test proving a new admin can clear a stale/malicious upgrade proposal via cancel_upgrade
  • Test proving a successful execute_recovery clears any in-flight PendingUpgrade automatically
  • Test proving propose_upgrade works again for the new admin afterward (no more permanent lockout)
  • cargo test --workspace output pasted below

Test Output

running 74 tests
test tests::test_add_asset_empty_code_fails ... ok
test tests::test_add_asset_overlong_code_fails ... ok
test tests::test_accept_by_wrong_address_fails ... ok
test tests::test_add_duplicate_guardian_fails ... ok
test tests::test_add_and_list_guardians ... ok
test tests::test_add_and_get_assets ... ok
test tests::test_add_asset_case_variant_duplicate_fails ... ok
test tests::test_add_duplicate_asset_fails ... ok
test tests::test_cancel_upgrade_not_pending_fails ... ok
test tests::test_cancel_upgrade_requires_admin ... ok
test tests::test_daily_spent_survives_temporary_ttl_eviction ... ok
test tests::test_cancel_admin_transfer ... ok
test tests::test_initialize ... ok
test tests::test_cancel_upgrade ... ok
test tests::test_admin_can_cancel_recovery_even_after_quorum ... ok
test tests::test_double_approval_rejected ... ok
test tests::test_cannot_initiate_second_recovery_while_one_pending ... ok
test tests::test_migrate_user_assets_within_limit_does_nothing ... ok
test tests::test_execute_recovery_rejects_new_admin_same_as_current_admin ... ok
test tests::test_no_limit_allows_any_spend ... ok
test tests::test_migrate_user_assets_trims_excess ... ok
test tests::test_native_code_with_issuer_is_contradictory_and_rejected ... ok
test tests::test_non_native_code_without_issuer_is_underspecified_and_rejected ... ok
test tests::test_initialize_twice_fails ... ok
test tests::test_migrate_user_assets_requires_admin ... ok
test tests::test_non_admin_cannot_add_guardian ... ok
test tests::test_propose_upgrade_accepts_any_hash_without_validation ... ok
test tests::test_propose_without_accept_keeps_admin_unchanged ... ok
test tests::test_record_spend_boundary_drift_awareness ... ok
test tests::test_pending_admin_cleared_after_accept_admin ... ok
test tests::test_record_spend_boundary_first_second_of_new_day_resets ... ok
test tests::test_max_guardians_limit ... ok
test tests::test_propose_upgrade_requires_admin ... 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_negative_amount_cannot_bypass_daily_limit ... ok
test tests::test_record_spend_exact_day_boundary ... ok
test tests::test_record_spend_exceeds_limit_fails ... ok
test tests::test_record_spend_within_limit ... ok
test tests::test_raise_spend_then_lower_limit ... ok
test tests::test_record_spend_negative_amount_fails ... ok
test tests::test_record_spend_zero_amount_fails ... ok
test tests::test_record_spend_overflow_does_not_poison_later_calls ... ok
test tests::test_record_spend_rejected_negative_amount_does_not_mutate_state ... ok
test tests::test_remove_asset ... ok
test tests::test_max_assets_limit ... ok
test tests::test_recovery_clears_any_in_flight_normal_admin_transfer ... ok
test tests::test_recovery_rejects_non_guardian ... ok
test tests::test_remove_guardian_who_never_approved_leaves_proposal_untouched ... ok
test tests::test_remove_guardian_below_threshold_fails ... ok
test tests::test_remove_nonexistent_asset_fails ... ok
test tests::test_remove_guardian_dequorated_proposal_can_requorum_with_fresh_timelock ... ok
test tests::test_recovery_clears_pending_upgrade_and_unblocks_future_proposals ... ok
test tests::test_recovery_happy_path_2_of_3 ... ok
test tests::test_require_admin_not_initialized ... ok
test tests::test_execute_upgrade_with_never_uploaded_hash_traps - should panic ... ok
test tests::test_revoke_recovery_approval_rejects_non_guardian ... ok
test tests::test_removed_guardian_cannot_initiate_recovery ... ok
test tests::test_set_recovery_config_rejects_single_guardian_threshold ... ok
test tests::test_propose_and_execute_upgrade - should panic ... ok
test tests::test_set_recovery_config_rejected_while_recovery_pending ... ok
test tests::test_spend_limit_set_and_get ... ok
test tests::test_removed_guardian_approval_no_longer_counts_toward_quorum ... ok
test tests::test_set_recovery_config_requires_min_guardians ... ok
test tests::test_spend_limit_ttl_extension_after_long_idle_period ... ok
test tests::test_revoking_approval_below_threshold_resets_timelock ... ok
test tests::test_upgrade_rejects_hash_mismatch ... ok
test tests::test_revoke_recovery_approval_rejects_removed_guardian ... ok
test tests::test_upgrade_propose_double_fails ... ok
test tests::test_set_recovery_config_rejects_threshold_above_guardian_count ... ok
test tests::test_upgrade_requires_admin_and_ready_time ... ok
test tests::test_transfer_admin ... ok
test tests::test_user_assets_ttl_extension_after_long_idle_period ... ok
test tests::test_stale_upgrade_cleared_by_new_admin_via_cancel_upgrade ... ok

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

     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.10s

     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_transfer_from_insufficient_allowance_fails ... ok
test tests::test_transfer_from_expired_allowance_fails ... ok
test tests::test_approve_overwrites_previous_allowance ... 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 5.96s

Payout Routing

  • EVM (Base / Arbitrum / Polygon / Ethereum): 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

1 participant