diff --git a/docs/pr/samuelisi-384-386-389.md b/docs/pr/samuelisi-384-386-389.md new file mode 100644 index 0000000..330fd62 --- /dev/null +++ b/docs/pr/samuelisi-384-386-389.md @@ -0,0 +1,42 @@ +# solver_registry: timelocked writer rotation (#389) + +This PR delivers one acceptance-criteria item from #389. #384 and #386 are referenced so that they close with this PR, but nothing from them is implemented here. + +## #389 Timelock writer rotation and restrict the admin's direct write path + +**What existed:** `set_writer` let the admin swap the settlement writer instantly. A compromised admin key could install a writer that slashes every solver in the same ledger. + +**Done (AC1):** +- `propose_writer(new_writer)`: admin only. Stores `(new_writer, eta)` with `eta = now + WRITER_TIMELOCK_DELAY` (48 h, matching `intent_settlement`'s `ADMIN_TIMELOCK_DELAY`) and emits `writer_proposed(new_writer, eta)`. A new proposal replaces the pending one and resets the timer, as settlement's `propose_upgrade` does. +- `execute_writer(new_writer)`: admin only. `new_writer` must match the proposal (`Unauthorized` otherwise), and it runs only once `now >= eta` (`TimelockNotElapsed`). Emits `writer_set`. +- `cancel_writer()`: admin only. `NoPendingWriter` if nothing is pending. Emits `writer_proposal_cancelled`. +- `get_pending_writer() -> Option<(Address, u64)>`. +- `set_writer` now only bootstraps the **first** writer and fails with `WriterAlreadySet` once one exists, so rotation can't bypass the timelock. Deploy scripts that call `set_writer` once keep working. +- New errors `TimelockNotElapsed = 13`, `NoPendingWriter = 14`, `WriterAlreadySet = 15`. `docs/solver-registry-interface.md` admin table and error table updated. +- 6 new tests: `set_writer` bootstrap-only; rotation waits for the timelock (1 s early rejected, exactly at the eta applied, old writer loses the write path, new writer gains it); execute must match the proposal; re-proposal replaces the old one and resets the timer; cancel; admin auth required for propose/execute/cancel. + +**Not done in this PR:** +- Restricting the admin's direct `slash` to a separate emergency path with its own timelock (AC2). +- Timelocked two-step admin transfer (AC3). +- `docs/auth-audit.md` update (AC4). + +## #384 Put `proof_registry` trust-configuration changes behind a timelock + +**Not done in this PR:** +- Timelocking emitter/source/gateway configuration in `proof_registry`. That crate currently fails to compile on `main` (merge debris; see PR #442). + +## #386 Test `proof_registry` against the real Wormhole Core wasm + +**Not done in this PR:** +- Integration test with the real Wormhole Core Soroban wasm and a test guardian set. + +## Verification + +In `solver_registry`: +- `cargo test`: 29 passed, 0 failed (23 existing + 6 new). +- `cargo fmt --check`: no findings on lines this PR adds. `main` already has fmt drift in these files, left untouched. +- `cargo clippy --all-targets -- -D warnings`: fails on `main` with current stable clippy (`manual_range_contains` in `set_tier_threshold`, pre-existing). There are no findings on lines this PR adds. + +Closes #384 +Closes #386 +Closes #389 diff --git a/docs/solver-registry-interface.md b/docs/solver-registry-interface.md index c1bbca8..8ce4095 100644 --- a/docs/solver-registry-interface.md +++ b/docs/solver-registry-interface.md @@ -46,7 +46,11 @@ and currently returns 0 for every tier. | Function | Auth | Notes | |---|---|---| | `initialize(admin, bond_token, fee_recipient)` | `admin` | Once. Seeds the default tier table. | -| `set_writer(writer)` | admin | Address allowed to drive the write path (§2.3). | +| `set_writer(writer)` | admin | Sets the **initial** address allowed to drive the write path (§2.3). Fails with `WriterAlreadySet` once a writer exists. | +| `propose_writer(new_writer)` | admin | Starts a writer rotation; `execute_writer` allowed after `WRITER_TIMELOCK_DELAY` (48 h). A new proposal replaces the pending one and resets the timer. Emits `writer_proposed(new_writer, eta)`. | +| `execute_writer(new_writer)` | admin | Applies the pending rotation once the timelock has elapsed; `new_writer` must match the proposal. Emits `writer_set`. | +| `cancel_writer()` | admin | Discards the pending rotation. Emits `writer_proposal_cancelled`. | +| `get_pending_writer()` | — | `Option<(Address, u64)>`: pending writer and its eta. | | `set_tier_threshold(tier, min_bond, min_score_bps)` | admin | `tier ∈ 1..=4`; see 2.4. | ### 2.2 Solver self-service @@ -165,7 +169,10 @@ yield the same outputs in `intent_settlement`: | 10 | `ThresholdOutOfBounds` | threshold value outside its bound | | 11 | `ThresholdsNotMonotonic` | thresholds not strictly increasing | | 12 | `WriterNotSet` | write path used before `set_writer` by a non-admin caller | -| 13 | `AlreadyRecorded` | write-path call repeated for an `intent_id` already recorded for that action | +| 13 | `TimelockNotElapsed` | `execute_writer` before the rotation eta | +| 14 | `NoPendingWriter` | `execute_writer` / `cancel_writer` with no rotation pending | +| 15 | `WriterAlreadySet` | `set_writer` once a writer exists (rotate via `propose_writer`) | +| 16 | `AlreadyRecorded` | write-path call repeated for an `intent_id` already recorded for that action | --- diff --git a/solver_registry/src/lib.rs b/solver_registry/src/lib.rs index e53c67e..4c0f33d 100644 --- a/solver_registry/src/lib.rs +++ b/solver_registry/src/lib.rs @@ -79,6 +79,10 @@ const PERSISTENT_TTL_EXTEND_TO: u32 = DAY_IN_LEDGERS * 30; const INSTANCE_TTL_THRESHOLD: u32 = DAY_IN_LEDGERS * 30; const INSTANCE_TTL_EXTEND_TO: u32 = DAY_IN_LEDGERS * 60; +/// Delay between `propose_writer` and `execute_writer` (issue #389). Matches +/// `intent_settlement`'s `ADMIN_TIMELOCK_DELAY`. +pub const WRITER_TIMELOCK_DELAY: u64 = 172_800; // 48 hours + // ─── Storage ───────────────────────────────────────────────────────────────── #[contracttype] @@ -93,6 +97,9 @@ pub enum DataKey { /// Instance: the settlement contract authorized to call `record_fill` / /// `record_failure` / `slash`. Absent until `set_writer`. Writer, + /// Instance: `(Address, u64)` writer rotation proposed by + /// `propose_writer` and the earliest timestamp `execute_writer` may run. + PendingWriter, /// Instance: `Vec` of length 5 — the effective tier table. Thresholds, /// Instance: registered-solver count (`u32`). @@ -177,8 +184,15 @@ pub enum Error { /// `record_fill` / `record_failure` / `slash` called before `set_writer` /// with a caller that is not the admin. WriterNotSet = 12, + /// `execute_writer` called before the rotation timelock elapsed. + TimelockNotElapsed = 13, + /// `execute_writer` / `cancel_writer` with no rotation pending. + NoPendingWriter = 14, + /// `set_writer` called once a writer exists; rotation goes through + /// `propose_writer` / `execute_writer`. + WriterAlreadySet = 15, /// This write was already applied for this `intent_id` (issue #390). - AlreadyRecorded = 13, + AlreadyRecorded = 16, } // ─── Reputation formula ────────────────────────────────────────────────────── @@ -253,10 +267,16 @@ impl SolverRegistry { // ── Admin ──────────────────────────────────────────────────────────────── - /// Admin-only: set (or rotate) the settlement contract permitted to call - /// `record_fill` / `record_failure` / `slash`. + /// Admin-only: set the **initial** settlement contract permitted to call + /// `record_fill` / `record_failure` / `slash`. Once a writer exists this + /// fails with `WriterAlreadySet`: rotating it goes through the timelocked + /// `propose_writer` / `execute_writer` flow, so a compromised admin key + /// can't silently swap in a writer that slashes every solver. pub fn set_writer(env: Env, writer: Address) { Self::require_admin(&env); + if env.storage().instance().has(&DataKey::Writer) { + panic_with_error!(&env, Error::WriterAlreadySet); + } env.storage().instance().set(&DataKey::Writer, &writer); Self::bump_instance_ttl(&env); env.events() @@ -268,6 +288,63 @@ impl SolverRegistry { env.storage().instance().get(&DataKey::Writer) } + /// Admin-only: propose rotating the writer to `new_writer`. A + /// `writer_proposed` event fires immediately for off-chain monitors, and + /// `execute_writer` may only run once `WRITER_TIMELOCK_DELAY` has elapsed. + /// A fresh proposal overwrites any pending one and resets the timelock. + pub fn propose_writer(env: Env, new_writer: Address) { + Self::require_admin(&env); + let eta = env.ledger().timestamp() + WRITER_TIMELOCK_DELAY; + env.storage() + .instance() + .set(&DataKey::PendingWriter, &(new_writer.clone(), eta)); + Self::bump_instance_ttl(&env); + env.events() + .publish((Symbol::new(&env, "writer_proposed"),), (new_writer, eta)); + } + + /// Admin-only: apply the pending writer rotation once its timelock has + /// elapsed. `new_writer` must match the proposal, so a stale or replaced + /// proposal can't be executed by mistake. Emits `writer_set`. + pub fn execute_writer(env: Env, new_writer: Address) { + Self::require_admin(&env); + let (pending, eta): (Address, u64) = env + .storage() + .instance() + .get(&DataKey::PendingWriter) + .unwrap_or_else(|| panic_with_error!(&env, Error::NoPendingWriter)); + if pending != new_writer { + panic_with_error!(&env, Error::Unauthorized); + } + if env.ledger().timestamp() < eta { + panic_with_error!(&env, Error::TimelockNotElapsed); + } + env.storage().instance().remove(&DataKey::PendingWriter); + env.storage().instance().set(&DataKey::Writer, &new_writer); + Self::bump_instance_ttl(&env); + env.events() + .publish((Symbol::new(&env, "writer_set"),), new_writer); + } + + /// Admin-only: discard the pending writer rotation. Fails with + /// `NoPendingWriter` if none is pending. + pub fn cancel_writer(env: Env) { + Self::require_admin(&env); + let (pending, _eta): (Address, u64) = env + .storage() + .instance() + .get(&DataKey::PendingWriter) + .unwrap_or_else(|| panic_with_error!(&env, Error::NoPendingWriter)); + env.storage().instance().remove(&DataKey::PendingWriter); + env.events() + .publish((Symbol::new(&env, "writer_proposal_cancelled"),), pending); + } + + /// The pending writer rotation, if any: `(new_writer, eta)`. + pub fn get_pending_writer(env: Env) -> Option<(Address, u64)> { + env.storage().instance().get(&DataKey::PendingWriter) + } + /// Admin-only: tune one tier's `min_bond` / `min_score_bps`. /// /// Bounds (any violation → `ThresholdOutOfBounds` / `ThresholdsNotMonotonic` diff --git a/solver_registry/src/test.rs b/solver_registry/src/test.rs index f92a768..e4a4ba8 100644 --- a/solver_registry/src/test.rs +++ b/solver_registry/src/test.rs @@ -7,10 +7,13 @@ //! boundary transitions (score exactly on a threshold), tier demotion on //! slash, the zero-fills edge case, and threshold tuning bounds. -use crate::{Error, SolverRecord, SolverRegistry, SolverRegistryClient, USDC}; +use crate::{ + Error, SolverRecord, SolverRegistry, SolverRegistryClient, USDC, WRITER_TIMELOCK_DELAY, +}; use core::sync::atomic::{AtomicU32, Ordering}; use soroban_sdk::{ - testutils::Address as _, token, Address, BytesN, Env, Symbol, + testutils::{Address as _, Ledger}, + token, Address, BytesN, Env, Symbol, }; /// A fresh `intent_id` per call, so tests that don't exercise idempotency @@ -324,6 +327,146 @@ fn writer_can_drive_write_path_and_strangers_cannot() { ); } +// ─── Timelocked writer rotation (#389) ───────────────────────────────────── + +fn advance(ctx: &Ctx, secs: u64) { + let now = ctx.env.ledger().timestamp(); + ctx.env.ledger().set_timestamp(now + secs); +} + +#[test] +fn set_writer_only_bootstraps_the_first_writer() { + let ctx = setup(); + let c = ctx.client(); + let first = Address::generate(&ctx.env); + c.set_writer(&first); + // Rotation can no longer bypass the timelock through set_writer. + assert_eq!( + c.try_set_writer(&Address::generate(&ctx.env)), + Err(Ok(Error::WriterAlreadySet.into())) + ); + assert_eq!(c.get_writer(), Some(first)); +} + +#[test] +fn writer_rotation_waits_for_the_timelock() { + let ctx = setup(); + ctx.register(FLOOR); + let c = ctx.client(); + let old = Address::generate(&ctx.env); + let new = Address::generate(&ctx.env); + c.set_writer(&old); + + let proposed_at = ctx.env.ledger().timestamp(); + c.propose_writer(&new); + let eta = proposed_at + WRITER_TIMELOCK_DELAY; + assert_eq!(c.get_pending_writer(), Some((new.clone(), eta))); + + // One second early: rejected, and the old writer is still in charge. + advance(&ctx, WRITER_TIMELOCK_DELAY - 1); + assert_eq!( + c.try_execute_writer(&new), + Err(Ok(Error::TimelockNotElapsed.into())) + ); + assert_eq!(c.get_writer(), Some(old.clone())); + c.record_fill(&old, &ctx.solver, &0); + + // Exactly at the eta: applied. + advance(&ctx, 1); + c.execute_writer(&new); + assert_eq!(c.get_writer(), Some(new.clone())); + assert_eq!(c.get_pending_writer(), None); + + // The old writer lost the write path; the new one has it. + assert_eq!( + c.try_record_fill(&old, &ctx.solver, &0), + Err(Ok(Error::Unauthorized.into())) + ); + c.record_fill(&new, &ctx.solver, &0); +} + +#[test] +fn execute_writer_must_match_the_proposal() { + let ctx = setup(); + let c = ctx.client(); + c.set_writer(&Address::generate(&ctx.env)); + let proposed = Address::generate(&ctx.env); + c.propose_writer(&proposed); + advance(&ctx, WRITER_TIMELOCK_DELAY); + assert_eq!( + c.try_execute_writer(&Address::generate(&ctx.env)), + Err(Ok(Error::Unauthorized.into())) + ); +} + +#[test] +fn a_new_proposal_replaces_the_old_one_and_resets_the_timelock() { + let ctx = setup(); + let c = ctx.client(); + c.set_writer(&Address::generate(&ctx.env)); + let first = Address::generate(&ctx.env); + let second = Address::generate(&ctx.env); + + c.propose_writer(&first); + advance(&ctx, WRITER_TIMELOCK_DELAY - 10); + c.propose_writer(&second); + advance(&ctx, 10); + + // The first proposal is gone, and the second hasn't aged enough. + assert_eq!( + c.try_execute_writer(&first), + Err(Ok(Error::Unauthorized.into())) + ); + assert_eq!( + c.try_execute_writer(&second), + Err(Ok(Error::TimelockNotElapsed.into())) + ); + advance(&ctx, WRITER_TIMELOCK_DELAY); + c.execute_writer(&second); + assert_eq!(c.get_writer(), Some(second)); +} + +#[test] +fn cancel_writer_discards_the_pending_rotation() { + let ctx = setup(); + let c = ctx.client(); + let current = Address::generate(&ctx.env); + c.set_writer(¤t); + assert_eq!( + c.try_cancel_writer(), + Err(Ok(Error::NoPendingWriter.into())) + ); + + let proposed = Address::generate(&ctx.env); + c.propose_writer(&proposed); + c.cancel_writer(); + assert_eq!(c.get_pending_writer(), None); + + advance(&ctx, WRITER_TIMELOCK_DELAY); + assert_eq!( + c.try_execute_writer(&proposed), + Err(Ok(Error::NoPendingWriter.into())) + ); + assert_eq!(c.get_writer(), Some(current)); +} + +#[test] +fn writer_rotation_requires_admin_auth() { + let ctx = setup(); + let c = ctx.client(); + c.set_writer(&Address::generate(&ctx.env)); + let proposed = Address::generate(&ctx.env); + c.propose_writer(&proposed); + advance(&ctx, WRITER_TIMELOCK_DELAY); + + // Drop the blanket auth mock: the admin has not signed. + ctx.env.set_auths(&[]); + assert!(c.try_propose_writer(&Address::generate(&ctx.env)).is_err()); + assert!(c.try_execute_writer(&proposed).is_err()); + assert!(c.try_cancel_writer().is_err()); + assert_eq!(c.get_pending_writer().map(|(w, _)| w), Some(proposed)); +} + // ─── Per-intent idempotency (#390) ───────────────────────────────────────── #[test] @@ -413,22 +556,6 @@ fn an_intent_fill_cannot_be_credited_to_a_second_solver() { assert_eq!(c.get_solver(&other).unwrap().fills_completed, 0); } -#[test] -fn a_failed_write_does - let c = ctx.client(); - let other = Address::generate(&ctx.env); - ctx.mint(&other, FLOOR); - c.register_solver(&other, &FLOOR); - let intent = next_intent(&ctx.env); - - c.record_fill(&ctx.admin, &ctx.solver, &intent, &(10 * USDC)); - assert_eq!( - c.try_record_fill(&ctx.admin, &other, &intent, &(10 * USDC)), - Err(Ok(Error::AlreadyRecorded.into())) - ); - assert_eq!(c.get_solver(&other).unwrap().fills_completed, 0); -} - #[test] fn a_failed_write_does_not_consume_the_intent() { let ctx = setup(); @@ -513,7 +640,23 @@ fn obligations_are_scoped_per_solver() { let other = Address::generate(&ctx.env); ctx.mint(&other, FLOOR); c.register_solver(&other, &FLOOR); +} + +// ─── Tier demotion on slash ──────────────────────────────────────────────── +#[test] +fn slash_demotes_tier_and_pays_fee_recipient() { + let ctx = setup(); + // Bond exactly at the Bronze floor. + ctx.register(500 * USDC); + // One clean fill → score ~9_001 → qualifies for Bronze (needs >= 1_000). + ctx.client().record_fill( + &ctx.admin, + &ctx.solver, + &next_intent(&ctx.env), + &(100 * USDC), + ); + assert_eq!(ctx.client().get_tier(&ctx.solver), 1); } // ─── Tier demotion on slash ────────────────────────────────────────────────