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 6 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. |
…ugfix # Conflicts: # src/test/app/Loan_test.cpp
|
All conflicts have been resolved. Assigned reviewers can now start or resume their review. |
There was a problem hiding this comment.
This is a focused, well-tested fix for a real gap: the fixed-shares withdraw branch lacked the zero-value guard that the fixed-assets branch already had, and the invariant didn't account for the legitimate zero-value case (fully impaired vault). The new effectiveAssetsTotalWithdraw/debitRoundsToNoOp helpers are sensible dedup/extraction, and the same dust-debit pattern is applied consistently to both VaultClawback and VaultWithdraw. I don't see a clear correctness bug in the changed lines; the one thing worth a quick look is that the new fixCleanup3_4_0 checks in VaultWithdraw::doApply were hoisted to run before the existing insufficient-funds/frozen-balance checks, which changes which tec code wins when multiple conditions are violated at once — worth confirming that's intentional.
| }(); | ||
|
|
||
| if (!issuerWithdrawal) | ||
| if (!issuerWithdrawal && !zeroDeltaIsLegitimate) |
There was a problem hiding this comment.
Account-delta cross-check shouldn't be gated by zeroDeltaIsLegitimate. Use zero fallback:
| if (!issuerWithdrawal && !zeroDeltaIsLegitimate) | |
| if (!issuerWithdrawal) |
There was a problem hiding this comment.
Done in 11fd4cc, with one adjustment: with the gate removed, a legitimate zero-value withdrawal has both destination deltas absent, which would trip the "must change one destination balance" check. So that specific case is tolerated explicitly (nothing moved anywhere, nothing to cross-check), while any one-sided balance change now runs the full cross-check against the zero-fallback vault delta. The sub-ULP check also had to switch from maybeVaultDeltaAssets->delta to the zero-defaulted vaultDeltaAssets.delta, since the optional can be empty on this path.
There was a problem hiding this comment.
✅ Auto-resolved: the latest review no longer flags this issue, so it appears to have been addressed. If that is not correct, reopen this thread and it will be re-checked on the next review.
- Cross-check the withdrawal destination delta even when a zero vault delta is legitimate, so a one-sided balance change still fails the ValidVault invariant - Verify shares are actually burnt in the zero-value withdraw test - Add a test isolating the AssetsTotal operand of debitRoundsToNoOp via a heavily-loaned vault (AssetsTotal >> AssetsAvailable) - Make a Vault helper const
…panito/lending-bugfix
There was a problem hiding this comment.
Reviewed the VaultWithdraw/VaultClawback precision-loss guards, the new VaultHelpers helpers, and the VaultInvariant zero-delta exemption logic. The added checks are internally consistent: debitRoundsToNoOp correctly special-cases amount==0, effectiveAssetsTotalWithdraw matches the pre-existing inline computation it replaces, and the invariant's zeroDeltaIsLegitimate gate is correctly scoped (both-changed still always fails, neither-changed only passes when the pool's effective value is provably non-positive, and the one-sided cross-check still runs regardless of the legitimate-zero flag). The reordering in VaultWithdraw::doApply that moves the new fixCleanup3_4_0 checks ahead of the freeze/insufficient-funds checks only changes which tec code is returned when multiple conditions are true simultaneously, not correctness. Test coverage (Vault_test, LoanRounding_test) exercises both the real-precision-loss and genuinely-zero-pool branches pre/post amendment for both XRP and IOU assets, which matches the logic added. No confident bugs found in the changed lines.
| * that case is rejected separately and unconditionally. | ||
| */ | ||
| [[nodiscard]] bool | ||
| debitRoundsToNoOp(Asset const& asset, Number const& total, Number const& amount); |
There was a problem hiding this comment.
the name debitRoundsToNoOp is ambiguous. It's unclear whether:
- it predicts that a debit would round to zero (a query), or
- it performs a debit and reports whether the result was a no-op (an action).
I suggest naming it as debitIsNonZeroDust, debitIsDustRelativeToTotal.
| * loss. | ||
| */ | ||
| [[nodiscard]] Number | ||
| effectiveAssetsTotalWithdraw(SLE::const_ref vault, WaiveUnrealizedLoss waive); |
There was a problem hiding this comment.
"withdraw" is a verb, which reads as an operation.
I suggest naming it as assetsTotalNetOfUnrealizedLoss or assetsTotalForWithdrawal
| // legitimate zero vault delta: the destination must | ||
| // then have moved by (rounded) zero as well. | ||
| auto const destinationDelta = // | ||
| maybeAccDelta ? *maybeAccDelta : *maybeOtherAccDelta; |
| // the pool still held positive effective value indicates a | ||
| // real accounting bug, not this exception. | ||
| bool const zeroDeltaIsLegitimate = view.rules().enabled(fixCleanup3_4_0) && | ||
| !maybeVaultDeltaAssets && beforeVault.assetsTotal <= beforeVault.lossUnrealized; |
There was a problem hiding this comment.
I don't think it's possible to have a legitimate case where beforeVault.assetsTotal < beforeVault.lossUnrealized. I think this should be changed to use ==
3 participants
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.