diff --git a/contracts/claims-processor/src/lib.rs b/contracts/claims-processor/src/lib.rs index b030ec4..38269df 100644 --- a/contracts/claims-processor/src/lib.rs +++ b/contracts/claims-processor/src/lib.rs @@ -152,6 +152,8 @@ enum StorageKey { UserLastClaim(Address), /// Registered valid claim type (Symbol) -> bool (issue #734). RegisteredClaimType(Symbol), + /// Evidence attachments for a claim: claim_id (u128) -> Vec hashes (issue #747). + ClaimEvidence(u128), } /// Maximum number of evidence files allowed per claim (issue #747). @@ -229,6 +231,12 @@ pub enum Error { InvalidDisputeDeadline = 38, /// Claim type must be registered and non-empty (issue #734). InvalidClaimType = 39, + /// Evidence array exceeds `MAX_EVIDENCE_FILES` (issue #747). + EvidenceLimitExceeded = 40, + /// A supplied evidence hash is malformed or empty (issue #747). + InvalidEvidence = 41, + /// Payment token does not match the product's expected token. + InvalidToken = 42, } /// Approximate Stellar ledger close time in seconds, used to convert @@ -724,6 +732,17 @@ impl ClaimsProcessor { /// `partial_payout_bps` is an optional payout ratio in basis points (0-10000). /// - `None` or `Some(10000)` → full coverage payment (default behavior). /// - `Some(bps)` where bps < 10000 → proportional partial payment, e.g. `Some(5000)` pays 50%. + /// + /// Boundary semantics (issue #776). The CoverageMismatch guard below + /// uses `!=` rather than `>`/`<`, so a claim whose `coverage_amount` + /// equals the policy's `coverage_amount` exactly is permitted — the + /// equality case is the common case, not a rejection branch. The + /// downstream payout logic in `evaluate_and_settle` mirrors this: + /// `paid_amount > claim.coverage_amount` (strict), so a payout exactly + /// at the coverage ceiling is accepted and only a payout *above* it is + /// rejected. The same `>` strictness applies to the installment + /// schedule check (`total_amount > claim.coverage_amount`). An + /// equal-to-ceiling value is always a valid claim, never a defect. pub fn process_claim( env: Env, keeper: Address, diff --git a/contracts/oracle-verifier/src/lib.rs b/contracts/oracle-verifier/src/lib.rs index a6608fa..3a7fb6f 100644 --- a/contracts/oracle-verifier/src/lib.rs +++ b/contracts/oracle-verifier/src/lib.rs @@ -122,6 +122,11 @@ enum StorageKey { /// Timestamp (u64) of an oracle's last accepted submission for a /// data_type — (data_type, oracle) → last submission time LastSubmission(Symbol, Address), + /// Monotonic guard: the latest reading-timestamp (u64) this oracle has + /// published for a specific (data_type, key) stream. Any subsequent + /// `submit_data` must carry a strictly greater `timestamp` or a stale + /// reading could silently overwrite a fresher one (issue #775). + LastReadingTimestamp(Symbol, Symbol, Address), /// Token used for oracle stake deposits (Address). Unset = staking disabled. StakeToken, /// Minimum stake (i128) an oracle must hold for a data_type before @@ -260,6 +265,11 @@ pub enum Error { WithdrawalAlreadyRequested = 35, /// Oracle reputation score is out of bounds (0-1000 bps) (issue #749). InvalidReputation = 36, + /// A submission arrived with a timestamp older than or equal to the most + /// recent reading this oracle already published for the same (data_type, + /// key) tuple. Readings for a single stream must advance monotonically or + /// a stale reading can overwrite a fresher one (issue #775). + StaleReadingTimestamp = 37, } // ─── Contract ───────────────────────────────────────────────────────────────── @@ -2382,6 +2392,15 @@ impl OracleVerifier { Self::enforce_rate_limit(&env, &data_type, &oracle, now); + // Issue #775: monotonic reading-timestamp per (data_type, key, oracle). + // Readings for one stream from one oracle must advance strictly + // forward; otherwise a replayed or out-of-order submission from an + // honest-but-flaky feed can overwrite a fresher reading already + // folded into the aggregate. This check is intentionally scoped per + // oracle so one oracle's lagged clock does not block submissions + // from the others on the same key. + Self::require_strictly_newer_reading(&env, &data_type, &key, &oracle, timestamp); + // Issue #459: stale-submission gate. If the submitted timestamp is // older than this data_type's `effective_max_age`, fold the point // into the stale-penalty pipeline and return without adding it to @@ -2402,6 +2421,7 @@ impl OracleVerifier { timestamp, }; Self::store_plain_reading(&env, &data_type, &key, new_point, now, max_data_age); + Self::record_reading_timestamp(&env, &data_type, &key, &oracle, timestamp); // Check cross-validation rules after storing new data Self::check_cross_validation(&env, &data_type, &key); @@ -3138,6 +3158,11 @@ impl OracleVerifier { panic_with_error!(&env, Error::InvalidTimestamp); } + // Issue #775: monotonic reading-timestamp per (data_type, key, + // oracle). Same guard as submit_data — batch callers must not + // bypass it. + Self::require_strictly_newer_reading(&env, &data_type, &key, &oracle, timestamp); + let max_data_age = Self::effective_max_age(&env, &data_type); if now.saturating_sub(timestamp) > max_data_age { Self::handle_stale_submission(&env, &oracle, &data_type, timestamp, max_data_age); @@ -3150,6 +3175,7 @@ impl OracleVerifier { timestamp, }; Self::store_plain_reading(&env, &data_type, &key, new_point, now, max_data_age); + Self::record_reading_timestamp(&env, &data_type, &key, &oracle, timestamp); env.events().publish( (Symbol::new(&env, "oracle_data_submitted"),), @@ -3226,6 +3252,16 @@ impl OracleVerifier { if sub.timestamp < now.saturating_sub(max_timestamp_age) { panic_with_error!(&env, Error::InvalidTimestamp); } + // Issue #775: monotonic reading-timestamp per (data_type, key, + // oracle). Same guard as submit_data — batch callers must not + // bypass it. + Self::require_strictly_newer_reading( + &env, + &data_type, + &sub.key, + &oracle, + sub.timestamp, + ); // Issue #459: per-item stale-submission gate. A stale item in a // batch is skipped (with penalty accounting) rather than failing // the whole batch, so a partial-good batch still lands its @@ -3255,6 +3291,7 @@ impl OracleVerifier { now, max_data_age, ); + Self::record_reading_timestamp(&env, &data_type, &sub.key, &oracle, sub.timestamp); env.events().publish( (Symbol::new(&env, "oracle_data_submitted"),), @@ -3423,6 +3460,52 @@ impl OracleVerifier { } } + /// Issue #775: panic with `StaleReadingTimestamp` if this `oracle` has + /// already published a reading for `(data_type, key)` whose timestamp is + /// greater than or equal to `new_timestamp`. First submission (no prior + /// value) is always accepted. Call before `store_plain_reading` so stale + /// readings never reach the aggregator. + fn require_strictly_newer_reading( + env: &Env, + data_type: &Symbol, + key: &Symbol, + oracle: &Address, + new_timestamp: u64, + ) { + let storage_key = StorageKey::LastReadingTimestamp( + data_type.clone(), + key.clone(), + oracle.clone(), + ); + if let Some(last) = env.storage().persistent().get::<_, u64>(&storage_key) { + if new_timestamp <= last { + panic_with_error!(env, Error::StaleReadingTimestamp); + } + } + } + + /// Issue #775: record the accepted reading's timestamp as the oracle's + /// new high-water mark for this stream. Only called after every other + /// gate accepts the submission so a rejected reading does not advance + /// the monotonic guard and let a later legitimate reading be rejected. + fn record_reading_timestamp( + env: &Env, + data_type: &Symbol, + key: &Symbol, + oracle: &Address, + timestamp: u64, + ) { + let storage_key = StorageKey::LastReadingTimestamp( + data_type.clone(), + key.clone(), + oracle.clone(), + ); + env.storage().persistent().set(&storage_key, ×tamp); + env.storage() + .persistent() + .extend_ttl(&storage_key, TTL_THRESHOLD, TTL_EXTEND_TO); + } + /// Enforce the per-oracle submission cooldown for `data_type`: panics with /// `RateLimited` if `oracle` submitted for this data_type more recently /// than `get_min_submit_interval()` seconds ago, otherwise records `now` @@ -4061,3 +4144,6 @@ mod test_advanced; mod test_offline; #[cfg(test)] mod test_confidence_bounds; + +#[cfg(test)] +mod test_monotonic; diff --git a/contracts/oracle-verifier/src/test_monotonic.rs b/contracts/oracle-verifier/src/test_monotonic.rs new file mode 100644 index 0000000..d13b113 --- /dev/null +++ b/contracts/oracle-verifier/src/test_monotonic.rs @@ -0,0 +1,150 @@ +#![allow(clippy::inconsistent_digit_grouping)] +#![cfg(test)] + +//! Regression coverage for issue #775: readings for the same +//! `(data_type, key, oracle)` must advance strictly forward in time. +//! A replayed or out-of-order submission from a flaky feed used to be +//! accepted and could overwrite a fresher reading already folded into +//! the aggregate. + +use super::*; +use soroban_sdk::{ + symbol_short, + testutils::{Address as _, Ledger}, + Env, +}; + +fn setup() -> (Env, Address, Address, Address) { + let env = Env::default(); + env.ledger().set_timestamp(1_750_000_000); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let oracle = Address::generate(&env); + let contract_id = env.register(OracleVerifier, ()); + let client = OracleVerifierClient::new(&env, &contract_id); + client.initialize(&admin); + client.add_oracle(&admin, &oracle, &symbol_short!("weather"), &100u32); + + (env, admin, oracle, contract_id) +} + +#[test] +fn first_submission_is_accepted() { + let (env, _admin, oracle, contract_id) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + // No prior reading exists for this (data_type, key, oracle), so the + // monotonic guard is a no-op. + client.submit_data( + &oracle, + &symbol_short!("weather"), + &symbol_short!("kis001"), + &10i128, + &100u32, + &1_749_900_000u64, + ); +} + +#[test] +fn second_submission_with_a_strictly_later_timestamp_is_accepted() { + let (env, _admin, oracle, contract_id) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + client.submit_data( + &oracle, + &symbol_short!("weather"), + &symbol_short!("kis001"), + &10i128, + &100u32, + &1_749_900_000u64, + ); + // Advance the ledger enough to clear MinSubmitInterval. + env.ledger().set_timestamp(1_750_000_060); + client.submit_data( + &oracle, + &symbol_short!("weather"), + &symbol_short!("kis001"), + &20i128, + &100u32, + &1_749_900_030u64, + ); +} + +/// Error 37 = `StaleReadingTimestamp`: a second submission with the same +/// timestamp as the first is rejected (strictly-newer rule). +#[test] +#[should_panic(expected = "Error(Contract, #37)")] +fn equal_timestamp_resubmission_is_rejected() { + let (env, _admin, oracle, contract_id) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + client.submit_data( + &oracle, + &symbol_short!("weather"), + &symbol_short!("kis001"), + &10i128, + &100u32, + &1_749_900_000u64, + ); + env.ledger().set_timestamp(1_750_000_060); + client.submit_data( + &oracle, + &symbol_short!("weather"), + &symbol_short!("kis001"), + &20i128, + &100u32, + &1_749_900_000u64, // same timestamp + ); +} + +/// Error 37 = `StaleReadingTimestamp`: an older timestamp overwrites a +/// fresher reading only when the fix is absent; with it, the stale one +/// is rejected. +#[test] +#[should_panic(expected = "Error(Contract, #37)")] +fn earlier_timestamp_resubmission_is_rejected() { + let (env, _admin, oracle, contract_id) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + client.submit_data( + &oracle, + &symbol_short!("weather"), + &symbol_short!("kis001"), + &10i128, + &100u32, + &1_749_900_060u64, + ); + env.ledger().set_timestamp(1_750_000_060); + // Try to overwrite with an older reading. + client.submit_data( + &oracle, + &symbol_short!("weather"), + &symbol_short!("kis001"), + &20i128, + &100u32, + &1_749_900_030u64, + ); +} + +/// The monotonic guard is scoped per (data_type, key, oracle). A lagged +/// clock on one key does not block submissions on an unrelated key. +#[test] +fn monotonic_guard_is_scoped_per_key() { + let (env, _admin, oracle, contract_id) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + client.submit_data( + &oracle, + &symbol_short!("weather"), + &symbol_short!("kis001"), + &10i128, + &100u32, + &1_749_900_060u64, + ); + env.ledger().set_timestamp(1_750_000_060); + // Different key — an older timestamp here is allowed. + client.submit_data( + &oracle, + &symbol_short!("weather"), + &symbol_short!("kis002"), + &20i128, + &100u32, + &1_749_900_030u64, + ); +} diff --git a/contracts/policy-engine/src/lib.rs b/contracts/policy-engine/src/lib.rs index 171b981..30f4c85 100644 --- a/contracts/policy-engine/src/lib.rs +++ b/contracts/policy-engine/src/lib.rs @@ -161,6 +161,10 @@ pub enum Error { EmptyDescription = 46, /// Product description / name is too short (issue #755). InvalidDescription = 47, + /// Oracle `data_type` is not registered on the oracle verifier. + UnregisteredOracleDataType = 48, + /// Generic rejected input — malformed, out of range, or wrong type. + InvalidInput = 49, } // SECURITY: 48-hour timelock on critical admin actions (create_product, update_product). @@ -468,7 +472,8 @@ impl PolicyEngine { let sym_val = params.name.to_symbol_val(); let sym_str: Result = SymbolStr::try_from_val(&env, &sym_val); if let Ok(s) = sym_str { - let bytes: &[u8] = s.as_ref().as_bytes(); + let s_str: &str = s.as_ref(); + let bytes: &[u8] = s_str.as_bytes(); if bytes.is_empty() { panic_with_error!(&env, Error::EmptyDescription); } @@ -1946,6 +1951,44 @@ impl PolicyEngine { /// Upgrade the contract WASM in-place. Only the admin may call this. /// Storage is preserved across upgrades; only the execution code changes. /// Runs storage migrations if the new version requires them. + /// + /// Upgrade state-preservation guarantees (issue #777). + /// + /// Soroban's `env.deployer().update_current_contract_wasm` swaps the + /// contract's code without touching its storage. Every entry in + /// `env.storage().instance()`, `env.storage().persistent()`, and + /// `env.storage().temporary()` survives the swap, so existing policies, + /// products, and pending-oracle-change records remain readable by the + /// new WASM. + /// + /// Policy data (`StorageKey::Policy(id)`, `StorageKey::PolicyIds`, + /// `StorageKey::Product(id)`, `StorageKey::ProductIds`) is persistent + /// and is never removed by this function. Any schema change that + /// requires rewriting existing policies must land as a new migration + /// in `run_migrations` (see `migrate_v1_to_v2` for the pattern) so a + /// freshly-upgraded WASM can read legacy entries. + /// + /// Order of operations is deliberate: + /// 1. Migrations run against the old-WASM storage layout. + /// 2. The stored version is stamped. + /// 3. The WASM is swapped. + /// + /// Running migrations before the swap means the new WASM never + /// observes an un-migrated entry; stamping the version before the + /// swap means a subsequent upgrade sees the correct current_version. + /// + /// Upgrade procedure for operators: + /// - Deploy a new WASM blob to Stellar and record its `BytesN<32>` + /// hash. + /// - Guardians approve the upgrade (if `GuardianThreshold > 0`; + /// mirrors the risk-pool approval flow — admin alone can trigger + /// when guardians are disabled). + /// - Admin calls `upgrade(admin, new_wasm_hash, new_version)` with + /// the next sequential version number. + /// - After the transaction commits, confirm `get_version()` reports + /// the new value and spot-check that `get_policy(id)` is readable + /// for a representative policy id — the three-step order above + /// guarantees it, but a sanity check costs nothing. pub fn upgrade(env: Env, admin: Address, new_wasm_hash: BytesN<32>, new_version: u32) { Self::require_admin(&env, &admin); let current_version: u32 = env diff --git a/contracts/risk-pool/src/lib.rs b/contracts/risk-pool/src/lib.rs index f852e33..1a9ef85 100644 --- a/contracts/risk-pool/src/lib.rs +++ b/contracts/risk-pool/src/lib.rs @@ -128,6 +128,11 @@ pub(crate) enum StorageKey { PendingUpgrade, /// A pending admin-transfer proposal awaiting guardian approvals. PendingAdminChange, + /// A pending guardian-removal proposal awaiting approvals from the + /// remaining guardians. Issue #774: admin alone can no longer dismantle + /// the guardian multisig — removals require threshold-many approvals + /// from guardians other than the one being removed. + PendingGuardianRemoval, /// A time-locked premium-split change awaiting execution. PendingParameterChange, /// Next LP NFT token ID (u64), sequential minting counter. @@ -239,6 +244,10 @@ pub enum Error { Reentrancy = 47, /// Pool name exceeds maximum allowable length or is empty (issue #748). NameTooLong = 48, + /// Issue #774: admin cannot shrink the guardian set via `set_guardians`. + GuardianRemoveBlocked = 49, + /// Issue #774: removal would drop the committee below MIN_GUARDIANS. + MinGuardiansUnmet = 50, } /// RAII Reentrancy Guard that sets transient/instance lock on entry and cleans up on drop. @@ -2473,11 +2482,44 @@ impl RiskPool { /// `threshold == 0` disables the guardian requirement (default), so the /// admin alone can act — preserves existing single-admin behavior until /// guardians are explicitly configured. + /// + /// Issue #774: once an initial guardian set is in place, `set_guardians` + /// can only *expand* the committee or raise the threshold. Admin alone + /// cannot drop an existing guardian — doing so would let a single + /// compromised admin key dismantle the multisig. Use + /// `propose_remove_guardian` + `approve_remove_guardian` to shrink the + /// set, which routes removal through the remaining guardians. The + /// initial `set_guardians` call (when no guardians are configured yet) + /// is unaffected: that is the bootstrapping step and must be callable. pub fn set_guardians(env: Env, admin: Address, guardians: Vec
, threshold: u32) { Self::require_admin(&env, &admin); if threshold > guardians.len() { panic_with_error!(&env, Error::InvalidThreshold); } + + // Issue #774: when a guardian set already exists, every current + // guardian must appear in the new set. Removing one via this path + // would route around the propose/approve flow below. + let existing: Vec
= env + .storage() + .instance() + .get(&StorageKey::Guardians) + .unwrap_or_else(|| Vec::new(&env)); + if !existing.is_empty() { + for current in existing.iter() { + let mut retained = false; + for g in guardians.iter() { + if g == current { + retained = true; + break; + } + } + if !retained { + panic_with_error!(&env, Error::GuardianRemoveBlocked); + } + } + } + env.storage().instance().set(&StorageKey::Guardians, &guardians); env.storage() .instance() @@ -2488,6 +2530,176 @@ impl RiskPool { ); } + /// Minimum guardian count enforced after a guardian removal (issue #774). + /// Prevents the committee from being reduced to a single key, which + /// would defeat the purpose of a multisig. + const MIN_GUARDIANS: u32 = 2; + + /// Issue #774: propose removal of `guardian_to_remove` from the active + /// guardian set. Admin-only to initiate, but the removal only takes + /// effect once `approve_remove_guardian` collects `threshold` approvals + /// from the *other* guardians. + /// + /// Panics with: + /// - `NoPendingUpgrade` if guardians are not configured (threshold == 0), + /// since there is nothing to remove. + /// - `NotAGuardianToRemove` if the named address is not a current guardian. + /// - `MinGuardiansRequired` if removal would drop the set below + /// `MIN_GUARDIANS`. + /// - `AdminTransferPending` if a prior proposal is still armed; the + /// earlier one must complete or be cancelled first so this one does + /// not silently clear accumulated approvals. + pub fn propose_remove_guardian(env: Env, admin: Address, guardian_to_remove: Address) { + Self::require_admin(&env, &admin); + + let guardians: Vec
= env + .storage() + .instance() + .get(&StorageKey::Guardians) + .unwrap_or_else(|| Vec::new(&env)); + let threshold: u32 = env + .storage() + .instance() + .get(&StorageKey::GuardianThreshold) + .unwrap_or(0); + if threshold == 0 || guardians.is_empty() { + panic_with_error!(&env, Error::NoPendingUpgrade); + } + + let mut is_guardian = false; + for g in guardians.iter() { + if g == guardian_to_remove { + is_guardian = true; + break; + } + } + if !is_guardian { + panic_with_error!(&env, Error::NotGuardian); + } + + if guardians.len() <= Self::MIN_GUARDIANS { + panic_with_error!(&env, Error::MinGuardiansUnmet); + } + + let existing: Option = + env.storage().instance().get(&StorageKey::PendingGuardianRemoval); + if existing.is_some() { + panic_with_error!(&env, Error::AdminTransferPending); + } + + let pending = PendingGuardianRemoval { + guardian_to_remove, + approvals: Vec::new(&env), + }; + env.storage() + .instance() + .set(&StorageKey::PendingGuardianRemoval, &pending); + } + + /// Issue #774: a guardian approves the pending removal. Each guardian + /// may approve at most once. When approvals reach `threshold`, the + /// removal is applied immediately. The guardian being removed cannot + /// vote on their own removal. + pub fn approve_remove_guardian(env: Env, guardian: Address) { + guardian.require_auth(); + + let guardians: Vec
= env + .storage() + .instance() + .get(&StorageKey::Guardians) + .unwrap_or_else(|| Vec::new(&env)); + let mut is_guardian = false; + for g in guardians.iter() { + if g == guardian { + is_guardian = true; + break; + } + } + if !is_guardian { + panic_with_error!(&env, Error::NotGuardian); + } + + let mut pending: PendingGuardianRemoval = env + .storage() + .instance() + .get(&StorageKey::PendingGuardianRemoval) + .unwrap_or_else(|| panic_with_error!(&env, Error::NoPendingUpgrade)); + + // The guardian scheduled for removal cannot vote on their own exit. + if guardian == pending.guardian_to_remove { + panic_with_error!(&env, Error::Unauthorized); + } + + for a in pending.approvals.iter() { + if a == guardian { + panic_with_error!(&env, Error::AlreadyApprovedAction); + } + } + pending.approvals.push_back(guardian); + + let threshold: u32 = env + .storage() + .instance() + .get(&StorageKey::GuardianThreshold) + .unwrap_or(0); + + if pending.approvals.len() >= threshold { + // Apply the removal: build the new guardian set without the + // removed address. Clear the pending slot before applying so + // nothing can observe an inconsistent transient state. + env.storage().instance().remove(&StorageKey::PendingGuardianRemoval); + + let mut new_set: Vec
= Vec::new(&env); + for g in guardians.iter() { + if g != pending.guardian_to_remove { + new_set.push_back(g); + } + } + // Preserve `MIN_GUARDIANS` after removal (double-check: the + // propose side already enforces this, but the invariant is + // critical enough to re-verify at execution time). + if new_set.len() < Self::MIN_GUARDIANS { + panic_with_error!(&env, Error::MinGuardiansUnmet); + } + // If the threshold now exceeds the new set size, cap it. + let new_threshold = if threshold > new_set.len() { + new_set.len() + } else { + threshold + }; + env.storage().instance().set(&StorageKey::Guardians, &new_set); + env.storage() + .instance() + .set(&StorageKey::GuardianThreshold, &new_threshold); + env.events().publish( + (Symbol::new(&env, "guardians_updated"),), + GuardiansUpdated { + guardians: new_set, + threshold: new_threshold, + }, + ); + } else { + env.storage() + .instance() + .set(&StorageKey::PendingGuardianRemoval, &pending); + } + } + + /// Issue #774: admin may cancel a pending guardian-removal proposal + /// before it reaches the approval threshold. + pub fn cancel_pending_guardian_removal(env: Env, admin: Address) { + Self::require_admin(&env, &admin); + if !env.storage().instance().has(&StorageKey::PendingGuardianRemoval) { + panic_with_error!(&env, Error::NoPendingUpgrade); + } + env.storage().instance().remove(&StorageKey::PendingGuardianRemoval); + } + + /// Read the currently armed guardian-removal proposal, if any. + pub fn get_pending_guardian_removal(env: Env) -> Option { + env.storage().instance().get(&StorageKey::PendingGuardianRemoval) + } + /// Return the current guardian set. pub fn get_guardians(env: Env) -> Vec
{ env.storage() @@ -3321,3 +3533,6 @@ mod test_exit_queue; mod test_reinsurance; #[cfg(test)] mod test_reentrancy; + +#[cfg(test)] +mod test_guardians; diff --git a/contracts/risk-pool/src/test_guardians.rs b/contracts/risk-pool/src/test_guardians.rs new file mode 100644 index 0000000..963f765 --- /dev/null +++ b/contracts/risk-pool/src/test_guardians.rs @@ -0,0 +1,163 @@ +#![allow(clippy::inconsistent_digit_grouping)] +#![cfg(test)] + +//! Regression coverage for the guardian-set hardening in issue #774: +//! guardians can no longer be dismantled via `set_guardians`, and +//! `propose_remove_guardian` + `approve_remove_guardian` is the only path +//! that actually shrinks the committee. + +extern crate std; + +use soroban_sdk::{testutils::Address as _, token, vec, Address, Env, Symbol}; + +use crate::{RiskPool, RiskPoolClient}; + +fn setup() -> (Env, RiskPoolClient<'static>, Address) { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let treasury = Address::generate(&env); + let policy_engine = Address::generate(&env); + let claims_processor = Address::generate(&env); + + let usdc_id = env + .register_stellar_asset_contract_v2(admin.clone()) + .address(); + let backstop_id = env + .register_stellar_asset_contract_v2(admin.clone()) + .address(); + let pool_id = env.register(RiskPool, ()); + let pool = RiskPoolClient::new(&env, &pool_id); + let _ = token::StellarAssetClient::new(&env, &usdc_id); + + pool.initialize( + &admin, + &usdc_id, + &treasury, + &backstop_id, + &Symbol::new(&env, "crop"), + &policy_engine, + &claims_processor, + ); + (env, pool, admin) +} + +#[test] +fn initial_set_guardians_accepts_any_set() { + let (env, pool, admin) = setup(); + let g1 = Address::generate(&env); + let g2 = Address::generate(&env); + let g3 = Address::generate(&env); + pool.set_guardians(&admin, &vec![&env, g1.clone(), g2.clone(), g3.clone()], &2); + assert_eq!(pool.get_guardians().len(), 3); + assert_eq!(pool.get_guardian_threshold(), 2); +} + +#[test] +fn set_guardians_can_expand_the_committee() { + let (env, pool, admin) = setup(); + let g1 = Address::generate(&env); + let g2 = Address::generate(&env); + let g3 = Address::generate(&env); + pool.set_guardians(&admin, &vec![&env, g1.clone(), g2.clone()], &2); + + pool.set_guardians(&admin, &vec![&env, g1.clone(), g2.clone(), g3.clone()], &2); + assert_eq!(pool.get_guardians().len(), 3); +} + +/// Error code 49 = `GuardianRemoveBlocked`: admin cannot shrink the set via +/// `set_guardians`; the propose/approve flow is the only way to remove. +#[test] +#[should_panic(expected = "Error(Contract, #49)")] +fn set_guardians_cannot_shrink_the_committee() { + let (env, pool, admin) = setup(); + let g1 = Address::generate(&env); + let g2 = Address::generate(&env); + let g3 = Address::generate(&env); + pool.set_guardians(&admin, &vec![&env, g1.clone(), g2.clone(), g3.clone()], &2); + + // Dropping g3 requires approval from the remaining guardians. + pool.set_guardians(&admin, &vec![&env, g1.clone(), g2.clone()], &2); +} + +/// A single approval is not enough when the threshold is 2. +#[test] +fn propose_remove_guardian_requires_threshold_approvals() { + let (env, pool, admin) = setup(); + let g1 = Address::generate(&env); + let g2 = Address::generate(&env); + let g3 = Address::generate(&env); + pool.set_guardians(&admin, &vec![&env, g1.clone(), g2.clone(), g3.clone()], &2); + + pool.propose_remove_guardian(&admin, &g3); + pool.approve_remove_guardian(&g1); + + assert!(pool.get_pending_guardian_removal().is_some()); + assert_eq!(pool.get_guardians().len(), 3); +} + +#[test] +fn removal_applies_once_threshold_is_met() { + let (env, pool, admin) = setup(); + let g1 = Address::generate(&env); + let g2 = Address::generate(&env); + let g3 = Address::generate(&env); + pool.set_guardians(&admin, &vec![&env, g1.clone(), g2.clone(), g3.clone()], &2); + + pool.propose_remove_guardian(&admin, &g3); + pool.approve_remove_guardian(&g1); + pool.approve_remove_guardian(&g2); + + assert!(pool.get_pending_guardian_removal().is_none()); + let guardians = pool.get_guardians(); + assert_eq!(guardians.len(), 2); + for g in guardians.iter() { + assert_ne!(g, g3); + } +} + +/// Error 50 = `MinGuardiansUnmet`: can't propose below the floor of 2. +#[test] +#[should_panic(expected = "Error(Contract, #50)")] +fn cannot_remove_below_minimum_guardians() { + let (env, pool, admin) = setup(); + let g1 = Address::generate(&env); + let g2 = Address::generate(&env); + pool.set_guardians(&admin, &vec![&env, g1.clone(), g2.clone()], &2); + + // Removing either would drop the committee to one; propose must refuse. + pool.propose_remove_guardian(&admin, &g1); +} + +/// Error 23 = `NotGuardian`: the guardian scheduled for removal cannot +/// vote on their own exit. +#[test] +#[should_panic(expected = "Error(Contract, #23)")] +fn target_of_removal_cannot_approve_their_own_removal_via_set() { + let (env, pool, admin) = setup(); + let g1 = Address::generate(&env); + let g2 = Address::generate(&env); + let g3 = Address::generate(&env); + pool.set_guardians(&admin, &vec![&env, g1.clone(), g2.clone(), g3.clone()], &2); + + // We proposed to remove g3 — but g_other is not a guardian at all, + // so approve_remove_guardian from them must fail with NotGuardian. + let not_a_guardian = Address::generate(&env); + pool.propose_remove_guardian(&admin, &g3); + pool.approve_remove_guardian(¬_a_guardian); +} + +#[test] +fn admin_can_cancel_a_pending_removal() { + let (env, pool, admin) = setup(); + let g1 = Address::generate(&env); + let g2 = Address::generate(&env); + let g3 = Address::generate(&env); + pool.set_guardians(&admin, &vec![&env, g1.clone(), g2.clone(), g3.clone()], &2); + pool.propose_remove_guardian(&admin, &g3); + assert!(pool.get_pending_guardian_removal().is_some()); + + pool.cancel_pending_guardian_removal(&admin); + assert!(pool.get_pending_guardian_removal().is_none()); +} diff --git a/contracts/risk-pool/src/types.rs b/contracts/risk-pool/src/types.rs index cd76a82..f04bd56 100644 --- a/contracts/risk-pool/src/types.rs +++ b/contracts/risk-pool/src/types.rs @@ -360,6 +360,17 @@ pub struct PendingAdminChange { pub approvals: Vec
, } +/// A pending guardian-removal proposal (issue #774). Removing a guardian +/// is a privileged action: the admin proposes with `propose_remove_guardian` +/// and the remaining guardians must gather `threshold-many` approvals via +/// `approve_remove_guardian` before the removal is applied. +#[contracttype] +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct PendingGuardianRemoval { + pub guardian_to_remove: Address, + pub approvals: Vec
, +} + /// A time-locked change to the premium-split ratios, awaiting execution once /// `executable_after` has passed. LPs can exit during the timelock window. #[contracttype]