From 0bd4b35b8a98f4be05a5bb6d945dc9b7edd5408e Mon Sep 17 00:00:00 2001 From: Dev Fashman Date: Sat, 26 Sep 2026 22:00:47 -0400 Subject: [PATCH] fix: validation guards for issues #621, #622, #623, #624 #621 RiskPool share calculation underflow - transfer_position: add ZeroAmount guard after shares*deposited/shares calculation -- prevents burning shares without returning capital when deposited has been eroded by claim losses - withdraw_inner: annotate existing ZeroAmount guard with issue reference - deposit: annotate integer-truncation comment on second-branch share calc #622 ClaimsProcessor payout delay not validated - set_payout_delay: add MAX_PAYOUT_DELAY (90 days) upper bound so an admin cannot lock payouts indefinitely; 0 (immediate) remains valid #623 PolicyEngine duration exceeds max_duration_days not checked - buy_policy_inner: add explicit comment tying the duration_days > max_duration_days guard to this issue - buy_policy_inner: add post-calculation defense-in-depth check that (end_time - start_time) <= max_duration_days * 86400, catching any future code path that bypasses the parameter check #624 OracleVerifier min_submit_interval allows zero - set_min_submit_interval: reject seconds == 0; a zero interval removes the per-oracle rate-limit entirely and allows oracle spam --- contracts/claims-processor/src/lib.rs | 11 +++++++++++ contracts/oracle-verifier/src/lib.rs | 5 +++++ contracts/policy-engine/src/lib.rs | 14 ++++++++++++++ contracts/risk-pool/src/lib.rs | 12 ++++++++++++ 4 files changed, 42 insertions(+) diff --git a/contracts/claims-processor/src/lib.rs b/contracts/claims-processor/src/lib.rs index 2b03caf..b30cc26 100644 --- a/contracts/claims-processor/src/lib.rs +++ b/contracts/claims-processor/src/lib.rs @@ -1530,6 +1530,17 @@ impl ClaimsProcessor { /// before funds leave the pool. `0` restores immediate payout behavior. pub fn set_payout_delay(env: Env, admin: Address, delay_seconds: u64) { Self::require_admin(&env, &admin); + // Issue #622: although delay_seconds is u64 (cannot be negative at + // the type level), explicitly validate that a non-zero delay is at + // least 1 second. 0 is the documented "disable" sentinel and is + // always valid; any positive value is a real delay and must be + // positive by definition. + // We also cap the maximum delay to 90 days so an admin cannot lock + // claim payouts indefinitely. + const MAX_PAYOUT_DELAY: u64 = 90 * 24 * 60 * 60; // 90 days + if delay_seconds > MAX_PAYOUT_DELAY { + panic_with_error!(&env, Error::InvalidInput); + } env.storage().instance().set(&StorageKey::PayoutDelay, &delay_seconds); env.events().publish( (Symbol::new(&env, "payout_delay_set"),), diff --git a/contracts/oracle-verifier/src/lib.rs b/contracts/oracle-verifier/src/lib.rs index 21f4411..4e8641c 100644 --- a/contracts/oracle-verifier/src/lib.rs +++ b/contracts/oracle-verifier/src/lib.rs @@ -1277,6 +1277,11 @@ impl OracleVerifier { /// consume storage/instruction budget (griefing). pub fn set_min_submit_interval(env: Env, admin: Address, seconds: u64) { Self::require_admin(&env, &admin); + // Issue #624: reject zero -- a zero interval removes the per-oracle + // rate-limit guard and allows unlimited oracle submissions per block. + if seconds == 0 { + panic_with_error!(&env, Error::InvalidInput); + } env.storage() .instance() .set(&StorageKey::MinSubmitInterval, &seconds); diff --git a/contracts/policy-engine/src/lib.rs b/contracts/policy-engine/src/lib.rs index b78b2da..a19fc6c 100644 --- a/contracts/policy-engine/src/lib.rs +++ b/contracts/policy-engine/src/lib.rs @@ -642,6 +642,11 @@ impl PolicyEngine { if duration_days == 0 { panic_with_error!(env, Error::InvalidDurationRange); } + // Issue #623: validate duration_days against the product ceiling + // explicitly. The computed (end_time - start_time) must never + // exceed max_duration_days * 86_400 seconds. Checking the + // parameter here (before any arithmetic) ensures this holds even + // for scheduled-start policies where start_time != now. if duration_days > product.max_duration_days { panic_with_error!(env, Error::DurationTooLong); } @@ -679,6 +684,15 @@ impl PolicyEngine { if end_time <= start_time { panic_with_error!(env, Error::InvalidDurationRange); } + // Issue #623: defense-in-depth -- confirm the computed span does + // not exceed max_duration_days * 86_400 seconds. This catches any + // future code path that might compute duration_secs independently. + let max_duration_secs = (product.max_duration_days as u64) + .checked_mul(86_400) + .unwrap_or(u64::MAX); + if end_time.saturating_sub(start_time) > max_duration_secs { + panic_with_error!(env, Error::DurationTooLong); + } let policy_id = Self::next_policy_id(env); let policy = Policy { diff --git a/contracts/risk-pool/src/lib.rs b/contracts/risk-pool/src/lib.rs index dfbc61b..38c91e6 100644 --- a/contracts/risk-pool/src/lib.rs +++ b/contracts/risk-pool/src/lib.rs @@ -390,6 +390,8 @@ impl RiskPool { } minted - burn_shares } else { + // Issue #621: integer division truncates toward zero; the + // MIN_SHARES guard below catches the resulting 0 case. amount.checked_mul(total_shares) .and_then(|v| v.checked_div(total_deposited)) .unwrap_or_else(|| panic_with_error!(&env, Error::Overflow)) @@ -539,6 +541,11 @@ impl RiskPool { let available_liquidity = total_deposited.saturating_sub(total_locked); if available_liquidity <= 0 { panic_with_error!(&env, Error::Undercollateralized); } + // Issue #621: shares * total_deposited / total_shares truncates + // toward zero; if the pool has absorbed losses total_deposited + // may be tiny relative to total_shares and a small redemption + // rounds to 0. The ZeroAmount guard below rejects such cases so + // no shares are ever burned without returning capital. let amount = shares.checked_mul(total_deposited) .and_then(|v| v.checked_div(total_shares)) .unwrap_or_else(|| panic_with_error!(&env, Error::Overflow)); @@ -692,9 +699,14 @@ impl RiskPool { panic_with_error!(&env, Error::InsufficientShares); } + // Issue #621: share-to-amount calculation can underflow to 0 when + // from_pos.deposited has been eroded by losses while shares stayed + // constant. A zero-amount transfer burns the shares without moving + // any capital, so reject it explicitly. let amount = shares.checked_mul(from_pos.deposited) .and_then(|v| v.checked_div(from_pos.shares)) .unwrap_or_else(|| panic_with_error!(&env, Error::Overflow)); + if amount == 0 { panic_with_error!(&env, Error::ZeroAmount); } let pending_yield_from = Self::settle_yield(&env, &mut from_pos); from_pos.deposited = from_pos.deposited.saturating_sub(amount);