fix: Reject VaultWithdraw fixed-share amounts that round to zero - #7950
fix: Reject VaultWithdraw fixed-share amounts that round to zero#7950Tapanito wants to merge 2 commits into
Conversation
The fixed-shares withdrawal branch had no guard against a requested share amount whose true value rounds down to zero in the vault asset's native (integral) representation, unlike the fixed-assets branch's existing check. This let the transaction proceed to burn shares for zero value, tripping a VaultInvariant check instead of failing cleanly with tecPRECISION_LOSS. Conversely, when the pool's effective value is genuinely zero (e.g. a fully impaired/insolvent vault), a zero-value withdrawal is legitimate and the invariant now allows it instead of failing. Both behaviors are gated behind fixCleanup3_4_0.
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
This is a well-scoped, carefully tested fix. I traced both new gating conditions end-to-end: (1) the VaultWithdraw.cpp precision-loss guard for the fixed-share branch correctly uses the same effectiveAssetsTotalWithdraw(vault, waiveUnrealizedLoss) computation that the actual conversion helper uses, so it can't diverge from the real payout calculation, and it correctly exempts the final-withdrawal case which is handled elsewhere; (2) the VaultInvariant.cpp zeroDeltaIsLegitimate exception is gated on !maybeVaultDeltaAssets, so it only ever relaxes the check when there truly was no recorded delta, and it still fires the original fatal checks whenever a delta exists or the pool had genuine effective value — preserving detection of real accounting bugs. The DeltaInfo default-value fallback and the skip of the account-delta reconciliation when zeroDeltaIsLegitimate are consistent with a genuinely zero-value transfer needing no reconciliation. I did not find a confident correctness, security, or resource-handling issue in the added lines.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
A recovered or withdrawn amount can be genuinely non-zero yet still be dust relative to a sfAssetsTotal/sfAssetsAvailable large enough to exceed STAmount's significant-digit precision: subtracting it rounds the stored total right back to where it started. The shares still move, so ValidVault previously failed after the fact with "must decrease vault balance" instead of a clean upfront rejection.
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
1 participant
Summary
VaultWithdraw::doApply()'s fixed-shares branch (withdraw a fixed number of shares, variable assets) had no guard against a requested share amount whose true value rounds down to zero in the vault asset's native (integral) representation — unlike the fixed-assets branch, which already returnstecPRECISION_LOSSin the analogous case. This let the transaction proceed to burn shares for zero value, tripping aVaultInvariantcheck (tecINVARIANT_FAILED) instead of failing cleanly.fixCleanup3_4_0.effectiveAssetsTotalWithdraw()helper inVaultHelpersto avoid duplicating theAssetsTotal - LossUnrealizedcomputation between the withdraw conversion helpers and the new transactor check.Test plan
testBugVaultWithdrawFixedSharesRoundsToZero(Loan_test.cpp) exercises both scenarios (XRP: real precision loss; IOU: genuinely zero pool) under both pre- and post-fixCleanup3_4_0amendment states.Loansuite: 1072 cases / 117,230 tests, 0 failures.Vaultsuite: 411 cases / 11,373 tests, 0 failures.Invariantssuite: 85 cases / 14,090 tests, 0 failures.