fee_collector has no way to rotate or recover the admin address — a lost key permanently locks withdraw and set_treasury

[abayomicornelius]

Overview

Every admin-gated function in fee_collector — withdraw (fee_collector/src/lib.rs:130-160) and set_treasury (:163-173) — authorizes against a single Address stored once, at initialize time, and never changed:

// fee_collector/src/lib.rs:67-81
pub fn initialize(
    env: Env,
    admin: Address,
    treasury: Address,
) -> Result<(), FeeCollectorError> {
    if env.storage().instance().has(&KEY_INIT) {
        return Err(FeeCollectorError::AlreadyInitialized);
    }
    admin.require_auth();

    env.storage().instance().set(&KEY_ADMIN, &admin);
    env.storage().instance().set(&KEY_TREASURY, &treasury);
    env.storage().instance().set(&KEY_INIT, &true);
    Ok(())
}

There is no set_admin, no transfer_admin/accept_admin pair, and no equivalent under any other name anywhere in fee_collector/src/lib.rs. KEY_ADMIN is written exactly once, in initialize, and read everywhere else purely to check require_auth() — it is never overwritten by any function in the contract. This means:

• If the admin's signing key is ever lost (hardware wallet destroyed, seed phrase lost, multisig threshold becomes unreachable due to lost signer keys), withdraw and set_treasury become permanently uncallable. Since this contract custodies real, accumulated protocol fee balances (get_balance, fee_collector/src/lib.rs:179-183, confirms the contract genuinely holds tokens, not just an accounting abstraction), a lost admin key means those balances are permanently stranded — there is no other path to withdraw them.
• If the admin key is ever compromised, there is no way to rotate away from it to a new key even after detecting the compromise — the only admin key this contract will ever have is whichever one was passed into initialize.

The repository's own stubs.rs backlog (fee_collector/src/stubs.rs:1, feat(fee_collector): add two-step admin rotation) independently confirms this is a known, planned-but-never-shipped gap in the real code, not an intentional immutability design choice — the README's Security section (README.md, under "Admin key management") also states "Admin rotation uses a two-step propose/accept pattern to prevent key loss," which is simply false for the code as it exists today in this contract.

Requirements

• Add a two-step admin rotation mechanism (propose_admin/transfer_admin followed by accept_admin, matching the README's own described pattern and the pattern stellar_send's README API reference already documents for that contract) so a single-transaction typo or premature revocation can't accidentally brick the contract, and so a detected key compromise can be recovered from.
• Ensure the rotation is itself properly authorized (current admin must authorize the proposal; new admin must authorize acceptance) and emits an event for observability.
• Until shipped, correct the README's Security section so it doesn't claim a capability that doesn't exist in fee_collector (or token_bridge, which has the identical gap — see the companion issue in this batch).

Acceptance Criteria

• fee_collector exposes a way to rotate the admin address without needing to redeploy the contract.
• The rotation requires both the outgoing and incoming admin's authorization (two-step, not a single-transaction set_admin that could brick the contract on a typo).
• A new event is emitted on proposal and on completion of the rotation.
• Tests cover: proposing a new admin, accepting it, confirming the old admin can no longer call withdraw/set_treasury, and confirming the new admin can.
• README's admin-rotation claim is either made true for fee_collector, or corrected to not overstate what's implemented, whichever ships first.

Additional Notes

Precise references: fee_collector/src/lib.rs:67-81 (initialize, the only place KEY_ADMIN is ever written), :130-143 (withdraw's admin check), :163-170 (set_treasury's admin check) — both read KEY_ADMIN via .get(&KEY_ADMIN).ok_or(FeeCollectorError::NotInitialized)? with no corresponding setter anywhere in the file. fee_collector/src/stubs.rs:1 confirms this is recognized, unshipped backlog. README.md's "Admin key management" section under "Security" claims a capability that doesn't exist in the code.

How this differs from issue #9 in this repository's existing "Consolidate duplicated admin/init/error boilerplate" issue: that issue is about deduplicating repeated admin/init/error scaffolding across the three multi-file contracts as a refactor; it does not itself add a rotation capability, and its own description doesn't claim to. This issue is about the missing capability itself, which would need to exist (in whatever form, consolidated or not) before there's anything to deduplicate.

Test/reproduction plan: in fee_collector/src/test.rs, following the existing setup()/test_initialize patterns, initialize the contract, then attempt to grep or call any set_admin/transfer_admin-shaped function — none exists today, so the first deliverable test is simply test_admin_rotation_two_step written against the new API once implemented: propose a new admin (old admin's auth), accept as the new admin (new admin's auth), then assert try_withdraw under old-admin auth now fails (e.g. with Unauthorized) while it succeeds under new-admin auth — mirroring how stellar_send's README already documents transfer_admin/accept_admin (README.md:338-344) as the intended pattern, even though that pair doesn't actually exist in stellar_send/src/lib.rs either (a distinct, already-implied gap via the existing "README documents features... not implemented in stellar_send" issue).