fix(fees): crystallize pending trading fees before pricing deposits/withdraws - #137
Closed
0x-SquidSol wants to merge 5 commits into
Closed
fix(fees): crystallize pending trading fees before pricing deposits/withdraws#1370x-SquidSol wants to merge 5 commits into
0x-SquidSol wants to merge 5 commits into
Conversation
dcccrypto#130) effective_junior_balance is used in every junior deposit (process_deposit_junior) for LP pricing and every junior withdrawal (process_withdraw) for collateral valuation. It composes the already-Kani-proven distribute_loss with a derivation of gross_senior from raw accounting fields. The wrapper composition itself — the net_loss == 0 short-circuit, the gross_senior derivation, and the final saturating_sub — had no formal verification, despite the historical BUG-6 fix where losses were previously double-applied on the junior side. Added 4 Kani proofs to tests/kani.rs: 1. proof_effective_junior_balance_no_panic Panic-freedom across arbitrary accounting states, including pathological cases (withdrawn > deposited, flushed > returned). 2. proof_effective_junior_balance_bounded_by_raw effective_junior_balance() <= junior_balance() for all inputs. Loss adjustment can only decrease, never inflate. Includes kani::cover! guard proving the loss-application branch is reachable beyond the trivial net_loss == 0 short-circuit. 3. proof_effective_junior_balance_no_loss_identity When total_flushed == total_returned (no outstanding loss), effective == junior_balance exactly. Pins the short-circuit against regression that would silently apply non-zero loss on lossless pools. 4. proof_effective_junior_tranche_conservation effective_junior + senior_balance == total_pool_value when both are well-defined. Regression guard in case senior_balance()'s definition ever changes away from being the tpv residual. All proofs use the established 1e9 bound for CBMC tractability, matching the style of the surrounding distribute_loss and distribute_fees proofs. All are inside the #[cfg(kani)] module — no effect on normal builds. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…to#129) .cargo/audit.toml ignores RUSTSEC-2022-0093 (ed25519-dalek 1.x) and RUSTSEC-2024-0344 (curve25519-dalek 3.x) with the rationale that these crates are dev-only and never reach the BPF binary. The dependency chain (solana-keypair -> ed25519-dalek-bip32 -> dalek 1.x) is reachable only via solana-sdk which lives under [dev-dependencies]. But no CI check mechanically verifies the "dev-only" claim. If a future dep change pulled either crate into the normal dep tree, the BPF binary would ship with known-vulnerable code and CI would say nothing — the ignore rules would silently become unsafe. Added .github/workflows/audit.yml with a bpf-tree-shake job that: 1. Runs cargo tree -e normal --no-default-features (excludes dev-dependencies and build-dependencies, matching the on-chain build's actual dep surface) 2. Greps the result for ed25519-dalek v1.x and curve25519-dalek v3.x 3. Fails if either appears, with a clear error pointing to the offending audit.toml ignore Verified locally: grep patterns are version-specific — curve25519-dalek v4.1.3 is legitimately present in the normal tree via solana-program, but only v3.x has the ignored CVE, so no false positive. Runs on PR, master push, weekly schedule (catches silent regressions). NOTE: the companion `cargo audit` step (tracked separately) is blocked by upstream cargo-audit 0.21.x lacking CVSS 4.0 parser support. Will be added when a fixed cargo-audit is released to crates.io. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
process_admin_set_tranche_config allowed the admin to change junior_fee_mult_bps at any time, even while junior LPs were deposited. Since process_accrue_fees reads the multiplier live (no per-epoch snapshot), a mid-life change immediately rewrites the fee split for every junior LP. Attack path: 1. Juniors deposit at multiplier M_1 2. Vault accumulates trading fees 3. Admin bumps multiplier to M_2 > M_1 4. Anyone calls AccrueFees — junior sub-pool captures outsized share 5. Admin (holding a junior position) withdraws at inflated value 6. Admin resets multiplier to M_1 to cover tracks The inverse also works: admin depresses the multiplier to silently reduce junior yield below what depositors were promised at deposit time. Fix: block any multiplier change when junior_total_lp() > 0. Idempotent re-writes (same value) still succeed so admin tooling can reapply config. Once all juniors withdraw (junior_total_lp back to 0), the multiplier is freely configurable for the next cohort. The "no disable" issue (set_tranche_enabled always hardcoded true) is left as-is — it's a design limitation, but because the current code cannot disable tranches, the related "disable mid-flight" attack surface that Agents A/C raised does not exist in practice. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…cccrypto#126) process_admin_set_insurance_policy forwarded a caller-supplied authority: Pubkey to wrapper Tag 22 without any validation. A malicious admin could set authority = their_wallet_key, causing the wrapper to record their wallet as the policy authority. They could then bypass the stake program entirely by calling the wrapper's WithdrawInsuranceLimited directly (Tag 23), signing as themselves. Funds extract to the attacker while pool.total_returned is NEVER incremented — process_admin_withdraw_insurance (the only path that updates total_returned) is skipped entirely. This silently desyncs total_pool_value() from the vault's actual balance. LP holders see no on-chain anomaly until they try to redeem against a pool whose accounting claims the funds are still in insurance, but the tokens are in the attacker's wallet. Fix: derive the expected vault_auth PDA and require the caller-supplied authority to equal it. This mirrors the exact check already present in process_admin_withdraw_insurance (lines 1418-1422). Non-breaking ABI change — the parameter is retained, just now validated. The vault_auth PDA is the ONLY signer the stake program can produce for WithdrawInsuranceLimited, so any other authority value breaks the program's own withdrawal path while enabling the bypass. There is no legitimate alternate value. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ithdraws On trading-LP (mode 1) pools, engine-paid trading fees sit in the vault as an un-accrued surplus (current_balance > total_pool_value()) until the permissionless AccrueFees folds them into total_fees_earned, lifting LP share price for all holders. process_deposit priced new LP against total_pool_value() WITHOUT crystallizing the pending surplus first, so a depositor could buy LP at the stale pre-fee price right before accrual (or self-trigger AccrueFees in the same tx) and capture a pro-rata share of fees earned before they joined, diluting the LPs who earned them. Extract the accrual core of process_accrue_fees into accrue_fees_inner(pool, current_balance) and call it (mode-1 gated) at the start of process_deposit, process_deposit_junior, and process_withdraw — reading the vault balance BEFORE the token transfer so only the fee surplus, not the operation's own collateral, is folded. The pre-increment tranche snapshot and the total_lp_supply > 0 first-depositor guard are preserved verbatim, so the junior/senior fee split and the anti-brick bootstrap are unchanged. Withdraw pre-accrues too, so an exiting LP realizes its fair share of earned fees and the HWM floor sees true TVL. No state-layout change, no ABI change, no new error variants. Adds tests/poc_jit_fee_snipe.rs (snipe profitable under stale pricing; neutralized when fees are crystallized first). Refs dcccrypto#136 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 29 minutes and 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
✨ 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 |
Owner
|
Integrated into |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the JIT fee-snipe reported in #136.
On trading-LP (
pool_mode == 1) pools, engine-paid trading fees sit in the vault as an un-accrued surplus (current_balance > total_pool_value()) until the permissionlessAccrueFeesfolds them intototal_fees_earned, lifting LP share price for all holders.process_depositpriced newly-minted LP againsttotal_pool_value()(which excludes the surplus) without crystallizing it first, so a depositor could buy LP at the stale pre-fee price right before accrual (or self-triggerAccrueFeesin the same tx) and capture a pro-rata share of fees earned before they joined — diluting the LPs who earned them.Change
process_accrue_feesintoaccrue_fees_inner(pool, current_balance)(guard + pre-increment tranche snapshot +distribute_fees+total_fees_earnedupdate).process_accrue_feesnow verifies the vault and calls the helper (behavior-preserving).accrue_fees_inner(mode-1 gated) at the start ofprocess_deposit,process_deposit_junior, andprocess_withdraw, reading the vault balance before the token transfer — so only the fee surplus, not the operation's own collateral, is folded. The deposit/withdraw then prices against the crystallized, fee-inclusive share price.Safety / blast radius
processor.rs.fee_delta = current_balance − total_pool_value()reflects only pending fees (never the deposit/withdrawal itself).total_lp_supply > 0guard makes the helper a no-op at zero supply, so the 1:1 bootstrap and the anti-brick protection are unchanged.distribute_feesis unchanged whether invoked fromAccrueFeesor a deposit.vault.owner == spl_token::id()check before unpacking in the deposit/withdraw paths (these previously relied on the SPL transfer to validate the vault) — strictly more validation.Tests
tests/poc_jit_fee_snipe.rs:jit_fee_snipe_is_profitable_with_current_pricing(documents the bug — Eve +500,000 of a 1,000,000 fee batch, stolen from the honest sole LP) andcrystallizing_fees_before_pricing_prevents_snipe(the fix — Eve profit 0, honest LP keeps the full 2,000,000).cargo build-sbf(deployment target) green; fix reproduced and neutralized against the compiled lib.Refs #136
🤖 Generated with Claude Code