Skip to content

ARITHMETIC_FIX_DETAILS.md

Latest commit

 

History

History
340 lines (247 loc) · 9.51 KB

ARITHMETIC_FIX_DETAILS.md

File metadata and controls

340 lines (247 loc) · 9.51 KB
Raw

Arithmetic Fix - Detailed Analysis

Issue Fixed: Unchecked addition in referred volume tracking
Severity: MEDIUM (Principle violation, low practical risk)
Locations: 2 (Lines 3340, 3363)
Fix Type: Consistency improvement

The Issue

Location 1: New Prediction with Referrer

File: contract/contracts/predifi-contract/src/lib.rs
Line: 3340 (in place_prediction())

Context: When a user places a new prediction with a referrer, track the referred volume

Before:

if let Some(ref referrer_addr) = referrer {
    let referrer_key = DataKey::Referrer(user.clone(), pool_id);
    env.storage().persistent().set(&referrer_key, referrer_addr);
    Self::extend_persistent(&env, &referrer_key);
    let vol_key = DataKey::ReferredVolume(referrer_addr.clone(), pool_id);
    let vol: i128 = env.storage().persistent().get(&vol_key).unwrap_or(0);
    env.storage().persistent().set(&vol_key, &(vol + amount));  // ❌ UNCHECKED
    Self::extend_persistent(&env, &vol_key);
}

After:

if let Some(ref referrer_addr) = referrer {
    let referrer_key = DataKey::Referrer(user.clone(), pool_id);
    env.storage().persistent().set(&referrer_key, referrer_addr);
    Self::extend_persistent(&env, &referrer_key);
    let vol_key = DataKey::ReferredVolume(referrer_addr.clone(), pool_id);
    let vol: i128 = env.storage().persistent().get(&vol_key).unwrap_or(0);
    // ✅ Use checked_add for overflow protection (consistency with all other arithmetic)
    let new_vol = vol.checked_add(amount).ok_or(PredifiError::InvalidAmount)?;
    env.storage().persistent().set(&vol_key, &new_vol);
    Self::extend_persistent(&env, &vol_key);
}

Location 2: Increasing Stake on Existing Prediction

File: contract/contracts/predifi-contract/src/lib.rs
Line: 3363 (in place_prediction())

Context: When a user increases their stake on an existing prediction with an existing referrer

Before:

if let Some(existing_pred) = existing_pred {
    // ... existing prediction logic ...
    
    // Track referred volume: if this user already has a referrer, add to their volume
    let referrer_key = DataKey::Referrer(user.clone(), pool_id);
    if let Some(referrer_addr) = env.storage().persistent().get::<_, Address>(&referrer_key) {
        Self::extend_persistent(&env, &referrer_key);
        let vol_key = DataKey::ReferredVolume(referrer_addr.clone(), pool_id);
        let vol: i128 = env.storage().persistent().get(&vol_key).unwrap_or(0);
        env.storage().persistent().set(&vol_key, &(vol + amount));  // ❌ UNCHECKED
        Self::extend_persistent(&env, &vol_key);
    }
}

After:

if let Some(existing_pred) = existing_pred {
    // ... existing prediction logic ...
    
    // Track referred volume: if this user already has a referrer, add to their volume
    let referrer_key = DataKey::Referrer(user.clone(), pool_id);
    if let Some(referrer_addr) = env.storage().persistent().get::<_, Address>(&referrer_key) {
        Self::extend_persistent(&env, &referrer_key);
        let vol_key = DataKey::ReferredVolume(referrer_addr.clone(), pool_id);
        let vol: i128 = env.storage().persistent().get(&vol_key).unwrap_or(0);
        // ✅ Use checked_add for overflow protection (consistency with all other arithmetic)
        let new_vol = vol.checked_add(amount).ok_or(PredifiError::InvalidAmount)?;
        env.storage().persistent().set(&vol_key, &new_vol);
        Self::extend_persistent(&env, &vol_key);
    }
}

Why This Matters

1. Consistency Principle

All other arithmetic operations in the contract use SafeMath or checked operations:

// Payout calculation - SafeMath
let winnings = SafeMath::calculate_share(
    prediction.amount, 
    winning_stake, 
    payout_pool
)?;

// Fee calculation - SafeMath
let protocol_fee_total = SafeMath::percentage(
    pool.total_stake, 
    fee_bps_i, 
    RoundingMode::ProtocolFavor
)?;

// Stake accumulation - checked_add
pool.total_stake = pool.total_stake.checked_add(amount).expect("overflow");

// Volume tracking - INCONSISTENT (unchecked +)
env.storage().persistent().set(&vol_key, &(vol + amount));  // ❌

Issue: Referred volume tracking stands out as an exception

2. Overflow Scenario

While extremely unlikely, unchecked addition could overflow:

Maximum i128: 9,223,372,036,854,775,807

Overflow scenario:
  Referrer total volume approaches i128::MAX
  New stake added pushes it over
  Result: Integer wraps to negative number
  
Practical probability: Extremely low
  - Would need >$10^27 in total volume (more than global GDP)
  - Across single referrer on single pool
  - Never in practice

But: Principle violation
  - Every other operation protected
  - Should all be consistent

3. Error Handling

With unchecked addition:

vol + amount  // If overflows, silently wraps to negative

With checked_add:

vol.checked_add(amount).ok_or(error)?
// If overflow: Returns Err → Transaction reverts
// Behavior: Explicit, intentional, safe

The Fix

Implementation

// Before
env.storage().persistent().set(&vol_key, &(vol + amount));

// After
let new_vol = vol.checked_add(amount).ok_or(PredifiError::InvalidAmount)?;
env.storage().persistent().set(&vol_key, &new_vol);

What Changes

  1. Overflow Detection: Catches if vol + amount > i128::MAX
  2. Error Propagation: Returns error instead of wrapping
  3. Consistency: Matches SafeMath pattern throughout code

What Stays the Same

  • Functionality: Same result for normal (non-overflowing) amounts
  • Performance: Minimal impact (one additional check)
  • Logic: Same behavior, just with explicit error handling

Impact Analysis

Positive Impacts

✅ Consistency: All arithmetic now uses same protection pattern
✅ Safety: Explicit error handling for edge cases
✅ Maintainability: Code now follows single principle
✅ Future-proofing: Template for similar fixes

Negative Impacts

None identified. Change is:

  • Backward compatible (same output for valid inputs)
  • Non-breaking (same error category)
  • Localized (only affects referred volume tracking)

Testing

What needs testing:

  1. Normal volume tracking (should work same as before)
  2. Overflow scenario (should return error gracefully)

Test cases:

// Test 1: Normal volume accumulation
vol = 1000
amount = 500
new_vol = 1500  // Should work ✓

// Test 2: Overflow scenario
vol = i128::MAX - 100
amount = 200
new_vol = ???
// Should return error, not wrap ✓

Why Not Earlier?

Why This Wasn't Caught

  1. Obscured by Low Risk: Overflow would require unrealistic volume
  2. Audit Focused: Concentrated on critical paths first (payouts, fees)
  3. Principle: Consistency checks happened after functional audit

Why Fixed Now

  1. Complete Audit: Comprehensive arithmetic review identified inconsistency
  2. Best Practices: All operations should follow same pattern
  3. Long-term Maintenance: Sets expectation for future code

Comparison with Similar Operations

Stake Accumulation (Lines 3264, 3323, 3377)

// All use checked_add
pool.total_stake = pool.total_stake.checked_add(amount).expect("overflow");
existing_pred.amount = existing_pred.amount.checked_add(amount).expect("overflow");
let new_total = pool.total_stake.checked_add(amount).expect("overflow");

Pattern: All protected ✅

Volume Tracking (Lines 3340, 3363) - BEFORE FIX

env.storage().persistent().set(&vol_key, &(vol + amount));

Pattern: Inconsistent ❌

Volume Tracking (Lines 3340, 3363) - AFTER FIX

let new_vol = vol.checked_add(amount).ok_or(PredifiError::InvalidAmount)?;
env.storage().persistent().set(&vol_key, &new_vol);

Pattern: Consistent ✅

Verification

Before Deploying

  • Code compiles without errors
  • No diagnostic warnings
  • Error type matches existing pattern
  • Both locations updated consistently

Compilation Status

✅ ALL CHECKS PASS

No diagnostics found
No warnings
No errors

Deployment Strategy

  1. Include in audit submission: Fix documented in ARITHMETIC_AUDIT_SUMMARY.md
  2. Test thoroughly: Add overflow test case to test suite
  3. Document change: Explain consistency improvement in commit
  4. Deploy with confidence: Low risk, high principle benefit

Code Review Checklist

For code reviewers:

  • Verify both locations (3340, 3363) are updated
  • Confirm error type (PredifiError::InvalidAmount) is appropriate
  • Check that ? operator properly propagates error
  • Verify no functional change for normal amounts
  • Confirm consistency with other SafeMath usage
  • Validate compilation succeeds

Historical Context

Arithmetic Operations in PrediFi:

Operation Version Protection Status
Payout calc v1.0 SafeMath ✅ Original
Fee calc v1.0 SafeMath ✅ Original
Stake accum v1.0 checked_add ✅ Original
Vol tracking v1.0 unchecked ⚠️ Identified in audit
Vol tracking v2.0 checked_add ✅ Fixed in audit

Conclusion

Fix Type: Consistency Improvement
Risk Level: Very Low (edge case only)
Principle Impact: High (critical for maintaining SafeMath principle)
Deployment: Safe, recommended ✅

This fix ensures that ALL arithmetic operations in the contract follow the same protective pattern, making the codebase more maintainable and consistent with best practices.