Skip to content

transfer_admin and update_admin are duplicate entrypoints with inconsistent event emission #916

Description

@Ejirowebfi

🔴 Critical · contracts/token-factory/src/lib.rs:851-881

Description

Two functions perform the identical state transition (state.admin = new_admin, guarded by the same auth/self-transfer checks):

pub fn transfer_admin(env: Env, admin: Address, new_admin: Address) -> Result<(), Error> { /* no event */ }
pub fn update_admin(env: Env, current_admin: Address, new_admin: Address) -> Result<(), Error> { /* emits adm_upd event */ }

Having two public entrypoints for the same privileged operation, one of which is silent on-chain and one of which isn't, is a real operational hazard: any off-chain monitoring/alerting built against the adm_upd event (a reasonable thing to build, given admin-key rotation is a critical security event worth alerting on) will miss an admin transfer performed via transfer_admin. An attacker who has compromised the admin key and wants to quietly rotate to a key they control before performing further damage has a documented, working way to do so without tripping event-based monitoring.

Tasks

  • Determine why both functions exist (check git blame / PR history for context) — if there's no remaining reason for two entrypoints, deprecate one.
  • If keeping only one: make transfer_admin call update_admin internally (or vice versa) so there is exactly one code path and one event-emission point, then mark the other #[deprecated] (if Soroban tooling supports contract-level deprecation) or remove it in a coordinated upgrade.
  • If both must remain for backward-compatibility with existing integrations, at minimum make transfer_admin emit the same adm_upd event as update_admin so no admin-rotation path is silent.
  • Add/update tests asserting both functions emit a monitorable event after this fix.
  • Audit the frontend (AdminPanel.tsx) and any indexer/monitoring code for which of the two functions it actually calls, and whether it currently listens for adm_upd.

Acceptance Criteria

  • Every code path that changes FactoryState.admin emits an event, verified by test.
  • The public contract surface has a single, unambiguous, documented way to rotate the admin (or two functions with clearly documented, distinct purposes — not accidental duplication).

Metadata

Metadata

Assignees

Labels

GrantFox OSSIssue tracked in GrantFox OSSMaybe RewardedIssue may be eligible for a GrantFox rewardOfficial Campaign | FWC26Campaign: Official Campaign | FWC26bugSomething isn't workingcontractscriticalCritical severity — address immediatelysecurity

Type

No type

Projects

No projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions