fix: Reject VaultWithdraw fixed-share amounts that round to zero

include/xrpl/ledger/helpers/VaultHelpers.h

@@ -1,7 +1,9 @@
 #pragma once
 
+#include <xrpl/basics/Number.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/protocol/AccountID.h>
+#include <xrpl/protocol/Asset.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/STAmount.h>
 #include <xrpl/protocol/STLedgerEntry.h>
@@ -52,6 +54,37 @@ 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);
+
+/**
+ * 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

src/libxrpl/ledger/helpers/VaultHelpers.cpp

@@ -65,6 +65,23 @@ 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]] 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<STAmount>
 assetsToSharesWithdraw(
     SLE::const_ref vault,
@@ -80,9 +97,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 +123,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;

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<DeltaInfo> {

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);

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

@@ -273,6 +273,44 @@ VaultWithdraw::doApply()
         return tecPATH_DRY;
     }
 
+    // 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)};
+
+    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)
+    {
+        // 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
     // (checkWithdrawFreeze), so IgnoreFreeze avoids a redundant check that
     // would incorrectly return zero for vault pseudo-accounts whose shares
@@ -287,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)
     {
@@ -309,8 +340,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

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();