fix(#169): compute total_pool_value() in i128 — stop mode-1 fee-withdraw underflow that permanently bricks trading pools

[0x-SquidSol]

Summary

Fixes #169. In a mode-1 (trading-LP) pool, total_pool_value() underflowed to None once cumulative withdrawals exceeded cumulative deposits, permanently bricking the pool — every caller does .ok_or(StakeError::Overflow)?, so deposits, withdrawals, and AccrueFees all revert and remaining LPs' funds are trapped. This is reachable in normal operation of any profitable trading pool (no attacker), because withdrawal payouts are fee-inclusive while total_deposited tracks principal only.

Root cause

total_pool_value() subtracted total_withdrawn from total_deposited first and added total_fees_earned last:

let base = total_deposited.checked_sub(total_withdrawn)?  // underflows when withdrawn > deposited
    .checked_sub(total_flushed)?.checked_add(total_returned)?;
if pool_mode == 1 { base.checked_add(total_fees_earned) } else { Some(base) }

A withdrawal is priced against the fee-inclusive TPV, so total_withdrawn += withdrawal_amount accumulates principal plus fees, while total_deposited is principal only. Once payouts exceed principal, the intermediate checked_sub underflows → None, even though the final value (after adding fees) is non-negative and correct.

Fix

Accumulate credits − debits in a wide signed intermediate (i128), then range-check the final net to u64:

let fees = if self.pool_mode == 1 { self.total_fees_earned as i128 } else { 0 };
let value = self.total_deposited as i128 + self.total_returned as i128 + fees
    - self.total_withdrawn as i128
    - self.total_flushed as i128;
if (0..=u64::MAX as i128).contains(&value) { Some(value as u64) } else { None }

i128 holds the sum/difference of five u64 counters with ~60 bits to spare (|value| < 5·2^64 < 2^67 ≪ 2^127), so no intermediate can spuriously under- or over-flow. The only path to None is now the explicit final check: net < 0 (genuine insolvency / over-flush — the over-withdraw guard callers rely on) or net > u64::MAX.

Why i128 over a u64 "credits-first" reorder

All three solution agents independently reached the same conclusion: a checked_add-credits-then-checked_sub-debits reorder fixes the reported underflow but introduces a residual spurious-None — on a long-lived pool the credit sum (deposited + returned + fees, all lifetime-cumulative) can overflow u64 even when the net value is small, relocating the brick rather than eliminating it. The i128 form removes the entire spurious-None class.

Behavior preserved

• mode-0 byte-identical to deposited − withdrawn − flushed + returned (fees ignored unless pool_mode == 1).
• Genuine insolvency / over-flush still returns None (the only behavioral change is converting the spurious mode-1 None into the correct Some).
• Some(0) at exact zero; the u64::MAX boundary is inclusive.

Tests

tests/poc_mode1_fee_withdraw_underflow.rs:

• mode1_fee_inclusive_withdraw_does_not_brick_pool — the fee-inclusive withdraw yields Some(250), not the pre-fix None.
• mode1_tpv_reads_zero_not_none_when_emptied_via_fees — an emptied-via-fees pool reads Some(0).
• mode0_total_pool_value_unchanged_and_insolvency_still_none — mode-0 unchanged, and a genuine over-flush still returns None.

Verification

• cargo build --lib ✔
• cargo build-sbf ✔ (on-chain target)
• Scratch runner confirms total_pool_value() returns Some(250) (was None) after the fee-inclusive withdrawal, and every other tranche/HWM/pricing scenario — all of which route through total_pool_value() — still passes, confirming no regression.

Note on scope

This is the mode-1 (trading-LP) path. The fix is correct and harmless regardless, but the real-world severity is CRITICAL if trading pools are deployed and merely latent if only mode-0 (insurance) pools are live — worth confirming on your side which modes are in production.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes

  • Fixed stake pool value calculation that caused trading mode pools to become permanently unresponsive when fee-inclusive withdrawals exceeded principal deposits, while maintaining standard mode behavior.

• Tests

  • Added test suite validating correct pool value calculations across trading and standard modes, including edge cases with fees, full pool depletion, and insolvency scenarios.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 6a4cfa29-49bf-4a03-860e-63b0d95641d9

📥 Commits

Reviewing files that changed from the base of the PR and between 3b5679d and 4943a47.

📒 Files selected for processing (2)

• src/state.rs
• tests/poc_mode1_fee_withdraw_underflow.rs

---

📝 Walkthrough

Walkthrough

StakePool::total_pool_value() is rewritten to accumulate all values into a single i128 signed intermediate—crediting deposits, returns, and (for mode-1) accrued fees before subtracting withdrawals and flushes—and returns Some(v as u64) only after a final range check. A new test file adds three regression tests covering mode-1 partial withdrawal, mode-1 full drain, and mode-0 unchanged semantics.

Changes

Mode-1 total_pool_value() underflow fix and regression tests

Layer / File(s)	Summary
i128 signed intermediate in total_pool_value() src/state.rs	Replaces the checked_sub/checked_add chain (lines 454–483) with a single i128 signed accumulation. Credits (total_deposited, total_returned, total_fees_earned for mode-1) are summed before debits (total_withdrawn, total_flushed) are subtracted. Returns Some(v as u64) only when the final signed value is within 0..=u64::MAX, otherwise None.
Regression tests: mode-1 and mode-0 tests/poc_mode1_fee_withdraw_underflow.rs	Adds a trading_pool() helper to construct a mode-1 StakePool. Three tests cover: (1) fee-inclusive partial withdrawal returning Some(250) not None; (2) full drain returning Some(0) not None; (3) mode-0 preserving correct arithmetic and still returning None for genuine insolvency.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• #169 (linked): This PR directly implements the fix described in the issue—replacing the intermediate-underflowing checked_sub chain with a signed i128 net computation, and includes the exact regression test from the issue's proof-of-concept.
• calc_lp_for_deposit silently treats accounting-corrupt pool as empty (unwrap_or(0) vs ?) percolator-vault#7: The fix prevents total_pool_value() from returning None inappropriately in mode-1, which removes the precondition that triggered the unwrap_or(0) silent fallback concern raised in that issue.
• Deposit cap enforcement includes accrued fees in TVL basis, causing unintended deposit lockout on trading pools #154: Both involve the fee-inclusive behavior of total_pool_value() in trading mode; this PR's change to include total_fees_earned in the signed net directly affects deposit-cap calculations discussed in that issue.

Possibly related PRs

• dcccrypto/percolator-stake#112: That PR propagates None from total_pool_value() inside calc_lp_for_deposit(); this PR fixes the root cause that produces spurious None returns in mode-1, making the two PRs tightly coupled at src/state.rs.

Poem

🐇 Hoppity-hop through the ledger I go,
No more None from a negative flow!
With i128 wide as a meadow in spring,
Fees and withdrawals no longer sting.
The pool stays solvent, the LPs rejoice —
Signed arithmetic: the rabbit's best choice! 🌸

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately and specifically describes the main change: fixing a critical mode-1 pool underflow bug in total_pool_value() via i128 arithmetic, which is precisely what the changeset implements.
Description check	✅ Passed	The description thoroughly covers the summary, root cause, fix details, behavior preservation, and comprehensive testing, but the checklist section is incomplete—no checkmarks indicate whether cargo test, clippy, format, or Kani verification have been run.
Linked Issues check	✅ Passed	The PR fully addresses issue #169 requirements: eliminates the spurious underflow in mode-1 pools via i128 computation, preserves genuine insolvency detection, maintains mode-0 byte-identity, and includes comprehensive regression tests validating all three scenarios.
Out of Scope Changes check	✅ Passed	All changes are strictly within scope: src/state.rs fixes the total_pool_value() function per #169, and tests/poc_mode1_fee_withdraw_underflow.rs validates the fix with the regression tests specified in the issue. No unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[dcccrypto]

Thanks @0x-SquidSol — the underlying bug is real (mode-1 total_pool_value() can underflow when fee-inclusive total_withdrawn exceeds total_deposited, bricking trading pools), and summing in a wide signed i128 intermediate is the right idea. But this needs changes before it can land:

1. It would silently revert Design: last junior exiting during an outstanding loss windfalls the recovery to senior (junior-withdraw side of #145) #161. This branch is cut from a pre-Design: last junior exiting during an outstanding loss windfalls the recovery to senior (junior-withdraw side of #145) #161 base, and the rewritten total_pool_value() omits the - realized_junior_loss() term that main now has (added by Design: last junior exiting during an outstanding loss windfalls the recovery to senior (junior-withdraw side of #145) #161/fix(stake): #161 fair recovery — realize forfeited junior loss at last-junior exit #173). Resolving the conflict by taking this PR's version would reintroduce the senior recovery-snipe windfall. Please rebase onto current main and re-fold realized_junior_loss() into the i128 sum.
2. Apply the same fix to principal_tvl() — it has the twin subtraction and the same underflow shape; the two must stay consistent.
3. The PR is currently CONFLICTING/DIRTY against main.

Rebase + keep both the RL term and the i128 widening and this is good — happy to re-review.

[dcccrypto]

Superseded by the merged fix in #187. Your i128 insight was the right call, @0x-SquidSol — I reimplemented it on current main so it keeps the post-#161 - realized_junior_loss() term (this branch's pre-#161 base dropped it, which would have reverted the #161 fair-recovery fix) and also fixes the twin principal_tvl() underflow that bricks deposits. Verified with a Kani proof + 5 regression tests. Thanks for finding this — credited in the commit. Closing as merged-equivalent.