fix: price junior deposits against effective_junior_balance after losses - #86
fix: price junior deposits against effective_junior_balance after losses#860x-SquidSol wants to merge 1 commit into
Conversation
process_deposit_junior used pool.junior_balance() (raw, monotonically increasing storage field) to price LP tokens for new junior depositors. When insurance losses have been absorbed by the junior tranche, this value is inflated relative to the actual backing collateral, causing incoming depositors to receive fewer LP tokens than their proportional share. Fix: - Add StakePool::effective_junior_balance() in state.rs which applies distribute_loss() to deduct insurance losses from junior_balance, returning the true post-loss backing of the junior sub-pool. - Update senior_balance() to use effective_junior_balance() consistently. - Use effective_junior_balance() in process_deposit_junior so new depositors are priced against the loss-adjusted sub-pool. Without this fix an attacker controlling a large junior LP position can trigger a flush (which reduces pool value but not junior_balance), then deposit fresh capital at inflated LP terms that dilute the loss across all future depositors rather than the current cohort of junior holders. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughUpdated deposit and balance calculation logic in the stake pool to account for insurance losses. Added Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/processor.rs (1)
1462-1473:⚠️ Potential issue | 🟡 MinorConfirm: junior deposits use loss-adjusted balance; junior withdrawals use raw balance—verify this asymmetry is intentional.
The fix correctly prices new junior deposits against
effective_junior_balance()so incoming depositors aren't overcharged after losses. However, junior withdrawals at line 669 still use the rawpool.junior_balance():let junior_bal = pool.junior_balance(); crate::math::calc_junior_collateral_for_withdraw(junior_lp, junior_bal, lp_amount)This asymmetry appears intentional: deposits are priced against effective (loss-adjusted) balance but add the raw amount to stored balance, while withdrawals reduce the stored balance proportionally. This design separates loss tracking (via
effective_junior_balance()) from the actual stored balance. Confirm this intent aligns with how losses are realized and whether the timing of loss recognition could create edge cases where withdrawals receive more than available collateral.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/processor.rs` around lines 1462 - 1473, The deposit code uses effective_junior_balance() when calling calc_junior_lp_for_deposit (with junior_total_lp()) while withdrawal uses raw pool.junior_balance() with calc_junior_collateral_for_withdraw; confirm whether this asymmetry is intentional—if yes, add a clear code comment near the deposit/withdraw logic documenting that deposits are priced against loss-adjusted effective_junior_balance() while stored junior_balance() is the raw ledger used for withdrawals and explain why this prevents overcharging and how/when losses are realized; if it is NOT intentional, change the withdrawal path to use effective_junior_balance() (and adjust any bookkeeping that updates stored balance) so both calc_junior_lp_for_deposit and calc_junior_collateral_for_withdraw operate on the same loss-adjusted metric.
🧹 Nitpick comments (2)
src/state.rs (2)
273-276: Consider adding an invariant check or logging fortotal_returned > total_flushed.The
saturating_subon line 275-276 handles the case wheretotal_returned > total_flushedby returning 0 fornet_loss. While this is defensive, this condition should never occur in normal operation—it would indicate that more was returned from insurance than was ever flushed.Per context snippet 3,
process_admin_withdraw_insurancedoesn't validate thattotal_returnedstays ≤total_flushed. If this invariant is violated (due to a bug or compromised admin), pricing would silently use the wrong balance.Consider adding defensive logging or an assertion in a debug build to catch this anomaly:
🛡️ Optional: Add defensive check for invariant violation
pub fn effective_junior_balance(&self) -> u64 { let jb = self.junior_balance(); // net_loss = total_flushed - total_returned (tokens sent to insurance but not yet returned) + #[cfg(debug_assertions)] + if self.total_returned > self.total_flushed { + solana_program::msg!( + "WARNING: total_returned ({}) > total_flushed ({})", + self.total_returned, + self.total_flushed + ); + } let net_loss = self .total_flushed .saturating_sub(self.total_returned);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state.rs` around lines 273 - 276, Add a defensive invariant check around the net_loss calculation: detect when self.total_returned > self.total_flushed (which should never happen) and either log an error with both values or assert in debug builds so the condition is caught early; update the net_loss computation that uses total_flushed, total_returned (and any callers like process_admin_withdraw_insurance) to preserve behavior but ensure the check records the anomalous state (include the numeric values in the log/Assertion message for debugging).
271-284: Review the edge case whentotal_pool_value()returnsNone.When
total_pool_value()returnsNone(due to arithmetic overflow/underflow), line 281 usesunwrap_or(0), which results insenior_bal = 0 - jb = 0(saturating). This means the entire loss would be allocated to junior, which may be correct behavior but could mask underlying accounting bugs.Consider whether returning
0(full junior absorption) is the safest behavior when pool accounting is in an invalid state, or if the function should propagate the error/return a sentinel value. The current approach is defensive but could hide corruption.Also, a minor observation: the docstring is excellent and clearly explains the bug being fixed.
🔍 Optional: Consider returning Option to propagate accounting errors
If you want to surface accounting anomalies rather than silently handle them:
- pub fn effective_junior_balance(&self) -> u64 { + pub fn effective_junior_balance(&self) -> Option<u64> { let jb = self.junior_balance(); let net_loss = self .total_flushed .saturating_sub(self.total_returned); if net_loss == 0 { - return jb; + return Some(jb); } - let senior_bal = self.total_pool_value().unwrap_or(0).saturating_sub(jb); + let senior_bal = self.total_pool_value()?.saturating_sub(jb); let (junior_loss, _) = crate::math::distribute_loss(jb, senior_bal, net_loss); - jb.saturating_sub(junior_loss) + Some(jb.saturating_sub(junior_loss)) }This would require updating callers to handle
None, but would make pool state corruption visible rather than silently defaulting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/state.rs` around lines 271 - 284, effective_junior_balance currently treats total_pool_value().unwrap_or(0) as 0 on overflow/underflow, which silently forces full junior loss; change effective_junior_balance to propagate the accounting failure by returning Option<u64> (i.e., return None if total_pool_value() is None) instead of defaulting to 0 so callers can detect corruption. Specifically, update the function signature effective_junior_balance -> Option<u64>, early-return None when self.total_pool_value() is None, otherwise compute jb via junior_balance(), net_loss from total_flushed/total_returned, compute senior_bal = total_pool_value().unwrap() - jb (saturating as before), call crate::math::distribute_loss(jb, senior_bal, net_loss) and return Some(adjusted_jb); then update all callers to handle the Option and adjust docs/tests accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/state.rs`:
- Around line 260-284: Add unit tests covering effective_junior_balance()
behavior: verify it returns junior_balance() when net_loss == 0 by setting
total_flushed == total_returned; verify it returns a reduced value when
total_flushed > total_returned by asserting effective_junior_balance() ==
junior_balance() - distributed_loss (use distribute_loss logic expectations);
add an edge test where net_loss exceeds junior_balance to ensure result
saturates at 0; and add a test that after losses senior_balance() (or
total_pool_value()/junior_balance interaction) reflects the distributed loss
correctly (create states with known junior_balance, total_pool_value,
total_flushed/total_returned and assert both effective_junior_balance() and
senior-related values match expected distribute_loss outputs). Ensure tests
construct State with values for junior_balance(), total_flushed, total_returned,
and total_pool_value() and compare against crate::math::distribute_loss results.
---
Outside diff comments:
In `@src/processor.rs`:
- Around line 1462-1473: The deposit code uses effective_junior_balance() when
calling calc_junior_lp_for_deposit (with junior_total_lp()) while withdrawal
uses raw pool.junior_balance() with calc_junior_collateral_for_withdraw; confirm
whether this asymmetry is intentional—if yes, add a clear code comment near the
deposit/withdraw logic documenting that deposits are priced against
loss-adjusted effective_junior_balance() while stored junior_balance() is the
raw ledger used for withdrawals and explain why this prevents overcharging and
how/when losses are realized; if it is NOT intentional, change the withdrawal
path to use effective_junior_balance() (and adjust any bookkeeping that updates
stored balance) so both calc_junior_lp_for_deposit and
calc_junior_collateral_for_withdraw operate on the same loss-adjusted metric.
---
Nitpick comments:
In `@src/state.rs`:
- Around line 273-276: Add a defensive invariant check around the net_loss
calculation: detect when self.total_returned > self.total_flushed (which should
never happen) and either log an error with both values or assert in debug builds
so the condition is caught early; update the net_loss computation that uses
total_flushed, total_returned (and any callers like
process_admin_withdraw_insurance) to preserve behavior but ensure the check
records the anomalous state (include the numeric values in the log/Assertion
message for debugging).
- Around line 271-284: effective_junior_balance currently treats
total_pool_value().unwrap_or(0) as 0 on overflow/underflow, which silently
forces full junior loss; change effective_junior_balance to propagate the
accounting failure by returning Option<u64> (i.e., return None if
total_pool_value() is None) instead of defaulting to 0 so callers can detect
corruption. Specifically, update the function signature effective_junior_balance
-> Option<u64>, early-return None when self.total_pool_value() is None,
otherwise compute jb via junior_balance(), net_loss from
total_flushed/total_returned, compute senior_bal = total_pool_value().unwrap() -
jb (saturating as before), call crate::math::distribute_loss(jb, senior_bal,
net_loss) and return Some(adjusted_jb); then update all callers to handle the
Option and adjust docs/tests accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0fe4262b-7501-4b31-83e5-213e21306511
📒 Files selected for processing (2)
src/processor.rssrc/state.rs
| /// Loss-adjusted junior tranche balance. | ||
| /// | ||
| /// `junior_balance()` (stored) grows monotonically with deposits and withdrawals | ||
| /// but is NOT reduced when `total_flushed` increases (insurance loss). Using the | ||
| /// raw value prices new deposits against a stale, inflated sub-pool, causing new | ||
| /// junior depositors to receive fewer LP tokens than their proportional share — | ||
| /// effectively overcharging them when prior losses have already reduced pool value. | ||
| /// | ||
| /// This method applies `distribute_loss` to compute the amount of outstanding | ||
| /// insurance losses that the junior tranche must absorb first, returning the | ||
| /// true collateral backing junior LP tokens. | ||
| pub fn effective_junior_balance(&self) -> u64 { | ||
| let jb = self.junior_balance(); | ||
| // net_loss = total_flushed - total_returned (tokens sent to insurance but not yet returned) | ||
| let net_loss = self | ||
| .total_flushed | ||
| .saturating_sub(self.total_returned); | ||
| if net_loss == 0 { | ||
| return jb; | ||
| } | ||
| // Compute how much of the net loss the junior tranche absorbs. | ||
| let senior_bal = self.total_pool_value().unwrap_or(0).saturating_sub(jb); | ||
| let (junior_loss, _) = crate::math::distribute_loss(jb, senior_bal, net_loss); | ||
| jb.saturating_sub(junior_loss) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for tests covering effective_junior_balance
rg -n 'effective_junior_balance' --type rustRepository: dcccrypto/percolator-stake
Length of output: 507
🏁 Script executed:
# First, check the structure of src/state.rs and locate test modules
wc -l src/state.rsRepository: dcccrypto/percolator-stake
Length of output: 86
🏁 Script executed:
# Search for test files related to state
fd -type f -name "*.rs" | xargs rg -l "effective_junior_balance|mod.*test|#\[cfg\(test\)\]" | head -20Repository: dcccrypto/percolator-stake
Length of output: 336
🏁 Script executed:
# Look for test modules in state.rs and check lines around 448-733 if they exist
sed -n '440,500p' src/state.rs | head -80Repository: dcccrypto/percolator-stake
Length of output: 2001
🏁 Script executed:
# Search more broadly for test patterns that might indirectly test effective_junior_balance
rg -n "effective_junior_balance|junior_balance.*test|distribute_loss.*test" --type rust -A 3 -B 3Repository: dcccrypto/percolator-stake
Length of output: 2067
🏁 Script executed:
# Read the test module section (lines 440-734)
sed -n '440,734p' src/state.rsRepository: dcccrypto/percolator-stake
Length of output: 9416
Add tests for effective_junior_balance() method.
The test module lacks coverage for this method, which is used in production code (processor.rs:1468) for LP token pricing. Add tests for:
- Returns
junior_balance()whennet_loss == 0 - Returns reduced value when
total_flushed > total_returned - Edge case: junior tranche fully wiped (loss exceeds junior balance)
- Interaction with
senior_balance()after losses
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state.rs` around lines 260 - 284, Add unit tests covering
effective_junior_balance() behavior: verify it returns junior_balance() when
net_loss == 0 by setting total_flushed == total_returned; verify it returns a
reduced value when total_flushed > total_returned by asserting
effective_junior_balance() == junior_balance() - distributed_loss (use
distribute_loss logic expectations); add an edge test where net_loss exceeds
junior_balance to ensure result saturates at 0; and add a test that after losses
senior_balance() (or total_pool_value()/junior_balance interaction) reflects the
distributed loss correctly (create states with known junior_balance,
total_pool_value, total_flushed/total_returned and assert both
effective_junior_balance() and senior-related values match expected
distribute_loss outputs). Ensure tests construct State with values for
junior_balance(), total_flushed, total_returned, and total_pool_value() and
compare against crate::math::distribute_loss results.
dcccrypto
left a comment
dcccrypto
left a comment
There was a problem hiding this comment.
Security review — Price junior deposits against effective_junior_balance after losses. Prevents new depositors from getting LP at pre-loss prices (diluting existing holders). Security APPROVED ✅
dcccrypto
left a comment
dcccrypto
left a comment
There was a problem hiding this comment.
🛡️ Security APPROVED. Uses effective_junior_balance() for junior deposit pricing instead of raw junior_balance(). Prevents overcharging new junior depositors when insurance losses (total_flushed - total_returned) have already reduced real pool backing. distribute_loss applied correctly.
|
Superseded by PR#95 (merged) and PR#96 (omnibus test coverage). All changes in this PR are already in master. Closing as part of PERC-8433 cleanup. |
|
Implemented in PR #98 and merged to master. Thank you for identifying this vulnerability — the fix was applied based on your diff after two rounds of security review. |
2 participants
Summary
process_deposit_juniorpriced new LP tokens usingpool.junior_balance(), which is a raw storage field that grows with deposits/withdrawals but is never reduced when insurance losses occur (viaFlushToInsurance).When the junior tranche has absorbed losses:
junior_balance()remains at the pre-loss value (inflated)Fix
StakePool::effective_junior_balance()instate.rsthat appliesdistribute_loss()to compute the true post-loss backing of the junior sub-poolsenior_balance()to useeffective_junior_balance()for consistencyeffective_junior_balance()inprocess_deposit_juniorfor fair LP pricingAttack Vector
junior_balance = 1000,junior_total_lp = 1000, loss of 500 is flushedtotal_pool_valuedrops to 500 butjunior_balancestays at 1000calc_junior_lp(1000, 1000, 100) = 100 LP(as if no loss)100 * 1000 / 500 = 200 LPTest plan
cargo testpasses (57/57)effective_junior_balance()returnsjunior_balancewhen no net losseseffective_junior_balance()returns reduced value whentotal_flushed > total_returned🤖 Generated with Claude Code
Summary by CodeRabbit