From 373f0791f2d5f6be90dd46cb4178e2f140a60a1a Mon Sep 17 00:00:00 2001 From: GoodnessJohn Date: Sun, 25 Oct 2026 10:16:57 +0100 Subject: [PATCH] fix: resolve issues #520 #521 #522 #523 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #520 — Remove stale placeholder begin_fill and duplicate fill_intent_inner The placeholder begin_fill (no token escrow, labeled 'For now, this is a placeholder') has been removed; the complete implementation with pull_exact escrow already existed below it. The duplicate fill_intent_inner stub (with a require_proof param but no body) has been merged into the single canonical definition. #521 — Implement per-solver bond tracking in accept_outbound_intent Replaced the 'Store solver bond (simplified — real impl would track per-solver)' comment block with real per-solver tracking: loads the solver's SolverRecord (requires solver to be registered), credits the posted bond via set_solver_bond_amount (keeping bond_amount, bond_tokens, and DataKey::SolverBond in sync per #187), increments active_intents, and pulls the bond from the solver into the contract via pull_exact (#409) for fee-on-transfer safety. Removes the unused cfg load. #522 — Implement IntentFillHistory writes in fill_intent_inner fill_intent_inner now appends (solver, fill_amount, now) to DataKey::IntentFillHistory on every fill, with FIFO eviction once MAX_FILL_HISTORY (10) entries is reached and extend_ttl after every append. Resolves the FIXME(fill-history-ttl) in test_archival.rs: intent_fill_history_ttl_bumped_if_key_exists now asserts the key exists and its TTL meets PERSISTENT_TTL_EXTEND_TO rather than skipping conditionally. #523 — Broaden SECURITY.md threat model beyond intent_settlement Added dedicated threat-model sections for solver_registry (bond custody, writer blast radius, obligation-lock griefing), proof_registry (Axelar path production gap, Guardian quorum, emitter allowlist, freshness window), reputation_badge (dual authority, TTL silent expiry), backstop_vault (settlement immutability, emergency mode, insolvency path, inflation attack mitigation), and arbiter_panel (PRNG predictability, abstention griefing, immutable post-deploy params). --- SECURITY.md | 150 ++++++++++++++++++++++++- intent_settlement/src/lib.rs | 121 ++++++++++++-------- intent_settlement/src/test_archival.rs | 28 ++--- 3 files changed, 231 insertions(+), 68 deletions(-) diff --git a/SECURITY.md b/SECURITY.md index 4992ecb..38c17be 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -240,4 +240,152 @@ hardware wallet — never a hot wallet or a key stored in plaintext. have a distinct admin key. Reusing a key across environments means a compromise on a lower-trust environment immediately threatens mainnet. -3. **Minimal admin transactio \ No newline at end of file +3. **Minimal admin transactio +ns per window.** Admin transactions should be batched and pre-announced where possible. An off-chain audit log of every admin call, indexed by ledger sequence, should be maintained and published. + +--- + +## Threat Model — `solver_registry` + +`solver_registry` is the canonical bond custodian for all registered solvers. It holds the USDC bond corpus and enforces tiered staking. + +### Assets at Risk + +| Asset | Where held | Value | +|-------|-----------|-------| +| Solver bonds | `solver_registry` contract account (USDC) | Sum of all registered bonds; Platinum tier requires ≥ 50,000 USDC per solver | +| Slashed bond proceeds | Transferred to `FeeRecipient` on slash | Up to 10 % of bond per slash event | + +### Trust Assumptions + +#### 1. Writer (settlement contract) blast radius + +The `writer` address — set once via `set_writer` and rotatable only through the 48-hour `propose_writer` / `execute_writer` timelock — is the most powerful role in `solver_registry`. It can call `record_fill`, `record_failure`, `slash`, `lock_obligation`, and `release_obligation`. A compromised writer can slash any solver's bond repeatedly (guarded per intent by `AlreadyRecorded` idempotency) and redirect proceeds to any `FeeRecipient`. The timelock provides a 48-hour monitoring window for off-chain detection before a malicious writer rotation completes. + +**Implication:** Monitor `writer_proposed` events. Any unexpected proposal must be treated as a compromise signal and escalated immediately. + +#### 2. Admin key blast radius + +The admin can tune tier thresholds within enforced bounds, initiate the writer rotation timelock, and initiate a two-step admin handover (also 48-hour timelocked). The admin **cannot** directly transfer bond tokens out — all token movements require writer or solver auth. However, a compromised admin can install a malicious writer, which can then drain bonds over time. The combined blast radius of admin + writer compromise is full bond corpus loss. + +#### 3. Obligation-lock griefing + +`lock_obligation` and `release_obligation` are writer-only (not admin-callable). If the settlement contract has a bug that locks obligations without ever releasing them, solvers cannot unstake or deregister until the lock is cleared. Solvers with perpetually locked obligations are effectively stuck — their bond is inaccessible. + +#### 4. Bond floor on slash + +The `slash` function reduces `bond_amount` with no floor. A solver slashed to zero bond retains their `SolverRecord` and can deregister to receive zero tokens back. There is no automatic deactivation at zero bond — the settlement contract is responsible for checking `is_active` / bond sufficiency before accepting new intents from the solver. + +--- + +## Threat Model — `proof_registry` + +`proof_registry` is a trust oracle. It converts Wormhole VAAs (and, in a future production path, Axelar GMP messages) into on-chain `ProofRecord` entries that `intent_settlement` uses to gate fills when `require_proof = true`. + +### Assets at Risk + +`proof_registry` holds no tokens. Its failure mode is **fraudulent proof injection**: a false `ProofRecord` would allow a solver to claim a fill was backed by a verified source-chain deposit when none occurred. This bypasses the economic-deterrent-only model and allows a solver to collect filled intent proceeds without having sent funds on the source chain. + +### Trust Assumptions + +#### 1. Wormhole Guardian quorum + +`receive_message` delegates cryptographic verification entirely to the Wormhole Core contract. A Guardian set compromise (≥ 2/3 of 19 Guardians colluding) would allow fraudulent VAAs to be injected. This residual risk is shared with all Wormhole-integrated protocols and is outside this contract's control. + +#### 2. Axelar path is not cryptographically verified (production gap) + +`receive_message_axelar` checks that the `source_address` string matches the configured authorized source, but does **not** call the Axelar Gateway contract for message signature verification. Any caller who knows the expected `source_address` string can inject an arbitrary `ProofRecord` via this path. **The Axelar path must not be enabled for production use until full Gateway signature verification is implemented.** + +#### 3. Emitter allowlist has no timelock + +`set_authorized_emitter` and `set_authorized_axelar_source` are admin-only with no timelock. A compromised admin can add a malicious emitter for any chain ID immediately, allowing fraudulent proofs to be inserted. Monitor `emitter_authorized` and `axelar_source_authorized` events. + +#### 4. Freshness window + +`get_fresh_proof` rejects proofs older than `PROOF_VALIDITY_WINDOW` (3600 seconds). Callers that use the raw `get_proof` / `has_proof` instead of `get_fresh_proof` bypass this check and may accept stale proofs. `intent_settlement` is intended to call `get_fresh_proof` exclusively on the proof-gated path. + +#### 5. testutils back-door + +`mock_set_proof` / `mock_remove_proof` are compiled only under the `testutils` Cargo feature. A release build without this feature does not expose them. **Never deploy a `testutils`-enabled build to a live network.** + +--- + +## Threat Model — `reputation_badge` + +`reputation_badge` holds no tokens. It records soulbound tier badges (`Bronze` / `Silver` / `Gold` / `Platinum`) for registered solvers. + +### Trust Assumptions + +#### 1. Dual admin / registry authority + +Both `mint_badge` and `burn_badge` accept calls from either the admin or the registered `solver_registry` contract. A compromised admin can mint or burn any badge without going through `solver_registry`'s tier logic. This is an intentional emergency override during the rollout period. + +#### 2. `set_solver_registry` has no timelock + +The admin can redirect the badge contract to any `solver_registry` address instantly. A compromised admin can point it at a contract that mints arbitrary badges for arbitrary addresses. Monitor `solver_registry_set` events. + +#### 3. Badge TTL expiry causes silent downgrade + +Badges are stored in persistent storage with a 30-day TTL. A badge that is not refreshed (because no tier-change event occurred) will eventually expire and `get_badge` will return `None` — equivalent to `Unranked` — without an explicit `burn_badge` call. Any consumer that gates privileges on badge presence must account for this silent expiry. + +--- + +## Threat Model — `backstop_vault` + +`backstop_vault` custodies LP deposits (USDC). LPs deposit in exchange for pro-rata shares; the vault absorbs settlement shortfalls via `cover()` and earns income via `receive_income()`. + +### Assets at Risk + +| Asset | Where held | Value | +|-------|-----------|-------| +| LP deposits | `backstop_vault` contract account (USDC) | Sum of all LP share redemption values | + +(Already included in the `intent_settlement` Assets at Risk table above; this section adds the vault-specific trust model.) + +### Trust Assumptions + +#### 1. Settlement contract as sole cover/income caller + +Only the `Settlement` address set at `initialize` can call `cover` and `receive_income`. This address is immutable after deployment — there is no `set_settlement` entrypoint. A compromised settlement contract can call `cover` to drain the vault to any recipient address. The immutability of the settlement address is a deliberate strong protection: the admin has no direct path to drain LP funds. + +#### 2. Emergency mode as LP-exit delay (not drain) + +`set_emergency_mode(true)` — admin-only — queues withdrawals instead of executing them. A compromised admin can delay LP exits indefinitely but cannot move tokens out of the vault directly. LPs can process their queued withdrawals once emergency mode is disabled or funds become available. + +#### 3. Vault insolvency under repeated cover + +`cover()` reduces `TotalAssets` and does not enforce a floor. Repeated cover calls can reduce `TotalAssets` to zero. Normal `withdraw` checks `assets_out > total_assets` and reverts with `VaultInsolvent`; `cover` silently caps its payout at available assets. LPs in a fully drained vault receive zero on withdrawal — this is socialized loss by design. + +#### 4. Share-price inflation attack (mitigated) + +The vault is seeded with `VIRTUAL_SHARES = 1,000,000,000` and `VIRTUAL_ASSETS = 1,000,000,000` at initialization. A dust first-deposit cannot manipulate the exchange rate to an extreme value. + +--- + +## Threat Model — `arbiter_panel` + +`arbiter_panel` custodies arbiter bonds (USDC) and drives the m-of-n dispute resolution process for `intent_settlement`. + +### Assets at Risk + +| Asset | Where held | Value | +|-------|-----------|-------| +| Arbiter bonds | `arbiter_panel` contract account | `bond_amount` × registered arbiter count | + +### Trust Assumptions + +#### 1. PRNG panel selection is deterministic + +Panel selection uses `env.prng()` seeded with the intent ID. Because the intent ID is derived from deterministic parameters, an attacker who can predict or influence the intent ID before registering as an arbiter may predict their panel placement. This is a documented limitation (see design doc). Panel selection is "random-ish, not cryptographically unpredictable." + +#### 2. Abstention-based quorum griefing + +If fewer than `threshold` (m) arbiters vote within `VOTE_WINDOW` (24 hours), `tally` closes the round without slashing anyone (`outcome = None`). A small coalition of selected arbiters can strategically abstain to prevent resolution while avoiding slash penalties, leaving the dispute open until `intent_settlement`'s `ARBITER_WINDOW` timeout applies and the user receives the full escrow. + +#### 3. Core parameters are immutable post-deploy + +`settlement`, `bond_token`, `bond_amount`, `panel_size`, and `threshold` are set only at `initialize` and cannot be changed post-deployment. This is a deliberate restriction: it prevents admin manipulation of dispute mechanics but also means parameter errors require a full redeployment. + +#### 4. Settlement cross-contract trust + +`open_vote` calls `settlement.dispute_parties(intent_id)` to determine which addresses to exclude from the panel. A malicious or misconfigured settlement contract could return arbitrary `(user, solver)` addresses, manipulating arbiter eligibility. diff --git a/intent_settlement/src/lib.rs b/intent_settlement/src/lib.rs index 00e0efc..2bf911f 100644 --- a/intent_settlement/src/lib.rs +++ b/intent_settlement/src/lib.rs @@ -3475,8 +3475,6 @@ impl IntentSettlement { /// (`require_auth()` is one-shot per address per invocation). The solver's /// signature over the batch call still covers the individual dst-token /// transfers each fill performs. - fn fill_intent_inner(env: Env, solver: Address, intent_id: BytesN<32>, fill_amount: i128, require_proof: bool) { - /// Body of `fill_intent` without the `solver.require_auth()` gate. /// /// Issue #375: uses `InvocationCtx` to load `Config`, `FeeRecipient`, /// and `TotalVolume` exactly once, and commits dirty counters in a single @@ -3607,6 +3605,36 @@ impl IntentSettlement { .set(&DataKey::Solver(solver.clone()), &solver_record); Self::bump_solver_ttl(&env, &solver); + // Issue #522 / #244: append this fill to the bounded on-chain fill-history + // log so indexers can reconstruct per-intent fill events without replaying + // the full event stream. Oldest entries are evicted (FIFO) once the cap + // of `MAX_FILL_HISTORY` entries is reached. TTL is bumped after every + // append so the key outlives the intent itself (#145). + { + let hist_key = DataKey::IntentFillHistory(intent_id.clone()); + let mut history: Vec<(Address, i128, u64)> = env + .storage() + .persistent() + .get(&hist_key) + .unwrap_or_else(|| Vec::new(&env)); + + if history.len() >= MAX_FILL_HISTORY as usize { + // FIFO eviction: remove the oldest entry to stay within the cap. + let mut trimmed: Vec<(Address, i128, u64)> = Vec::new(&env); + for i in 1..history.len() { + trimmed.push_back(history.get(i as u32).unwrap()); + } + history = trimmed; + } + history.push_back((solver.clone(), fill_amount, now)); + env.storage().persistent().set(&hist_key, &history); + env.storage().persistent().extend_ttl( + &hist_key, + PERSISTENT_TTL_THRESHOLD, + PERSISTENT_TTL_EXTEND_TO, + ); + } + ctx.add_volume(fill_amount); ctx.commit(&env); Self::save_intent(&env, &intent_id, &intent); @@ -3833,47 +3861,6 @@ impl IntentSettlement { } /// Solver begins fill by depositing dst_token into escrow. Starts dispute window. - /// Replaces the direct transfer in fill_intent once this design is implemented. - /// For now, this is a placeholder establishing the interface. - pub fn begin_fill(env: Env, solver: Address, intent_id: BytesN<32>, fill_amount: i128) { - solver.require_auth(); - Self::require_not_paused(&env); - Self::bump_instance_ttl(&env); - - let mut intent: IntentRecord = env - .storage() - .persistent() - .get(&DataKey::Intent(intent_id.clone())) - .unwrap_or_else(|| panic_with_error!(&env, Error::IntentNotFound)); - - if intent.solver.as_ref() != Some(&solver) { - panic_with_error!(&env, Error::Unauthorized); - } - - if intent.state != IntentState::Accepted { - panic_with_error!(&env, Error::IntentNotAccepted); - } - - let now = env.ledger().timestamp(); - if now >= intent.deadline { - panic_with_error!(&env, Error::FillWindowExpired); - } - - // Transition to Filling and set dispute window deadline - intent.state = IntentState::Filling; - intent.dispute_deadline = Some(now + DISPUTE_WINDOW); - - env.storage() - .persistent() - .set(&DataKey::Intent(intent_id.clone()), &intent); - Self::bump_intent_ttl(&env, &intent_id); - - env.events().publish( - (Symbol::new(&env, "fill_begun"),), - (intent_id, solver, fill_amount), - ); - } - /// Permissionless: slash a solver that accepted but didn't fill within FILL_WINDOW pub fn slash_solver(env: Env, intent_id: BytesN<32>) { Self::bump_instance_ttl(&env); @@ -5047,26 +5034,66 @@ impl IntentSettlement { panic_with_error!(&env, Error::IntentExpired); } - let cfg = Self::load_config(&env); let min_bond = Self::get_adjusted_min_bond(&env, &bond_token); if bond_amount < min_bond { panic_with_error!(&env, Error::SolverBondTooLow); } - // Store solver bond (simplified — real impl would track per-solver) + // Issue #521: per-solver bond tracking for outbound intents. + // + // Load (or create) the solver's SolverRecord and credit the posted + // bond in the chosen token using the same helpers as the forward- + // direction flow (`register_solver_inner` / `accept_intent`). The + // bond is pulled from the solver into the contract via `pull_exact` + // so fee-on-transfer tokens are handled correctly (#409), and + // `set_solver_bond_amount` keeps `bond_amount`, `bond_tokens`, and + // the per-token `DataKey::SolverBond` entries in sync (#187). + let mut solver_record: SolverRecord = env + .storage() + .persistent() + .get(&DataKey::Solver(solver.clone())) + .unwrap_or_else(|| panic_with_error!(&env, Error::SolverNotRegistered)); + + let existing_bond = Self::get_solver_bond_amount(&env, &solver_record, &bond_token); + + // ── Effects first (CEI) ───────────────────────────────────────────── + let new_bond = existing_bond.saturating_add(bond_amount); + Self::set_solver_bond_amount(&env, &mut solver_record, &bond_token, new_bond); + solver_record.active_intents += 1; + outbound_intent.solver = Some(solver.clone()); outbound_intent.state = OutboundIntentState::SolverAccepted; - outbound_intent.bond_token = bond_token; + outbound_intent.bond_token = bond_token.clone(); outbound_intent.solver_tier = Self::solver_tier(&env, &solver); + env.storage() + .persistent() + .set(&DataKey::Solver(solver.clone()), &solver_record); + Self::bump_solver_ttl(&env, &solver); env.storage() .persistent() .set(&DataKey::OutboundIntent(outbound_intent_id.clone()), &outbound_intent); Self::bump_intent_ttl(&env, &outbound_intent_id); + // ── Interaction: pull bond from solver into contract (#409) ───────── + let received = Self::pull_exact(&env, &bond_token, &solver, bond_amount); + if received != bond_amount { + // Correct for fee-on-transfer: re-derive stored bond from actual delta. + let corrected = existing_bond.saturating_add(received); + let mut corrected_record: SolverRecord = env + .storage() + .persistent() + .get(&DataKey::Solver(solver.clone())) + .unwrap(); + Self::set_solver_bond_amount(&env, &mut corrected_record, &bond_token, corrected); + env.storage() + .persistent() + .set(&DataKey::Solver(solver.clone()), &corrected_record); + } + env.events().publish( (Symbol::new(&env, "outbound_intent_accepted"), solver.clone()), - outbound_intent_id.clone(), + (outbound_intent_id.clone(), bond_token, bond_amount), ); } diff --git a/intent_settlement/src/test_archival.rs b/intent_settlement/src/test_archival.rs index 2105823..bc324c2 100644 --- a/intent_settlement/src/test_archival.rs +++ b/intent_settlement/src/test_archival.rs @@ -538,26 +538,14 @@ fn intent_fill_history_ttl_bumped_if_key_exists() { ctx.dst_admin().mint(&ctx.solver, &FILL); ctx.client().fill_intent(&ctx.solver, &id, &FILL, &false); - // Check whether IntentFillHistory was written. If not yet implemented, - // we skip the TTL assertion and note the gap in the doc. - let history_exists = ctx.env.as_contract(&ctx.contract_id, || { - ctx.env - .storage() - .persistent() - .has(&DataKey::IntentFillHistory(id.clone())) - }); - - if history_exists { - let ttl = ctx.persistent_ttl(&DataKey::IntentFillHistory(id)); - assert!( - ttl >= PERSISTENT_TTL_EXTEND_TO - 1, - "IntentFillHistory TTL {ttl} below threshold — history could be \ - archived before close_intent runs, silently losing fill records" - ); - } - // If fill history is not yet written, the test passes with a note: - // FIXME(fill-history-ttl): when IntentFillHistory writes are added, - // ensure extend_ttl is called after every append. + // Issue #522: IntentFillHistory is now written by fill_intent on every fill. + // Assert that the key exists and its TTL meets the persistence threshold. + let ttl = ctx.persistent_ttl(&DataKey::IntentFillHistory(id)); + assert!( + ttl >= PERSISTENT_TTL_EXTEND_TO - 1, + "IntentFillHistory TTL {ttl} below threshold — history could be \ + archived before close_intent runs, silently losing fill records" + ); } // ─── 7. IntentTombstone key ──────────────────────────────────────────────────