Skip to content

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

Description

@ndii-dev

Context

GlobeWallet::accept_admin and GlobeWallet::execute_recovery, contracts/globe-wallet/src/lib.rs. Both change DataKey::Admin and both emit the same event.

Problem

execute_recovery's own comment explains the choice: "Same event name/shape as a normal transfer: downstream indexers and the mobile app don't need to special-case recovery-driven admin changes."

// accept_admin:
env.events().publish((Symbol::new(&env, "admin_transferred"),), (admin, candidate));

// execute_recovery:
env.events().publish((Symbol::new(&env, "admin_transferred"),), (old_admin, proposal.new_admin));

Identical topic, identical payload shape. There is no on-chain signal distinguishing "the admin rotated their own key, routine maintenance" from "a guardian quorum just seized control of this wallet because the admin key was presumed lost or compromised" — arguably the single highest-signal security event this contract can produce.

Impact

Any off-chain monitoring/alerting integration (the exact kind of system a wallet handling real funds should have — "notify the user via a side channel the moment their admin key changes through the emergency path, since that's precisely when they most need to know, and precisely when the compromised device/channel they normally use might not be trustworthy") cannot distinguish these events without separately watching for recovery_quorum_reached/recovery_approved events and correlating timing/addresses after the fact — fragile, and easy to get wrong or miss under load. The simplification that helps client-side display code stay simple comes at the cost of making the one event that most deserves special, urgent handling indistinguishable from routine housekeeping.

Suggested fix

Keep admin_transferred for backward compatibility (existing indexers/mobile app keep working unchanged, satisfying the original design goal), but also emit a distinct recovery_completed (or similar) event alongside it specifically in execute_recovery, carrying (old_admin, new_admin) plus whatever context is useful for an alert (e.g. the guardian addresses that approved it). This is additive — no existing consumer needs to change, but a consumer that wants to distinguish the emergency path now can.

Definition of done

  • execute_recovery emits both the existing admin_transferred event (unchanged, for backward compatibility) and a new, distinctly-named recovery-specific event
  • The new event's payload includes enough information for a monitoring integration to act on it (at minimum old admin, new admin; consider including the approving guardian set)
  • Test proving both events fire on a successful execute_recovery, with the correct topic/payload for each
  • Test proving accept_admin (routine transfer) still emits only admin_transferred, unchanged
  • Rationale for the event shape/topic name written out in the PR (this is a design-decision issue per CONTRIBUTING.md — the write-up matters as much as the code)
  • cargo test --workspace output pasted

Metadata

Metadata

Assignees

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardThird CampaignCampaign: Third CampaignenhancementNew feature or requestvery hardDifficulty: very hard

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions