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 a8ef0d3157..ed2c31d1c9 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/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) diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index 04f31c9526..3594942387 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 @@ -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(); } };