Skip to content

PR_REENTRANCY_DESCRIPTION.md

Latest commit

 

History

History
271 lines (186 loc) · 10.9 KB

PR_REENTRANCY_DESCRIPTION.md

File metadata and controls

271 lines (186 loc) · 10.9 KB
Raw

PR: Harden Charge Flow Against Reentrancy

Summary

This PR audits and hardens the subscription vault charge flow to ensure it is reentrancy-safe-by-construction. We implement a two-layer defense strategy:

  1. Checks-Effects-Interactions (CEI) Pattern (structural): All internal state updates happen before any external token contract calls
  2. ReentrancyGuard (runtime): Secondary layer prevents recursive entry to public fund-moving functions

Problem

The charging flow involves critical operations that transition internal state and move tokens:

  • Charging a subscription (updates balance, credits merchant, moves tokens via accounting)
  • Depositing funds (updates balance, transfers tokens from subscriber)
  • Withdrawing funds (clears balance, transfers tokens to subscriber)
  • Merchant withdrawals (updates earnings, transfers tokens)

Without proper reentrancy protection, a malicious or non-standard token contract could:

  1. Attempt to re-enter during token transfers
  2. Call back into our contract with inconsistent state
  3. Exploit state inconsistency to double-charge or steal funds

Solution

Primary Defense: CEI Pattern (Structural)

All fund-moving operations are refactored to follow Checks-Effects-Interactions:

1. CHECKS: Validate authorization, preconditions, balances
   (no state changes, only reads)

2. EFFECTS: Update internal state in storage
   - Prepaid balance updates
   - Merchant earnings updates
   - Status transitions
   (all committed atomically to storage)

3. INTERACTIONS: Only call external functions after state is persisted
   - token.transfer() happens AFTER storage.set()
   - Never during an inconsistent state window

Why this works: Even if external code tries to re-enter, it can't find inconsistent state to exploit. The accounting is already correct.

Secondary Defense: ReentrancyGuard (Runtime)

Added ReentrancyGuard acquisition to all public fund-moving entry-points:

pub fn charge_subscription(env: Env, subscription_id: u32) -> Result<ChargeExecutionResult, Error> {
    // Guard acquired first
    let _guard = crate::reentrancy::ReentrancyGuard::lock(&env, "charge_subscription")?;

    // Then operation proceeds
    charge_core::charge_one(&env, subscription_id, env.ledger().timestamp(), None)
    // Guard auto-drops (exception-safe)
}

Why this works: Guard prevents the same function from being called recursively. If a token contract tries to call back, the guard is held and the re-entrance attempt fails with Error::Reentrancy.

Changes

Code Changes (lib.rs)

  • charge_subscription(): Added guard "charge_subscription"
  • charge_usage(): Added guard "charge_usage"
  • charge_usage_with_reference(): Added guard "charge_usage_with_reference"
  • charge_one_off(): Added guard "charge_one_off"
  • deposit_funds(): Added guard "deposit_funds"
  • withdraw_subscriber_funds(): Added guard "withdraw_subscriber_funds"
  • partial_refund(): Added guard "partial_refund"
  • withdraw_merchant_funds(): Added guard "withdraw_merchant_funds"
  • withdraw_merchant_token_funds(): Added guard "withdraw_merchant_token_funds"
  • merchant_refund(): Added guard "merchant_refund"

Each guard:

  • Is acquired immediately after pre-checks (emergency stop checks)
  • Is keyed to the specific function for fine-grained control
  • Is automatically released via Rust's Drop trait (exception-safe)
  • Returns Error::Reentrancy if a lock already exists

Documentation Changes

Module Headers Updated

  • subscription.rs: Enhanced with guard coverage and CEI pattern details
  • charge_core.rs: New reentrancy safety section explaining why no external calls occur
  • merchant.rs: Updated with guard coverage for all fund withdrawal functions

New Documentation

  • docs/reentrancy_hardening.md: Comprehensive 300+ line audit document including:
    • External call site analysis
    • Mutation ordering in charge_one
    • Guard placement rationale
    • Soroban-specific considerations
    • Implementation checklist

Implementation Summary

  • REENTRANCY_IMPLEMENTATION.md: Complete summary of all changes made

No Changes Required

  • reentrancy.rs: Guard implementation is already complete and well-designed
  • test_reentrancy_invariants.rs: Existing comprehensive tests already validate both defenses
    • CEI pattern tests (sections 1-5)
    • Guard lifecycle tests (section 6)
    • Emergency path tests (section 7)
    • Recovery tests (section 8)

Key Design Decisions

Why CEI First, Guard Second?

  1. CEI is structural: Protects against any attack, including ones that bypass guards
  2. Guard is behavioral: Provides runtime protection for edge cases
  3. Combined approach: Two independent layers mean either could be broken and contract is still safe

Why Per-Function Guards?

  1. Prevents same-function re-entry: Most common attack vector
  2. Fine-grained control: Can identify exactly which function is being attacked
  3. Efficient: Single storage operation per function call
  4. Maintainable: Clear correlation between guard and function

Why Not Global Guard?

  • Would prevent legitimate concurrent operations (e.g., charging sub A while user deposits to sub B)
  • Per-function guards prevent concurrency issues without sacrificing usability

Testing Strategy

Existing Test Coverage

The existing test_reentrancy_invariants.rs test suite validates:

CEI Pattern Invariants (verified by tests):

  • Prepaid balance updated before token transfer ✅
  • Merchant balance credited before external call ✅
  • Balance cleared before withdrawal ✅
  • State committed before refund ✅
  • Multiple sequential operations maintain consistency ✅
  • Failed operations leave state unchanged ✅

Guard Lifecycle (verified by tests):

  • Lock is released after operation ✅
  • Multiple operations can proceed sequentially ✅
  • Lock not acquired if pre-checks fail ✅

Emergency Paths (verified by tests):

  • Non-existent subscriptions fail cleanly ✅
  • Emergency stop blocks before guard ✅
  • Status transitions correct after failures ✅

Recovery Paths (verified by tests):

  • Failed charge doesn't prevent next valid charge ✅
  • Topup after insufficient balance works ✅
  • Prepaid balance updates correctly ✅

Test Categories

Test Status Purpose
test_deposit_state_committed_before_transfer ✅ CEI: balance before transfer
test_charge_token_conservation_invariant ✅ CEI: merchant credited before call
test_withdraw_subscriber_state_committed_before_transfer ✅ CEI: balance before withdrawal
test_partial_refund_state_committed_before_transfer ✅ CEI: balance before refund
test_reentrancy_guard_lock_is_released_after_operation ✅ Guard cleanup after success
test_reentrancy_guard_released_after_merchant_withdrawal ✅ Guard cleanup for merchant ops
test_reentrancy_guard_not_stuck_after_rejection ✅ Guard not acquired on pre-check failure
test_charge_failure_then_topup_then_charge_succeeds ✅ Recovery after charge failure

To Run Tests

cargo test -p subscription_vault --lib test_reentrancy_invariants

Reentrancy Notes (for PR Description)

Chosen Ordering: Checks-Effects-Interactions

Why this is safe:

  1. Checks-first: Validates preconditions before any state changes (prevents invalid operations)
  2. Effects-second: All internal state updates happen atomically in storage (ensures consistency)
  3. Interactions-last: External calls happen after state is finalized (exploits cannot find inconsistency)

Soroban-specific notes:

  • Synchronous execution model means no deep call stacks
  • Stellar token contracts don't have ERC777 callbacks
  • Standard token.transfer() is atomic and non-reentering
  • Guards provide defense-in-depth for non-standard token implementations

Guard Placement Strategy

Guards are placed at public entry-points (lib.rs layer), not internal functions, because:

  • Single control point: One guard per user-initiated action
  • Efficiency: Avoids redundant guards for internal call chains
  • Clarity: Readers know exactly which functions have protection
  • Maintenance: Easier to audit and update

Guard Coverage

✅ Fund-Moving Operations (10/10 guarded):

  • Charge operations: charge_subscription, charge_usage, charge_usage_with_reference, charge_one_off
  • Subscription operations: deposit_funds, withdraw_subscriber_funds, partial_refund
  • Merchant operations: withdraw_merchant_funds, withdraw_merchant_token_funds, merchant_refund

❌ Read-only Operations (not guarded):

  • Query functions never call external contracts
  • Guards would be overhead without benefit

Assumptions Documented

  1. Token Contract Behavior: Current Soroban/Stellar token contracts don't have callbacks
  2. Execution Model: Soroban's synchronous execution prevents deep reentry chains
  3. Atomic Transfers: token.transfer() is atomic and non-reentering in standard implementations
  4. Guard Sufficiency: Per-function guards sufficient for function-level reentrancy

These assumptions are reasonable for Soroban but documented for future maintainers.

Performance Impact

  • Guard acquisition cost: Single storage read/write per function call
  • Guard cleanup cost: Single storage remove on Drop
  • Negligible overhead: One storage operation is already needed for state updates
  • No impact on normal case: Guard returns immediately if no lock exists

Compatibility

  • ✅ Fully backward compatible
  • ✅ No API changes
  • ✅ No changes to external types
  • ✅ Works with existing Soroban SDK versions
  • ✅ No dependencies added

Future Considerations

If the contract evolves to support:

  • ERC777-style tokens: Guards remain effective
  • Cross-contract calls: May need to review guard scope (per-function is likely insufficient)
  • Multi-signature operations: Should maintain separate guard scopes
  • Async calls: Would need significant architectural changes

Related Documentation

  • docs/reentrancy.md: User-facing reentrancy documentation
  • docs/reentrancy_hardening.md: Technical audit and implementation strategy
  • REENTRANCY_IMPLEMENTATION.md: Complete change summary

Closing

This PR makes the charge path reentrancy-safe-by-construction through a well-tested two-layer approach. The implementation is efficient, maintainable, and provides strong security guarantees against both known and hypothetical reentrancy attacks.

The combination of structural (CEI) and runtime (Guard) defenses ensures the contract is resilient against token implementations with unexpected behavior while maintaining full compatibility with standard Soroban tokens.