farming-pool: emergency_withdraw transfers funds before clearing UserStake/Position, opening a reentrant double-withdraw window

[prodbycorne]

Problem

emergency_withdraw — the admin's break-glass function for returning funds during an incident — transfers tokens out for both the lock (Position) and stake (UserStake) systems before clearing either record:

pub fn emergency_withdraw(env: Env, user: Address) -> Result<i128, PoolError> {
    // ...
    let mut total_returned = 0i128;
    let mut banked_credits = 0i128;
    let stake_token = get_stake_token(&env)?;
    let token = token::TokenClient::new(&env, &stake_token);

    if let Some(position) = get_position(&env, &user) {
        token.transfer(&env.current_contract_address(), &user, &position.amount);  // <-- transfer first
        total_returned += position.amount;
        banked_credits += position.total_credits;
        remove_position(&env, &user);                                              // <-- cleared after
    }

    if let Some(stake) = get_user_stake(&env, &user) {
        token.transfer(&env.current_contract_address(), &user, &stake.amount);     // <-- transfer first
        total_returned += stake.amount;
        banked_credits += stake.credits_banked;
        remove_user_stake(&env, &user);                                            // <-- cleared after
    }
    // ...
}

emergency_withdraw is admin-authorized (admin.require_auth()), so the reentrancy surface here is narrower than the user-facing functions, but the risk is not eliminated: stake_token is still an admin-chosen address, and this function is specifically designed to run during a declared emergency (pool_is_paused must be true) — precisely the scenario where the token itself, or its configuration, is most likely to be unusual, compromised, or under active attention. If transfer for the position leg re-enters emergency_withdraw (or get_position) for the same user before remove_position runs, the reentrant call still sees the full, uncredited Position and could authorize transferring the same locked amount a second time before the outer call's remove_position ever commits — doubling the payout for a single user's locked funds out of the contract's token balance, at the exact moment the contract is trying to safely return funds to everyone.

Impact

• Fund loss during the exact scenario meant to prevent it: a double-payout via reentrancy in the admin's own emergency-exit function would drain more of the pool's remaining token balance than intended, at the worst possible time (an active incident), potentially leaving other users unable to recover their funds.
• Same underlying pattern as lock_assets/unlock_assets/stake/unstake: fixing this in isolation without also fixing the companion issues leaves the rest of the fund-moving surface exposed.

Fix

Clear each record before transferring the corresponding funds:

pub fn emergency_withdraw(env: Env, user: Address) -> Result<i128, PoolError> {
    // ...
    let mut total_returned = 0i128;
    let mut banked_credits = 0i128;
    let stake_token = get_stake_token(&env)?;
    let token = token::TokenClient::new(&env, &stake_token);

    if let Some(position) = get_position(&env, &user) {
        remove_position(&env, &user);                 // effects first
        total_returned += position.amount;
        banked_credits += position.total_credits;
        token.transfer(&env.current_contract_address(), &user, &position.amount);
    }

    if let Some(stake) = get_user_stake(&env, &user) {
        remove_user_stake(&env, &user);                // effects first
        total_returned += stake.amount;
        banked_credits += stake.credits_banked;
        token.transfer(&env.current_contract_address(), &user, &stake.amount);
    }
    // ...
}

Acceptance Criteria

• remove_position/remove_user_stake are called before their corresponding token.transfer in emergency_withdraw.
• Add a reentrancy test using a mock malicious token contract whose transfer re-enters emergency_withdraw for the same user mid-call, asserting only a single payout is possible.
• Existing test_emergency_withdraw_while_paused continues to pass with the same expected total_returned/banked_credits values.
• Document the checks-effects-interactions ordering requirement for this function given its role as the incident-response path.

Additional Notes

Confirmed against current source: emergency_withdraw (farming-pool/src/lib.rs:351-392) transfers the position amount at line 366 and only calls remove_position(&env, &user) at line 369; transfers the stake amount at line 373 and only calls remove_user_stake(&env, &user) at line 376 — confirming both transfer-before-clear orderings. Confirmed this function requires admin.require_auth() (line 354) and pool_is_paused must be true (lines 355-357, using the correctly-typed PoolError::NotPaused already), narrowing (but per the issue, not eliminating) the reentrancy surface relative to the user-facing functions in #69-71.

Edge cases:

• Both branches (position at lines 365-370, stake at lines 372-377) share the identical bug pattern within one function — the fix must reorder both independently, not just one, since a reentrant call could target either the still-live Position or the still-live UserStake depending on which branch's transfer is currently in flight.
• total_returned/banked_credits accumulate across both branches (lines 360-361, 367-368, 374-375) before the final if total_returned == 0 check (379-381) and set_banked_credits call (383-385) — confirm the reordering doesn't change what banked_credits/total_returned reflect at the point set_banked_credits runs; moving remove_position/remove_user_stake earlier (before their respective transfers) doesn't affect these accumulator reads since they already capture position.amount/position.total_credits/stake.amount/stake.credits_banked into locals before the transfer in the current code too (lines 367-368, 374-375) — the fix only reorders the storage mutation, not the accumulator logic.
• This is the admin's incident-response path — if the shared mock-reentrant-token test (see farming-pool: lock_assets calls token.transfer before persisting the updated Position #69) is used here too, note that emergency_withdraw additionally requires pool_is_paused() == true as a precondition, so the test setup must call pause() before invoking emergency_withdraw with the malicious token, unlike farming-pool: lock_assets calls token.transfer before persisting the updated Position #69-71's tests which run on an unpaused pool.
• Interacts with farming-pool: no restore/recovery path exists for archived per-user persistent storage entries after TTL expiry #73 (no restore path for archived storage): emergency_withdraw is the designated recovery path for administratively-forced exits, and per farming-pool: no restore/recovery path exists for archived per-user persistent storage entries after TTL expiry #73, if a user's Position/UserStake key has already been archived (TTL-expired) before an incident, emergency_withdraw itself cannot read it (same as unlock_assets/unstake) — worth a one-line cross-reference in the fix's code comment since this function is explicitly the "last resort" path and inherits the same archival limitation.

Implementation sketch: in the position branch (lines 365-370), move remove_position(&env, &user); before token.transfer(...); in the stake branch (lines 372-377), move remove_user_stake(&env, &user); before its token.transfer(...), exactly as the issue's fix snippet shows, keeping the local-variable captures (total_returned +=, banked_credits +=) in their current relative position (they read from the already-fetched position/stake values, not from storage, so reordering the storage-mutation call relative to them is safe either way, but matching the issue's snippet keeps the diff minimal).

Test plan: using the shared mock-reentrant-token helper (see #69), add test_emergency_withdraw_reentrant_transfer_allows_only_single_payout, remembering to call pause() first (precondition). Confirm test_emergency_withdraw_while_paused (confirmed present) still passes with identical total_returned/banked_credits values.

Cross-references: #69/#70/#71 (identical CEI pattern, shared test infra — implement as one coordinated set), #73 (archived-storage recovery limitation applies equally to this "last resort" function).

[Achievers-sketch]

Experienced Soroban security engineer. I’ll enforce checks-effects-interactions, add reentrancy tests with a malicious token mock, preserve behavior, and document security guarantees.