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

Activity

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

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