From 346ea40f69f9bc316c0aae4ba8b3f20cb3f9f7cc Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Wed, 2 Sep 2026 13:45:52 +0000 Subject: [PATCH] fix: Allow zero-value MPT vault withdraw when the asset holding is missing (#8153) --- src/libxrpl/ledger/View.cpp | 15 +- src/libxrpl/tx/invariants/VaultInvariant.cpp | 4 +- src/test/app/vault/VaultBugs_test.cpp | 493 +++++++++++++++++++ 3 files changed, 505 insertions(+), 7 deletions(-) diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp index 0cd082ff47..75a49187b4 100644 --- a/src/libxrpl/ledger/View.cpp +++ b/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 { diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index ff3a20c8ff..69c3ce92e0 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -1129,9 +1129,7 @@ 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. bool const tolerateZeroDelta = view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral(); auto const invalidBalanceChange = tolerateZeroDelta diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index 5b6e756e59..02949b8619 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include @@ -1677,6 +1678,495 @@ private: } } + // Bug: a fully impaired vault may pay zero assets for a share burn. + // Sending zero MPT is a no-op, so the vault pseudo-account's asset + // MPToken is never written and ValidVault, which only records deltas for + // created, modified or deleted entries, sees no vault delta at all. + // + // Pre-fixCleanup3_4_0 that alone makes the withdrawal impossible: + // zeroDeltaIsLegitimate is gated on the amendment, so the absent vault + // delta fails "withdrawal must change vault balance". Every pre-amendment + // arm below dies there, before any destination-side check runs. + // + // The destination side differs per arm, and only the vault-delta return + // hides that pre-amendment. With Alice's asset MPToken already present + // nothing touches it, so she has no delta either. With it missing, + // doWithdraw still called addEmptyHolding for a self-destination on a + // zero payout and created her MPToken at amount 0; a created MPToken is + // recorded even at zero, so she arrives with a present-and-zero delta, + // which for an integral MPT asset the destination check would reject if + // it were reached. + // + // ValidMPTIssuance is a separate checker and still runs. It only trips on + // the one arm that both creates and deletes an MPToken: Alice's last + // share with the asset MPToken missing, where addEmptyHolding creates the + // asset token while her share token is deleted (created + deleted > 1). + // Leftover shares with the token missing is create-only, and a last share + // with the token present is delete-only; neither exceeds one. Bob still + // owns shares throughout, so this is never the vault's final outstanding + // share. + // + // Post-fixCleanup3_4_0, doWithdraw skips addEmptyHolding on a zero + // payout and zeroDeltaIsLegitimate lets the vault-delta and + // missing-recipient-delta checks accept the transfer. A present + // destination delta of zero is still rejected. + 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 withdrawAllAliceShares, + TER expected) { + testcase( + std::string{"bug: MPT vault zero-value withdraw "} + + (removeAssetToken ? "without asset MPToken" : "with asset MPToken") + + (withdrawAllAliceShares ? ", Alice's last share" : ", Alice has 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 = withdrawAllAliceShares ? 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 (withdrawAllAliceShares) + { + 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 /* removeAssetToken */, false /* withdrawAllAliceShares */, tesSUCCESS); + runScenario( + all_, false /* removeAssetToken */, true /* withdrawAllAliceShares */, tesSUCCESS); + runScenario( + all_, true /* removeAssetToken */, false /* withdrawAllAliceShares */, tesSUCCESS); + runScenario( + all_, true /* removeAssetToken */, true /* withdrawAllAliceShares */, tesSUCCESS); + runScenario( + all_ - fixCleanup3_4_0, + false /* removeAssetToken */, + false /* withdrawAllAliceShares */, + tecINVARIANT_FAILED); + runScenario( + all_ - fixCleanup3_4_0, + false /* removeAssetToken */, + true /* withdrawAllAliceShares */, + tecINVARIANT_FAILED); + runScenario( + all_ - fixCleanup3_4_0, + true /* removeAssetToken */, + false /* withdrawAllAliceShares */, + tecINVARIANT_FAILED); + runScenario( + all_ - fixCleanup3_4_0, + true /* removeAssetToken */, + true /* withdrawAllAliceShares */, + tecINVARIANT_FAILED); + } + + // IOU analogue of the missing-MPToken case above. Alice removes her + // zero-balance trust line after depositing, then burns one unit from her + // scaled share balance after the vault is fully impaired. Bob's share + // balance keeps this out of the sole-shareholder loss-waiver and + // final-outstanding-share paths. A zero payout must not recreate Alice's + // unsolicited trust line. + void + testBugIouZeroWithdrawMissingTrustLine() + { + using namespace test::jtx; + using namespace loan_broker; + using namespace loan; + using namespace std::chrono_literals; + + Env env(*this, all_); + + 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(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const asset = issuer["USD"]; + env.trust(asset(100), owner); + env.trust(asset(100), alice); + env.trust(asset(100), bob); + env.trust(asset(100), 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(); + + auto const assetLine = keylet::trustLine(alice, asset.raw().get()); + if (!BEAST_EXPECT(env.le(assetLine))) + return; + env.trust(asset(0), alice); + env.close(); + BEAST_EXPECT(!env.le(assetLine)); + + 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 tokenAlice = env.le(keylet::mptoken(shareId, alice.id())); + if (!BEAST_EXPECT(tokenAlice)) + return; + std::uint64_t const sharesBefore = tokenAlice->getFieldU64(sfMPTAmount); + // Default IOU vault scale is 6, so 2 USD mints 2e6 shares. Redeem one + // leftover share; do not require 1:1 like the MPT case. + BEAST_EXPECT(sharesBefore > 1); + STAmount const redeemShares{MPTIssue{shareId}, Number(1)}; + + 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(tesSUCCESS)); + env.close(); + + // A regression in the View guard would recreate this line even though + // no asset value was paid. + BEAST_EXPECT(!env.le(assetLine)); + + auto const shareAfter = env.le(keylet::mptoken(shareId, alice.id())); + if (!BEAST_EXPECT(shareAfter)) + return; + BEAST_EXPECT(shareAfter->getFieldU64(sfMPTAmount) == sharesBefore - 1); + + 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()); + } + + // Same zero-payout withdrawal as testBugMptZeroWithdrawMissingHolding, but + // the vault asset is XRP. addEmptyHolding is a no-op for native assets. + // Sequence processing still touches the sender AccountRoot; a sponsored + // fee leaves that XRP balance economically unchanged. After the + // sponsored-withdraw fee-payer fix, deltaAssetsForParty collapses that + // economically-zero XRP delta to absence, so tesSUCCESS takes the + // missing-recipient-delta arm gated by zeroDeltaIsLegitimate. This test + // covers that live SUCCESS path. Pre-fixCleanup3_4_0 still fails the + // invariant. + void + testBugXrpZeroWithdrawSponsoredFee() + { + using namespace test::jtx; + using namespace loan_broker; + using namespace loan; + using namespace std::chrono_literals; + + auto runScenario = [this](FeatureBitset features, TER expected) { + testcase( + std::string{"bug: XRP vault zero-value withdraw with sponsored fee"} + + (features[fixCleanup3_4_0] ? " (post-fixCleanup3_4_0)" : " (pre-fixCleanup3_4_0)")); + + Env env(*this, features); + + Account const owner{"owner"}; + Account const alice{"alice"}; + Account const bob{"bob"}; + Account const borrower{"borrower"}; + Account const sponsor{"sponsor"}; + + env.fund(XRP(100'000), owner, alice, bob, borrower, sponsor); + env.close(); + + PrettyAsset const asset{xrpIssue()}; + 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 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); + STAmount const redeemShares{MPTIssue{shareId}, Number(1)}; + + std::uint32_t const redemptionDate = vaultImpaired->at(sfRedemptionDate); + env.close(NetClock::time_point{NetClock::duration{redemptionDate}} + 1s); + + auto const aliceBalanceBefore = env.balance(alice); + auto const sponsorBalanceBefore = env.balance(sponsor); + auto const fee = env.current()->fees().base; + + env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = redeemShares}), + Fee(fee), + sponsor::As(sponsor, spfSponsorFee), + Sig(sfSponsorSignature, sponsor), + Ter(expected)); + env.close(); + + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore - fee); + BEAST_EXPECT(env.balance(alice) == aliceBalanceBefore); + + if (expected != tesSUCCESS) + return; + + auto const shareAfter = env.le(keylet::mptoken(shareId, alice.id())); + if (!BEAST_EXPECT(shareAfter)) + return; + BEAST_EXPECT(shareAfter->getFieldU64(sfMPTAmount) == sharesBefore - 1); + + 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()); + }; + + runScenario(all_, tesSUCCESS); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); + } + // addEmptyHolding() used to check isGlobalFrozen(issuer) and // !lsfDefaultRipple before the "line already exists" tecDUPLICATE // short circuit. doWithdraw() calls addEmptyHolding() for a @@ -2383,6 +2873,9 @@ public: testBugClawbackRoundTripOvershoot(); testBugWithdrawRoundTripOvershoot(); testBugClawbackAfterLoanImpair(); + testBugMptZeroWithdrawMissingHolding(); + testBugIouZeroWithdrawMissingTrustLine(); + testBugXrpZeroWithdrawSponsoredFee(); testBugSelfWithdrawAfterIssuerClearsDefaultRipple(); testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient(); testBugSponsorAsDestinationFeeMisappliedToPayout();