Skip to content

fix(globe-wallet): enforce MIN_GUARDIANS_FOR_RECOVERY floor in remove_guardian (#90) - #102

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

fix(globe-wallet): enforce MIN_GUARDIANS_FOR_RECOVERY floor in remove_guardian (#90)#102
s6pa1rta3n-lab wants to merge 1 commit into
Orbit-Wal:mainfrom
s6pa1rta3n-lab:fix-issue-90

Conversation

@s6pa1rta3n-lab

Copy link
Copy Markdown

Closes #90

Summary

  • Re-validates against GlobeWallet::MIN_GUARDIANS_FOR_RECOVERY (3) in remove_guardian whenever a RecoveryConfig exists, preventing guardian count from degrading below the minimum allowed for recovery.
  • Made MIN_GUARDIANS_FOR_RECOVERY pub const to align with MAX_GUARDIANS and MAX_ASSETS.
  • Added tests verifying:
    • Guardian removal degrading below MIN_GUARDIANS_FOR_RECOVERY while recovery is configured is rejected with WalletError::NotEnoughGuardians.
    • Guardian removal still works normally when no RecoveryConfig exists.
    • Guardian removal down to exactly MIN_GUARDIANS_FOR_RECOVERY succeeds at the boundary.
  • Updated existing tests to start with sufficient guardians where applicable.

Definition of Done Verification

  • remove_guardian rejects removal that would drop the guardian count below MIN_GUARDIANS_FOR_RECOVERY whenever RecoveryConfig is set, in addition to the existing threshold check
  • Test proving the reproduction case above is now rejected (test_remove_guardian_degrades_below_min_guardians_for_recovery)
  • Test proving guardian removal still works normally when no RecoveryConfig exists (test_remove_guardian_without_recovery_config_succeeds_below_min_guardians)
  • Test proving guardian removal down to exactly MIN_GUARDIANS_FOR_RECOVERY still succeeds (test_remove_guardian_down_to_min_guardians_for_recovery_succeeds)
  • cargo test --workspace output pasted

Test Output (cargo test --workspace)

running 72 tests
test tests::test_add_duplicate_guardian_fails ... ok
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_asset_fails ... ok
test tests::test_add_and_list_guardians ... ok
test tests::test_add_asset_case_variant_duplicate_fails ... ok
test tests::test_add_and_get_assets ... 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_admin_can_cancel_recovery_even_after_quorum ... ok
test tests::test_double_approval_rejected ... 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_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_non_native_code_without_issuer_is_underspecified_and_rejected ... ok
test tests::test_pending_admin_cleared_after_accept_admin ... 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_record_spend_boundary_first_second_of_new_day_resets ... ok
test tests::test_propose_upgrade_requires_admin ... ok
test tests::test_execute_upgrade_with_never_uploaded_hash_traps - should panic ... 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_propose_and_execute_upgrade - should panic ... ok
test tests::test_record_spend_negative_amount_fails ... ok
test tests::test_raise_spend_then_lower_limit ... ok
test tests::test_migrate_user_assets_trims_excess ... ok
test tests::test_record_spend_exact_day_boundary ... ok
test tests::test_record_spend_exceeds_limit_fails ... ok
test tests::test_record_spend_negative_amount_cannot_bypass_daily_limit ... ok
test tests::test_record_spend_overflow_does_not_poison_later_calls ... ok
test tests::test_record_spend_zero_amount_fails ... 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_remove_asset ... ok
test tests::test_recovery_rejects_non_guardian ... ok
test tests::test_recovery_happy_path_2_of_3 ... ok
test tests::test_recovery_clears_any_in_flight_normal_admin_transfer ... ok
test tests::test_remove_guardian_degrades_below_min_guardians_for_recovery ... ok
test tests::test_remove_guardian_below_threshold_fails ... ok
test tests::test_remove_nonexistent_asset_fails ... ok
test tests::test_require_admin_not_initialized ... ok
test tests::test_remove_guardian_without_recovery_config_succeeds_below_min_guardians ... ok
test tests::test_remove_guardian_down_to_min_guardians_for_recovery_succeeds ... ok
test tests::test_remove_guardian_dequorated_proposal_can_requorum_with_fresh_timelock ... 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_max_assets_limit ... 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_spend_limit_set_and_get ... ok
test tests::test_revoke_recovery_approval_rejects_removed_guardian ... 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_set_recovery_config_rejects_threshold_above_guardian_count ... ok
test tests::test_spend_limit_ttl_extension_after_long_idle_period ... ok
test tests::test_transfer_admin ... ok
test tests::test_upgrade_propose_double_fails ... 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_set_recovery_config_rejected_while_recovery_pending ... ok
test tests::test_user_assets_ttl_extension_after_long_idle_period ... ok

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

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

     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_transfer_from_happy_path ... ok
test tests::test_approve_overwrites_previous_allowance ... ok
test tests::test_transfer_from_succeeds_exactly_at_expiry_ledger ... ok
test tests::test_transfer_from_zero_amount_fails ... 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.03s

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.

[Bug]: MIN_GUARDIANS_FOR_RECOVERY is enforced only when set_recovery_config is called — remove_guardian can silently degrade below it

1 participant