fix: performance fee debt-carry write-off (hold reference unless crystallized)#202
fix: performance fee debt-carry write-off (hold reference unless crystallized)#202maxencerb wants to merge 4 commits into
Conversation
The performance reference (lastTotalAssets/lastDebt) was advanced to the current state on every accrual and every flow tail. Under a periodically repriced collateral quote this wrote the debt interest accrued during flat stretches off the performance basis, so the fee at each repricing step was charged as if only the last accrual interval's carry had been incurred (systematic over-charge), and a drawdown reset the reference down so the recovery was re-taxed (high-water-mark violation). The reference now advances only when a positive basis crystallizes (or on bootstrap/empty vault) and is held otherwise. Capital flows (deposit, withdraw, burn, rebalance, module add/remove) rebase it so the pending per-share basis is preserved: exits shed their proportional slice of the carried debt cost, re-entries re-attach it, and supply-neutral flows keep it unchanged. Storage layout is unchanged; the two existing slots are reinterpreted as the reference.
…t paths Review findings: a flow executed while every position was excluded as bad debt reached rebaseSnapshot with empty good-debt aggregates, computed zero carry, and collapsed the held high-water mark to the trough (or the bootstrap sentinel), re-charging the subsequent recovery. Hold the reference when the good-debt universe is empty on both sides of the flow, matching what accruals do during the same episode. A flow that brings the pool back above water still re-anchors at the post-flow state; documented as a limitation of the binary bad-debt exclusion. Also rewrites the stale bad-debt NatSpec paragraph that described the removed always-advance behavior, and adds tests for the four advance-without-mint branches (no recipient, zero perf rate, gain below the mgmt deduction, fee-cap early return) plus underwater-flow, rescue re-anchor, and partial-bad-debt scenarios.
📝 WalkthroughWalkthroughThis PR replaces the position manager's snapshot-based performance-fee tracking with a held high-water-mark reference model. It adds a ChangesHeld performance reference and management-fee accumulator
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant PositionManagerLP
participant PositionManagerBase
participant LibStorage
Caller->>PositionManagerLP: deposit/withdraw/burn
PositionManagerLP->>PositionManagerBase: _accrueFees()
PositionManagerBase->>PositionManagerBase: _pendingFees() computes basis, advanceReference, heldManagementFees_
PositionManagerBase-->>PositionManagerLP: totalAssetsBefore, debtBefore
PositionManagerLP->>PositionManagerBase: _rebaseReference(totalAssetsBefore, debtBefore, totalSupply)
PositionManagerBase->>LibStorage: rebaseSnapshot(...)
LibStorage-->>PositionManagerBase: updated lastTotalAssets, lastDebt, heldManagementFeeAssets
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
A permanent drawdown or realized liquidation loss would hold the reference (high-water mark) forever, suppressing performance fees until a recovery that may never come. resetPerformanceReference (owner-only) accrues fees first, then force-advances the reference to the current state: it can never mint on past gains, it only forgives the carried negative basis so future gains are charged from the current state. Documented timing rule: the reset also writes off the debt carry accrued since the last crystallization, which the next positive accrual then overcharges by; call it as soon as possible after a positive performance fee charge.
The performance fee must be net of management fees, but management fees mint on every accrual while the performance reference can stay held for many intervals. Only the current interval's management fee was deducted from the crystallizing basis, so fees minted during held intervals were forgotten and the next crystallization overcharged by perfRate times those amounts. heldManagementFeeAssets (appended to the ERC-7201 struct, zero on upgrade) accumulates the management fees charged while the reference is held; the crystallizing basis deducts accumulator plus current interval, and the accumulator clears on any reference advance and on resetPerformanceReference. Flows scale it with the share supply like the debt carry; a rescue flow out of a full bad-debt episode keeps it nominal (shares mint against a zero asset base there, so scaling would inflate the deduction beyond the fees ever charged). feeData() exposes the accumulator as a sixth return value.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/libs/manager/LibStorage.t.sol (1)
155-191: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFuzz test should exercise
prevCollat != refCollatto cover themulDivUpscaling path.The fuzz test always sets
prevCollat = refCollat(line 170), which makesscaledRefDebt = refDebt.mulDivUp(refCollat, refCollat) = refDebttrivially. In production, pre-flow collateral typically drifts from the reference collateral between crystallizations, soprevCollat != refCollatis the common case. ThemulDivUpscaling — the core of the carry computation — is never fuzzed with different numerator and denominator values. All deterministic tests share the same limitation.♻️ Proposed fuzz test with independent `prevCollat`
function testFuzz_rebaseSnapshot_preservesPerShareBasis( uint96 refTotalAssets, uint96 refDebt, + uint96 prevCollatSeed, uint96 carrySeed, uint96 heldMgmtSeed, uint96 newCollat, uint96 newDebt, uint64 prevSupply, uint64 newSupply ) public { vm.assume(refDebt > 0 && prevSupply > 0 && newSupply > 0); vm.assume(newCollat >= newDebt); uint256 refCollat = uint256(refTotalAssets) + refDebt; - // Build a pre-flow state whose basis is exactly -carry at reference LTV. - uint256 prevCollat = refCollat; + // Pre-flow collateral drifts independently from the reference collateral. + uint256 prevCollat = bound(uint256(prevCollatSeed), 1, type(uint96).max); uint256 carry = uint256(carrySeed); - uint256 prevDebt = uint256(refDebt) + carry; + // Build a pre-flow state whose basis is exactly -carry at reference LTV. + uint256 scaledRefDebt = FixedPointMathLib.mulDivUp(uint256(refDebt), prevCollat, refCollat); + uint256 prevDebt = scaledRefDebt + carry; harness.setReference(refTotalAssets, refDebt); harness.setHeldManagementFeeAssets(heldMgmtSeed); harness.rebaseSnapshot(prevCollat, prevDebt, prevSupply, newCollat, newDebt, newSupply); - uint256 expectedCarry = FixedPointMathLib.mulDiv(carry, newSupply, prevSupply); + uint256 prevCarry = FixedPointMathLib.zeroFloorSub(prevDebt, scaledRefDebt); + uint256 expectedCarry = FixedPointMathLib.mulDiv(prevCarry, newSupply, prevSupply); if (expectedCarry > newDebt) expectedCarry = newDebt; assertEq(harness.getLastDebt(), uint256(newDebt) - expectedCarry, "reference debt carries the scaled basis"); assertEq( harness.getLastTotalAssets(), uint256(newCollat) - (uint256(newDebt) - expectedCarry), "reference NAV re-anchored on post-flow collateral" ); assertEq( harness.getHeldManagementFeeAssets(), FixedPointMathLib.mulDiv(heldMgmtSeed, newSupply, prevSupply), "held management fee deduction scales with supply" ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/libs/manager/LibStorage.t.sol` around lines 155 - 191, The fuzz test in testFuzz_rebaseSnapshot_preservesPerShareBasis currently forces prevCollat to equal refCollat, so it never exercises the mulDivUp carry-scaling path under differing collateral bases. Update the setup to let prevCollat vary independently from the reference collateral while still preserving the intended pre-flow basis, and keep the assertions validating harness.rebaseSnapshot, getLastDebt, getLastTotalAssets, and getHeldManagementFeeAssets against the scaled carry behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/libs/manager/LibStorage.t.sol`:
- Around line 155-191: The fuzz test in
testFuzz_rebaseSnapshot_preservesPerShareBasis currently forces prevCollat to
equal refCollat, so it never exercises the mulDivUp carry-scaling path under
differing collateral bases. Update the setup to let prevCollat vary
independently from the reference collateral while still preserving the intended
pre-flow basis, and keep the assertions validating harness.rebaseSnapshot,
getLastDebt, getLastTotalAssets, and getHeldManagementFeeAssets against the
scaled carry behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc524b3f-e2b1-4e5d-934b-99bd53f48bf9
📒 Files selected for processing (15)
src/interfaces/manager/IPositionManager.solsrc/interfaces/manager/base/IPositionManagerAdmin.solsrc/libs/manager/LibStorage.solsrc/manager/PositionManager.solsrc/manager/base/PositionManagerAdmin.solsrc/manager/base/PositionManagerBase.solsrc/manager/base/PositionManagerLP.solsrc/manager/base/PositionManagerRebalancing.soltest/libs/manager/LibStorage.t.soltest/manager/PositionManager.invariant.t.soltest/manager/PositionManagerBase.t.soltest/manager/PositionManagerFee.t.soltest/manager/PositionManagerFeeReference.t.soltest/mock/libs/LibManagerStorageHarness.soltest/mock/manager/PositionManagerHandler.sol
Performance fee: hold the reference unless crystallized, rebase on flows
Problem 1 (fixed): debt-carry write-off / high-water-mark violation
The performance reference (
lastTotalAssets,lastDebt, i.e.LTV_ref) was advanced to the current state on every accrual (_accrueFees) and every flow tail (updateSnapshotin_settleShares,burn, rebalance, module add/remove).With a collateral quote that only reprices periodically (the fund token's
virtualPricesteps ~monthly) while Morpho debt accrues continuously:LTV_refup, absorbing the accrued interest into the reference.On-chain evidence (tx
0xbea2e5f8ec03680324a89198290e2f69d2337d7888f17ea1b465c254b0339e4d, block 25448276, manager0x15ad372AA8b288FFAb36f3E260f8f41F03b23a62): 286.07 fee shares minted (12.26 mgmt + 273.81 perf).currentDebt(380,291 USDC) contained a full month of interest, but the reference had been reset ~7 days earlier, so the basis (1,918 USDC) netted only ~400 USDC of carry instead of ~2,000. With the full month's carry netted the perf fee would have been roughly a third of what was charged. State verified against the RPC at block 25448275.The fix
Two coupled rules (handoff Solution B):
Advance on crystallization only (
_accrueFees/_pendingFees): the reference advances to the current state only when the basis is positive (a fee crystallizes, at the configured rate which may be zero), on bootstrap (lastDebt == 0sentinel), or on an empty vault. A non-positive basis holds the reference, so accrued debt carry and drawdowns stay inside the basis (true high-water mark).lastFeeAccrualTimestampstill advances every accrual (management fee is time-based and unaffected).Flows rebase instead of reset (
LibStorage.rebaseSnapshot, replacingupdateSnapshot): deposits, withdrawals, burns, rebalances and module add/remove preserve the pending per-share basis across the flow:Exits shed their proportional slice of the carried debt cost (they don't dump it on stayers and don't escape it), re-entries re-attach it (no exit/re-enter gaming), and supply-neutral flows (rebalance, module changes) keep it unchanged. Rounding matches
_pendingFees(mulDivUpon the scaled reference debt), so a flow can never manufacture a positive basis.Owner escape hatch for permanent losses (
resetPerformanceReference, owner-only): a true high-water mark cuts both ways — after a permanent drawdown or a realized liquidation loss the held reference would suppress performance fees until the pool recovers past the old mark, which may never happen. The owner can force-advance the reference to the current state: fees accrue first (a positive pending basis crystallizes normally to the current recipient), then the reference re-anchors, so the reset can never charge past gains — it only forgives the carried negative basis, and future gains are charged from the current state onward. Called while every position is underwater it writes the bootstrap sentinel (the next accrual reseeds), which is the same documented semantics as a rescue flow.Operational timing rule (documented in the NatSpec): the reset writes off all carried basis — not just the loss being accepted but also the debt interest accrued since the last crystallization — plus the pending management fee deduction (below), and the next positive accrual then overcharges by exactly those forgiven amounts (this is the per-accrual reset behavior of the old code, restored at one chosen moment). So reset as soon as possible after a positive performance fee charge, when the pending carry and deduction are smallest; a reset deep into a flat-quote period re-introduces the debt-carry write-off for that period.
Management fees charged while held are netted from the next crystallization (
heldManagementFeeAssets): the performance fee must be net of management fees, but management fees are minted on every accrual (they are time-based and must keep being charged, including to mid-period exiters, who pay them through each flow's accrual). Previously only the current interval's management fee was deducted from the basis, so management fees minted during held (non-positive basis) intervals were forgotten and the next crystallization overcharged byperfRate ×those amounts. Now each held accrual adds its management fee assets to an accumulator; the crystallizing basis is reduced byaccumulator + current interval, and the accumulator clears whenever the reference advances (any excess above the basis is forgiven, not carried past the new mark) and onresetPerformanceReference(a reset forgives the pending deduction along with the carry). Flows scale the accumulator with the share-supply change exactly like the debt carry — an exit takes its slice of the pending deduction along, a deposit re-attaches it — so it cannot be dumped on stayers or shed by round trips. The accumulator is a new storage slot appended to the end of the ERC-7201 namespaced struct (no existing field moves; on the live-vault upgrade it reads zero, so netting starts fresh with no retroactive adjustment). Note for integrators: thefeeData()view gained a sixth return value (heldManagementFees) — an ABI change to that view.Bad-debt episodes: while every position is excluded as bad debt, both accruals and flows hold the reference, so the high-water mark survives an underwater dip and the recovery is not re-charged (an adversarial review round found and fixed a case where a flow during the dip collapsed the reference to the trough). A flow that brings the pool back above water re-anchors at the post-flow state; this is documented as a limitation of the binary bad-debt exclusion in
LibView.totalAssets(unreachable in practice before liquidation).Worked examples (old vs fixed)
All examples use the same starting position: 10,000 collateral units quoted at 1.00, 5,000 debt, so
LTV_ref = 50%; 15% performance fee, management fee set to 0 for clarity; Morpho debt accrues ≈ 0.41%/month. Fee figures are in asset units;basis = LTV_ref × collatQuoted − debt. Numbers below are actual outputs of the test-suite mechanics (Morpho fork mocks), not hand math.1. Flat month, then the oracle steps +2% (the core bug)
LTV_ref = 50.21%The old fee ignores the month of debt interest except the slice since the last accrual (27% over-charge here; on the mainnet trigger tx the same mechanism produced a ~3x over-charge because the last reset was only 7 days before the jump). The fixed fee is also invariant to how many accruals ran during the flat month (tested for 1 vs 10).
2. Collateral drawdown and recovery (high-water mark)
LTV_ref 41.7%LTV_ref = 52.7%Round-trip total: fixed 30.2 vs old 228.9.
If the drawdown were permanent (the quote never recovers past the 1.20 mark), the owner can call
resetPerformanceReference()at the trough: the reference re-anchors at 0.95 and fees resume on gains from there, without waiting for a recovery that never comes.3. Liquidation during a crash
Continues example 1's flat month (carry −20.6), then the quote crashes to 0.60 (position at 83.7% LTV > 80% LLTV, liquidatable). A liquidator seizes 2,000 collateral units (quoted 1,200) and repays 1,128 debt (Morpho incentive ≈ 6.4%), leaving C 4,800 / D 3,892.6.
LTV_ref = 83.7%repaid − LTV_ref × seizedQuoted = 1,128 − 600 = 528of the carry shield; still deeply negativeLTV_ref = 81.1%)Two things to note. First, the fixed residual (16.1) is not zero: a liquidation repays debt by selling collateral, which lands the pool at a lower LTV than the reference, and the levered-slice basis reads that as debt-slice outperformance once the quote recovers. It is bounded by
repaid × (1 − LTV_ref × LIF)and only materializes on a genuine price recovery. This semantic is unchanged from the old code (the basis taxes the levered slice, not a NAV high-water mark; NAV here is still 4,107 vs 5,000 at entry). Second, what the fix removes is the amplification: the old trough resets ratchetedLTV_reffrom 50% to 81%, taxing the entire recovery (407.9 total vs 16.1, ~25x).If the post-liquidation loss is judged permanent (no recovery to the old mark expected), the owner can
resetPerformanceReference()to re-anchor at the post-liquidation state and resume charging on gains from there.4. Bad-debt episode (all positions underwater)
Quote crashes to 0.40: C quoted 4,000 < D 5,020.6, so the module drops out of the good-debt aggregates entirely (NAV 0, aggregates (0,0)).
Caveat (documented in
rebaseSnapshotNatSpec and pinned by test): a rescue flow that lifts the pool back above water in the same transaction (e.g. a rebalancer repaying 2,000 while underwater, leaving 4,000 quoted vs 3,000 debt) re-anchors the reference at the post-rescue state, because the pre-flow basis is not measurable against empty aggregates; the recovery beyond that point is charged. In production this whole family of states requires the price to gap past the 78% liquidation LTV to >100% without liquidation, which is not reachable under normal operation.Storage layout is append-only (ERC-7201): the two existing slots are reinterpreted as the reference, and one new slot (
heldManagementFeeAssets) is appended at the end of the namespaced struct — no existing field moves. On upgrade, a live vault continues from its last-accrual snapshot; carry accrued after the upgrade is netted at the next repricing, and the management fee accumulator starts at zero (fees charged before the upgrade are not retro-netted). No retroactive charge or forgiveness.Fee direction: absent an owner reset, the fix only ever reduces the performance fee relative to the old behavior (under continuously repriced, monotonically rising collateral the two are identical); it favors LPs and removes the over-charge. A positive basis still advances the reference even when it mints nothing (no recipient, zero rate, gain below the management-fee deduction, fee-cap guard), preserving the existing "gains before fees are enabled are never retroactively charged" semantics. An owner reset is bounded by the same rule: because it accrues first and then re-anchors at the observed current state, the worst it can do is restore the old per-accrual reset behavior at one chosen moment (forgiving held carry going forward); it cannot mint on past gains or set an arbitrary reference.
Optimized (
profile.ci)PositionManagerruntime size: 21,816 → 22,501 bytes (margin 2,075 bytes under the EIP-170 limit).Problem 2 (not fixed here, documented): stale-quote entry
A mid-flat-period entrant is booked at the stale (low) collateral quote: under-credited at entry, then charged performance fee on the phantom repricing "gain" that predates their entry. This is an entry-pricing problem, not a fee-reference problem — it exists under any performance-reference mechanism and can only be fixed at the pricing layer:
virtualPricecadence on the collateral oracle (fixes both problems at the source, but the oracle is external), orPer the handoff, this PR documents the limitation and leaves the pricing-layer decision open.
Tests
New
test/manager/PositionManagerFeeReference.t.sol(handoff §5 scenarios) on real Morpho +IrmMock(debt accrues withvm.warp, quote stepped via the oracle mock):pendingFees()in lockstep with_accrueFeesmints both while held and at crystallizationheld + current interval(the netting removes exactlyperfRate × heldversus the single-interval deduction); the netted perf fee is invariant to the accrual cadence (1 vs 5 vs 10 accruals through the same flat month, exact share-level equality); a small gain below the pending deduction advances without a perf mint and the consumed deduction does not carry past the new mark; the deduction scales with supply across flows (unit + fuzz + integration)LibStorage.t.solunit tests forrebaseSnapshot(sentinel, supply-neutral, exit scaling, carry clamp, underwater hold, per-share-preservation fuzz).Full suite: 1,765 tests + invariant suites pass. Each commit was run through an adversarial multi-agent review (independent finder lenses; every finding verified by refutation-oriented agents, majority-confirm):
setFeeDataup to the capped rates, and every reset is observable viaPerformanceReferenceReset), plus two test gaps covered in that commit;Change size (production
src/only, vsmain)Measured against the merge-base with
main(96a18e3), Solidity undersrc/only (tests and scripts excluded).src/files changedSummary by CodeRabbit
New Features
Bug Fixes