Skip to content

feat: add reentrancy protection for external calls and token transfers#172

Merged
greatest0fallt1me merged 2 commits into
Predictify-org:masterfrom
ikemHood:feat-reentrancy-guard
Sep 28, 2025
Merged

feat: add reentrancy protection for external calls and token transfers#172
greatest0fallt1me merged 2 commits into
Predictify-org:masterfrom
ikemHood:feat-reentrancy-guard

Conversation

@ikemHood

Copy link
Copy Markdown
Contributor

📋 Basic Information

Type of Change

  • ✨ New feature
  • 📚 Documentation update
  • 🧪 Test addition/update
  • 🔒 Security fix

Related Issues

Priority Level

  • 🔴 Critical

📝 Detailed Description

What does this PR do?

  • Implements a global, cross-function reentrancy guard to protect against reentrancy during external calls.
  • Centralizes token transfer and oracle invocation protections.
  • Ensures state is updated only after successful external calls and provides a helper for state restoration on failure.

Why is this change needed?

  • Prior to this PR, the contract had no reentrancy guard, leaving it vulnerable to cross-function reentrancy attacks. Attackers could reenter other public functions while the contract was mid external call (token transfer, oracle invoke), potentially observing or mutating inconsistent state.

How was this tested?

  • Added unit tests for the guard lifecycle and blocking behavior in reentrancy_guard.rs using env.register_contract + env.as_contract.
  • Ensured all external call sites are covered and that public entrypoints reject when the guard is active.
  • Ran full test suite: 150 tests passing.

Alternative Solutions Considered

  • Per-function local locks. Rejected in favor of a single global lock to protect cross-function reentrancy comprehensively and avoid duplicated logic.

🏗️ Smart Contract Specific

Contract Changes

  • Core contract logic modified (security wrappers)
  • New functions added
  • Existing functions modified
  • Error handling improved
  • Access control changes (entrypoint pre-checks)

Oracle Integration

  • Reflector oracle integration affected (guarded invoke_contract)
  • Price feed handling modified (guarded call chain)
  • Oracle fallback mechanisms (guard-friendly)
  • Price validation logic (unchanged, downstream of guard)

Market Resolution Logic

  • Dispute mechanism modified (entrypoints guarded, stake transfer guarded)
  • Voting mechanism changes (centralize guarded transfers)

Security Considerations

  • Access control reviewed
  • Reentrancy protection
  • Input validation (unchanged)
  • Overflow/underflow protection (unchanged)
  • Oracle manipulation protection (unchanged)

🧪 Testing

Test Coverage

  • Unit tests added/updated (reentrancy guard tests)
  • Integration tests passing
  • All tests passing locally
  • Edge cases covered (lock state transitions)
  • Error conditions tested (blocked on reentry)
  • Cross-contract interactions tested (oracle client guarded)

Test Results

cargo test
# Result: 150 passed; 0 failed
Screenshot 2025-09-27 at 4 48 38 PM

Manual Testing Steps

  1. Call vote concurrently during an external call path (e.g., fee collection or oracle fetch) — second call should fail with REENTRANCY_GUARD_ACTIVE.
  2. Simulate payouts via claim_winnings while a guarded transfer is in-flight — reentry should be blocked.
  3. Trigger oracle resolution (fetch_oracle_result) then attempt to reenter other entrypoints — blocked until guard released.

📚 Documentation

Documentation Updates

  • Code comments added/updated
  • Architecture documentation to be updated in follow-up (VOTING_SYSTEM.md/GAS docs) to include guard model and gas overhead notes

Breaking Changes

  • None. Behavior is stricter under attack scenarios but compatible with normal flows.

🔍 Code Quality

Code Review Checklist

  • No unnecessary code duplication (centralized guarded transfers)
  • Error handling appropriate (new error codes for guard/extern failures)
  • Logging not changed; events unaffected
  • Security considerations addressed
  • Readable and well-commented

Performance Impact

  • Gas Usage: Slight increase around guarded external calls (before/after lock writes)
  • Storage Impact: One boolean flag in persistent storage for guard state
  • Computational Complexity: Minimal overhead

Security Review

  • No obvious vulnerabilities introduced
  • Access control enforced at entrypoints
  • Input validation unchanged and intact
  • Oracle path guarded at lowest-level contract invoke

🚀 Deployment & Integration

Deployment Notes

  • No migration required.
  • Ensure downstream tooling expects REENTRANCY_GUARD_ACTIVE and EXTERNAL_CALL_FAILED errors where applicable.

Integration Points

  • Frontend should surface guard errors cleanly.
  • Monitoring can alert on frequent REENTRANCY_GUARD_ACTIVE occurrences.

📊 Impact Assessment

User Impact

  • End Users: Stronger security; unchanged UX in normal use.
  • Developers: Centralized utilities for transfers; consistent guard model.
  • Admins: Administrative actions respect guard state.

Business Impact

  • Security posture: Improved; aligns with audit expectations.
  • Technical Debt: Reduced duplication around transfers; standardized guard helpers.

✅ Final Checklist

Pre-Submission

  • Code follows Rust/Soroban best practices
  • All CI checks passing locally
  • No breaking changes
  • Ready for review
  • PR description complete and accurate
  • Tests included
  • Documentation updates queued (follow-up PR)

Review Readiness

  • Self-review completed
  • Code clean and formatted
  • Commit messages clear
  • Branch up to date
  • No merge conflicts

🔗 Implementation Summary (Files Touched)

  • contracts/predictify-hybrid/src/reentrancy_guard.rs
  • contracts/predictify-hybrid/src/errors.rs
  • contracts/predictify-hybrid/src/lib.rs
  • contracts/predictify-hybrid/src/voting.rs
  • contracts/predictify-hybrid/src/fees.rs
  • contracts/predictify-hybrid/src/oracles.rs
  • contracts/predictify-hybrid/src/resolution.rs
  • contracts/predictify-hybrid/src/markets.rs

Key Entry Points Guarded

  • PredictifyHybrid::vote
  • PredictifyHybrid::claim_winnings
  • PredictifyHybrid::dispute_market
  • PredictifyHybrid::vote_on_dispute
  • PredictifyHybrid::resolve_dispute
  • PredictifyHybrid::collect_fees
  • PredictifyHybrid::extend_market
  • PredictifyHybrid::fetch_oracle_result (delegates to guarded oracle client)

External Call Sites Guarded

  • Token transfers: VotingUtils::transfer_stake, VotingUtils::transfer_winnings, FeeUtils::transfer_fees_to_admin, FeeManager::process_creation_fee
  • Oracle calls: ReflectorOracleClient::{lastprice, price, twap}

New Errors

  • REENTRANCY_GUARD_ACTIVE (600)
  • EXTERNAL_CALL_FAILED (601)
Screenshot 2025-09-27 at 4 49 07 PM

@greatest0fallt1me

Copy link
Copy Markdown
Contributor

Nicely done. Appreciate your work on this! @ikemHood

@greatest0fallt1me greatest0fallt1me merged commit 1b1c510 into Predictify-org:master Sep 28, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add Reentrancy Guards to Prevent Cross-Function Reentrancy Attacks

2 participants