fix: junior tranche absorbs insurance losses first via effective_junior_balance() - #77
Conversation
…or_balance() distribute_loss() in math.rs was defined but never called, meaning the junior-first-loss guarantee of the tranche system was unimplemented. When total_flushed > total_returned (insurance was tapped), junior_balance was not adjusted, so losses fell on the senior tranche instead of junior. Fix: - Add StakePool::effective_junior_balance() which deducts realized insurance losses from junior_balance using distribute_loss(), matching the intended junior-absorbs-first semantics. - Update senior_balance() to use effective_junior_balance() so senior LP holders are protected from losses that junior can absorb. - Use effective_junior_balance() in process_withdraw for junior tranche withdrawals so payouts correctly reflect the loss-adjusted sub-pool. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes introduce a loss-absorption mechanism for the junior tranche. A new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ 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: 2
🤖 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/processor.rs`:
- Around line 667-672: The withdraw pricing uses effective_junior_balance()
while deposits use junior_balance(), causing asymmetric share pricing; update
the deposit path to use effective_junior_balance() (or propagate Option if
effective_junior_balance() becomes Option in src/state.rs) so both minting and
withdrawal use the same valuation; specifically replace usages of
junior_balance() in deposit/mint logic with effective_junior_balance() (or
handle the Option and fail-closed per the state change) and ensure
calc_junior_collateral_for_withdraw continues to receive the same junior balance
value type.
In `@src/state.rs`:
- Around line 266-277: The method effective_junior_balance currently masks an
invalid pool accounting by using unwrap_or(jb) on total_pool_value(), which
silently computes a synthetic pool value; change it to fail closed by not
falling back—if self.total_pool_value() is None, return an explicit failure
(e.g. panic via expect with a clear message or otherwise propagate an error) so
callers immediately see invalid accounting; update the code in
effective_junior_balance to call self.total_pool_value().expect("invalid pool
accounting: total_pool_value missing") (or propagate a Result) before computing
sb and calling crate::math::distribute_loss.
🪄 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: 55c51560-cc3e-4299-bcc0-7c1f761cf71f
📒 Files selected for processing (2)
src/processor.rssrc/state.rs
| // Junior withdrawal: valued against junior sub-pool after loss absorption. | ||
| // effective_junior_balance() deducts insurance losses that junior absorbs first, | ||
| // so junior LP holders correctly receive a reduced payout when the pool lost funds. | ||
| let junior_lp = pool.junior_total_lp(); | ||
| let junior_bal = pool.junior_balance(); | ||
| let junior_bal = pool.effective_junior_balance(); | ||
| crate::math::calc_junior_collateral_for_withdraw(junior_lp, junior_bal, lp_amount) |
There was a problem hiding this comment.
Junior tranche valuation is now asymmetric across deposit vs withdraw.
Line 671 prices junior withdrawals with effective_junior_balance(), but junior deposits still mint against raw junior_balance() (Line 1465). After a loss, this creates inconsistent share pricing between entry and exit.
Proposed fix
- let junior_bal = pool.junior_balance();
+ let junior_bal = pool.effective_junior_balance()
+ .ok_or(StakeError::Overflow)?;If you keep effective_junior_balance() -> u64, use it directly; if you apply the fail-closed change in src/state.rs, propagate Option here as above.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/processor.rs` around lines 667 - 672, The withdraw pricing uses
effective_junior_balance() while deposits use junior_balance(), causing
asymmetric share pricing; update the deposit path to use
effective_junior_balance() (or propagate Option if effective_junior_balance()
becomes Option in src/state.rs) so both minting and withdrawal use the same
valuation; specifically replace usages of junior_balance() in deposit/mint logic
with effective_junior_balance() (or handle the Option and fail-closed per the
state change) and ensure calc_junior_collateral_for_withdraw continues to
receive the same junior balance value type.
| pub fn effective_junior_balance(&self) -> u64 { | ||
| let jb = self.junior_balance(); | ||
| let loss = self.total_flushed.saturating_sub(self.total_returned); | ||
| if loss == 0 { | ||
| return jb; | ||
| } | ||
| // senior_balance is the pool value not attributed to junior — used by | ||
| // distribute_loss to cap the allocation of loss to the junior tranche. | ||
| let pv = self.total_pool_value().unwrap_or(jb); | ||
| let sb = pv.saturating_sub(jb); | ||
| let (junior_loss, _) = crate::math::distribute_loss(jb, sb, loss); | ||
| jb.saturating_sub(junior_loss) |
There was a problem hiding this comment.
Fail closed when total_pool_value() is invalid.
Line 274 falls back with unwrap_or(jb). If pool accounting is already invalid (None), this silently prices junior state with a synthetic value instead of rejecting the read path.
Proposed fix
- pub fn effective_junior_balance(&self) -> u64 {
+ pub fn effective_junior_balance(&self) -> Option<u64> {
let jb = self.junior_balance();
let loss = self.total_flushed.saturating_sub(self.total_returned);
if loss == 0 {
- return jb;
+ return Some(jb);
}
@@
- let pv = self.total_pool_value().unwrap_or(jb);
+ let pv = self.total_pool_value()?;
let sb = pv.saturating_sub(jb);
let (junior_loss, _) = crate::math::distribute_loss(jb, sb, loss);
- jb.saturating_sub(junior_loss)
+ Some(jb.saturating_sub(junior_loss))
}- pub fn senior_balance(&self) -> Option<u64> {
- let pv = self.total_pool_value()?;
- Some(pv.saturating_sub(self.effective_junior_balance()))
- }
+ pub fn senior_balance(&self) -> Option<u64> {
+ let pv = self.total_pool_value()?;
+ let ejb = self.effective_junior_balance()?;
+ Some(pv.saturating_sub(ejb))
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub fn effective_junior_balance(&self) -> u64 { | |
| let jb = self.junior_balance(); | |
| let loss = self.total_flushed.saturating_sub(self.total_returned); | |
| if loss == 0 { | |
| return jb; | |
| } | |
| // senior_balance is the pool value not attributed to junior — used by | |
| // distribute_loss to cap the allocation of loss to the junior tranche. | |
| let pv = self.total_pool_value().unwrap_or(jb); | |
| let sb = pv.saturating_sub(jb); | |
| let (junior_loss, _) = crate::math::distribute_loss(jb, sb, loss); | |
| jb.saturating_sub(junior_loss) | |
| pub fn effective_junior_balance(&self) -> Option<u64> { | |
| let jb = self.junior_balance(); | |
| let loss = self.total_flushed.saturating_sub(self.total_returned); | |
| if loss == 0 { | |
| return Some(jb); | |
| } | |
| // senior_balance is the pool value not attributed to junior — used by | |
| // distribute_loss to cap the allocation of loss to the junior tranche. | |
| let pv = self.total_pool_value()?; | |
| let sb = pv.saturating_sub(jb); | |
| let (junior_loss, _) = crate::math::distribute_loss(jb, sb, loss); | |
| Some(jb.saturating_sub(junior_loss)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/state.rs` around lines 266 - 277, The method effective_junior_balance
currently masks an invalid pool accounting by using unwrap_or(jb) on
total_pool_value(), which silently computes a synthetic pool value; change it to
fail closed by not falling back—if self.total_pool_value() is None, return an
explicit failure (e.g. panic via expect with a clear message or otherwise
propagate an error) so callers immediately see invalid accounting; update the
code in effective_junior_balance to call self.total_pool_value().expect("invalid
pool accounting: total_pool_value missing") (or propagate a Result) before
computing sb and calling crate::math::distribute_loss.
dcccrypto
left a comment
dcccrypto
left a comment
There was a problem hiding this comment.
Security review — Junior tranche loss absorption. Ensures junior_balance absorbs insurance losses first via effective_junior_balance(). Prevents senior depositors from bearing junior losses. Security APPROVED ✅
|
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
Problem
distribute_loss()inmath.rswas defined but never called. The junior-tranche first-loss guarantee was a dead letter: whentotal_flushed > total_returned(the insurance fund was tapped by a market loss),junior_balancewas never adjusted. As a result, losses fell on the senior tranche instead of junior — the exact opposite of the intended design.Why it's wrong without the fix
With the fix:
| junior (correct) | 100 (absorbed the 200 loss) |
| senior (correct) | 800 - 100 = 700 (protected) |
Fix
StakePool::effective_junior_balance()— deducts realized insurance losses fromjunior_balanceusingdistribute_loss(), implementing junior-absorbs-first semantics without new stored state.senior_balance()to useeffective_junior_balance()for correct senior protection.effective_junior_balance()inprocess_withdrawfor junior tranche payouts.This also resolves the dead-code concern for
distribute_loss— it is now called on every junior/senior balance read.Tests
cargo test— all 57 unit tests pass.Summary by CodeRabbit