diff --git a/contracts/predictify-hybrid/src/bets.rs b/contracts/predictify-hybrid/src/bets.rs index 0845e1ae..b8547155 100644 --- a/contracts/predictify-hybrid/src/bets.rs +++ b/contracts/predictify-hybrid/src/bets.rs @@ -24,6 +24,7 @@ use soroban_sdk::{contracttype, symbol_short, Address, Env, Map, String, Symbol} use crate::errors::Error; use crate::events::EventEmitter; use crate::markets::{MarketStateManager, MarketUtils, MarketValidator}; +use crate::reentrancy_guard::ReentrancyGuard; use crate::types::{Bet, BetLimits, BetStats, BetStatus, Market, MarketState}; use crate::validation; @@ -723,9 +724,14 @@ impl BetUtils { /// # Returns /// /// Returns `Ok(())` if transfer succeeds, `Err(Error)` otherwise. + /// + /// Reentrancy: takes the reentrancy lock before the token transfer and + /// releases it after. Prevents reentrant calls into the contract during transfer. pub fn lock_funds(env: &Env, user: &Address, amount: i128) -> Result<(), Error> { + ReentrancyGuard::before_external_call(env).map_err(|_| Error::InvalidState)?; let token_client = MarketUtils::get_token_client(env)?; token_client.transfer(user, &env.current_contract_address(), &amount); + ReentrancyGuard::after_external_call(env); Ok(()) } @@ -743,6 +749,10 @@ impl BetUtils { /// # Returns /// /// Returns `Ok(())` if transfer succeeds, `Err(Error)` otherwise. + /// + /// Reentrancy: caller must hold the reentrancy lock (e.g. cancel_event holds + /// the lock for the entire refund_market_bets batch). Do not call + /// before_external_call/after_external_call here to allow batch refunds. pub fn unlock_funds(env: &Env, user: &Address, amount: i128) -> Result<(), Error> { let token_client = MarketUtils::get_token_client(env)?; token_client.transfer(&env.current_contract_address(), user, &amount); diff --git a/contracts/predictify-hybrid/src/edge_cases.rs b/contracts/predictify-hybrid/src/edge_cases.rs index 23079107..76de2a4b 100644 --- a/contracts/predictify-hybrid/src/edge_cases.rs +++ b/contracts/predictify-hybrid/src/edge_cases.rs @@ -157,7 +157,7 @@ impl EdgeCaseHandler { /// ``` pub fn handle_zero_stake_scenario(env: &Env, market_id: Symbol) -> Result<(), Error> { // Check reentrancy protection - ReentrancyGuard::check_reentrancy_state(env); + ReentrancyGuard::check_reentrancy_state(env).map_err(|_| Error::InvalidState)?; // Get market data let market = MarketStateManager::get_market(env, &market_id)?; diff --git a/contracts/predictify-hybrid/src/lib.rs b/contracts/predictify-hybrid/src/lib.rs index 512affb1..d47999bf 100644 --- a/contracts/predictify-hybrid/src/lib.rs +++ b/contracts/predictify-hybrid/src/lib.rs @@ -489,7 +489,7 @@ impl PredictifyHybrid { /// - User authentication via `require_auth()` /// - Balance validation before fund transfer /// - Atomic fund locking with bet creation - /// - Reentrancy protection via Soroban's design + /// - Reentrancy protection via reentrancy guard (guard flag in storage) pub fn place_bet( env: Env, user: Address, @@ -497,6 +497,9 @@ impl PredictifyHybrid { outcome: String, amount: i128, ) -> crate::types::Bet { + if ReentrancyGuard::check_reentrancy_state(&env).is_err() { + panic_with_error!(env, Error::InvalidState); + } // Use the BetManager to handle the bet placement match bets::BetManager::place_bet(&env, user, market_id, outcome, amount) { Ok(bet) => bet, @@ -736,6 +739,9 @@ impl PredictifyHybrid { /// - User must not have previously claimed winnings pub fn claim_winnings(env: Env, user: Address, market_id: Symbol) { user.require_auth(); + if ReentrancyGuard::check_reentrancy_state(&env).is_err() { + panic_with_error!(env, Error::InvalidState); + } let mut market: Market = env .storage() @@ -781,10 +787,15 @@ impl PredictifyHybrid { Err(_) => panic_with_error!(env, Error::ConfigurationNotFound), }; let fee_percent = cfg.fees.platform_fee_percentage; - let user_share = - (user_stake * (PERCENTAGE_DENOMINATOR - fee_percent)) / PERCENTAGE_DENOMINATOR; + let user_share = (user_stake + .checked_mul(PERCENTAGE_DENOMINATOR - fee_percent) + .unwrap_or_else(|| panic_with_error!(env, Error::InvalidInput))) + / PERCENTAGE_DENOMINATOR; let total_pool = market.total_staked; - let payout = (user_share * total_pool) / winning_total; + let product = user_share + .checked_mul(total_pool) + .unwrap_or_else(|| panic_with_error!(env, Error::InvalidInput)); + let payout = product / winning_total; // Mark as claimed market.claimed.set(user.clone(), true); @@ -1480,6 +1491,9 @@ impl PredictifyHybrid { /// /// This function emits `WinningsClaimedEvent` for each user who receives a payout. pub fn distribute_payouts(env: Env, market_id: Symbol) -> Result { + if ReentrancyGuard::check_reentrancy_state(&env).is_err() { + return Err(Error::InvalidState); + } let mut market: Market = env .storage() .persistent() @@ -1545,15 +1559,22 @@ impl PredictifyHybrid { let user_stake = market.stakes.get(user.clone()).unwrap_or(0); if user_stake > 0 { let fee_denominator = 10000i128; - let user_share = - (user_stake * (fee_denominator - fee_percent)) / fee_denominator; - let payout = (user_share * total_pool) / winning_total; + let user_share = (user_stake + .checked_mul(fee_denominator - fee_percent) + .ok_or(Error::InvalidInput)?) + / fee_denominator; + let payout = (user_share + .checked_mul(total_pool) + .ok_or(Error::InvalidInput)?) + / winning_total; if payout >= 0 { // Allow 0 payout but mark as claimed market.claimed.set(user.clone(), true); if payout > 0 { - total_distributed += payout; + total_distributed = total_distributed + .checked_add(payout) + .ok_or(Error::InvalidInput)?; EventEmitter::emit_winnings_claimed(&env, &market_id, &user, payout); } } @@ -1774,6 +1795,9 @@ impl PredictifyHybrid { /// ``` pub fn withdraw_collected_fees(env: Env, admin: Address, amount: i128) -> Result { admin.require_auth(); + if ReentrancyGuard::check_reentrancy_state(&env).is_err() { + return Err(Error::InvalidState); + } // Verify admin let stored_admin: Address = env @@ -1803,8 +1827,10 @@ impl PredictifyHybrid { amount }; - // Update collected fees - let remaining_fees = collected_fees - withdrawal_amount; + // Update collected fees (checked to prevent underflow) + let remaining_fees = collected_fees + .checked_sub(withdrawal_amount) + .ok_or(Error::InvalidInput)?; env.storage().persistent().set(&fees_key, &remaining_fees); // Emit fee withdrawal event @@ -2354,8 +2380,16 @@ impl PredictifyHybrid { market.state = MarketState::Cancelled; env.storage().persistent().set(&market_id, &market); - // Refund all bets - bets::BetManager::refund_market_bets(&env, &market_id)?; + // Refund all bets under reentrancy lock (batch of token transfers) + if ReentrancyGuard::check_reentrancy_state(&env).is_err() { + return Err(Error::InvalidState); + } + if ReentrancyGuard::before_external_call(&env).is_err() { + return Err(Error::InvalidState); + } + let refund_result = bets::BetManager::refund_market_bets(&env, &market_id); + ReentrancyGuard::after_external_call(&env); + refund_result?; // Calculate total refunded (sum of all bets) let total_refunded = market.total_staked; diff --git a/contracts/predictify-hybrid/src/markets.rs b/contracts/predictify-hybrid/src/markets.rs index e5d96989..e22804e7 100644 --- a/contracts/predictify-hybrid/src/markets.rs +++ b/contracts/predictify-hybrid/src/markets.rs @@ -1881,8 +1881,14 @@ impl MarketUtils { return Err(Error::NothingToClaim); } - let user_share = (user_stake * (100 - fee_percentage)) / 100; - let payout = (user_share * total_pool) / winning_total; + let user_share = (user_stake + .checked_mul(100 - fee_percentage) + .ok_or(Error::InvalidInput)?) + / 100; + let payout = (user_share + .checked_mul(total_pool) + .ok_or(Error::InvalidInput)?) + / winning_total; Ok(payout) } diff --git a/contracts/predictify-hybrid/src/test.rs b/contracts/predictify-hybrid/src/test.rs index d93024f8..1cfdd582 100644 --- a/contracts/predictify-hybrid/src/test.rs +++ b/contracts/predictify-hybrid/src/test.rs @@ -2446,6 +2446,200 @@ fn test_reentrancy_protection_claim() { assert_eq!(market.state, MarketState::Resolved); } +// ===== REENTRANCY GUARD AND SECURITY HARDENING TESTS ===== +// Tests for reentrancy protection and overflow/underflow safety + +/// Sets the reentrancy lock in contract storage (same key as ReentrancyGuard). +fn set_reentrancy_lock(env: &Env, contract_id: &Address) { + env.as_contract(contract_id, || { + let key = Symbol::new(env, "reent_lk"); + env.storage().persistent().set(&key, &true); + }); +} + +#[test] +#[should_panic(expected = "Error(Contract, #400)")] // InvalidState (reentrancy guard active) +fn test_reentrancy_guard_blocks_place_bet_when_locked() { + let test = PredictifyTest::setup(); + let market_id = test.create_test_market(); + let client = PredictifyHybridClient::new(&test.env, &test.contract_id); + set_reentrancy_lock(&test.env, &test.contract_id); + test.env.mock_all_auths(); + let outcome = String::from_str(&test.env, "yes"); + let amount = 10_000_000; + client.place_bet(&test.user, &market_id, &outcome, &amount); +} + +#[test] +#[should_panic(expected = "Error(Contract, #400)")] // InvalidState (reentrancy guard active) +fn test_reentrancy_guard_blocks_claim_winnings_when_locked() { + let test = PredictifyTest::setup(); + let market_id = test.create_test_market(); + let client = PredictifyHybridClient::new(&test.env, &test.contract_id); + test.env.mock_all_auths(); + client.vote( + &test.user, + &market_id, + &String::from_str(&test.env, "yes"), + &100_0000000, + ); + let market = test.env.as_contract(&test.contract_id, || { + test.env + .storage() + .persistent() + .get::(&market_id) + .unwrap() + }); + test.env.ledger().set(LedgerInfo { + timestamp: market.end_time + 1, + protocol_version: 22, + sequence_number: test.env.ledger().sequence(), + network_id: Default::default(), + base_reserve: 10, + min_temp_entry_ttl: 1, + min_persistent_entry_ttl: 1, + max_entry_ttl: 10000, + }); + test.env.mock_all_auths(); + client.resolve_market_manual(&test.admin, &market_id, &String::from_str(&test.env, "yes")); + set_reentrancy_lock(&test.env, &test.contract_id); + test.env.mock_all_auths(); + client.claim_winnings(&test.user, &market_id); +} + +#[test] +#[should_panic(expected = "Error(Contract, #400)")] // InvalidState (reentrancy guard active) +fn test_reentrancy_guard_blocks_distribute_payouts_when_locked() { + let test = PredictifyTest::setup(); + let market_id = test.create_test_market(); + let client = PredictifyHybridClient::new(&test.env, &test.contract_id); + test.env.mock_all_auths(); + client.vote( + &test.user, + &market_id, + &String::from_str(&test.env, "yes"), + &100_0000000, + ); + let market = test.env.as_contract(&test.contract_id, || { + test.env + .storage() + .persistent() + .get::(&market_id) + .unwrap() + }); + test.env.ledger().set(LedgerInfo { + timestamp: market.end_time + 1, + protocol_version: 22, + sequence_number: test.env.ledger().sequence(), + network_id: Default::default(), + base_reserve: 10, + min_temp_entry_ttl: 1, + min_persistent_entry_ttl: 1, + max_entry_ttl: 10000, + }); + test.env.mock_all_auths(); + client.resolve_market_manual(&test.admin, &market_id, &String::from_str(&test.env, "yes")); + set_reentrancy_lock(&test.env, &test.contract_id); + let _ = client.distribute_payouts(&market_id); +} + +#[test] +#[should_panic(expected = "Error(Contract, #400)")] // InvalidState (reentrancy guard active) +fn test_reentrancy_guard_blocks_withdraw_fees_when_locked() { + let test = PredictifyTest::setup(); + let client = PredictifyHybridClient::new(&test.env, &test.contract_id); + test.env.as_contract(&test.contract_id, || { + test.env + .storage() + .persistent() + .set(&Symbol::new(&test.env, "tot_fees"), &50_000_000); + }); + set_reentrancy_lock(&test.env, &test.contract_id); + test.env.mock_all_auths(); + client.withdraw_collected_fees(&test.admin, &0); +} + +#[test] +#[should_panic(expected = "Error(Contract, #400)")] // InvalidState (reentrancy guard active) +fn test_reentrancy_guard_blocks_cancel_event_when_locked() { + let test = PredictifyTest::setup(); + let market_id = test.create_test_market(); + let client = PredictifyHybridClient::new(&test.env, &test.contract_id); + set_reentrancy_lock(&test.env, &test.contract_id); + test.env.mock_all_auths(); + client.cancel_event( + &test.admin, + &market_id, + &Some(String::from_str(&test.env, "test")), + ); +} + +#[test] +fn test_overflow_protection_calculate_payout() { + use crate::markets::MarketUtils; + let env = Env::default(); + let max_i128 = i128::MAX; + let result = MarketUtils::calculate_payout(max_i128, 1, max_i128, 0); + assert!(result.is_err()); + assert_eq!(result.unwrap_err(), Error::InvalidInput); +} + +#[test] +fn test_checks_effects_before_interactions_claim() { + // After resolve_market_manual, distribute_payouts runs and sets claimed for winners + // before any external interaction (checks-effects-interactions). Verify winner is marked claimed. + let test = PredictifyTest::setup(); + let market_id = test.create_test_market(); + let client = PredictifyHybridClient::new(&test.env, &test.contract_id); + test.env.mock_all_auths(); + client.vote( + &test.user, + &market_id, + &String::from_str(&test.env, "yes"), + &100_0000000, + ); + let market = test.env.as_contract(&test.contract_id, || { + test.env + .storage() + .persistent() + .get::(&market_id) + .unwrap() + }); + test.env.ledger().set(LedgerInfo { + timestamp: market.end_time + 1, + protocol_version: 22, + sequence_number: test.env.ledger().sequence(), + network_id: Default::default(), + base_reserve: 10, + min_temp_entry_ttl: 1, + min_persistent_entry_ttl: 1, + max_entry_ttl: 10000, + }); + test.env.mock_all_auths(); + client.resolve_market_manual(&test.admin, &market_id, &String::from_str(&test.env, "yes")); + let market_after = test.env.as_contract(&test.contract_id, || { + test.env + .storage() + .persistent() + .get::(&market_id) + .unwrap() + }); + assert!(market_after.claimed.get(test.user.clone()).unwrap_or(false)); +} + +#[test] +fn test_guard_allows_place_bet_when_unlocked() { + let test = PredictifyTest::setup(); + let market_id = test.create_test_market(); + let client = PredictifyHybridClient::new(&test.env, &test.contract_id); + test.env.mock_all_auths(); + let outcome = String::from_str(&test.env, "yes"); + let amount = 10_000_000; + let bet = client.place_bet(&test.user, &market_id, &outcome, &amount); + assert_eq!(bet.amount, amount); + assert_eq!(bet.outcome, outcome); +} + // ===== COMPREHENSIVE QUERY FUNCTION TESTS ===== // These tests ensure 95% coverage for all query functions with edge cases and gas efficiency