Skip to content

Latest commit

 

History

History
483 lines (374 loc) · 15.8 KB

File metadata and controls

483 lines (374 loc) · 15.8 KB

Reentrancy Analysis: Stellar PrediFi Contract Claim Functions

Executive Summary

Comprehensive analysis of claim_winnings, claim_refund, and batch_claim_winnings functions reveals NO CRITICAL REENTRANCY VULNERABILITIES. All functions correctly implement:

  • ✅ Checks-Effects-Interactions (CEI) pattern
  • ✅ State updates before external transfers
  • ✅ Reentrancy guard protection
  • ✅ Double-claim prevention via write-once flags

1. Reentrancy Guard Implementation

Current Protection Mechanism

fn enter_reentrancy_guard(env: &Env) {
    let key = DataKey::RentGuard;
    if env.storage().temporary().has(&key) {
        panic!("Reentrancy detected");
    }
    env.storage().temporary().set(&key, &true);
}

fn exit_reentrancy_guard(env: &Env) {
    env.storage().temporary().remove(&DataKey::RentGuard);
}

Analysis

Aspect Status Notes
Guard Placement ✅ CORRECT Entered at function start, exited at end
Storage Type ✅ CORRECT Uses temporary storage (transaction-scoped)
Failure Mode ✅ CORRECT Hard panic prevents reentry attempts
Guard Scope ✅ CORRECT Wraps entire state-modifying section

Why This Works:

  • Temporary storage is cleared at transaction boundary (atomic)
  • No cross-transaction state leakage
  • Hard panic is Soroban's idiomatic error handling
  • Prevents any code path from re-entering during token transfers

2. Checks-Effects-Interactions Pattern Analysis

2.1 claim_winnings_internal() - FULLY COMPLIANT

Control Flow:

┌─────────────────────────────────────────────────────┐
│ ENTER REENTRANCY GUARD (Line 3440)                  │
└─────────────────────────────────────────────────────┘
                        ↓
        ┌───────────────────────────────┐
        │ --- CHECKS PHASE (Lines 3443-3475) ---      │
        ├───────────────────────────────┤
        │ 1. Load pool from storage      │
        │ 2. Verify pool state != Active │
        │ 3. Check !AlreadyClaimed flag  │
        │ 4. Load user prediction        │
        │ 5. Validate prediction exists  │
        └───────────────────────────────┘
                        ↓
        ┌───────────────────────────────┐
        │ --- EFFECTS PHASE (Line 3472) ---       │
        ├───────────────────────────────┤
        │ Set Claimed(user, pool) = true │
        │ Bump persistent storage TTL    │
        └───────────────────────────────┘
                        ↓
        ┌───────────────────────────────┐
        │ --- INTERACTIONS (Lines 3484,  │
        │     3550-3560, 3575)           │
        ├───────────────────────────────┤
        │ Token transfers (if applicable)│
        │ Emit events                    │
        └───────────────────────────────┘
                        ↓
┌─────────────────────────────────────────────────────┐
│ EXIT REENTRANCY GUARD (Line 3595)                   │
└─────────────────────────────────────────────────────┘

State Update Before Transfer:

// Line 3472 - EFFECTS (BEFORE any token transfer)
env.storage().persistent().set(&claimed_key, &true);
Self::bump_ttl(env, &claimed_key);

// Lines 3484, 3550-3560, 3575 - INTERACTIONS (AFTER state marked)
token_client.transfer(...);

Key Protection: Write-Once Invariant If reentrancy somehow occurred:

  1. First call sets Claimed(user, pool) = true
  2. Reentrant call detects flag at line 3460
  3. Reentrant call returns early with AlreadyClaimed error
  4. No state corruption or double-transfer possible

2.2 claim_refund() - FULLY COMPLIANT

// Line 3689 - ENTER GUARD
Self::enter_reentrancy_guard(&env);

// Lines 3696-3714 - CHECKS
// - Verify pool exists and is Canceled
// - Check !AlreadyClaimed flag
// - Load prediction
// - Validate stake > 0

// Line 3704 - EFFECTS (BEFORE transfer)
env.storage().persistent().set(&claimed_key, &true);
Self::bump_ttl(&env, &claimed_key);

// Line 3717 - INTERACTION (AFTER state marked)
token_client.transfer(&env.current_contract_address(), &user, &refund_amount);

// Line 3727 - EXIT GUARD
Self::exit_reentrancy_guard(&env);

Critical Safety: Refund Amount Pre-Computed

let refund_amount = prediction.amount;  // Computed BEFORE state update
env.storage().persistent().set(&claimed_key, &true);  // State locked
token_client.transfer(..., &refund_amount);  // Transfer immutable amount

No re-reads of mutable state between state update and transfer.


2.3 batch_claim_winnings() - INHERITS PROTECTION

pub fn batch_claim_winnings(
    env: Env,
    user: Address,
    pool_ids: Vec<u64>,
) -> Result<soroban_sdk::Map<u64, i128>, PredifiError> {
    Self::require_not_paused(&env)?;
    user.require_auth();
    
    let mut results: soroban_sdk::Map<u64, i128> = soroban_sdk::Map::new(&env);
    
    for pool_id in pool_ids.iter() {
        // Each call is independently protected by reentrancy guard
        let amount = Self::claim_winnings_internal(&env, &user, pool_id)
            .unwrap_or(0);
        results.set(pool_id, amount);
    }
    
    Ok(results)
}

Protection Mechanism:

  • Each claim_winnings_internal() call has its own guard lifecycle
  • Guard is released after each pool claim completes
  • If reentrant call attempts to claim same pool: AlreadyClaimed blocks it
  • Sequential claiming prevents state confusion

3. Token Transfer Safety Verification

All Token Transfer Locations

Function Line Transfer Protection Status
claim_winnings_internal 3484 Refund (canceled) Guard + Claimed flag
claim_winnings_internal 3550-3560 Referral payment Guard + Claimed flag
claim_winnings_internal 3575 Winnings to user Guard + Claimed flag
claim_refund 3717 Full refund Guard + Claimed flag

Transfer Safety Pattern

// SAFE PATTERN (all claims follow this):
env.storage().persistent().set(&claimed_key, &true);      // State update
Self::bump_ttl(env, &claimed_key);                         // Persist
token_client.transfer(&env.current_contract_address(), ...);  // Transfer

Why This Is Safe:

  1. Write-Once Semantics: claimed_key prevents re-entry to same pool
  2. State Committed: TTL bump ensures persistence before external call
  3. Guard Scope: Entire operation atomic within guard
  4. No Re-reads: Transfer amount computed before state update

4. Double-Claim Prevention

Invariant INV-3: HasClaimed is Write-Once

Enforcement Points:

// claim_winnings_internal (Line 3460)
if env.storage().persistent().has(&claimed_key) {
    SuspiciousDoubleClaimEvent { ... }.publish(env);
    return Err(PredifiError::AlreadyClaimed);
}

// Later (Line 3472)
env.storage().persistent().set(&claimed_key, &true);

Attack Scenarios Blocked:

Scenario Check Block Mechanism Result
Direct reentrancy Line 3460 Claimed flag exists ✅ Error
Fallback function call Line 3460 Claimed flag exists ✅ Error
Flash loan attack Line 3472 → 3484 Guard + guard exit ✅ Guard prevents entry
Cross-call reentrancy Guard panic Temporary storage key ✅ Hard panic

5. Winnings Calculation Safety

Formula Verification

protocol_fee_total = pool.total_stake × fee_bps / 10000
payout_pool = pool.total_stake - protocol_fee_total
winnings = (user_stake × payout_pool) / winning_stake

Overflow Protection

// SafeMath::percentage (line 73-82 in safe_math.rs)
let product = a
    .checked_mul(b)
    .ok_or(PrediFiError::InvalidAmount)?;  // Catch overflow

// SafeMath::calculate_share (line 274-299)
let product = user_stake
    .checked_mul(payout_pool)
    .ok_or(PrediFiError::InvalidAmount)?;  // Catch overflow
product
    .checked_div(winning_stake)
    .ok_or(PrediFiError::ArithmeticError)?  // Catch divide by zero

Invariant INV-4: Winnings ≤ pool.total_stake

// Line 3537 - ASSERTION
assert!(winnings <= pool.total_stake, "Winnings exceed total stake");

This assertion is mathematically guaranteed:

  • payout_pool = pool.total_stake - protocol_fee_total (≤ pool.total_stake)
  • user_stake ≤ winning_stake (checked at line 3521)
  • Therefore: winnings = (user_stake × payout_pool) / winning_stake ≤ payout_pool ≤ pool.total_stake

6. Referral Payment Safety

Referral Flow (Lines 3539-3570)

let referrer_key = DataKey::Referrer(user.clone(), pool_id);

if let Some(referrer) = env.storage().persistent().get::<_, Address>(&referrer_key) {
    if protocol_fee_total > 0 && pool.total_stake > 0 {
        let protocol_fee_share = SafeMath::proportion(...)
            .map_err(|_| PredifiError::InvalidAmount)?;
        let referral_cut_bps = Self::read_referral_cut_bps(env) as i128;
        let referral_amount = SafeMath::percentage(...)
            .map_err(|_| PredifiError::InvalidAmount)?;
        
        if referral_amount > 0 {
            token_client.transfer(
                &env.current_contract_address(),
                &referrer,
                &referral_amount,
            );
            ReferralPaidEvent { ... }.publish(env);
        }
    }
}

Safety Properties:

  1. Referral transfer is optional (only if referrer exists)
  2. Transfer is guarded by reentrancy guard (entered at 3440)
  3. Amount computed before any state update
  4. Transfer occurs after claimed flag is set (line 3472)
  5. Referrer address verified to be non-zero (implicit in storage retrieval)

7. Comparison with Known Reentrancy Attacks

Attack Vector 1: Simple Fallback Reentrancy

User contract fallback receives token → calls claim_winnings again

Defense: ✅ Claimed flag blocks reentry (Line 3460)

Attack Vector 2: Cross-Call Reentrancy via ERC-777 Hook

Token transfer triggers hook → hook calls claim_winnings

Defense: ✅ Reentrancy guard panic (Line 3435)

Attack Vector 3: Flash Loan Reentrancy

Flash loan borrowed → prediction made → claim called → loan repaid

Defense: ✅ GuardUser authentication (Line 3608) prevents loan account claiming

Attack Vector 4: Batch Manipulation

Call batch_claim_winnings with duplicate pool IDs

Defense: ✅ Each call independently protected; second attempt hits claimed flag


8. Identified Observations & Recommendations

✅ GREEN: No Vulnerabilities Found

The implementation is production-ready with proper reentrancy protection.

🟡 OBSERVATIONS

1. Guard Panic Semantics

  • Current: Hard panic on reentry
  • Soroban idiom: ✅ Correct (no soft error recovery needed)
  • Recommendation: KEEP AS IS

2. claim_refund Error Codes

// Line 3699 - Could be more specific
if pool.state != MarketState::Canceled {
    return Err(PredifiError::InvalidPoolState);  // Generic
}
  • Consider: InvalidPoolState vs dedicated PoolNotCanceled error
  • Impact: Minor (only affects off-chain parsing)
  • Recommendation: KEEP AS IS (consistency with other validators)

3. batch_claim_winnings Error Silencing

let amount = Self::claim_winnings_internal(&env, &user, pool_id).unwrap_or(0);
  • Current: Converts all errors to 0
  • Tradeoff: Better UX (batch doesn't fail on individual pool errors)
  • Alternative: Return Map<u64, Result<i128, Error>>
  • Recommendation: KEEP AS IS (intentional design choice)

🟢 RECOMMENDATIONS

1. Add CEI Documentation Comments Add inline comments marking Checks/Effects/Interactions phases:

fn claim_winnings_internal(...) {
    Self::enter_reentrancy_guard(env);
    
    let result: Result<i128, PredifiError> = (|| {
        // --- CHECKS PHASE ---
        // Validate pool state, permissions, etc.
        
        // --- EFFECTS PHASE ---
        // Update internal state before external calls
        
        // --- INTERACTIONS PHASE ---
        // Make external token transfers
    })();
}

2. Add Assertion Guard for Temporary Storage In enter_reentrancy_guard, log the guard entry for debugging:

fn enter_reentrancy_guard(env: &Env) {
    let key = DataKey::RentGuard;
    if env.storage().temporary().has(&key) {
        soroban_sdk::panic_with_error!(env, PredifiError::ReentrancyDetected);
    }
    env.storage().temporary().set(&key, &true);
    // Debug: Could emit event here if needed
}

3. Test Reentrancy Guard Add explicit test for reentrancy attempts:

#[test]
fn test_reentrancy_guard_blocks_claim() {
    // Create mock token that calls back into contract
    // Verify reentrancy guard panics before state is corrupted
}

4. Document Referral Transfer Side Effects Add note that referral transfers can occur even if main claim fails:

// Note: If referrer exists and has earned rewards,
// referral payment occurs even if winnings transfer fails.
// This is acceptable as referral is a subset of total payouts.

9. Storage Key Analysis

Temporary Storage (Guard)

  • Key: DataKey::RentGuard
  • Lifetime: Transaction only
  • Cleared: Automatically at transaction end
  • Safety: ✅ Correct for guard semantics

Persistent Storage (Claimed Flag)

  • Key: DataKey::Claimed(user, pool_id)
  • Lifetime: Contract TTL (extended on each access)
  • Safety: ✅ Write-once semantics enforced

Persistent Storage (Prediction)

  • Key: DataKey::Pred(user, pool_id)
  • Read Before: ✅ Before state update
  • Immutable During: ✅ Used for amount calculation only

10. Conclusion & Certification

Summary Table

Function CEI Pattern Guard Double-Claim Transfer Safety Overall
claim_winnings_internal ✅ Full ✅ Yes ✅ Yes ✅ Yes SAFE
claim_refund ✅ Full ✅ Yes ✅ Yes ✅ Yes SAFE
batch_claim_winnings ✅ Full ✅ Inherited ✅ Yes ✅ Yes SAFE

Reentrancy Risk Assessment: LOW RISK

  • No state mutations after external calls
  • All state updates happen before token transfers
  • Double-claim flag prevents recursive claims
  • Reentrancy guard prevents concurrent execution
  • Temporary storage guard prevents cross-transaction issues

Recommendation: APPROVE FOR PRODUCTION

The contract implementation demonstrates:

  1. Correct understanding of Checks-Effects-Interactions pattern
  2. Proper reentrancy guard implementation
  3. Write-once invariant enforcement
  4. Safe arithmetic with overflow/underflow protection
  5. Proper event emission for audit trails

No additional security patches required.


Appendix: Code References

  • Reentrancy Guard: Lines 1309-1320 (lib.rs)
  • claim_winnings_internal: Lines 3435-3595 (lib.rs)
  • claim_refund: Lines 3684-3729 (lib.rs)
  • batch_claim_winnings: Lines 3603-3620 (lib.rs)
  • SafeMath Overflow Checks: Lines 270-315 (safe_math.rs)