From ed6a13051bfeb5a494c1348d34620caefe994fdd Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Tue, 4 Aug 2026 15:09:45 +0200 Subject: [PATCH 1/3] fix: Reject VaultWithdraw fixed-share amounts that round to zero 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. --- include/xrpl/ledger/helpers/VaultHelpers.h | 14 ++ src/libxrpl/ledger/helpers/VaultHelpers.cpp | 17 +- src/libxrpl/tx/invariants/VaultInvariant.cpp | 27 ++- .../tx/transactors/vault/VaultWithdraw.cpp | 21 ++- src/test/app/Loan_test.cpp | 163 ++++++++++++++++++ 5 files changed, 229 insertions(+), 13 deletions(-) diff --git a/include/xrpl/ledger/helpers/VaultHelpers.h b/include/xrpl/ledger/helpers/VaultHelpers.h index 5681cc57e86..5f08865468c 100644 --- a/include/xrpl/ledger/helpers/VaultHelpers.h +++ b/include/xrpl/ledger/helpers/VaultHelpers.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -52,6 +53,19 @@ enum class TruncateShares : bool { No = false, Yes = true }; */ enum class WaiveUnrealizedLoss : bool { No = false, Yes = true }; +/** + * Returns the effective total of assets backing outstanding shares, i.e. + * sfAssetsTotal, discounted by sfLossUnrealized unless waived. This is the + * numerator used by both withdraw conversion helpers (assetsToSharesWithdraw + * and sharesToAssetsWithdraw) to compute the share/asset exchange rate. + * + * @param vault The vault SLE. + * @param waive Whether to waive (i.e. not subtract) the vault's unrealized + * loss. + */ +[[nodiscard]] Number +effectiveAssetsTotalWithdraw(SLE::const_ref vault, WaiveUnrealizedLoss waive); + /** * From the perspective of a vault, return the number of shares to demand from * the depositor when they ask to withdraw a fixed amount of assets. Since diff --git a/src/libxrpl/ledger/helpers/VaultHelpers.cpp b/src/libxrpl/ledger/helpers/VaultHelpers.cpp index 78f64d20774..045f2edfc76 100644 --- a/src/libxrpl/ledger/helpers/VaultHelpers.cpp +++ b/src/libxrpl/ledger/helpers/VaultHelpers.cpp @@ -65,6 +65,15 @@ sharesToAssetsDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount co return assets; } +[[nodiscard]] Number +effectiveAssetsTotalWithdraw(SLE::const_ref vault, WaiveUnrealizedLoss waive) +{ + Number assetTotal = vault->at(sfAssetsTotal); + if (waive == WaiveUnrealizedLoss::No) + assetTotal -= vault->at(sfLossUnrealized); + return assetTotal; +} + [[nodiscard]] std::optional assetsToSharesWithdraw( SLE::const_ref vault, @@ -80,9 +89,7 @@ assetsToSharesWithdraw( if (assets.negative() || assets.asset() != vault->at(sfAsset)) return std::nullopt; // LCOV_EXCL_LINE - Number assetTotal = vault->at(sfAssetsTotal); - if (waive == WaiveUnrealizedLoss::No) - assetTotal -= vault->at(sfLossUnrealized); + Number const assetTotal = effectiveAssetsTotalWithdraw(vault, waive); STAmount shares{vault->at(sfShareMPTID)}; if (assetTotal == 0) return shares; @@ -108,9 +115,7 @@ sharesToAssetsWithdraw( if (shares.negative() || shares.asset() != vault->at(sfShareMPTID)) return std::nullopt; // LCOV_EXCL_LINE - Number assetTotal = vault->at(sfAssetsTotal); - if (waive == WaiveUnrealizedLoss::No) - assetTotal -= vault->at(sfLossUnrealized); + Number const assetTotal = effectiveAssetsTotalWithdraw(vault, waive); STAmount assets{vault->at(sfAsset)}; if (assetTotal == 0) return assets; diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index eca50eb8093..831ca807dac 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -805,19 +805,36 @@ ValidVault::finalize( auto const& beforeVault = beforeVault_[0]; auto const maybeVaultDeltaAssets = deltaAssets(afterVault.pseudoId); - if (!maybeVaultDeltaAssets) + + // Post-fixCleanup3_4_0: a withdrawal that redeems shares from a + // pool with no effective value left to back them (e.g. fully + // impaired/insolvent) legitimately moves zero assets on both + // sides — VaultWithdraw::doApply does not touch either + // balance-holding entry for a zero-value transfer, so no delta + // is recorded. VaultWithdraw::doApply separately rejects + // (tecPRECISION_LOSS) the case where a *positive* per-share + // value merely rounds down to zero, so a missing delta while + // 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; + + if (!maybeVaultDeltaAssets && !zeroDeltaIsLegitimate) { JLOG(j.fatal()) << "Invariant failed: withdrawal must change vault balance"; return false; // That's all we can do } + DeltaInfo const vaultDeltaAssets = maybeVaultDeltaAssets.value_or( + DeltaInfo{.delta = kNumZero, .scale = std::nullopt}); + // Get the posterior scale to round calculations to - auto const minScale = computeVaultMinScale(*maybeVaultDeltaAssets, view.rules()); + auto const minScale = computeVaultMinScale(vaultDeltaAssets, view.rules()); auto const vaultPseudoDeltaAssets = - roundToAsset(vaultAsset, maybeVaultDeltaAssets->delta, minScale); + roundToAsset(vaultAsset, vaultDeltaAssets.delta, minScale); - if (vaultPseudoDeltaAssets >= kZero) + if (!zeroDeltaIsLegitimate && vaultPseudoDeltaAssets >= kZero) { JLOG(j.fatal()) << "Invariant failed: withdrawal must decrease vault balance"; result = false; @@ -832,7 +849,7 @@ ValidVault::finalize( return destination == vaultAsset.getIssuer(); }(); - if (!issuerWithdrawal) + if (!issuerWithdrawal && !zeroDeltaIsLegitimate) { auto const maybeAccDelta = deltaAssetsTxAccount(tx, fee); auto const maybeOtherAccDelta = [&]() -> std::optional { diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index 353b72c30d1..65450a28ca0 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -273,6 +273,25 @@ VaultWithdraw::doApply() return tecPATH_DRY; } + // A withdrawal for a fixed share amount (variable assets) has no requested-asset amount to + // check for rounding, unlike the fixed-assets branch above. sfLossUnrealized in particular + // means a small enough share amount can be legitimately worth zero assets. The "final + // withdrawal" rule handles its own zero-value case using sfAssetsAvailable directly, so it is + // exempt here. + bool const isFinalWithdrawal = + sharesRedeemed == STAmount{share, sleIssuance->at(sfOutstandingAmount)}; + + if (view().rules().enabled(fixCleanup3_4_0) && amount.asset() == share && + assetsWithdrawn == beast::kZero && !isFinalWithdrawal && + effectiveAssetsTotalWithdraw(vault, waiveUnrealizedLoss) != beast::kZero) + { + // The vault still has a nonzero effective value backing outstanding + // shares, but the requested share amount rounds down to zero assets + // in the vault asset's native (integral) representation. + JLOG(j_.debug()) << "VaultWithdraw: fixed-share withdrawal rounds to zero assets"; + return tecPRECISION_LOSS; + } + // Post-fixCleanup3_3_0: preclaim already validated all freeze conditions // (checkWithdrawFreeze), so IgnoreFreeze avoids a redundant check that // would incorrectly return zero for vault pseudo-accounts whose shares @@ -309,8 +328,6 @@ VaultWithdraw::doApply() // When the rule applies, the payout is the remaining sfAssetsAvailable; in a clean vault // the helper result should already equal that value, and any mismatch is a rounding artifact // worth logging. - bool const isFinalWithdrawal = - sharesRedeemed == STAmount{share, sleIssuance->at(sfOutstandingAmount)}; if (view().rules().enabled(fixCleanup3_2_0) && isFinalWithdrawal) { // Unreachable: a final withdrawal with lossUnrealized > 0 has diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 8a6f1669df8..024d60118cd 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -7442,6 +7442,167 @@ class Loan_test : public beast::unit_test::Suite attemptWithdrawShares(depositorB, sharesLpB, tesSUCCESS); } + // Pre-fixCleanup3_4_0 bug: VaultWithdraw for a fixed *share* amount that + // rounds to zero assets trips tecINVARIANT_FAILED instead of failing + // cleanly or succeeding, depending on why it's zero. The fixed-shares + // branch had no zero guard, unlike the fixed-assets branch. + // XRP case: pool value is nonzero (2,000,000) but 1 share's worth (0.5 + // drops) truncates to zero drops -> real precision loss -> tecPRECISION_LOSS. + // IOU case: loan drew 100% of the vault and is fully impaired, so + // AssetsTotal == LossUnrealized exactly -> pool value is genuinely zero + // -> legitimate zero-value withdrawal -> tesSUCCESS. + void + testBugVaultWithdrawFixedSharesRoundsToZero(FeatureBitset features) + { + testcase("bug: VaultWithdraw fixed shares round down to zero assets"); + + using namespace jtx; + using namespace loan; + + bool const fixed = features[fixCleanup3_4_0]; + + Env env(*this, features); + + Account const lender{"lender"}; + Account const depositorB{"depositorB"}; + Account const borrower{"borrower"}; + + env.fund(XRP(10'000'000), lender, depositorB, borrower); + env.close(); + + // asset(n) == n drops. + PrettyAsset const xrpAsset{xrpIssue(), 1}; + + auto const broker = createVaultAndBroker( + env, + xrpAsset, + lender, + {.vaultDeposit = 1'000'000, .debtMax = 3'000'000, .coverDeposit = 1'000'000}); + + Vault v{env}; + env(v.deposit( + {.depositor = depositorB, + .id = broker.vaultKeylet().key, + .amount = xrpAsset(3'000'000)})); + env.close(); + + auto const brokerSle = env.le(broker.brokerKeylet()); + if (!BEAST_EXPECT(brokerSle)) + return; + auto const loanKeylet = keylet::loan(broker.brokerID, brokerSle->at(sfLoanSequence)); + + env(set(borrower, broker.brokerID, Number{2'000'000}), + Sig(sfCounterpartySignature, lender), + kPaymentTotal(2), + kPaymentInterval(600), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + // Impair the loan so LossUnrealized > 0. + env(manage(lender, loanKeylet.key, tfLoanImpair), Ter(tesSUCCESS)); + env.close(); + + auto const vaultSle = env.le(broker.vaultKeylet()); + if (!BEAST_EXPECT(vaultSle)) + return; + BEAST_EXPECT(vaultSle->at(sfLossUnrealized) > beast::kZero); + + // (AssetsTotal 4M - LossUnrealized 2M) * 1 share / 4M shares = 0.5, + // rounds down to zero drops. + auto const shareAsset = vaultSle->at(sfShareMPTID); + STAmount const oneShare{MPTIssue{shareAsset}, Number(1)}; + + env(v.withdraw({.depositor = lender, .id = broker.vaultKeylet().key, .amount = oneShare}), + Ter(fixed ? tecPRECISION_LOSS : tecINVARIANT_FAILED)); + env.close(); + + // Same bug, IOU asset. Needs a 2nd, minimal depositor: a sole + // shareholder would waive the loss subtraction (fixCleanup3_2_0), + // returning full value instead of zero. + { + Account const issuer{"issuer"}; + Account const iouLender{"iouLender"}; + Account const iouDepositorB{"iouDepositorB"}; + Account const iouBorrower{"iouBorrower"}; + + env.fund(XRP(10'000'000), issuer, iouLender, iouDepositorB, iouBorrower); + env.close(); + + PrettyAsset const iouAsset = issuer[iouCurrency_]; + env(trust(iouLender, iouAsset(10'000'000))); + env(trust(iouDepositorB, iouAsset(10'000'000))); + env(trust(iouBorrower, iouAsset(10'000'000))); + // iouLender funds the vault deposit and the broker's cover deposit. + env(pay(issuer, iouLender, iouAsset(9'000'000))); + env(pay(issuer, iouDepositorB, iouAsset(1))); + env.close(); + + // No management fee -> LossUnrealized ends up == AssetsTotal. + auto const iouBroker = createVaultAndBroker( + env, + iouAsset, + iouLender, + {.vaultDeposit = 3'999'999, + .debtMax = 4'000'000, + .coverDeposit = 4'000'000, + .managementFeeRate = TenthBips16{0}}); + + env(v.deposit( + {.depositor = iouDepositorB, + .id = iouBroker.vaultKeylet().key, + .amount = iouAsset(1)})); + env.close(); + + auto const iouBrokerSle = env.le(iouBroker.brokerKeylet()); + if (!BEAST_EXPECT(iouBrokerSle)) + return; + auto const iouLoanKeylet = + keylet::loan(iouBroker.brokerID, iouBrokerSle->at(sfLoanSequence)); + + // Draw the entire vault out as a single loan. + env(set(iouBorrower, iouBroker.brokerID, Number{4'000'000}), + Sig(sfCounterpartySignature, iouLender), + kPaymentTotal(2), + kPaymentInterval(600), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + env(manage(iouLender, iouLoanKeylet.key, tfLoanImpair), Ter(tesSUCCESS)); + env.close(); + + auto const iouVaultSle = env.le(iouBroker.vaultKeylet()); + if (!BEAST_EXPECT(iouVaultSle)) + return; + BEAST_EXPECT(iouVaultSle->at(sfLossUnrealized) == iouVaultSle->at(sfAssetsTotal)); + + auto const iouShareAsset = iouVaultSle->at(sfShareMPTID); + STAmount const oneIouShare{MPTIssue{iouShareAsset}, Number(1)}; + + auto const iouLenderBalanceBefore = env.balance(iouLender, iouAsset); + auto const iouVaultAvailableBefore = iouVaultSle->at(sfAssetsAvailable); + env(v.withdraw( + {.depositor = iouLender, + .id = iouBroker.vaultKeylet().key, + .amount = oneIouShare}), + fixed ? Ter(tesSUCCESS) : Ter(tecINVARIANT_FAILED)); + env.close(); + + if (fixed) + { + // Confirm this was a true zero-value transfer: balances + // unchanged even though a share was burned. + BEAST_EXPECT(env.balance(iouLender, iouAsset) == iouLenderBalanceBefore); + auto const iouVaultAfter = env.le(iouBroker.vaultKeylet()); + if (BEAST_EXPECT(iouVaultAfter)) + { + BEAST_EXPECT(iouVaultAfter->at(sfAssetsAvailable) == iouVaultAvailableBefore); + } + } + } + } + // A residual overpayment can reduce the stored principal by one scale-unit // *less* than computeOverpaymentComponents predicts, firing the // "principal change agrees" XRPL_ASSERT_PARTS in doOverpayment: @@ -9547,6 +9708,8 @@ class Loan_test : public beast::unit_test::Suite testLendingCanTradeDisabledNoImpact(); testBugOverpaymentPrincipalChange(); testBugOverpayUnroundedAmount(); + testBugVaultWithdrawFixedSharesRoundsToZero(all_ - fixCleanup3_4_0); + testBugVaultWithdrawFixedSharesRoundsToZero(all_); for (auto const flags : {0u, tfLoanOverpayment}) testYieldTheftRounding(flags); From c8e46d55facbd27467c53d9110f46b4df08882e0 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Tue, 4 Aug 2026 16:56:48 +0200 Subject: [PATCH 2/3] fix: Reject VaultClawback/VaultWithdraw debits that round to no-op 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. --- include/xrpl/ledger/helpers/VaultHelpers.h | 19 ++++ src/libxrpl/ledger/helpers/VaultHelpers.cpp | 8 ++ .../tx/transactors/vault/VaultClawback.cpp | 14 +++ .../tx/transactors/vault/VaultWithdraw.cpp | 52 +++++++---- src/test/app/Vault_test.cpp | 92 +++++++++++++++++++ 5 files changed, 165 insertions(+), 20 deletions(-) diff --git a/include/xrpl/ledger/helpers/VaultHelpers.h b/include/xrpl/ledger/helpers/VaultHelpers.h index 5f08865468c..b059ef53c89 100644 --- a/include/xrpl/ledger/helpers/VaultHelpers.h +++ b/include/xrpl/ledger/helpers/VaultHelpers.h @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -66,6 +67,24 @@ enum class WaiveUnrealizedLoss : bool { No = false, Yes = true }; [[nodiscard]] Number effectiveAssetsTotalWithdraw(SLE::const_ref vault, WaiveUnrealizedLoss waive); +/** + * Returns whether debiting `amount` from `total` — the current value of a + * vault's sfAssetsTotal or sfAssetsAvailable field — would canonicalize back + * to the exact same STAmount value it started at. This happens when a + * genuinely non-zero debit is dust relative to a `total` large enough to + * exceed STAmount's significant-digit precision: the shares still move, but + * the stored total doesn't change, which otherwise trips the ValidVault + * invariant after the fact instead of failing cleanly upfront. + * + * @param asset The vault's underlying asset, used to canonicalize both sides + * the same way the ledger will when the field is stored. + * @param total The field's current value. + * @param amount The amount to debit. A value of zero always returns false; + * that case is rejected separately and unconditionally. + */ +[[nodiscard]] bool +debitRoundsToNoOp(Asset const& asset, Number const& total, Number const& amount); + /** * From the perspective of a vault, return the number of shares to demand from * the depositor when they ask to withdraw a fixed amount of assets. Since diff --git a/src/libxrpl/ledger/helpers/VaultHelpers.cpp b/src/libxrpl/ledger/helpers/VaultHelpers.cpp index 045f2edfc76..9f2bd04ad82 100644 --- a/src/libxrpl/ledger/helpers/VaultHelpers.cpp +++ b/src/libxrpl/ledger/helpers/VaultHelpers.cpp @@ -74,6 +74,14 @@ effectiveAssetsTotalWithdraw(SLE::const_ref vault, WaiveUnrealizedLoss waive) return assetTotal; } +[[nodiscard]] bool +debitRoundsToNoOp(Asset const& asset, Number const& total, Number const& amount) +{ + if (amount == 0) + return false; + return STAmount{asset, total - amount} == STAmount{asset, total}; +} + [[nodiscard]] std::optional assetsToSharesWithdraw( SLE::const_ref vault, diff --git a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp index d77286b667b..1a0626c74e4 100644 --- a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp @@ -383,6 +383,20 @@ VaultClawback::doApply() if (sharesDestroyed == beast::kZero) return tecPRECISION_LOSS; + // A recovered 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 below rounds the stored total right back to where it started. + // The shares still move, so ValidVault would fail after the fact with "clawback must + // decrease vault balance" instead of a clean upfront rejection. + if (view().rules().enabled(fixCleanup3_4_0) && + (debitRoundsToNoOp(vaultAsset, assetsTotal, assetsRecovered) || + debitRoundsToNoOp(vaultAsset, assetsAvailable, assetsRecovered))) + { + JLOG(j_.debug()) << "VaultClawback: clawback amount too small to change stored vault" + " balance"; + return tecPRECISION_LOSS; + } + assetsTotal -= assetsRecovered; assetsAvailable -= assetsRecovered; view().update(vault); diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index 65450a28ca0..63e1c6763f2 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -273,23 +273,42 @@ VaultWithdraw::doApply() return tecPATH_DRY; } - // A withdrawal for a fixed share amount (variable assets) has no requested-asset amount to - // check for rounding, unlike the fixed-assets branch above. sfLossUnrealized in particular - // means a small enough share amount can be legitimately worth zero assets. The "final - // withdrawal" rule handles its own zero-value case using sfAssetsAvailable directly, so it is - // exempt here. + // The "final withdrawal" rule below handles its own zero-value case using + // sfAssetsAvailable directly, so it is exempt from the checks below. bool const isFinalWithdrawal = sharesRedeemed == STAmount{share, sleIssuance->at(sfOutstandingAmount)}; - if (view().rules().enabled(fixCleanup3_4_0) && amount.asset() == share && - assetsWithdrawn == beast::kZero && !isFinalWithdrawal && - effectiveAssetsTotalWithdraw(vault, waiveUnrealizedLoss) != beast::kZero) + auto assetsAvailable = vault->at(sfAssetsAvailable); + auto assetsTotal = vault->at(sfAssetsTotal); + auto const lossUnrealized = vault->at(sfLossUnrealized); + XRPL_ASSERT( + lossUnrealized <= (assetsTotal - assetsAvailable), + "xrpl::VaultWithdraw::doApply : loss and assets do balance"); + + if (view().rules().enabled(fixCleanup3_4_0) && !isFinalWithdrawal) { - // The vault still has a nonzero effective value backing outstanding - // shares, but the requested share amount rounds down to zero assets - // in the vault asset's native (integral) representation. - JLOG(j_.debug()) << "VaultWithdraw: fixed-share withdrawal rounds to zero assets"; - return tecPRECISION_LOSS; + // A withdrawal for a fixed share amount (variable assets) has no requested-asset + // amount to check for rounding, unlike the fixed-assets branch above: a small enough + // share amount can round down to an exact zero even though the vault still holds + // positive effective value backing outstanding shares. + if (amount.asset() == share && assetsWithdrawn == beast::kZero && + effectiveAssetsTotalWithdraw(vault, waiveUnrealizedLoss) != beast::kZero) + { + JLOG(j_.debug()) << "VaultWithdraw: fixed-share withdrawal rounds to zero assets"; + return tecPRECISION_LOSS; + } + + // assetsWithdrawn can also be genuinely non-zero and still too small to move + // sfAssetsTotal or sfAssetsAvailable once canonicalized to STAmount's precision. Either + // way the shares still move, so ValidVault would otherwise fail after the fact instead + // of a clean upfront rejection. + if (debitRoundsToNoOp(vaultAsset, assetsTotal, assetsWithdrawn) || + debitRoundsToNoOp(vaultAsset, assetsAvailable, assetsWithdrawn)) + { + JLOG(j_.debug()) << "VaultWithdraw: withdrawal amount too small to change stored" + " vault balance"; + return tecPRECISION_LOSS; + } } // Post-fixCleanup3_3_0: preclaim already validated all freeze conditions @@ -306,13 +325,6 @@ VaultWithdraw::doApply() return tecINSUFFICIENT_FUNDS; } - auto assetsAvailable = vault->at(sfAssetsAvailable); - auto assetsTotal = vault->at(sfAssetsTotal); - auto const lossUnrealized = vault->at(sfLossUnrealized); - XRPL_ASSERT( - lossUnrealized <= (assetsTotal - assetsAvailable), - "xrpl::VaultWithdraw::doApply : loss and assets do balance"); - // The vault must have enough assets on hand. if (*assetsAvailable < assetsWithdrawn) { diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index bd596d61499..549f8e16f39 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -6947,6 +6947,97 @@ class Vault_test : public beast::unit_test::Suite } } + // Bug: a debit can be genuinely non-zero yet still be dust relative to a + // sfAssetsTotal/sfAssetsAvailable large enough to exceed STAmount's precision, e.g. + // AssetsTotal 2e12 minus a 1e-6 debit needs 19 significant digits and rounds straight + // back to 2e12. The shares still move, so ValidVault later fails with "must decrease + // vault balance" instead of a clean upfront rejection. + // + // Fix (fixCleanup3_4_0): reject upfront with tecPRECISION_LOSS if the debit would + // canonicalize back to the prior stored value. + void + testBugVaultDustDebitCanonicalizesToNoOp() + { + using namespace test::jtx; + + // Fund a single depositor and have them deposit `total` USD in one shot (default + // scale 6, so shares mint at exactly total*1e6). + auto const seedVault = [](Env& env, Number const& total) { + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Account const holder{"holder"}; + + env.fund(XRP(1'000'000), issuer, owner, holder); + env.close(); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + env(trust(holder, usd(100'000'000'000'000LL))); + env.close(); + env(pay(issuer, holder, usd(total))); + env.close(); + + Vault const vault{env}; + auto const [tx, keylet] = vault.create({.owner = owner, .asset = usd.raw()}); + env(tx); + env.close(); + env(vault.deposit({.depositor = holder, .id = keylet.key, .amount = usd(total)}), + Ter(tesSUCCESS)); + env.close(); + + return keylet; + }; + + { + auto runScenario = [&](FeatureBitset features, TER expected) { + Env env(*this, features); + Number const total{2, 12}; + auto const keylet = seedVault(env, total); + + Account const issuer{"issuer"}; + PrettyAsset const usd{issuer["USD"]}; + + // 1 share's worth of assets: 1e-6, below AssetsTotal's storage precision. + env(Vault::clawback( + {.issuer = issuer, + .id = keylet.key, + .holder = Account{"holder"}, + .amount = usd(Number{1, -6}).value()}), + Ter(expected)); + env.close(); + }; + + testcase("bug: VaultClawback dust debit fires invariant (pre-fixCleanup3_4_0)"); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); + testcase("bug: VaultClawback dust debit rejected cleanly (post-fixCleanup3_4_0)"); + runScenario(all_, tecPRECISION_LOSS); + } + + { + auto runScenario = [&](FeatureBitset features, TER expected) { + Env env(*this, features); + Number const total{2, 12}; + auto const keylet = seedVault(env, total); + + MPTIssue const share{env.le(keylet)->at(sfShareMPTID)}; + + // Redeem 1 share, worth 1e-6 assets, below AssetsTotal's storage precision. + env(Vault::withdraw( + {.depositor = Account{"holder"}, + .id = keylet.key, + .amount = STAmount{share, 1}}), + Ter(expected)); + env.close(); + }; + + testcase("bug: VaultWithdraw dust debit fires invariant (pre-fixCleanup3_4_0)"); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); + testcase("bug: VaultWithdraw dust debit rejected cleanly (post-fixCleanup3_4_0)"); + runScenario(all_, tecPRECISION_LOSS); + } + } + // Bug: when a depositor's IOU trustline balance is very large (e.g. // ~1e17), adding a small deposit (e.g. 1 USD) leaves sfAssetsTotal // unchanged at IOU precision because the increment is sub-ULP at the @@ -8375,6 +8466,7 @@ class Vault_test : public beast::unit_test::Suite testBugMakeDeltaAnteriorScale(); testVaultDepositCanonicalizeToZero(); testVaultWithdrawCanonicalizeToZero(); + testBugVaultDustDebitCanonicalizesToNoOp(); testVaultDepositNegativeBalanceFromOppositeLimit(); testSequences(); testPreflight(); From 11fd4cc28e45b290d8cf1bb0e763db67e876bb11 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Thu, 6 Aug 2026 17:53:09 +0200 Subject: [PATCH 3/3] fix: Address review feedback on dust-debit rejection - 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 --- src/libxrpl/tx/invariants/VaultInvariant.cpp | 125 ++++++++++--------- src/test/app/Vault_test.cpp | 5 + src/test/app/lending/LoanRounding_test.cpp | 95 +++++++++++++- 3 files changed, 168 insertions(+), 57 deletions(-) diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index 831ca807dac..946cffde606 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -849,7 +849,7 @@ ValidVault::finalize( return destination == vaultAsset.getIssuer(); }(); - if (!issuerWithdrawal && !zeroDeltaIsLegitimate) + if (!issuerWithdrawal) { auto const maybeAccDelta = deltaAssetsTxAccount(tx, fee); auto const maybeOtherAccDelta = [&]() -> std::optional { @@ -861,63 +861,76 @@ ValidVault::finalize( if (maybeAccDelta.has_value() == maybeOtherAccDelta.has_value()) { - JLOG(j.fatal()) << // - "Invariant failed: withdrawal must change one destination balance"; - return false; - } - - auto const destinationDelta = // - maybeAccDelta ? *maybeAccDelta : *maybeOtherAccDelta; - - // the scale of destinationDelta can be coarser than - // minScale, so we take that into account when rounding - auto const destinationScale = computeCoarsestScale({destinationDelta}); - auto const localMinScale = std::max(minScale, destinationScale); - - auto const roundedDestinationDelta = - roundToAsset(vaultAsset, destinationDelta.delta, localMinScale); - - // Post-fixCleanup3_2_0: Tolerate zero-rounded destination deltas for IOUs only. - // If the receiver's trust line sits at a coarser scale, the inflow may - // safely round down to zero. - // - // XRP and MPT remain strict. Because they are integer-exact, a zero - // destination delta indicates a true accounting bug, not a rounding artifact. - bool const tolerateZeroDelta = - view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral(); - auto const invalidBalanceChange = tolerateZeroDelta - ? roundedDestinationDelta < kZero - : roundedDestinationDelta <= kZero; - if (invalidBalanceChange) - { - JLOG(j.fatal()) << // - "Invariant failed: withdrawal must increase destination balance"; - result = false; + // Both changed is always a bug. Neither changed is + // consistent only with a legitimate zero-value + // withdrawal, which moves nothing on either side — + // there is nothing left to cross-check. + if (!zeroDeltaIsLegitimate || maybeAccDelta.has_value()) + { + JLOG(j.fatal()) << // + "Invariant failed: withdrawal must change one destination balance"; + return false; + } } - - auto const localPseudoDeltaAssets = - roundToAsset(vaultAsset, vaultPseudoDeltaAssets, localMinScale); - // For IOU assets near a precision boundary the destination's STAmount - // exponent can shift, making part of the sent value unrepresentable at the - // receiver's new scale — that portion is irreversibly absorbed by the IOU - // rail. Tolerate the mismatch only when the destroyed amount (vault outflow - // minus destination inflow, in Number space) is itself sub-ULP at the - // destination's scale. Floor rounding is used so that values exactly at the - // step boundary are not mistakenly dismissed. Any representable discrepancy - // indicates a real accounting bug and must be caught. - auto const destroyedIsSubUlp = tolerateZeroDelta && - roundToAsset( - vaultAsset, - maybeVaultDeltaAssets->delta * -1 - destinationDelta.delta, - destinationScale, - Number::RoundingMode::Downward) == kZero; - if (!destroyedIsSubUlp && - localPseudoDeltaAssets * -1 != roundedDestinationDelta) + else { - JLOG(j.fatal()) << "Invariant failed: " << // - "withdrawal must change vault and destination balance by equal " - "amount"; - result = false; + // A one-sided change is cross-checked even for a + // legitimate zero vault delta: the destination must + // then have moved by (rounded) zero as well. + auto const destinationDelta = // + maybeAccDelta ? *maybeAccDelta : *maybeOtherAccDelta; + + // the scale of destinationDelta can be coarser than + // minScale, so we take that into account when rounding + auto const destinationScale = computeCoarsestScale({destinationDelta}); + auto const localMinScale = std::max(minScale, destinationScale); + + auto const roundedDestinationDelta = + roundToAsset(vaultAsset, destinationDelta.delta, localMinScale); + + // Post-fixCleanup3_2_0: Tolerate zero-rounded destination deltas for IOUs + // only. If the receiver's trust line sits at a coarser scale, the inflow + // may safely round down to zero. + // + // XRP and MPT remain strict. Because they are integer-exact, a zero + // destination delta indicates a true accounting bug, not a rounding + // artifact. + bool const tolerateZeroDelta = + view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral(); + auto const invalidBalanceChange = tolerateZeroDelta + ? roundedDestinationDelta < kZero + : roundedDestinationDelta <= kZero; + if (invalidBalanceChange) + { + JLOG(j.fatal()) << // + "Invariant failed: withdrawal must increase destination balance"; + result = false; + } + + auto const localPseudoDeltaAssets = + roundToAsset(vaultAsset, vaultPseudoDeltaAssets, localMinScale); + // For IOU assets near a precision boundary the destination's STAmount + // exponent can shift, making part of the sent value unrepresentable at + // the receiver's new scale — that portion is irreversibly absorbed by the + // IOU rail. Tolerate the mismatch only when the destroyed amount (vault + // outflow minus destination inflow, in Number space) is itself sub-ULP at + // the destination's scale. Floor rounding is used so that values exactly + // at the step boundary are not mistakenly dismissed. Any representable + // discrepancy indicates a real accounting bug and must be caught. + auto const destroyedIsSubUlp = tolerateZeroDelta && + roundToAsset( + vaultAsset, + vaultDeltaAssets.delta * -1 - destinationDelta.delta, + destinationScale, + Number::RoundingMode::Downward) == kZero; + if (!destroyedIsSubUlp && + localPseudoDeltaAssets * -1 != roundedDestinationDelta) + { + JLOG(j.fatal()) << "Invariant failed: " << // + "withdrawal must change vault and destination balance by equal " + "amount"; + result = false; + } } } diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index ad0a2cd9454..a5269cdf4dd 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -6955,6 +6955,11 @@ class Vault_test : public beast::unit_test::Suite // // Fix (fixCleanup3_4_0): reject upfront with tecPRECISION_LOSS if the debit would // canonicalize back to the prior stored value. + // + // With a single depositor AssetsTotal == AssetsAvailable, so both + // debitRoundsToNoOp operands trip together here. LoanRounding_test's + // "dust debit vs AssetsTotal only" case isolates the AssetsTotal operand + // via a heavily-loaned vault. void testBugVaultDustDebitCanonicalizesToNoOp() { diff --git a/src/test/app/lending/LoanRounding_test.cpp b/src/test/app/lending/LoanRounding_test.cpp index 69619ca8234..139c2c36549 100644 --- a/src/test/app/lending/LoanRounding_test.cpp +++ b/src/test/app/lending/LoanRounding_test.cpp @@ -922,7 +922,7 @@ class LoanRounding_test : public LoanTestBase lender, {.vaultDeposit = 1'000'000, .debtMax = 3'000'000, .coverDeposit = 1'000'000}); - Vault v{env}; + Vault const v{env}; env(v.deposit( {.depositor = depositorB, .id = broker.vaultKeylet().key, @@ -1025,6 +1025,18 @@ class LoanRounding_test : public LoanTestBase auto const iouLenderBalanceBefore = env.balance(iouLender, iouAsset); auto const iouVaultAvailableBefore = iouVaultSle->at(sfAssetsAvailable); + // Env::balance can't be used for shares: it resolves the issuer + // name, and the share issuer is the vault pseudo-account, which + // Env doesn't know. + auto const lenderShares = [&]() -> std::uint64_t { + auto const sle = env.le(keylet::mptoken(iouShareAsset, iouLender.id())); + return sle ? sle->at(sfMPTAmount) : 0; + }; + auto const iouLenderSharesBefore = lenderShares(); + auto const iouIssuanceBefore = env.le(keylet::mptokenIssuance(iouShareAsset)); + if (!BEAST_EXPECT(iouIssuanceBefore)) + return; + auto const iouSharesOutstandingBefore = iouIssuanceBefore->at(sfOutstandingAmount); env(v.withdraw( {.depositor = iouLender, .id = iouBroker.vaultKeylet().key, @@ -1037,6 +1049,14 @@ class LoanRounding_test : public LoanTestBase // Confirm this was a true zero-value transfer: balances // unchanged even though a share was burned. BEAST_EXPECT(env.balance(iouLender, iouAsset) == iouLenderBalanceBefore); + BEAST_EXPECT(lenderShares() == iouLenderSharesBefore - 1); + auto const iouIssuanceAfter = env.le(keylet::mptokenIssuance(iouShareAsset)); + if (BEAST_EXPECT(iouIssuanceAfter)) + { + BEAST_EXPECT( + iouIssuanceAfter->at(sfOutstandingAmount) == + iouSharesOutstandingBefore - 1); + } auto const iouVaultAfter = env.le(iouBroker.vaultKeylet()); if (BEAST_EXPECT(iouVaultAfter)) { @@ -1046,6 +1066,77 @@ class LoanRounding_test : public LoanTestBase } } + // Companion to the Vault_test dust-debit tests, which use a single + // depositor so AssetsTotal == AssetsAvailable and both debitRoundsToNoOp + // operands in VaultWithdraw::doApply trip together. Here a loan draws + // almost the entire vault, leaving AssetsTotal (1e7) far above + // AssetsAvailable (100): redeeming 1 share moves 1e-10 assets, which is + // dust against AssetsTotal but representable against AssetsAvailable, so + // the AssetsTotal operand alone carries the rejection. + void + testBugVaultWithdrawDustVsAssetsTotal(FeatureBitset features) + { + testcase("bug: VaultWithdraw dust debit vs AssetsTotal only"); + + using namespace jtx; + using namespace loan; + + bool const fixed = features[fixCleanup3_4_0]; + + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(10'000'000), issuer, lender, borrower); + env.close(); + + PrettyAsset const iouAsset = issuer[iouCurrency_]; + env(trust(lender, iouAsset(100'000'000))); + env(trust(borrower, iouAsset(100'000'000))); + env(pay(issuer, lender, iouAsset(20'000'000))); + env.close(); + + // Scale 10 so 1 share is worth 1e-10 assets against the 1e7 pool. + auto const broker = createVaultAndBroker( + env, + iouAsset, + lender, + {.vaultDeposit = 10'000'000, + .debtMax = 10'000'000, + .coverDeposit = 1'000'000, + .vaultScale = 10}); + + // Draw all but 100 units: AssetsAvailable drops to 100 while + // AssetsTotal stays at 1e7 (the loan is still an asset of the vault). + env(set(borrower, broker.brokerID, Number{9'999'900}), + Sig(sfCounterpartySignature, lender), + kPaymentTotal(2), + kPaymentInterval(600), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + auto const vaultSle = env.le(broker.vaultKeylet()); + if (!BEAST_EXPECT(vaultSle)) + return; + BEAST_EXPECT(vaultSle->at(sfAssetsTotal) == Number{10'000'000}); + BEAST_EXPECT(vaultSle->at(sfAssetsAvailable) == Number{100}); + + // 1 share redeems 1e7 * 1 / 1e17 = 1e-10 assets. Subtracting that + // from AssetsTotal needs 18 significant digits and canonicalizes + // straight back to 1e7 (no-op), while AssetsAvailable would become + // 99.9999999999 — perfectly representable. + auto const shareAsset = vaultSle->at(sfShareMPTID); + STAmount const oneShare{MPTIssue{shareAsset}, Number(1)}; + + Vault const v{env}; + env(v.withdraw({.depositor = lender, .id = broker.vaultKeylet().key, .amount = oneShare}), + Ter(fixed ? tecPRECISION_LOSS : tecINVARIANT_FAILED)); + env.close(); + } + // A near-zero interest rate on a 100 USD loan // produces total interest of ~6 units at loanScale -9. Numerical error // in the amortization formula pushes the theoretical principal above @@ -1125,6 +1216,8 @@ class LoanRounding_test : public LoanTestBase testBugOverpayUnroundedAmount(); testBugVaultWithdrawFixedSharesRoundsToZero(all_ - fixCleanup3_4_0); testBugVaultWithdrawFixedSharesRoundsToZero(all_); + testBugVaultWithdrawDustVsAssetsTotal(all_ - fixCleanup3_4_0); + testBugVaultWithdrawDustVsAssetsTotal(all_); testBugInterestDueDeltaCrash(); }