farming-pool: unlock_assets transfers tokens to the user before removing/updating the Position record

[prodbycorne]

Problem

unlock_assets transfers tokens out to the user before the Position record reflecting the reduced (or removed) balance is written:

pub fn unlock_assets(env: Env, user: Address, amount: i128) -> Result<(), PoolError> {
    // ...
    checkpoint_position(&env, &mut position);
    let total_credits = position.total_credits;
    position.amount -= amount;                 // computed in-memory only

    let stake_token = get_stake_token(&env)?;
    token::TokenClient::new(&env, &stake_token).transfer(
        &env.current_contract_address(),
        &user,
        &amount,
    );                                          // <-- external call happens here, with old
                                                  //     position.amount still in storage

    if position.amount == 0 {
        remove_position(&env, &user);
    } else {
        set_position(&env, &user, &position);    // <-- write happens after the call returns
    }
    // ...
}

Because stake_token is an admin-chosen address (not guaranteed to be a well-behaved SAC — see the lock_assets reentrancy issue for the same root cause), its transfer implementation can call back into FarmingPool before this call's own set_position/remove_position executes. At that point, persistent storage still shows the user's full pre-withdrawal position.amount, even though the tokens for amount have already left the contract. A reentrant unlock_assets/emergency_withdraw call in that window would compute against the stale, still-full balance and could authorize transferring out the same tokens a second time before the first call's set_position/remove_position ever commits.

Impact

• Fund loss risk: a reentrant call during the transfer-out window sees a Position that has not yet been decremented, so an amount could be withdrawn twice from the contract's token balance for a single locked position, if stake_token is (or becomes, via a future admin-driven token migration) a non-standard contract.
• Same root cause across the withdrawal surface: unstake and emergency_withdraw have the identical ordering (see companion issues), so a fix here should be applied uniformly.

Fix

Persist the updated/removed position before transferring funds out:

pub fn unlock_assets(env: Env, user: Address, amount: i128) -> Result<(), PoolError> {
    // ...
    checkpoint_position(&env, &mut position);
    let total_credits = position.total_credits;
    position.amount -= amount;

    if position.amount == 0 {
        remove_position(&env, &user);
    } else {
        set_position(&env, &user, &position);
    }

    let stake_token = get_stake_token(&env)?;
    token::TokenClient::new(&env, &stake_token).transfer(
        &env.current_contract_address(), &user, &amount,
    );

    env.events().publish(/* ... */);
    Ok(())
}

Acceptance Criteria

• set_position/remove_position is called before token::TokenClient::transfer in unlock_assets.
• Add a test with a mock reentrant token contract whose transfer implementation calls back into unlock_assets/get_user_position for the same user, and assert the reentrant call observes the already-decremented (or removed) position, and cannot double-withdraw.
• Existing test_unlock_assets_full_returns_tokens_and_credits, test_unlock_assets_partial_keeps_remaining_position continue to pass.
• Document the checks-effects-interactions ordering requirement in a code comment.

Additional Notes

Confirmed against current source: unlock_assets (farming-pool/src/lib.rs:265-303) computes position.amount -= amount in memory at line 283, calls token::TokenClient::new(&env, &stake_token).transfer(&env.current_contract_address(), &user, &amount) at lines 286-290, and only then does if position.amount == 0 { remove_position(...) } else { set_position(...) } at lines 292-296 — confirming the transfer-before-persist ordering exactly.

Edge cases:

• This function has the densest overlap with other issues in the batch: .expect("no active position") at line 272 (fixed by farming-pool: unlock_assets and unstake panic via untyped .expect() instead of returning typed PoolError #65), assert!(amount > 0, ...) at line 269 / assert!(amount <= position.amount, ...) at line 273 / assert!(current >= position.unlock_ledger, ...) at lines 276-279 (fixed by farming-pool: lock_assets/unlock_assets/stake/set_boost validate amounts with untyped assert! instead of typed PoolError variants #66), assert!(!pool_is_paused(&env), ...) at line 268 (fixed by farming-pool: pause guard has no typed PoolError variant — every gated function panics via assert! on a paused pool #68), and this issue's CEI reordering of lines 283-296 — five different issues touching one ~40-line function. Recommend implementing all five in a single coordinated pass over unlock_assets to avoid five separate rebases on the same lines; the resulting function's final structure should be validated against every acceptance-criteria checklist at once.
• The total_credits value (captured at line 282, before the amount decrement) is returned to the caller via the event at line 300 regardless of transfer/persist ordering — confirm the CEI fix doesn't accidentally change when total_credits is read relative to checkpoint_position (line 281), since checkpointing must still happen before the amount decrement for the credit accounting to be correct (this ordering is unaffected by moving the transfer, only by moving the persist step earlier — the sketch in the issue body preserves this correctly).
• Uses the same shared mock-reentrant-token test infrastructure flagged in farming-pool: lock_assets calls token.transfer before persisting the updated Position #69 — do not duplicate.

Implementation sketch: move the if position.amount == 0 { remove_position(&env, &user); } else { set_position(&env, &user, &position); } block (currently lines 292-296) to before the get_stake_token/transfer block (currently lines 285-290), exactly as the issue's fix snippet shows, while preserving checkpoint_position/total_credits capture ordering (lines 281-282) which happens earlier and is unaffected.

Test plan: using the shared mock-reentrant-token helper (see #69), add test_unlock_assets_reentrant_transfer_cannot_double_withdraw where the malicious token's transfer callback attempts a second unlock_assets/emergency_withdraw for the same user mid-call, asserting the reentrant call observes the already-decremented/removed position and fails with NoActiveStake or an amount-exceeds-balance error rather than succeeding. Confirm test_unlock_assets_full_returns_tokens_and_credits and test_unlock_assets_partial_keeps_remaining_position (confirmed present) still pass unmodified.

Cross-references: #65 (.expect on the same function), #66 (amount/lock-period asserts on the same function), #68 (pause guard on the same function) — all four should land as one coordinated change to unlock_assets. #69/#71/#72 (identical CEI pattern elsewhere, shared test infra).

[nazteeemba]

please assign me to this issue

[chonilius]

I can work on this