farming-pool: stake() and unstake() perform the token transfer before persisting UserStake changes

[prodbycorne]

Problem

Both stake and unstake perform the external token transfer before the UserStake record is persisted or removed:

pub fn stake(env: Env, from: Address, amount: i128) -> Result<(), PoolError> {
    // ... compute `new_stake` in memory ...
    new_stake.credit_rate = read_credit_rate(&env);

    let stake_token = get_stake_token(&env)?;
    token::TokenClient::new(&env, &stake_token).transfer(
        &from, &env.current_contract_address(), &amount,
    );                                    // <-- external call before persisting

    set_user_stake(&env, &from, &new_stake);
    Ok(())
}

pub fn unstake(env: Env, from: Address) -> Result<i128, PoolError> {
    let mut stake = get_user_stake(&env, &from).expect("no active stake");
    checkpoint(&env, &from, &mut stake);
    let total_credits = stake.credits_banked;

    let stake_token = get_stake_token(&env)?;
    token::TokenClient::new(&env, &stake_token).transfer(
        &env.current_contract_address(), &from, &stake.amount,
    );                                    // <-- external call before removing state

    remove_user_stake(&env, &from);
    Ok(total_credits)
}

Just as with lock_assets/unlock_assets (see companion issues), stake_token is an admin-supplied address, not guaranteed to be a passive Stellar Asset Contract. A transfer call to a non-standard token can re-enter FarmingPool before set_user_stake/remove_user_stake runs. In unstake specifically, that means a reentrant call sees the full, not-yet-removed UserStake.amount and could authorize a second unstake/emergency_withdraw payout for the same underlying tokens before the first call's remove_user_stake commits.

Impact

• Fund loss risk in unstake: reentrant double-payout of stake.amount before the record is cleared, if stake_token is a non-standard/malicious contract.
• State-consistency risk in stake: a reentrant read of get_user_stake/get_credits mid-call would observe the stale pre-deposit UserStake (or none, on first stake), inconsistent with the deposit that has already left the caller's wallet.

Fix

pub fn stake(env: Env, from: Address, amount: i128) -> Result<(), PoolError> {
    // ... compute `new_stake` ...
    new_stake.credit_rate = read_credit_rate(&env);
    set_user_stake(&env, &from, &new_stake);   // effects first

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

pub fn unstake(env: Env, from: Address) -> Result<i128, PoolError> {
    let mut stake = get_user_stake(&env, &from).ok_or(PoolError::NoActiveStake)?;
    checkpoint(&env, &from, &mut stake);
    let total_credits = stake.credits_banked;
    let amount = stake.amount;
    remove_user_stake(&env, &from);            // effects first

    let stake_token = get_stake_token(&env)?;
    token::TokenClient::new(&env, &stake_token).transfer(
        &env.current_contract_address(), &from, &amount,
    );
    Ok(total_credits)
}

Acceptance Criteria

• set_user_stake in stake is called before the external transfer.
• remove_user_stake in unstake is called before the external transfer.
• Add a reentrancy test using a mock malicious token contract that re-enters unstake/get_stake from within transfer, asserting the reentrant call cannot obtain a second payout and observes the already-cleared UserStake.
• Existing test_unstake_returns_tokens_and_credits, test_additional_stake_checkpoints_credits continue to pass.

Additional Notes

Confirmed against current source: stake (farming-pool/src/lib.rs:404-436) builds new_stake in memory (lines 412-423), transfers at lines 427-432, and only calls set_user_stake(&env, &from, &new_stake) at line 434. unstake (438-457) calls get_user_stake(&env, &from).expect("no active stake") at line 444, checkpoint at 445, transfers at lines 448-453, and only calls remove_user_stake(&env, &from) at line 455 — confirming both orderings exactly as described.

Edge cases:

• unstake's .expect("no active stake") at line 444 is the exact line farming-pool: unlock_assets and unstake panic via untyped .expect() instead of returning typed PoolError #65 targets for a typed-error fix (.ok_or(PoolError::NoActiveStake)?). This issue's own fix snippet (in the issue body) already shows that rewrite as part of its unstake sketch — meaning farming-pool: unlock_assets and unstake panic via untyped .expect() instead of returning typed PoolError #65 and farming-pool: stake() and unstake() perform the token transfer before persisting UserStake changes #71 both modify line 444 of the current file. Implement together; whichever PR lands second must rebase onto the other rather than blindly reapplying a stale line-444 patch.
• stake's pause/init guards run in the order assert!(!pool_is_paused) (line 406) then require_initialized (line 407) — the opposite order from lock_assets/unlock_assets/set_boost. This ordering quirk is called out in farming-pool: pause guard has no typed PoolError variant — every gated function panics via assert! on a paused pool #68's notes; the CEI reordering in this issue (moving set_user_stake earlier) is independent of that guard-ordering question and shouldn't need to touch lines 406-407 at all — confirm the CEI patch only touches the post-computation persist/transfer block (lines ~425-436), not the guards.
• stake's new set_user_stake (moved earlier) needs new_stake.credit_rate already assigned (currently line 425, right before the transfer) — the CEI fix must move the persist call to after line 425, not between the checkpoint (existing/else branch construction, 412-423) and the credit_rate assignment, or it would persist a stake missing its current credit-rate snapshot.
• Interacts with farming-pool: get_credits retroactively reprices a user's entire uncheckpointed accrual window when the admin changes the global multiplier #60: once UserStake gains a multiplier field (per farming-pool: get_credits retroactively reprices a user's entire uncheckpointed accrual window when the admin changes the global multiplier #60), both the stake-branch construction (lines 417-422, the else arm building a fresh UserStake) and the reordered set_user_stake call need to include/preserve that field — sequence farming-pool: get_credits retroactively reprices a user's entire uncheckpointed accrual window when the admin changes the global multiplier #60 before this issue, or rebase.
• Uses the same shared mock-reentrant-token infrastructure flagged in farming-pool: lock_assets calls token.transfer before persisting the updated Position #69.

Implementation sketch: in stake, move set_user_stake(&env, &from, &new_stake); (currently line 434) to immediately after new_stake.credit_rate = read_credit_rate(&env); (line 425) and before the get_stake_token/transfer block (427-432). In unstake, replace .expect("no active stake") (line 444) with .ok_or(PoolError::NoActiveStake)? (coordinating with #65), capture stake.amount into a local amount binding before calling remove_user_stake(&env, &from) (currently line 455) earlier — immediately after checkpoint/total_credits capture (line 446) — and before the transfer block (448-453), exactly as the issue's fix snippet shows.

Test plan: using the shared mock-reentrant-token helper (see #69), add test_stake_reentrant_transfer_observes_post_deposit_state and test_unstake_reentrant_transfer_cannot_double_payout, the latter specifically asserting a reentrant unstake/get_stake call inside the malicious token's transfer sees None/an already-cleared UserStake and cannot obtain a second payout. Confirm test_unstake_returns_tokens_and_credits and test_additional_stake_checkpoints_credits (confirmed present) still pass unmodified.

Cross-references: #65 (same line 444 in unstake — coordinate), #60 (struct-field addition affects the persist-timing fix here), #69/#70/#72 (identical CEI pattern, shared test infra).

[Achievers-sketch]

Experienced Soroban/Rust security developer. I’ll apply checks-effects-interactions, prevent reentrant stake/unstake exploits, add malicious-token tests, and preserve existing behavior and coverage.