Skip to content

fix(globe-wallet): emit distinct recovery_executed event alongside admin_transferred (#91) - #103

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

fix(globe-wallet): emit distinct recovery_executed event alongside admin_transferred (#91)#103
s6pa1rta3n-lab wants to merge 1 commit into
Orbit-Wal:mainfrom
s6pa1rta3n-lab:fix-issue-91

Conversation

@s6pa1rta3n-lab

Copy link
Copy Markdown

Summary of Changes

Closes #91

This PR distinguishes emergency guardian recovery events from routine admin key rotations while preserving full backward compatibility for existing indexers and client applications.

Design Decision & Rationale

  • Problem: Previously, GlobeWallet::execute_recovery and GlobeWallet::accept_admin both published the identical admin_transferred event with payload (old_admin, new_admin). Off-chain security monitoring and user alert pipelines could not distinguish routine key maintenance from an emergency wallet seizure without fragile out-of-band correlation against preceding approval events.
  • Additive Approach for Backward Compatibility:
    • execute_recovery continues to publish admin_transferred with (old_admin, new_admin) unchanged so existing mobile apps and indexers relying on this event continue operating seamlessly.
    • In addition, execute_recovery publishes a distinct recovery_executed event carrying (old_admin, new_admin, approvals: Vec<Address>).
  • Event Topic & Payload Design:
    • Topic: (Symbol::new(&env, "recovery_executed"),) — adheres to the established contract lifecycle naming pattern (recovery_initiated, recovery_approved, recovery_cancelled, and upgrade_executed).
    • Payload: (old_admin: Address, new_admin: Address, approvals: Vec<Address>) — provides the full security context to alerting systems, explicitly identifying the prior admin, the new admin, and the specific quorum of guardians who approved the recovery.
  • Routine Transfers Untouched: GlobeWallet::accept_admin continues to publish only admin_transferred, ensuring routine key rotations do not trigger emergency recovery alerts.

Checklist & Definition of Done

  • execute_recovery emits both the existing admin_transferred event (unchanged) and the new recovery_executed event.
  • The new recovery_executed payload contains (old_admin, new_admin, approvals).
  • Added test_execute_recovery_emits_both_admin_transferred_and_recovery_executed_events testing both events and payloads upon recovery execution.
  • Added test_accept_admin_routine_transfer_emits_only_admin_transferred_event proving routine transfer only emits admin_transferred without any recovery_executed event.
  • All 83 workspace tests pass cleanly (cargo test --workspace).

Test Verification

    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.26s
     Running unittests src/lib.rs (target/debug/deps/globe_wallet-c063bb43cfb6b03b)

running 71 tests
test tests::test_add_asset_overlong_code_fails ... ok
test tests::test_add_asset_empty_code_fails ... ok
test tests::test_accept_by_wrong_address_fails ... ok
test tests::test_add_duplicate_asset_fails ... ok
test tests::test_accept_admin_routine_transfer_emits_only_admin_transferred_event ... ok
test tests::test_add_and_get_assets ... ok
test tests::test_add_and_list_guardians ... ok
test tests::test_add_asset_case_variant_duplicate_fails ... ok
test tests::test_add_duplicate_guardian_fails ... 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_cannot_initiate_second_recovery_while_one_pending ... ok
test tests::test_double_approval_rejected ... ok
test tests::test_execute_recovery_emits_both_admin_transferred_and_recovery_executed_events ... ok
test tests::test_admin_can_cancel_recovery_even_after_quorum ... ok
test tests::test_execute_recovery_rejects_new_admin_same_as_current_admin ... 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_native_code_with_issuer_is_contradictory_and_rejected ... 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_pending_admin_cleared_after_accept_admin ... ok
test tests::test_non_native_code_without_issuer_is_underspecified_and_rejected ... ok
test tests::test_propose_without_accept_keeps_admin_unchanged ... ok
test tests::test_record_spend_boundary_drift_awareness ... ok
test tests::test_max_guardians_limit ... 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_migrate_user_assets_trims_excess ... ok
test tests::test_record_spend_exceeds_limit_fails ... 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_bucket_is_integer_division ... ok
test tests::test_execute_upgrade_with_never_uploaded_hash_traps - should panic ... ok
test tests::test_record_spend_overflow_does_not_poison_later_calls ... ok
test tests::test_propose_and_execute_upgrade - should panic ... ok
test tests::test_record_spend_negative_amount_cannot_bypass_daily_limit ... ok
test tests::test_record_spend_within_limit ... ok
test tests::test_record_spend_rejected_negative_amount_does_not_mutate_state ... ok
test tests::test_record_spend_zero_amount_fails ... ok
test tests::test_remove_asset ... ok
test tests::test_remove_nonexistent_asset_fails ... ok
test tests::test_recovery_rejects_non_guardian ... ok
test tests::test_remove_guardian_below_threshold_fails ... ok
test tests::test_require_admin_not_initialized ... ok
test tests::test_remove_guardian_who_never_approved_leaves_proposal_untouched ... ok
test tests::test_recovery_clears_any_in_flight_normal_admin_transfer ... ok
test tests::test_recovery_happy_path_2_of_3 ... ok
test tests::test_removed_guardian_cannot_initiate_recovery ... 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_revoke_recovery_approval_rejects_non_guardian ... ok
test tests::test_set_recovery_config_rejects_single_guardian_threshold ... ok
test tests::test_spend_limit_set_and_get ... ok
test tests::test_set_recovery_config_requires_min_guardians ... ok
test tests::test_set_recovery_config_rejects_threshold_above_guardian_count ... ok
test tests::test_upgrade_rejects_hash_mismatch ... ok
test tests::test_spend_limit_ttl_extension_after_long_idle_period ... ok
test tests::test_upgrade_propose_double_fails ... ok
test tests::test_revoking_approval_below_threshold_resets_timelock ... ok
test tests::test_transfer_admin ... 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_revoke_recovery_approval_rejects_removed_guardian ... ok
test tests::test_set_recovery_config_rejected_while_recovery_pending ... ok
test tests::test_max_assets_limit ... ok

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

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

     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_past_expiry_fails ... ok
test tests::test_approve_and_allowance ... ok
test tests::test_transfer_from_expired_allowance_fails ... ok
test tests::test_transfer_from_insufficient_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 1.26s

   Doc-tests globe_wallet

running 0 tests

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

   Doc-tests token_wrapper

running 0 tests

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

Payout Routing

  • EVM (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.

[Enhancement]: admin_transferred event is identical for routine transfer and emergency guardian recovery — undermines security monitoring

1 participant