fix: Allow zero-value MPT vault withdraw when the asset holding is missing

A fully impaired pool can legitimately pay nothing for a share burn.
doWithdraw used to insert an empty self-destination MPToken for that
zero payout, which tripped ValidVault and ValidMPTIssuance.

src/libxrpl/ledger/View.cpp

@@ -543,12 +543,19 @@ doWithdraw(
 {
     auto const dstSle = ctx.view.read(keylet::account(dstAcct));
 
-    // Create trust line or MPToken for the receiving account
+    // Create a trust line or MPToken for a self-destination only when there
+    // is a payout to credit. Post-fixCleanup3_4_0, a zero-value withdraw
+    // (e.g. share redemption from a fully impaired vault) must not insert
+    // an empty holding: that records a one-sided zero delta and can also
+    // create+delete MPTokens in the same transaction.
     if (dstAcct == senderAcct)
     {
-        if (auto const ter = addEmptyHolding(ctx, senderAcct, priorBalance, amount.asset(), j);
-            !isTesSuccess(ter) && ter != tecDUPLICATE)
-            return ter;
+        if (amount > beast::kZero || !ctx.view.rules().enabled(fixCleanup3_4_0))
+        {
+            if (auto const ter = addEmptyHolding(ctx, senderAcct, priorBalance, amount.asset(), j);
+                !isTesSuccess(ter) && ter != tecDUPLICATE)
+                return ter;
+        }
     }
     else
     {

src/libxrpl/tx/invariants/VaultInvariant.cpp

@@ -1069,12 +1069,14 @@ ValidVault::finalize(
                         // 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.
+                        // XRP and MPT remain strict for rounding artifacts. A zero destination
+                        // delta is still allowed when zeroDeltaIsLegitimate, as a backstop if
+                        // some other apply path records a one-sided zero change (pre-amendment
+                        // doWithdraw still inserts an empty self-destination holding).
                         bool const tolerateZeroDelta =
                             view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral();
-                        auto const invalidBalanceChange = tolerateZeroDelta
+                        auto const invalidBalanceChange =
+                            (tolerateZeroDelta || zeroDeltaIsLegitimate)
                             ? roundedDestinationDelta < kZero
                             : roundedDestinationDelta <= kZero;
                         if (invalidBalanceChange)

src/test/app/vault/VaultBugs_test.cpp

@@ -7,6 +7,7 @@
 #include <test/jtx/credentials.h>
 #include <test/jtx/fee.h>
 #include <test/jtx/flags.h>
+#include <test/jtx/mpt.h>
 #include <test/jtx/pay.h>
 #include <test/jtx/sig.h>
 #include <test/jtx/ter.h>
@@ -1674,6 +1675,193 @@ private:
         }
     }
 
+    // Bug: a fully impaired vault may pay zero assets for a share burn.
+    // doWithdraw used to call addEmptyHolding for a self-destination even
+    // when the payout was zero, so a missing asset MPToken was created at
+    // amount 0. ValidVault then saw a one-sided zero destination delta and
+    // fired for integral assets; redeeming the last share in the same tx
+    // also created that token while deleting the share MPToken, which
+    // ValidMPTIssuance rejects (created + deleted > 1).
+    //
+    // Post-fixCleanup3_4_0, doWithdraw skips addEmptyHolding on a zero
+    // payout. ValidVault also treats a one-sided zero destination delta as
+    // valid when zeroDeltaIsLegitimate.
+    void
+    testBugMptZeroWithdrawMissingHolding()
+    {
+        using namespace test::jtx;
+        using namespace loan_broker;
+        using namespace loan;
+        using namespace std::chrono_literals;
+
+        auto runScenario = [this](
+                               FeatureBitset features,
+                               bool removeAssetToken,
+                               bool withdrawAllShares,
+                               TER expected) {
+            testcase(
+                std::string{"bug: MPT vault zero-value withdraw "} +
+                (removeAssetToken ? "without asset MPToken" : "with asset MPToken (control)") +
+                (withdrawAllShares ? ", last share" : ", leftover shares") +
+                (features[fixCleanup3_4_0] ? " (post-fixCleanup3_4_0)" : " (pre-fixCleanup3_4_0)"));
+
+            Env env(*this, features);
+
+            Account const issuer{"issuer"};
+            Account const owner{"owner"};
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+            Account const borrower{"borrower"};
+
+            env.fund(XRP(100'000), issuer, owner, alice, bob, borrower);
+            env.close();
+
+            MPTTester mptt{env, issuer, kMptInitNoFund};
+            mptt.create({.flags = tfMPTCanTransfer});
+            PrettyAsset const asset = mptt.issuanceID();
+            mptt.authorize({.account = owner});
+            mptt.authorize({.account = alice});
+            mptt.authorize({.account = bob});
+            mptt.authorize({.account = borrower});
+            env.close();
+
+            env(pay(issuer, alice, asset(2)));
+            env(pay(issuer, bob, asset(8)));
+            env.close();
+
+            Vault const vault{env};
+            auto const [createTx, vaultKeylet, subscriptionDate] = vault.createClosedEnded(
+                {.owner = owner, .asset = asset, .subscriptionOffset = 60s});
+            env(createTx);
+            env.close();
+
+            env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(2)}));
+            env(vault.deposit({.depositor = bob, .id = vaultKeylet.key, .amount = asset(8)}));
+            env.close();
+
+            vault.closePastSubscription(subscriptionDate);
+
+            auto const brokerKeylet =
+                keylet::loanBroker(owner.id(), SeqProxy::rawSequence(env.seq(owner)));
+            env(set(owner, vaultKeylet.key));
+            env.close();
+
+            auto const sleBroker = env.le(brokerKeylet);
+            if (!BEAST_EXPECT(sleBroker))
+                return;
+            auto const loanKeylet = keylet::loan(
+                brokerKeylet.key, SeqProxy::rawSequence(sleBroker->at(sfLoanSequence)));
+
+            env(set(borrower, brokerKeylet.key, asset(10).value()),
+                kInterestRate(percentageToTenthBips(0)),
+                kGracePeriod(60),
+                kPaymentInterval(120),
+                kPaymentTotal(10),
+                Sig(sfCounterpartySignature, owner),
+                Fee(env.current()->fees().base * 2),
+                Ter(tesSUCCESS));
+            env.close();
+
+            auto const loanBefore = env.le(loanKeylet);
+            if (!BEAST_EXPECT(loanBefore))
+                return;
+            std::uint32_t const dueDate = loanBefore->at(sfNextPaymentDueDate);
+            env.close(NetClock::time_point{NetClock::duration{dueDate}} + 1s);
+
+            env(manage(owner, loanKeylet.key, tfLoanImpair), Ter(tesSUCCESS));
+            env.close();
+
+            auto const vaultImpaired = env.le(vaultKeylet);
+            if (!BEAST_EXPECT(vaultImpaired))
+                return;
+            BEAST_EXPECT(vaultImpaired->at(sfAssetsAvailable) == asset(0).value());
+            BEAST_EXPECT(vaultImpaired->at(sfAssetsTotal) == vaultImpaired->at(sfLossUnrealized));
+            Number const totalBefore = vaultImpaired->at(sfAssetsTotal);
+            Number const lossBefore = vaultImpaired->at(sfLossUnrealized);
+
+            MPTID const shareId = vaultImpaired->at(sfShareMPTID);
+            auto const issuanceBefore = env.le(keylet::mptokenIssuance(shareId));
+            if (!BEAST_EXPECT(issuanceBefore))
+                return;
+            std::uint64_t const outstandingBefore =
+                issuanceBefore->getFieldU64(sfOutstandingAmount);
+
+            auto const tokenAlice = env.le(keylet::mptoken(shareId, alice.id()));
+            if (!BEAST_EXPECT(tokenAlice))
+                return;
+            std::uint64_t const sharesBefore = tokenAlice->getFieldU64(sfMPTAmount);
+            BEAST_EXPECT(sharesBefore == 2);
+            std::uint64_t const sharesToRedeem = withdrawAllShares ? sharesBefore : 1;
+            STAmount const redeemShares{MPTIssue{shareId}, Number(sharesToRedeem)};
+
+            auto const assetTokenKeylet = keylet::mptoken(mptt.issuanceID(), alice.id());
+            if (removeAssetToken)
+            {
+                mptt.authorize({.account = alice, .flags = tfMPTUnauthorize});
+                env.close();
+                BEAST_EXPECT(!env.le(assetTokenKeylet));
+            }
+            else
+            {
+                auto const existing = env.le(assetTokenKeylet);
+                if (!BEAST_EXPECT(existing))
+                    return;
+                BEAST_EXPECT(existing->getFieldU64(sfMPTAmount) == 0);
+            }
+
+            std::uint32_t const redemptionDate = vaultImpaired->at(sfRedemptionDate);
+            env.close(NetClock::time_point{NetClock::duration{redemptionDate}} + 1s);
+
+            env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = redeemShares}),
+                Ter(expected));
+            env.close();
+            if (expected != tesSUCCESS)
+                return;
+
+            if (removeAssetToken)
+            {
+                BEAST_EXPECT(!env.le(assetTokenKeylet));
+            }
+            else
+            {
+                auto const assetAfter = env.le(assetTokenKeylet);
+                if (!BEAST_EXPECT(assetAfter))
+                    return;
+                BEAST_EXPECT(assetAfter->getFieldU64(sfMPTAmount) == 0);
+            }
+
+            auto const shareAfter = env.le(keylet::mptoken(shareId, alice.id()));
+            if (withdrawAllShares)
+            {
+                BEAST_EXPECT(!shareAfter);
+            }
+            else if (BEAST_EXPECT(shareAfter))
+            {
+                BEAST_EXPECT(shareAfter->getFieldU64(sfMPTAmount) == sharesBefore - sharesToRedeem);
+            }
+
+            auto const vaultAfter = env.le(vaultKeylet);
+            if (!BEAST_EXPECT(vaultAfter))
+                return;
+            BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == totalBefore);
+            BEAST_EXPECT(vaultAfter->at(sfLossUnrealized) == lossBefore);
+            BEAST_EXPECT(vaultAfter->at(sfAssetsAvailable) == asset(0).value());
+
+            auto const issuanceAfter = env.le(keylet::mptokenIssuance(shareId));
+            if (!BEAST_EXPECT(issuanceAfter))
+                return;
+            BEAST_EXPECT(
+                issuanceAfter->getFieldU64(sfOutstandingAmount) ==
+                outstandingBefore - sharesToRedeem);
+        };
+
+        runScenario(all_, false, false, tesSUCCESS);
+        runScenario(all_, false, true, tesSUCCESS);
+        runScenario(all_, true, false, tesSUCCESS);
+        runScenario(all_, true, true, tesSUCCESS);
+        runScenario(all_ - fixCleanup3_4_0, true, true, tecINVARIANT_FAILED);
+    }
+
 public:
     void
     run() override
@@ -1695,6 +1883,7 @@ public:
         testBugClawbackRoundTripOvershoot();
         testBugWithdrawRoundTripOvershoot();
         testBugClawbackAfterLoanImpair();
+        testBugMptZeroWithdrawMissingHolding();
     }
 };