diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index dfdac5551e..69c3ce92e0 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -1129,14 +1129,10 @@ 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 for rounding artifacts. A zero destination - // delta is still allowed when zeroDeltaIsLegitimate, to tolerate a - // one-sided rounded-zero change during a legitimate zero-value - // withdrawal. + // XRP and MPT remain strict for rounding artifacts. bool const tolerateZeroDelta = view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral(); - auto const invalidBalanceChange = - (tolerateZeroDelta || zeroDeltaIsLegitimate) + auto const invalidBalanceChange = tolerateZeroDelta ? 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 0cdbdb564d..552b7b44d8 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -1680,9 +1680,10 @@ private: // 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). + // fired for integral assets; redeeming Alice's last share in the same tx + // also created that token while deleting Alice's share MPToken, which + // ValidMPTIssuance rejects (created + deleted > 1). Bob still owns shares, + // so this is not the vault's final outstanding share. // // Post-fixCleanup3_4_0, doWithdraw skips addEmptyHolding on a zero // payout. ValidVault also treats a one-sided zero destination delta as @@ -1698,12 +1699,12 @@ private: auto runScenario = [this]( FeatureBitset features, bool removeAssetToken, - bool withdrawAllShares, + bool withdrawAllAliceShares, TER expected) { testcase( std::string{"bug: MPT vault zero-value withdraw "} + (removeAssetToken ? "without asset MPToken" : "with asset MPToken (control)") + - (withdrawAllShares ? ", last share" : ", leftover shares") + + (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); @@ -1792,7 +1793,7 @@ private: return; std::uint64_t const sharesBefore = tokenAlice->getFieldU64(sfMPTAmount); BEAST_EXPECT(sharesBefore == 2); - std::uint64_t const sharesToRedeem = withdrawAllShares ? sharesBefore : 1; + std::uint64_t const sharesToRedeem = withdrawAllAliceShares ? sharesBefore : 1; STAmount const redeemShares{MPTIssue{shareId}, Number(sharesToRedeem)}; auto const assetTokenKeylet = keylet::mptoken(mptt.issuanceID(), alice.id()); @@ -1832,7 +1833,7 @@ private: } auto const shareAfter = env.le(keylet::mptoken(shareId, alice.id())); - if (withdrawAllShares) + if (withdrawAllAliceShares) { BEAST_EXPECT(!shareAfter); } @@ -1856,15 +1857,150 @@ private: outstandingBefore - sharesToRedeem); }; - runScenario(all_, false /* removeAssetToken */, false /* withdrawAllShares */, tesSUCCESS); - runScenario(all_, false /* removeAssetToken */, true /* withdrawAllShares */, tesSUCCESS); - runScenario(all_, true /* removeAssetToken */, false /* withdrawAllShares */, tesSUCCESS); - runScenario(all_, true /* removeAssetToken */, true /* withdrawAllShares */, tesSUCCESS); + 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, true /* removeAssetToken */, - true /* withdrawAllShares */, + 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 of her two + // shares after the vault is fully impaired. Bob's eight shares keep 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); + 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); + + 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 @@ -2353,6 +2489,7 @@ public: testBugWithdrawRoundTripOvershoot(); testBugClawbackAfterLoanImpair(); testBugMptZeroWithdrawMissingHolding(); + testBugIouZeroWithdrawMissingTrustLine(); testBugXrpZeroWithdrawSponsoredFee(); testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient(); testBugSponsorAsDestinationFeeMisappliedToPayout();