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

include/xrpl/ledger/helpers/VaultHelpers.h

@@ -1,5 +1,6 @@
 #pragma once
 
+#include <xrpl/basics/Number.h>
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Protocol.h>
@@ -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

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

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/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

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