diff --git a/contracts/claims-processor/src/lib.rs b/contracts/claims-processor/src/lib.rs index 2b03caf..875d13c 100644 --- a/contracts/claims-processor/src/lib.rs +++ b/contracts/claims-processor/src/lib.rs @@ -298,8 +298,17 @@ impl ClaimsProcessor { /// Admin-only: authorize `keeper` to call process_claim / auto_process / /// batch_auto_process. Without this, no address can settle claims. + /// + /// `keeper` must be a well-formed Stellar address (issue #572). A keeper + /// entry is a settlement authority: whatever holds it can move coverage + /// USDC out of the pool. Admitting an address whose StrKey encoding is + /// malformed writes a permanent registry entry that can never be + /// legitimately exercised and that off-chain indexers will silently fail + /// to resolve when reconciling `keeper_added` events, so the format is + /// checked up front and the write is refused. pub fn add_keeper(env: Env, admin: Address, keeper: Address) { Self::require_admin(&env, &admin); + Self::validate_stellar_address(&env, &keeper); env.storage().persistent().set(&StorageKey::Keeper(keeper.clone()), &true); env.storage().persistent().extend_ttl(&StorageKey::Keeper(keeper.clone()), TTL_THRESHOLD, TTL_EXTEND_TO); env.events().publish( @@ -309,6 +318,12 @@ impl ClaimsProcessor { } /// Admin-only: revoke a keeper's settlement authority. + /// + /// Deliberately does *not* re-run the format check that `add_keeper` + /// applies. This is the remediation path for a bad registry entry: an + /// admin must be able to evict a keeper that was admitted before the + /// check existed, or a malformed entry would be unremovable. Removing an + /// unknown address is a no-op either way. pub fn remove_keeper(env: Env, admin: Address, keeper: Address) { Self::require_admin(&env, &admin); env.storage().persistent().remove(&StorageKey::Keeper(keeper.clone())); @@ -2162,19 +2177,19 @@ impl ClaimsProcessor { id } + /// Panic unless `address` is a well-formed Stellar address. + /// + /// Delegates the actual format decision to [`is_well_formed_strkey`] so + /// the rule lives in one pure, directly testable function instead of + /// being spread across the call sites that need it (issue #572). fn validate_stellar_address(env: &Env, address: &Address) { let addr_str = address.to_string(); - - // Check length: Stellar public keys are exactly 56 characters - if addr_str.len() != 56 { + let mut buf = [0u8; STRKEY_LEN]; + if addr_str.len() as usize != STRKEY_LEN { panic_with_error!(env, Error::InvalidAddress); } - - let mut buf = [0u8; 56]; addr_str.copy_into_slice(&mut buf); - - // Check prefix: G (Stellar account) or C (Stellar contract) - if buf[0] != b'G' && buf[0] != b'C' { + if !is_well_formed_strkey(&buf) { panic_with_error!(env, Error::InvalidAddress); } } @@ -2373,9 +2388,49 @@ fn map_comparison( } } +/// Length of a Stellar `StrKey` (a `G…` account id or a `C…` contract id): +/// one version character plus 55 base32 characters encoding the 33-byte +/// (version + 32-byte payload) key, rounded up to a whole 5-bit group. +const STRKEY_LEN: usize = 56; + +/// Whether `bytes` is a syntactically well-formed Stellar address string +/// (issue #572). +/// +/// A `StrKey` is 56 bytes long and encodes the 33-byte +/// `version || payload` buffer in unpadded base32 (RFC 4648). Two version +/// characters are meaningful on the network: `G` for a classic account and +/// `C` for a contract. Everything after the version byte must therefore be a +/// base32 symbol — `A`–`Z` or `2`–`7` — which is what makes this a real format +/// check rather than the prefix test it replaced. Note that the StrKey +/// checksum is intentionally *not* verified: the host already guarantees a +/// canonical encoding for any `Address` it hands us, so a checksum failure is +/// not reachable from here, and re-deriving one on-chain would cost a CRC16 +/// for no additional guarantee. +/// +/// Takes bytes rather than `&str` because `soroban_sdk::String` (what +/// `Address::to_string` returns) cannot be borrowed as `str`. Pure and +/// infallible, so it can be exercised directly against byte strings that no +/// `Address` could ever be constructed from. +fn is_well_formed_strkey(bytes: &[u8]) -> bool { + if bytes.len() != STRKEY_LEN { + return false; + } + // Version byte: G (account) or C (contract). + if bytes[0] != b'G' && bytes[0] != b'C' { + return false; + } + bytes[1..] + .iter() + .all(|&b| (b >= b'A' && b <= b'Z') || (b >= b'2' && b <= b'7')) +} + #[cfg(test)] mod test; #[cfg(test)] mod test_integration; #[cfg(test)] mod test_advanced; +#[cfg(test)] +mod test_keeper_registry; +#[cfg(test)] +mod test_claim_uniqueness; diff --git a/contracts/claims-processor/src/test_claim_uniqueness.rs b/contracts/claims-processor/src/test_claim_uniqueness.rs new file mode 100644 index 0000000..9e1bce6 --- /dev/null +++ b/contracts/claims-processor/src/test_claim_uniqueness.rs @@ -0,0 +1,472 @@ +//! One claim per policy (issue #569). +//! +//! A policy is the unit of risk: it carries exactly one `coverage_amount` +//! earmarked in the risk pool, and `pay_claim` settles it once. If more than +//! one claim record could exist for the same policy, a second record would be +//! a second handle on the same coverage — so the invariant is enforced by a +//! `policy_id → claim_id` index (`StorageKey::PolicyClaim`) that every claim +//! creation path must consult. +//! +//! The guard already existed on `submit_claim`; what was not pinned down is +//! that *every* creation path shares it, and that a rejected duplicate leaves +//! nothing behind. That is what these tests fix in place. +#![cfg(test)] + +extern crate std; + +use super::*; +use parashield_oracle_verifier::{OracleVerifier, OracleVerifierClient}; +use parashield_policy_engine::{ + CreateProductParams, PolicyEngine, PolicyEngineClient, TriggerComparison, TriggerType, +}; +use parashield_risk_pool::{RiskPool, RiskPoolClient}; +use soroban_sdk::{ + symbol_short, + testutils::{Address as _, Ledger}, + token::StellarAssetClient, +}; + +const COVERAGE: i128 = 1_000_000_000; // 100 USDC + +struct World { + env: Env, + admin: Address, + keeper: Address, + oracle_w: Address, + usdc: Address, + oracle_id: Address, + policy_id: Address, + claims_id: Address, + pool_id: Address, + /// The risk pool refuses an admin LP position (issue #568), so the pool's + /// capital has to come from a third party. + lp: Address, + /// Product created on first use. Cached because product *names* are unique + /// (issue #570), so a test that buys more than one batch cannot create a + /// second product under the same name. + product_id: Option, +} + +fn deploy() -> World { + let env = Env::default(); + env.mock_all_auths(); + env.cost_estimate().budget().reset_unlimited(); + + let admin = Address::generate(&env); + let keeper = Address::generate(&env); + let oracle_wallet = Address::generate(&env); + let lp = Address::generate(&env); + + let usdc = env + .register_stellar_asset_contract_v2(admin.clone()) + .address(); + + let oracle_id = env.register(OracleVerifier, ()); + OracleVerifierClient::new(&env, &oracle_id).initialize(&admin); + OracleVerifierClient::new(&env, &oracle_id).add_oracle( + &admin, + &oracle_wallet, + &symbol_short!("weather"), + &90u32, + ); + + let backstop = env + .register_stellar_asset_contract_v2(Address::generate(&env)) + .address(); + let treasury = Address::generate(&env); + let pool_id = env.register(RiskPool, ()); + let policy_id = env.register(PolicyEngine, ()); + let claims_id = env.register(ClaimsProcessor, ()); + + RiskPoolClient::new(&env, &pool_id).initialize( + &admin, + &usdc, + &treasury, + &backstop, + &symbol_short!("crop"), + &policy_id, + &claims_id, + ); + PolicyEngineClient::new(&env, &policy_id).initialize(&admin, &usdc, &oracle_id); + ClaimsProcessorClient::new(&env, &claims_id).initialize( + &admin, + &policy_id, + &pool_id, + &oracle_id, + &604_800u64, + ); + ClaimsProcessorClient::new(&env, &claims_id).add_keeper(&admin, &keeper); + PolicyEngineClient::new(&env, &policy_id).set_claims_processor(&admin, &claims_id); + + World { + env, + admin, + keeper, + oracle_w: oracle_wallet, + usdc, + oracle_id, + policy_id, + claims_id, + pool_id, + lp, + product_id: None, + } +} + +fn create_crop_product(w: &World) -> u128 { + PolicyEngineClient::new(&w.env, &w.policy_id).create_product( + &w.admin, + &CreateProductParams { + name: symbol_short!("uniq_cism"), + category: symbol_short!("crop"), + oracle_key: symbol_short!("kis2606"), + trigger_type: TriggerType::Threshold, + oracle_data_type: symbol_short!("weather"), + trigger_threshold: 50_000_000, + trigger_comparison: TriggerComparison::LessThan, + coverage_min: 100_000_000, + coverage_max: 10_000_000_000, + premium_rate_bps: 500, + max_duration_days: 365, + }, + ) +} + +/// The world's single product, created on first use. +fn product(w: &mut World) -> u128 { + if let Some(id) = w.product_id { + return id; + } + let id = create_crop_product(w); + w.product_id = Some(id); + id +} + +/// Buy `n` distinct policies for `buyer`, funding the pool once. The pool is +/// deliberately over-funded relative to the coverage it will have to lock, so +/// the tests exercise the claims index rather than the pool's solvency checks. +fn buy_policies(w: &mut World, buyer: &Address, n: usize) -> std::vec::Vec { + let product_id = product(w); + let pool_funding = 100_000_000_000i128; + StellarAssetClient::new(&w.env, &w.usdc).mint(buyer, &(COVERAGE * 10)); + StellarAssetClient::new(&w.env, &w.usdc).mint(&w.lp, &pool_funding); + RiskPoolClient::new(&w.env, &w.pool_id).deposit(&w.lp, &pool_funding, &0i128, &false); + StellarAssetClient::new(&w.env, &w.usdc).mint(&w.policy_id, &100_000_000_000i128); + + let mut out = std::vec::Vec::new(); + for i in 0..n { + let oracle_key = match i { + 0 => symbol_short!("kis2606"), + 1 => symbol_short!("kis2607"), + 2 => symbol_short!("kis2608"), + _ => symbol_short!("kis2609"), + }; + let policy_id = PolicyEngineClient::new(&w.env, &w.policy_id).buy_policy( + buyer, + &product_id, + &COVERAGE, + &30u32, + &oracle_key, + ); + RiskPoolClient::new(&w.env, &w.pool_id).lock_for_policy(&w.admin, &policy_id, &COVERAGE); + out.push(policy_id); + } + out +} + +fn submit_rainfall(w: &World, key: soroban_sdk::Symbol, mm_7dec: i128) { + OracleVerifierClient::new(&w.env, &w.oracle_id).submit_data( + &w.oracle_w, + &symbol_short!("weather"), + &key, + &mm_7dec, + &95u32, + &w.env.ledger().timestamp(), + ); +} + +/// The contract error a `try_*` client call raised, or `None` if it did not +/// fail with one. +/// +/// Returning `Option` rather than unwrapping keeps the failure path in the +/// caller's `assert_eq!`, which reports the actual error code on mismatch and +/// needs no `panic!` of its own. +fn contract_error( + res: Result, Result>, +) -> Option { + res.err().and_then(|inner| inner.ok()) +} + +/// `Error::AlreadyClaimed` — the code the one-claim-per-policy guard raises. +fn already_claimed() -> soroban_sdk::Error { + soroban_sdk::Error::from_contract_error(6) +} + +// ── The one-claim-per-policy index is the single source of truth ─────────── + +/// A second `submit_claim` on the same policy is refused, and the index still +/// points at the first claim. +#[test] +fn second_submit_claim_on_same_policy_is_refused() { + let mut w = deploy(); + let buyer = Address::generate(&w.env); + let policies = buy_policies(&mut w, &buyer, 1); + let policy_id = policies[0]; + + let cp = ClaimsProcessorClient::new(&w.env, &w.claims_id); + let first = cp.submit_claim(&buyer, &policy_id); + assert_eq!( + contract_error(cp.try_submit_claim(&buyer, &policy_id)), + Some(already_claimed()), + "a duplicate claim must be refused with AlreadyClaimed" + ); + assert_eq!(cp.get_claim_id_for_policy(&policy_id), Some(first)); +} + +/// The rejection must be total: no second claim record, no extra queue entry, +/// and no claim id burned by the failed attempt. +#[test] +fn refused_duplicate_leaves_no_trace() { + let mut w = deploy(); + let buyer = Address::generate(&w.env); + let policies = buy_policies(&mut w, &buyer, 1); + let policy_id = policies[0]; + + let cp = ClaimsProcessorClient::new(&w.env, &w.claims_id); + let first = cp.submit_claim(&buyer, &policy_id); + let pending_after_first = cp.get_pending_claims().len(); + + assert!(cp.try_submit_claim(&buyer, &policy_id).is_err()); + + assert_eq!( + cp.get_pending_claims().len(), + pending_after_first, + "the rejected duplicate must not enqueue anything" + ); + assert_eq!(cp.get_claim_id_for_policy(&policy_id), Some(first)); + + // The id counter is untouched, so the next genuine claim on another policy + // still gets the very next id. + let other = buy_policies(&mut w, &buyer, 2)[1]; + let second = cp.submit_claim(&buyer, &other); + assert_eq!( + second, + first + 1, + "a refused duplicate must not consume an id" + ); +} + +// ── Every creation path shares the guard ─────────────────────────────────── + +/// `batch_submit_claims` goes through the same guarded body, so two entries +/// for the same policy inside one batch cannot both be admitted. The batch is +/// a single transaction, so the whole call reverts — the first claim is not +/// left dangling either. +#[test] +fn batch_with_repeated_policy_id_reverts() { + let mut w = deploy(); + let buyer = Address::generate(&w.env); + let policy_id = buy_policies(&mut w, &buyer, 1)[0]; + + let cp = ClaimsProcessorClient::new(&w.env, &w.claims_id); + let dupes = soroban_sdk::vec![&w.env, policy_id, policy_id]; + assert_eq!( + contract_error(cp.try_batch_submit_claims(&buyer, &dupes)), + Some(already_claimed()), + "a batch must not admit the same policy twice" + ); + assert_eq!( + cp.get_claim_id_for_policy(&policy_id), + None, + "the reverted batch must not leave a claim behind" + ); + assert_eq!(cp.get_pending_claims().len(), 0); +} + +/// A batch is still allowed to cover several *distinct* policies, and each one +/// gets its own claim. +#[test] +fn batch_over_distinct_policies_admits_one_claim_each() { + let mut w = deploy(); + let buyer = Address::generate(&w.env); + let policies = buy_policies(&mut w, &buyer, 2); + + let cp = ClaimsProcessorClient::new(&w.env, &w.claims_id); + let ids = soroban_sdk::vec![&w.env, policies[0], policies[1]]; + let claims = cp.batch_submit_claims(&buyer, &ids); + + assert_eq!(claims.len(), 2); + assert_eq!( + cp.get_claim_id_for_policy(&policies[0]), + Some(claims.get_unchecked(0)) + ); + assert_eq!( + cp.get_claim_id_for_policy(&policies[1]), + Some(claims.get_unchecked(1)) + ); +} + +/// A claim filed through `batch_submit_claims` blocks a later single +/// submission on the same policy. +#[test] +fn batch_claim_blocks_later_single_submission() { + let mut w = deploy(); + let buyer = Address::generate(&w.env); + let policy_id = buy_policies(&mut w, &buyer, 1)[0]; + + let cp = ClaimsProcessorClient::new(&w.env, &w.claims_id); + let ids = soroban_sdk::vec![&w.env, policy_id]; + let claims = cp.batch_submit_claims(&buyer, &ids); + + assert_eq!( + contract_error(cp.try_submit_claim(&buyer, &policy_id)), + Some(already_claimed()) + ); + assert_eq!( + cp.get_claim_id_for_policy(&policy_id), + Some(claims.get_unchecked(0)) + ); +} + +/// The critical cross-path case: the keeper-driven `auto_process` path creates +/// a claim too, so a claimant cannot slip a second record in behind it. This +/// is the ordering that actually matters in production — parametric policies +/// are normally settled by `auto_process` first, and the claimant's own +/// `submit_claim` arriving afterwards must be refused, not treated as a new +/// claim. +#[test] +fn submit_claim_after_auto_process_is_refused() { + let mut w = deploy(); + let buyer = Address::generate(&w.env); + let policy_id = buy_policies(&mut w, &buyer, 1)[0]; + // Trigger met → auto_process settles the policy. + submit_rainfall(&w, symbol_short!("kis2606"), 20_000_000); + + let cp = ClaimsProcessorClient::new(&w.env, &w.claims_id); + cp.auto_process(&w.keeper, &policy_id, &None); + + assert_eq!( + contract_error(cp.try_submit_claim(&buyer, &policy_id)), + Some(already_claimed()), + "auto_process already owns this policy's single claim" + ); +} + +/// `auto_process` settles the *existing* claim instead of minting a second +/// record for the same policy, so the `PolicyClaim` index keeps pointing at +/// the original id and no additional claim id is ever consumed. +#[test] +fn auto_process_settles_the_existing_claim_in_place() { + let mut w = deploy(); + let buyer = Address::generate(&w.env); + let policy_id = buy_policies(&mut w, &buyer, 1)[0]; + // Trigger not met, so the claim is rejected rather than paid and the test + // does not depend on the pool's payout path succeeding. + submit_rainfall(&w, symbol_short!("kis2606"), 90_000_000); + + let cp = ClaimsProcessorClient::new(&w.env, &w.claims_id); + let filed = cp.submit_claim(&buyer, &policy_id); + assert_eq!(cp.get_claim_id_for_policy(&policy_id), Some(filed)); + assert_eq!( + cp.get_pending_claims().len(), + 1, + "the filed claim must be queued exactly once" + ); + + assert_eq!( + cp.auto_process(&w.keeper, &policy_id, &None), + ClaimResult::Rejected + ); + + assert_eq!( + cp.get_claim_id_for_policy(&policy_id), + Some(filed), + "auto_process must settle the existing claim, not create another" + ); + assert_eq!(cp.get_claim(&filed).status, ClaimStatus::Rejected); + assert_eq!( + cp.get_pending_claims().len(), + 0, + "the settled claim must leave the queue" + ); + + // A fresh claim on another policy gets the very next id, proving the + // auto_process call consumed none. + let other = buy_policies(&mut w, &buyer, 2)[1]; + assert_eq!(cp.submit_claim(&buyer, &other), filed + 1); +} + +/// A settled policy keeps its single-claim reservation: the payout path must +/// not open the door to a follow-up claim once the money has moved. +#[test] +fn settled_policy_cannot_be_claimed_again() { + let mut w = deploy(); + let buyer = Address::generate(&w.env); + let policy_id = buy_policies(&mut w, &buyer, 1)[0]; + // Trigger not met → the claim is rejected rather than paid, so the test + // does not depend on pool settlement succeeding. + submit_rainfall(&w, symbol_short!("kis2606"), 90_000_000); + + let cp = ClaimsProcessorClient::new(&w.env, &w.claims_id); + let claim_id = cp.submit_claim(&buyer, &policy_id); + cp.process_claim(&w.keeper, &claim_id, &None); + assert_eq!(cp.get_claim(&claim_id).status, ClaimStatus::Rejected); + + assert_eq!( + contract_error(cp.try_submit_claim(&buyer, &policy_id)), + Some(already_claimed()) + ); + assert_eq!(cp.get_claim_id_for_policy(&policy_id), Some(claim_id)); +} + +/// Claim ids and the `PolicyClaim` index stay in lockstep across a mix of +/// paths: every admitted claim is reachable from its policy, and the pending +/// queue holds no duplicates. +#[test] +fn index_stays_consistent_across_mixed_paths() { + let mut w = deploy(); + let buyer = Address::generate(&w.env); + let policies = buy_policies(&mut w, &buyer, 3); + + let cp = ClaimsProcessorClient::new(&w.env, &w.claims_id); + let via_batch = cp.batch_submit_claims(&buyer, &soroban_sdk::vec![&w.env, policies[0]]); + let single = cp.submit_claim(&buyer, &policies[1]); + let other = cp.submit_claim(&buyer, &policies[2]); + + let expected: [u128; 3] = [via_batch.get_unchecked(0), single, other]; + for (i, policy_id) in policies.iter().enumerate() { + let claim_id = expected[i]; + assert_eq!(cp.get_claim_id_for_policy(policy_id), Some(claim_id)); + assert_eq!(cp.get_claim(&claim_id).policy_id, *policy_id); + assert_eq!(cp.get_claim(&claim_id).claimant, buyer); + } + + let pending = cp.get_pending_claims(); + assert_eq!(pending.len(), 3); + for claim_id in expected { + let occurrences = (0..pending.len()) + .filter(|i| pending.get_unchecked(*i) == claim_id) + .count(); + assert_eq!(occurrences, 1, "each claim must be queued exactly once"); + } +} + +/// Time passing does not release the reservation: the guard is on the index, +/// not on a timer, so a stale-but-present claim still blocks a new one. +#[test] +fn reservation_survives_ledger_time_advances() { + let mut w = deploy(); + let buyer = Address::generate(&w.env); + let policy_id = buy_policies(&mut w, &buyer, 1)[0]; + + let cp = ClaimsProcessorClient::new(&w.env, &w.claims_id); + let first = cp.submit_claim(&buyer, &policy_id); + + w.env.ledger().with_mut(|l| l.timestamp += 86_400); + + assert_eq!( + contract_error(cp.try_submit_claim(&buyer, &policy_id)), + Some(already_claimed()) + ); + assert_eq!(cp.get_claim_id_for_policy(&policy_id), Some(first)); +} diff --git a/contracts/claims-processor/src/test_keeper_registry.rs b/contracts/claims-processor/src/test_keeper_registry.rs new file mode 100644 index 0000000..e97413f --- /dev/null +++ b/contracts/claims-processor/src/test_keeper_registry.rs @@ -0,0 +1,246 @@ +//! Keeper registry — address-format validation on `add_keeper` (issue #572). +//! +//! `add_keeper` is the only way to mint settlement authority, so it now +//! applies the same `validate_stellar_address` check that `initialize` +//! applies to the four addresses it wires up. The format rule itself lives in +//! the pure helper `is_well_formed_strkey`, which is what these tests drive +//! directly: a real `Address` is always canonically encoded by the host, so +//! the string-level rule is only reachable — and therefore only regression- +//! testable — as a pure function. +#![cfg(test)] + +extern crate alloc; + +use super::*; +use alloc::vec::Vec; +use soroban_sdk::testutils::Address as _; + +/// The contract error a `try_*` client call raised, or `None` if it did not +/// fail with one. +/// +/// Returning `Option` rather than unwrapping keeps the failure path in the +/// caller's `assert_eq!`, which reports the actual error code on mismatch and +/// needs no `panic!` of its own. +fn contract_error( + res: Result, Result>, +) -> Option { + res.err().and_then(|inner| inner.ok()) +} + +/// A 56-byte `StrKey` whose body cycles through the full base32 alphabet +/// (`A`–`Z`, `2`–`7`) under a leading `G`, so the alphabet boundary itself is +/// exercised rather than just the happy path. +fn full_alphabet_strkey() -> Vec { + let mut alphabet = Vec::new(); + for b in b'A'..=b'Z' { + alphabet.push(b); + } + for b in b'2'..=b'7' { + alphabet.push(b); + } + assert_eq!(alphabet.len(), 32); + assert!( + STRKEY_LEN - 1 > alphabet.len(), + "body must wrap to cover the alphabet twice" + ); + + let mut bytes = Vec::new(); + bytes.push(b'G'); + for i in 0..(STRKEY_LEN - 1) { + bytes.push(alphabet[i % alphabet.len()]); + } + assert_eq!(bytes.len(), STRKEY_LEN); + bytes +} + +/// Copy an `Address`'s canonical encoding into a fixed-size buffer. +fn strkey_bytes(address: &Address) -> [u8; STRKEY_LEN] { + let mut buf = [0u8; STRKEY_LEN]; + address.to_string().copy_into_slice(&mut buf); + buf +} + +// ── Format rule ───────────────────────────────────────────────────────────── + +#[test] +fn strkey_accepts_generated_account_address() { + let env = Env::default(); + let addr = Address::generate(&env); + assert!( + is_well_formed_strkey(&strkey_bytes(&addr)), + "a generated account address must pass format validation" + ); +} + +#[test] +fn strkey_accepts_generated_contract_address() { + let env = Env::default(); + let contract_id = env.register_stellar_asset_contract_v2(Address::generate(&env)); + assert!( + is_well_formed_strkey(&strkey_bytes(&contract_id.address())), + "a registered contract address must pass format validation" + ); +} + +#[test] +fn strkey_accepts_full_base32_alphabet() { + assert!(is_well_formed_strkey(&full_alphabet_strkey())); +} + +#[test] +fn strkey_accepts_contract_version_prefix() { + let mut bytes = full_alphabet_strkey(); + bytes[0] = b'C'; + assert!(is_well_formed_strkey(&bytes)); +} + +#[test] +fn strkey_rejects_wrong_length() { + let bytes = full_alphabet_strkey(); + assert!(!is_well_formed_strkey(&bytes[..STRKEY_LEN - 1]), "55 bytes"); + assert!(!is_well_formed_strkey(&[]), "empty"); + let mut long = bytes.clone(); + long.push(b'A'); + assert!(!is_well_formed_strkey(&long), "57 bytes"); +} + +#[test] +fn strkey_rejects_unknown_version_prefix() { + // Muxed accounts (M…), secret seeds (S…), and bare base32 payloads are all + // invalid here: a keeper is registered by an account or contract id, + // nothing else. + for &prefix in b"MSA02gc" { + let mut bytes = full_alphabet_strkey(); + bytes[0] = prefix; + assert!( + !is_well_formed_strkey(&bytes), + "prefix {:?} must be rejected", + prefix as char + ); + } +} + +#[test] +fn strkey_rejects_non_base32_body() { + // Base32 is `A`–`Z` + `2`–`7`; `0`, `1`, `8`, `9`, `-` and `=` (RFC 4648 + // padding) are not. The old length-and-prefix check accepted all of these. + for &bad in b"0189-=! " { + let mut bytes = full_alphabet_strkey(); + bytes[10] = bad; + assert!( + !is_well_formed_strkey(&bytes), + "body byte {:?} must be rejected", + bad as char + ); + } +} + +#[test] +fn strkey_rejects_non_base32_in_the_final_position() { + let mut bytes = full_alphabet_strkey(); + let last = bytes.len() - 1; + bytes[last] = b'8'; + assert!(!is_well_formed_strkey(&bytes)); +} + +// ── Wiring: the contract entry points that consume the rule ───────────────── + +struct Deployed { + env: Env, + admin: Address, + claims_id: Address, +} + +fn deploy() -> Deployed { + let env = Env::default(); + env.mock_all_auths(); + + let admin = Address::generate(&env); + // Contract addresses for the three cross-contract links — the same `C…` + // shape `initialize` requires for a deployed protocol. + let policy_engine = env.register_stellar_asset_contract_v2(Address::generate(&env)); + let risk_pool = env.register_stellar_asset_contract_v2(Address::generate(&env)); + let oracle_verifier = env.register_stellar_asset_contract_v2(Address::generate(&env)); + let claims_id = env.register(ClaimsProcessor, ()); + + ClaimsProcessorClient::new(&env, &claims_id).initialize( + &admin, + &policy_engine.address(), + &risk_pool.address(), + &oracle_verifier.address(), + &604_800u64, + ); + + Deployed { + env, + admin, + claims_id, + } +} + +#[test] +fn initialize_accepts_account_and_contract_shaped_addresses() { + let d = deploy(); + // initialize did not panic, so the admin's `G…` address and the three + // `C…` links all cleared the format check. Assert the round-trip so a + // future tightening that rejects real addresses fails here rather than in + // production. + let client = ClaimsProcessorClient::new(&d.env, &d.claims_id); + assert_eq!(client.get_admin(), d.admin); + assert!(!client.is_paused()); +} + +#[test] +fn add_keeper_accepts_account_and_contract_addresses() { + let d = deploy(); + let account_keeper = Address::generate(&d.env); + let contract_keeper = d + .env + .register_stellar_asset_contract_v2(Address::generate(&d.env)) + .address(); + + let client = ClaimsProcessorClient::new(&d.env, &d.claims_id); + client.add_keeper(&d.admin, &account_keeper); + client.add_keeper(&d.admin, &contract_keeper); + + assert!(client.is_keeper(&account_keeper)); + assert!(client.is_keeper(&contract_keeper)); +} + +#[test] +fn add_keeper_rejects_non_admin() { + let d = deploy(); + let stranger = Address::generate(&d.env); + let keeper = Address::generate(&d.env); + let client = ClaimsProcessorClient::new(&d.env, &d.claims_id); + + let res = client.try_add_keeper(&stranger, &keeper); + assert_eq!( + contract_error(res), + Some(soroban_sdk::Error::from_contract_error(3)), + "a non-admin must be rejected as Unauthorized, never admitted as a keeper" + ); + assert!( + !client.is_keeper(&keeper), + "a rejected call must not write state" + ); +} + +#[test] +fn remove_keeper_revokes_settlement_authority() { + let d = deploy(); + let keeper = Address::generate(&d.env); + let client = ClaimsProcessorClient::new(&d.env, &d.claims_id); + + client.add_keeper(&d.admin, &keeper); + assert!(client.is_keeper(&keeper)); + + client.remove_keeper(&d.admin, &keeper); + assert!(!client.is_keeper(&keeper)); + + // Removing an address that was never registered is a no-op, not an error — + // which is what keeps the registry safely repairable and is why + // `remove_keeper` deliberately does not re-run the format check that + // `add_keeper` applies. + client.remove_keeper(&d.admin, &keeper); +} diff --git a/contracts/oracle-verifier/src/lib.rs b/contracts/oracle-verifier/src/lib.rs index 11dbdd0..6f8860e 100644 --- a/contracts/oracle-verifier/src/lib.rs +++ b/contracts/oracle-verifier/src/lib.rs @@ -3412,3 +3412,5 @@ mod test; mod test_advanced; #[cfg(test)] mod test_offline; +#[cfg(test)] +mod test_confidence_bounds; diff --git a/contracts/oracle-verifier/src/test_confidence_bounds.rs b/contracts/oracle-verifier/src/test_confidence_bounds.rs new file mode 100644 index 0000000..057dc13 --- /dev/null +++ b/contracts/oracle-verifier/src/test_confidence_bounds.rs @@ -0,0 +1,359 @@ +//! `confidence_score` must stay inside 0-100 on every submission path +//! (issue #571). +//! +//! `submit_data` already carried the bounds check and had its 0 / 101 cases +//! pinned. What was *not* pinned is the rest of the surface: there are four +//! ways to get an `OracleDataPoint` into storage, and a bounds check that only +//! exists on one of them is not a bounds check. An out-of-range score that +//! reaches storage is not cosmetic — `get_aggregated` and +//! `reputation_weighted_confidence` weight the median by it, so a single +//! `u32::MAX` reading can dominate an aggregation that is supposed to be a +//! consensus of 0-100 reliability ratings. +//! +//! Note the range is `[1, 100]`, not `[0, 100]`: a zero-confidence reading +//! carries no reliability information at all and would otherwise satisfy every +//! `min_confidence` filter by accident. +#![cfg(test)] + +use super::*; +use soroban_sdk::{ + symbol_short, + testutils::{Address as _, Ledger}, + Address, Bytes, BytesN, Env, Symbol, Vec, +}; + +/// A fixed observation timestamp, comfortably inside the freshness window +/// relative to the ledger time the harness installs. +const OBSERVED_AT: u64 = 1_748_736_000; + +/// Values that must never reach storage, from "just out of range" to "as large +/// as the type allows". +const OUT_OF_RANGE: [u32; 5] = [0, 101, 200, 1_000, u32::MAX]; + +/// The two ends of the accepted range. +const IN_RANGE_BOUNDARIES: [u32; 2] = [1, 100]; + +fn setup() -> (Env, Address, Address, Address) { + let env = Env::default(); + env.ledger().set_timestamp(OBSERVED_AT); + env.mock_all_auths(); + + let admin = Address::generate(&env); + let contract_id = env.register(OracleVerifier, ()); + OracleVerifierClient::new(&env, &contract_id).initialize(&admin); + + let oracle = Address::generate(&env); + OracleVerifierClient::new(&env, &contract_id).add_oracle( + &admin, + &oracle, + &symbol_short!("weather"), + &90u32, + ); + + (env, admin, contract_id, oracle) +} + +fn data_type() -> Symbol { + symbol_short!("weather") +} + +fn key() -> Symbol { + symbol_short!("kis2606") +} + +fn other_key() -> Symbol { + symbol_short!("kis2607") +} + +fn ciphertext(env: &Env) -> Bytes { + Bytes::from_slice(env, b"ciphertext-blob") +} + +fn nonce(env: &Env) -> BytesN<12> { + BytesN::from_array(env, &[3u8; 12]) +} + +/// `Error::InvalidConfidence` — the code every bounds violation must raise. +fn invalid_confidence() -> soroban_sdk::Error { + soroban_sdk::Error::from_contract_error(7) +} + +/// The contract error a `try_*` client call raised, or `None` if it did not +/// fail with one. +/// +/// Returning `Option` rather than unwrapping keeps the failure path in the +/// caller's `assert_eq!`, which reports the actual error code on mismatch and +/// needs no `panic!` of its own. +fn contract_error( + res: Result, Result>, +) -> Option { + res.err().and_then(|inner| inner.ok()) +} + +// ── submit_data ───────────────────────────────────────────────────────────── + +#[test] +fn submit_data_accepts_both_ends_of_the_range() { + for confidence in IN_RANGE_BOUNDARIES { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + client.submit_data( + &oracle, + &data_type(), + &key(), + &32_000_000i128, + &confidence, + &OBSERVED_AT, + ); + assert_eq!( + client.get_data(&data_type(), &key()).confidence, + confidence, + "confidence {} must be accepted verbatim", + confidence + ); + } +} + +#[test] +fn submit_data_rejects_every_out_of_range_score() { + for confidence in OUT_OF_RANGE { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + assert_eq!( + contract_error(client.try_submit_data( + &oracle, + &data_type(), + &key(), + &32_000_000i128, + &confidence, + &OBSERVED_AT, + )), + Some(invalid_confidence()), + "confidence {} must be rejected", + confidence + ); + } +} + +#[test] +fn rejected_submit_data_leaves_no_reading_behind() { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + + assert!(client + .try_submit_data( + &oracle, + &data_type(), + &key(), + &32_000_000i128, + &500, + &OBSERVED_AT + ) + .is_err()); + + assert!( + client.try_get_data(&data_type(), &key()).is_err(), + "an out-of-range submission must not have been persisted" + ); +} + +// ── submit_encrypted_data ─────────────────────────────────────────────────── + +#[test] +fn submit_encrypted_data_accepts_both_ends_of_the_range() { + for confidence in IN_RANGE_BOUNDARIES { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + client.submit_encrypted_data( + &oracle, + &data_type(), + &key(), + &ciphertext(&env), + &nonce(&env), + &confidence, + &OBSERVED_AT, + ); + let points = client.get_encrypted_data(&data_type(), &key()); + assert_eq!(points.len(), 1); + assert_eq!(points.get_unchecked(0).confidence, confidence); + } +} + +#[test] +fn submit_encrypted_data_rejects_every_out_of_range_score() { + for confidence in OUT_OF_RANGE { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + assert_eq!( + contract_error(client.try_submit_encrypted_data( + &oracle, + &data_type(), + &key(), + &ciphertext(&env), + &nonce(&env), + &confidence, + &OBSERVED_AT, + )), + Some(invalid_confidence()), + "confidence {} must be rejected", + confidence + ); + } +} + +// ── batch_submit_data ─────────────────────────────────────────────────────── + +#[test] +fn batch_submit_data_accepts_both_ends_of_the_range() { + for confidence in IN_RANGE_BOUNDARIES { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + let mut subs: Vec<(Symbol, i128, u32, u64)> = Vec::new(&env); + subs.push_back((key(), 30_000_000i128, confidence, OBSERVED_AT)); + client.batch_submit_data(&oracle, &data_type(), &subs); + assert_eq!(client.get_data(&data_type(), &key()).confidence, confidence); + } +} + +#[test] +fn batch_submit_data_rejects_every_out_of_range_score() { + for confidence in OUT_OF_RANGE { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + let mut subs: Vec<(Symbol, i128, u32, u64)> = Vec::new(&env); + subs.push_back((key(), 30_000_000i128, confidence, OBSERVED_AT)); + assert_eq!( + contract_error(client.try_batch_submit_data(&oracle, &data_type(), &subs)), + Some(invalid_confidence()), + "confidence {} must be rejected", + confidence + ); + } +} + +/// The batch is one transaction, so a single bad reading must take the whole +/// call down — otherwise a rejected reading could still be half-written while +/// its neighbours persisted. +#[test] +fn batch_submit_data_rejects_the_whole_call_on_one_bad_reading() { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + + let mut subs: Vec<(Symbol, i128, u32, u64)> = Vec::new(&env); + subs.push_back((key(), 30_000_000i128, 90u32, OBSERVED_AT)); + subs.push_back((other_key(), 31_000_000i128, u32::MAX, OBSERVED_AT)); + + assert_eq!( + contract_error(client.try_batch_submit_data(&oracle, &data_type(), &subs)), + Some(invalid_confidence()) + ); + assert!( + client.try_get_data(&data_type(), &key()).is_err(), + "the valid sibling reading must not survive the revert" + ); +} + +// ── submit_data_batch ─────────────────────────────────────────────────────── + +fn submissions(env: &Env, confidence: u32) -> Vec { + Vec::from_array( + env, + [OracleDataSubmission { + key: key(), + value: 30_000_000i128, + confidence, + timestamp: OBSERVED_AT, + }], + ) +} + +#[test] +fn submit_data_batch_accepts_both_ends_of_the_range() { + for confidence in IN_RANGE_BOUNDARIES { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + client.submit_data_batch(&oracle, &data_type(), &submissions(&env, confidence)); + assert_eq!(client.get_data(&data_type(), &key()).confidence, confidence); + } +} + +#[test] +fn submit_data_batch_rejects_every_out_of_range_score() { + for confidence in OUT_OF_RANGE { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + let subs = submissions(&env, confidence); + assert_eq!( + contract_error(client.try_submit_data_batch(&oracle, &data_type(), &subs)), + Some(invalid_confidence()), + "confidence {} must be rejected", + confidence + ); + } +} + +#[test] +fn submit_data_batch_rejects_the_whole_call_on_one_bad_reading() { + let (env, _admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + + let subs = Vec::from_array( + &env, + [ + OracleDataSubmission { + key: key(), + value: 30_000_000i128, + confidence: 90u32, + timestamp: OBSERVED_AT, + }, + OracleDataSubmission { + key: other_key(), + value: 31_000_000i128, + confidence: 0u32, + timestamp: OBSERVED_AT, + }, + ], + ); + assert_eq!( + contract_error(client.try_submit_data_batch(&oracle, &data_type(), &subs)), + Some(invalid_confidence()) + ); + assert!(client.try_get_data(&data_type(), &key()).is_err()); +} + +// ── A bound on the aggregate itself ──────────────────────────────────────── + +/// The bounds check is what keeps the weighted average inside 0-100. With +/// every accepted score already in range, the aggregate cannot escape it, and +/// an out-of-range score cannot reach the aggregation in the first place — +/// which is the property the issue is really about. +#[test] +fn aggregated_confidence_stays_within_zero_to_one_hundred() { + let (env, admin, contract_id, oracle) = setup(); + let client = OracleVerifierClient::new(&env, &contract_id); + let second = Address::generate(&env); + client.add_oracle(&admin, &second, &data_type(), &100u32); + + for (addr, confidence) in [(&oracle, 100u32), (&second, 1u32)] { + client.submit_data( + addr, + &data_type(), + &key(), + &30_000_000i128, + &confidence, + &OBSERVED_AT, + ); + } + + let agg = client.get_aggregated(&data_type(), &key()); + assert!( + (1..=100).contains(&agg.confidence), + "aggregated confidence {} escaped 0-100", + agg.confidence + ); + assert!( + (1..=100).contains(&agg.min_confidence), + "minimum confidence {} escaped 0-100", + agg.min_confidence + ); +} diff --git a/contracts/policy-engine/src/lib.rs b/contracts/policy-engine/src/lib.rs index b78b2da..6f5ed0d 100644 --- a/contracts/policy-engine/src/lib.rs +++ b/contracts/policy-engine/src/lib.rs @@ -489,6 +489,18 @@ impl PolicyEngine { /// Admin-only: permanently retire a product. It is removed from the active list and its /// `(category, oracle_key)` slot is freed so another product may reuse it. + /// + /// The name index is released too (issue #570). Freeing the name matters + /// as much as freeing the oracle key: `create_product` rejects a name that + /// is still mapped, so leaving the uniqueness index behind would outlive + /// the product it describes and the admin could never re-launch the same + /// product line under the same name. It would also silently reserve a + /// name that no live product answers to, which is the behaviour users see + /// as "that name is taken" for a product that no longer exists. + /// + /// The `(category, oracle_key)` slot and the pool's product count keep + /// their existing decrement behaviour, so retiring a product never leaves + /// the pool's accounting or a live product's key untouched. pub fn deprecate_product(env: Env, admin: Address, product_id: u128) { Self::require_admin(&env, &admin); let mut product: InsuranceProduct = Self::load_product(&env, product_id); @@ -529,6 +541,15 @@ impl PolicyEngine { .extend_ttl(&pool_count_key, TTL_THRESHOLD, TTL_EXTEND_TO); } + // Release the name so a successor product can take it (issue #570). + // Only remove the index entry when it still points at *this* product: + // a stale mapping left by an earlier product must not be dropped just + // because a different product happened to share the name. + let name_key = StorageKey::ProductName(product.name.clone()); + if env.storage().persistent().get::<_, u128>(&name_key) == Some(product_id) { + env.storage().persistent().remove(&name_key); + } + let key = (product.category, product.oracle_key); env.storage() .persistent() diff --git a/contracts/policy-engine/src/test_product_names.rs b/contracts/policy-engine/src/test_product_names.rs index 19ee90e..965e184 100644 --- a/contracts/policy-engine/src/test_product_names.rs +++ b/contracts/policy-engine/src/test_product_names.rs @@ -1,4 +1,11 @@ -//! Product name uniqueness (issue #514). +//! Product name uniqueness (issue #514) and the lifetime of the name index +//! (issue #570). +//! +//! `create_product` refuses a name that is already mapped to a live product. +//! For that index to mean "the name is taken by a product users can actually +//! buy" rather than "the name was once typed into this contract", retiring a +//! product has to release its name — the same way it already releases its +//! `(category, oracle_key)` slot. #![cfg(test)] use super::*; @@ -17,6 +24,31 @@ fn setup() -> (Env, Address, PolicyEngineClient<'static>) { (env, admin, client) } +/// Unwrap a `try_*` client call that is expected to succeed. +/// +/// The error types are spelled out rather than left generic because `expect` +/// needs them to be `Debug`, and because every generated client method returns +/// exactly these two. +fn expect_ok( + res: Result< + Result, + Result, + >, +) -> T { + res.expect("expected the call to succeed") + .expect("expected the call to succeed without raising a contract error") +} + +/// Assert a `try_*` client call fails, without pinning which error it raises. +fn expect_rejected( + res: Result< + Result, + Result, + >, +) { + assert!(res.is_err(), "expected the call to be rejected"); +} + fn params(name: Symbol, category: Symbol, oracle_key: Symbol) -> CreateProductParams { CreateProductParams { name, @@ -67,3 +99,123 @@ fn rejected_duplicate_does_not_consume_a_slot_or_id() { let next = client.create_product(&admin, ¶ms(symbol_short!("hail"), symbol_short!("crop"), symbol_short!("key_b"))); assert_eq!(next, first + 1); } + +// ── Name index lifetime (issue #570) ─────────────────────────────────────── + +/// Retiring a product frees its name, so the successor can be launched under +/// it. Without the release, `create_product`'s uniqueness check rejects the +/// name forever and the product line can never be re-launched. +#[test] +fn deprecating_a_product_frees_its_name() { + let (_, admin, client) = setup(); + let original = client.create_product( + &admin, + ¶ms(symbol_short!("rain"), symbol_short!("crop"), symbol_short!("key_a")), + ); + client.deprecate_product(&admin, &original); + + let successor = expect_ok(client.try_create_product( + &admin, + ¶ms(symbol_short!("rain"), symbol_short!("crop"), symbol_short!("key_a")), + )); + assert_ne!(successor, original, "a successor must be a new product id"); +} + +/// The name *and* the `(category, oracle_key)` slot are both released, so a +/// successor can reuse the whole identity of the retired product rather than +/// just half of it. +#[test] +fn deprecating_a_product_frees_name_and_oracle_key() { + let (_, admin, client) = setup(); + let original = client.create_product( + &admin, + ¶ms(symbol_short!("rain"), symbol_short!("crop"), symbol_short!("key_a")), + ); + client.deprecate_product(&admin, &original); + + // Same name *and* same (category, oracle_key): the slot is free again. + let successor = expect_ok(client.try_create_product( + &admin, + ¶ms(symbol_short!("rain"), symbol_short!("crop"), symbol_short!("key_a")), + )); + assert_ne!(successor, original); +} + +/// Releasing a name must not touch a *different* product's name mapping. The +/// removal is keyed on the product id, so deprecating one product can only ever +/// clear the entry that still points at that product. +#[test] +fn deprecating_one_product_does_not_clear_another_products_name() { + let (_, admin, client) = setup(); + let rain = client.create_product( + &admin, + ¶ms(symbol_short!("rain"), symbol_short!("crop"), symbol_short!("key_a")), + ); + let hail = client.create_product( + &admin, + ¶ms(symbol_short!("hail"), symbol_short!("crop"), symbol_short!("key_b")), + ); + + client.deprecate_product(&admin, &rain); + + // `hail` is untouched and still holds its name, so the name is refused. + expect_rejected(client.try_create_product( + &admin, + ¶ms(symbol_short!("hail"), symbol_short!("crop"), symbol_short!("key_c")), + )); + // And the live product is still addressable under its own name. + let replacement = expect_ok(client.try_create_product( + &admin, + ¶ms(symbol_short!("drought"), symbol_short!("crop"), symbol_short!("key_c")), + )); + assert!(replacement > hail); +} + +/// A name is released exactly once: the successor takes it over, and the +/// uniqueness invariant is intact again from that point — a third product +/// under the same name is refused. +#[test] +fn name_is_reusable_exactly_once_after_deprecation() { + let (_, admin, client) = setup(); + let original = client.create_product( + &admin, + ¶ms(symbol_short!("rain"), symbol_short!("crop"), symbol_short!("key_a")), + ); + client.deprecate_product(&admin, &original); + + let successor = expect_ok(client.try_create_product( + &admin, + ¶ms(symbol_short!("rain"), symbol_short!("crop"), symbol_short!("key_a")), + )); + + // The name now belongs to the successor, so a third claim on it fails and + // the successor keeps it. + expect_rejected(client.try_create_product( + &admin, + ¶ms(symbol_short!("rain"), symbol_short!("crop"), symbol_short!("key_z")), + )); + let live = client.get_product(&successor); + assert_eq!( + live.status, + ProductStatus::Active, + "the successor must be the live owner of the re-registered name" + ); +} + +/// Pausing is a reversible operational state, not a retirement, so the name +/// stays reserved. This is the boundary between the two: only +/// `deprecate_product` frees a name. +#[test] +fn pausing_a_product_keeps_its_name_reserved() { + let (_, admin, client) = setup(); + let original = client.create_product( + &admin, + ¶ms(symbol_short!("rain"), symbol_short!("crop"), symbol_short!("key_a")), + ); + client.pause_product(&admin, &original); + + expect_rejected(client.try_create_product( + &admin, + ¶ms(symbol_short!("rain"), symbol_short!("crop"), symbol_short!("key_b")), + )); +}