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;
@@ -844,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;
+                        // 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 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)
+                    else
                     {
-                        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,
-                            maybeVaultDeltaAssets->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;
+                        // 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;
+                        }
                     }
                 }
 

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