Skip to content

fix(contract): signal incomplete rotation from legacy admin aliases (#1159) - #1175

Merged
Ejirowebfi merged 1 commit into
Favourorg:mainfrom
Dennis-Ritchie1:fix/1159-legacy-admin-alias-deprecation
Aug 25, 2026
Merged

fix(contract): signal incomplete rotation from legacy admin aliases (#1159)#1175
Ejirowebfi merged 1 commit into
Favourorg:mainfrom
Dennis-Ritchie1:fix/1159-legacy-admin-alias-deprecation

Conversation

@Dennis-Ritchie1

Copy link
Copy Markdown
Contributor

Closes #1159

Problem

transfer_admin / update_admin used to complete an admin rotation in one transaction. They now delegate to propose_admin — and that downgrade was invisible. Tooling and runbooks written against the old semantics see a successful transaction, conclude the handover is done, and (per the old documented procedure) decommission the outgoing key. If accept_admin never lands, the proposal expires after ~28.8 hours and the factory is stuck permanently under a key that no longer exists: no guardian override, no timelock bypass.

What changed

Contract — the aliases now say what they actually did, twice over:

  • Return value. Both return an AdminRotationReceipt instead of ():

    Field Value
    rotation_complete always falsestate.admin is unchanged
    pending_admin the address that must accept
    expires_at_ledger ledger at which the proposal lapses
    required_next_call accept_admin

    A client decoding the old void return fails loudly instead of silently reporting success.

  • Event. Both emit adm_dep(current_admin, new_admin, expiry_ledger, deprecated_entrypoint)in addition to adm_prop, so existing rotation monitoring keeps working while gaining a distinct signal that some caller still assumes one-step semantics.

Docs:

  • docs/mainnet-deployment-checklist.md gains an Admin Key Rotation section: 8 ordered steps where accept_admin and a get_state() verification (admin = <new> and pending_admin = null) are required before the old key may be retired.
  • docs/incident-response.md: break-glass activation (§7.4) and emergency rotation (§6.1) are rewritten as two transactions plus verification; new §2.5 Stale pending admin proposals documents thresholds, response order, and the TTL decision; adm_dep added to the alerting list; the tabletop exercise now rehearses both signatures.
  • README, SECURITY.md and docs/contract-abi.md mark the aliases deprecated and document the receipt and the new event.

Monitoring: scripts/check-pending-admin-proposal.sh — cron-able (≤15 min) check that reads get_state(), ages the proposal against the TTL and exits 1 (warn, 6h), 2 (page, 12h), 3 (already expired), 4 (check failed).

TTL decision (documented either way, as the issue asked): left at 17,280 ledgers with no renew entrypoint. Renewal would have to be authorized by the current admin — the very key that is unavailable in the scenario a longer TTL is meant to rescue — while a long-lived proposal is a standing, accept-anytime claim on the factory if the proposed key is later lost. Recorded in lib.rs next to the constant and in runbook §2.5.

Frontend: adm_dep added to CONTRACT_TOPIC_MAP, ContractEventType and parseRpcEvent (decodes the entrypoint name so the stale caller can be traced).

Tests

  • test_transfer_admin_receipt_signals_incomplete_rotation / test_update_admin_receipt_signals_incomplete_rotation — the receipt states the rotation has not happened and names accept_admin; its expiry matches state.
  • test_legacy_aliases_emit_an_extra_deprecation_event — the aliases emit two events where propose_admin emits one.
  • test_transfer_admin_alone_never_rotates_admin_even_after_expiry / ..._update_admin_... — locks in current behaviour: calling an alias alone leaves state.admin unchanged, accept_admin fails with ProposalExpired afterwards, and the old admin retains every privilege.
  • Frontend: adm_dep decode test.

205 contract tests pass; cargo fmt --check and cargo clippy --all-targets clean; frontend typecheck, lint and stellar-impl tests pass; event-topic, ABI-doc and validation drift checks pass.

Acceptance criteria

  • The on-chain result of transfer_admin / update_admin cannot be read as a completed rotation — rotation_complete: false in the return value, adm_dep in the event stream.
  • The deployment checklist and every admin-rotation runbook sequence accept_admin (and its verification) as required before decommissioning the old key.
  • A stale pending-admin-proposal monitoring recommendation exists in docs/incident-response.md (§2.5), backed by a script.

…avourorg#1159)

`transfer_admin` / `update_admin` used to rotate the admin in a single
transaction. They now delegate to `propose_admin`, and that downgrade was
invisible: a caller built against the old semantics saw a successful
transaction, concluded the rotation was done, and — following the old
runbook — could decommission the outgoing key while the factory was still
owned by it. If `accept_admin` then never landed, the proposal expired and
the factory was left permanently under a key that no longer existed, with
no guardian override and no timelock bypass.

Both aliases now report what they actually did, twice over:

- They return an `AdminRotationReceipt` instead of `()`, carrying
  `rotation_complete: false`, the pending admin, the expiry ledger, and
  `required_next_call: accept_admin`. A client decoding the old `void`
  return fails loudly rather than silently reporting success.
- They emit an `adm_dep` event — `(current_admin, new_admin, expiry_ledger,
  deprecated_entrypoint)` — alongside the usual `adm_prop`, so existing
  rotation monitoring is unaffected while gaining a distinct signal that
  some caller still assumes one-step semantics.

Also:

- Docs: the mainnet checklist gains an "Admin Key Rotation" section that
  sequences `accept_admin` and the `get_state()` verification as required
  steps before the old key may be retired; the incident-response runbook's
  break-glass and emergency-rotation procedures are rewritten as two
  transactions plus a verification, and section 2.5 documents stale
  pending-admin-proposal monitoring. README, SECURITY.md and the ABI
  reference mark the aliases deprecated and document the receipt and event.
- `scripts/check-pending-admin-proposal.sh`: cron-able check that ages a
  pending proposal against the ~28.8h TTL, exiting 1 (warn, 6h), 2 (page,
  12h) or 3 (already expired).
- The TTL is deliberately left at 17,280 ledgers with no renew entrypoint —
  renewal would need the current admin key, the very key missing in the
  scenario a longer TTL would rescue, while a long-lived proposal is a
  standing claim on the factory. Documented in lib.rs and runbook §2.5.
- Tests: the aliases' receipts, the extra `adm_dep` event, and — locking in
  current behaviour — that calling either alias alone leaves `state.admin`
  unchanged even after the proposal expires. Frontend parses and tests the
  new `adm_dep` topic.
@Ejirowebfi
Ejirowebfi merged commit 6dcaf07 into Favourorg:main Aug 25, 2026
20 checks passed
github-actions Bot pushed a commit that referenced this pull request Aug 25, 2026
## [1.8.6](v1.8.5...v1.8.6) (2026-08-25)

### Bug Fixes

* **contract:** signal incomplete rotation from legacy admin aliases ([#1159](#1159)) ([#1175](#1175)) ([6dcaf07](6dcaf07))
@github-actions

Copy link
Copy Markdown

🎉 This PR is included in version 1.8.6 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Legacy transfer_admin/update_admin silently downgraded — permanent admin-lockout risk

2 participants