fix(vault): deduct queued withdrawal liabilities from total_assets so NAV isn't overstated - #647
Merged
abayomicornelius merged 2 commits intoSep 26, 2026
Conversation
withdraw() burns shares and enqueues a QueuedClaim when liquidity is short, but total_assets() kept counting that USDC, so the share price of remaining holders jumped by ~usdc_owed / remaining_supply until claim(). New depositors overpaid and concurrent withdrawers over-extracted. - Track VaultKey::QueuedLiabilities: += usdc_owed on enqueue, -= on each claim() payout. - total_assets = liquid + investments + expected - queued_liabilities. - CachedTotalAssets drops at enqueue and is unchanged by claim(). - WithdrawQueued / WithdrawClaimed carry queued_liabilities.
…iability Test that a queued withdrawal lowers total_assets by exactly the amount owed, leaves the remaining holder's share price unchanged, lets a new depositor buy in at that fair price, and that claim() keeps it unchanged. Document VaultKey::QueuedLiabilities in docs/STORAGE.md and the new queued_liabilities field of withdraw_queued / withdraw_claimed in EVENTS.md.
|
@presidoclintonbased-alt Great news! 🎉 Based on an automated assessment of this PR, the linked Wave issue(s) no longer count against your application limits. You can now already apply to more issues while waiting for a review of this PR. Keep up the great work! 🚀 |
Contributor
Author
|
I tried to run the tests locally but couldn't. Even after patching the three syntax errors listed in the description,
My changes are at |
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
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.
Problem
When
withdraw()can't be paid from liquid USDC, it burns the caller's shares and enqueues aQueuedClaim { from, usdc_owed }. Buttotal_assets()=liquid + TotalInvestments + expected_returnsnever subtracted what the queue still owes. After the burn,total_supplyfell while NAV didn't. The share price of everyone still in the vault jumped by aboutusdc_owed / remaining_supplyuntilclaim()paid out. During that window:Fix (
investment_vault)VaultKey::QueuedLiabilities: i128.withdrawaddsusdc_owedwhen it enqueues.claim()subtracts each entry as it is paid.read_total_assets(used bytotal_assets,convert_to_shares/convert_to_assetsandshares_for_assets) is nowliquid + investments + expected - queued_liabilities.CachedTotalAssets:usdc_owedat enqueue.claim()no longer lowers it. Liquid USDC and the liability both fall bytotal_paid, so NAV doesn't change. The old decrement would have counted the payout twice once the liability is deducted at enqueue.WithdrawQueuedandWithdrawClaimednow include aqueued_liabilitiesfield with the running total, for indexers.Tests
test_queued_withdrawal_liability_and_claim_keep_price_faircovers:total_assetsby exactlyowed.convert_to_assets(1 share)is unchanged, within ±1 stroop of rounding.claim()pays exactlyowedand leaves the share price unchanged.Docs
docs/STORAGE.md:QueuedLiabilities, in both the key table and the size table.EVENTS.md: the newqueued_liabilitiesfield onwithdraw_queuedandwithdraw_claimed.scripts/check_event_field_types.pypasses.Notes
The issue's proptest invariant for interleaved deposit, withdraw and claim is left for a follow-up. The deterministic test above covers the same sequence.
maindoesn't currently compileinvestment-vault:storage.rs:37has#c[cfg)test].storage.rs:49has anassert_eq!missing its closing).test.rs:349has a stray}before prolonged inactivity.fragment.This PR doesn't include fixes for these. I patched them only in my local checkout to run the tests; the results are posted below.
Commits
fix(vault): deduct queued redemption liabilities from total_assetstest(vault): queued withdrawals keep the share price fair; document liabilityCloses security: queued withdrawal claims are not deducted from total_assets(), so NAV is overstated for remaining holders #613
Closes bug: the redemption queue is unreachable whenever utilization ≥ 50% — the graduated limit reverts first #614
Closes security: protect the vault against first-depositor / donation share-price inflation #615
Closes feat: add slippage protection (min_shares_out) and a deadline to deposit() #616