diff --git a/.gitignore b/.gitignore index df572d2..eef57ba 100644 --- a/.gitignore +++ b/.gitignore @@ -8,3 +8,8 @@ test_snapshots/ issues.md generate_issues.py waves-issues.md +*.log +*.tmp +*.bak +.DS_Store +Thumbs.db diff --git a/contracts/risk_registry/src/lib.rs b/contracts/risk_registry/src/lib.rs index 8938080..1bc114e 100644 --- a/contracts/risk_registry/src/lib.rs +++ b/contracts/risk_registry/src/lib.rs @@ -34,13 +34,15 @@ pub struct RiskRegistryContract; impl RiskRegistryContract { /// One-time initialization. Sets admin and the authorized invoice_nft address. pub fn initialize(env: Env, admin: Address, invoice_nft: Address) -> Result<(), KoraError> { - if env.storage().instance().has(&DataKey::Admin) { + if env.storage().persistent().has(&DataKey::Admin) { return Err(KoraError::AlreadyInitialized); } - env.storage().instance().set(&DataKey::Admin, &admin); + env.storage().persistent().set(&DataKey::Admin, &admin); + Self::bump_persistent(&env, &DataKey::Admin); env.storage() - .instance() + .persistent() .set(&DataKey::InvoiceNft, &invoice_nft); + Self::bump_persistent(&env, &DataKey::InvoiceNft); Ok(()) } @@ -48,7 +50,8 @@ impl RiskRegistryContract { pub fn transfer_admin(env: Env, admin: Address, new_admin: Address) -> Result<(), KoraError> { admin.require_auth(); Self::require_admin(&env, &admin)?; - env.storage().instance().set(&DataKey::Admin, &new_admin); + env.storage().persistent().set(&DataKey::Admin, &new_admin); + Self::bump_persistent(&env, &DataKey::Admin); events::admin_transferred(&env, &new_admin); Ok(()) } @@ -271,7 +274,7 @@ impl RiskRegistryContract { pub fn get_admin(env: Env) -> Result { env.storage() - .instance() + .persistent() .get(&DataKey::Admin) .ok_or(KoraError::NotInitialized) } @@ -281,7 +284,7 @@ impl RiskRegistryContract { fn require_admin(env: &Env, caller: &Address) -> Result<(), KoraError> { let admin: Address = env .storage() - .instance() + .persistent() .get(&DataKey::Admin) .ok_or(KoraError::NotInitialized)?; if &admin != caller { @@ -305,7 +308,7 @@ impl RiskRegistryContract { fn require_invoice_nft(env: &Env, caller: &Address) -> Result<(), KoraError> { let invoice_nft: Address = env .storage() - .instance() + .persistent() .get(&DataKey::InvoiceNft) .ok_or(KoraError::NotInitialized)?; if &invoice_nft != caller { diff --git a/contracts/shared/src/lib.rs b/contracts/shared/src/lib.rs index d0eb264..ee85dca 100644 --- a/contracts/shared/src/lib.rs +++ b/contracts/shared/src/lib.rs @@ -1,21 +1,15 @@ #![no_std] -//! # Kora Shared Library — Audit Findings +//! # Kora Shared Library //! -//! ## Summary of Findings and Fixes +//! Common types, errors, events, and validation utilities for the Kora Protocol. //! -//! ### 1. Missing Doc Comments on Validation Helpers (validation.rs) -//! - **Issue:** All public validation functions lacked doc comments -//! - **Fix:** AUDIT FIX: Added comprehensive /// doc comments to every public function -//! - **Severity:** Medium — Documentation completeness -//! -//! ### 2. Incorrect Error Type for Empty Bytes (validation.rs:39-43) -//! - **Issue:** `require_non_empty_bytes()` returned `EmptyString` error (semantically wrong for bytes) -//! - **Fix:** AUDIT FIX: Changed to return dedicated `EmptyBytes` error for semantic clarity -//! - **Severity:** Low — Error categorization/semantics -//! -//! All arithmetic operations use checked methods. All type definitions are well-documented. -//! Error enum is comprehensive and specific. +//! ## Modules +//! - `types` — Core on-chain data structures (Invoice, Listing, Pool, etc.) +//! - `errors` — Protocol-wide error enum (`KoraError`) +//! - `events` — Standardized event emission helpers +//! - `validation` — Input validation and safe arithmetic helpers +//! - `reentrancy` — RAII reentrancy guard pub mod errors; pub mod events; diff --git a/contracts/shared/src/reentrancy.rs b/contracts/shared/src/reentrancy.rs index 58d064e..c4896b4 100644 --- a/contracts/shared/src/reentrancy.rs +++ b/contracts/shared/src/reentrancy.rs @@ -11,35 +11,6 @@ pub enum GuardKey { Lock, } -// ── RAII guard ──────────────────────────────────────────────────────────────── - -/// RAII reentrancy guard. Acquires the lock on construction and releases it -/// when dropped, ensuring the lock is always released even on early returns. -/// -/// # Usage -/// ```rust,ignore -/// let _guard = ReentrancyGuard::new(&env)?; -/// // ... protected logic ... -/// // lock is released automatically when _guard goes out of scope -/// ``` -pub struct ReentrancyGuard<'a> { - env: &'a Env, -} - -impl<'a> ReentrancyGuard<'a> { - /// Acquire the reentrancy lock. Returns `KoraError::Reentrancy` if already held. - pub fn new(env: &'a Env) -> Result { - acquire_guard(env)?; - Ok(Self { env }) - } -} - -impl<'a> Drop for ReentrancyGuard<'a> { - fn drop(&mut self) { - release_guard(self.env); - } -} - // ── Low-level helpers ───────────────────────────────────────────────────────── /// Acquire the reentrancy lock. diff --git a/contracts/treasury/src/lib.rs b/contracts/treasury/src/lib.rs index a5d3dbe..c25ed27 100644 --- a/contracts/treasury/src/lib.rs +++ b/contracts/treasury/src/lib.rs @@ -3,7 +3,8 @@ use kora_shared::{ errors::KoraError, events, - validation::require_valid_fee_bps, + reentrancy::ReentrancyGuard, + validation::{require_non_zero_amount, require_valid_fee_bps}, }; use soroban_sdk::{contract, contractimpl, contracttype, token, Address, Env}; @@ -21,8 +22,8 @@ pub enum DataKey { FeeBps, /// Accumulated fees per token (informational). Collected(Address), - /// Reentrancy guard for withdrawal functions. - WithdrawalLock, + /// Whitelisted token flag. + WhitelistedToken(Address), } // ── Contract ────────────────────────────────────────────────────────────────── @@ -34,7 +35,6 @@ pub struct TreasuryContract; impl TreasuryContract { /// One-time initialization. Sets admin and protocol fee. pub fn initialize(env: Env, admin: Address, fee_bps: u32) -> Result<(), KoraError> { - // Use persistent storage consistently — same store read by require_admin if env.storage().persistent().has(&DataKey::Admin) { return Err(KoraError::AlreadyInitialized); } @@ -113,7 +113,7 @@ impl TreasuryContract { } /// Withdraw accumulated fees to a recipient. Admin only. - /// Protected against reentrancy via an instance-storage lock key. + /// Protected against reentrancy via RAII guard. pub fn withdraw( env: Env, admin: Address, @@ -127,31 +127,22 @@ impl TreasuryContract { require_non_zero_amount(amount)?; Self::require_whitelisted_token(&env, &token)?; - // Validate amount before acquiring the lock to avoid unnecessary state mutation - if amount <= 0 { - return Err(KoraError::InvalidAmount); - } - - Self::acquire_lock(&env)?; + let _guard = ReentrancyGuard::new(&env)?; let token_client = token::Client::new(&env, &token); let balance = token_client.balance(&env.current_contract_address()); if balance < amount { - // Release lock before returning error — must not leave lock stuck - Self::release_lock(&env); return Err(KoraError::InsufficientPoolBalance); } // ── Effects ─────────────────────────────────────────────────────────── - // Deduct from informational accounting if tracked let collected_key = DataKey::Collected(token.clone()); if let Some(collected) = env .storage() .persistent() .get::<_, i128>(&collected_key) { - // Saturating sub: accounting is informational, don't revert on mismatch let new_collected = collected.saturating_sub(amount); env.storage() .persistent() @@ -167,7 +158,7 @@ impl TreasuryContract { } /// Emergency drain — withdraw entire token balance. Admin only. - /// Protected against reentrancy via an instance-storage lock key. + /// Protected against reentrancy via RAII guard. pub fn emergency_withdraw( env: Env, admin: Address, @@ -179,27 +170,17 @@ impl TreasuryContract { Self::require_admin(&env, &admin)?; Self::require_whitelisted_token(&env, &token)?; - // Acquire reentrancy guard — released automatically when _guard drops let _guard = ReentrancyGuard::new(&env)?; let token_client = token::Client::new(&env, &token); let balance = token_client.balance(&env.current_contract_address()); + // ── Interactions ────────────────────────────────────────────────────── if balance > 0 { token_client.transfer(&env.current_contract_address(), &recipient, &balance); - } - - // Always release lock regardless of whether a transfer occurred - Self::release_lock(&env); - - if balance > 0 { events::emergency_withdrawn(&env, &admin, &token, balance); } - // ── Interactions ────────────────────────────────────────────────────── - token_client.transfer(&env.current_contract_address(), &recipient, &balance); - - events::emergency_withdrawn(&env, &admin, &token, balance); Ok(()) } @@ -249,8 +230,12 @@ impl TreasuryContract { Ok(()) } - fn release_lock(env: &Env) { - env.storage().instance().set(&DataKey::WithdrawalLock, &false); + fn bump_persistent(env: &Env, key: &DataKey) { + env.storage().persistent().extend_ttl( + key, + PERSISTENT_LIFETIME_THRESHOLD, + PERSISTENT_BUMP_AMOUNT, + ); } } @@ -409,7 +394,7 @@ mod tests { let (env, admin, client) = setup(); let token = Address::generate(&env); let recipient = Address::generate(&env); - // Fails due to insufficient balance — lock must be released + // Fails due to token not whitelisted — lock must be released let _ = client.try_withdraw(&admin, &token, &recipient, &1_000i128); // Subsequent admin operation must succeed (lock not stuck) assert!(client.try_set_fee_bps(&admin, &100u32).is_ok());