From f6c80fef683aedfac382b5334263da5f6c0e4351 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Fri, 11 Sep 2026 16:59:08 +0200 Subject: [PATCH] fix: Relax MPT authorize cap for LoanSet and VaultWithdraw --- src/libxrpl/tx/invariants/MPTInvariant.cpp | 59 ++++++---- .../app/invariants/InvariantsMPT_test.cpp | 69 ++++++++++++ src/test/app/lending/LoanSet_test.cpp | 102 ++++++++++++++++++ src/test/app/vault/VaultBugs_test.cpp | 12 +-- src/test/app/vault/VaultLifecycle_test.cpp | 80 ++++++++++++++ 5 files changed, 294 insertions(+), 28 deletions(-) diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index e38e8f2b93..46d1037acf 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -299,27 +299,46 @@ ValidMPTIssuance::finalize( return false; } } - else if (lendingProtocolEnabled && (mptokensCreated_ + mptokensDeleted_) > 1) + else { - JLOG(j.fatal()) << "Invariant failed: MPT authorize succeeded " - "but created/deleted bad number mptokens"; - return false; - } - else if (submittedByIssuer && (mptokensCreated_ > 0 || mptokensDeleted_ > 0)) - { - JLOG(j.fatal()) << "Invariant failed: MPT authorize submitted by issuer " - "succeeded but created/deleted mptokens"; - return false; - } - else if ( - !submittedByIssuer && hasPrivilege(tx, Privilege::MustAuthorizeMpt) && - (mptokensCreated_ + mptokensDeleted_ != 1)) - { - // if the holder submitted this tx, then a mptoken must be - // either created or deleted. - JLOG(j.fatal()) << "Invariant failed: MPT authorize submitted by holder " - "succeeded but created/deleted bad number of mptokens"; - return false; + // Cap on MPToken creates and deletes while featureLendingProtocol is enabled. + // - LoanSet: at most two creates and no deletes. + // - VaultWithdraw: at most one create and one delete. + // - Other MayAuthorizeMpt types: created + deleted <= 1. + // - MustAuthorizeMpt still requires exactly one create or delete below. + auto const mptokensExceedAuthorizeCap = [&] { + if (!lendingProtocolEnabled) + return false; + if (rules.enabled(fixCleanup3_4_0)) + { + if (txnType == ttLOAN_SET) + return mptokensDeleted_ != 0 || mptokensCreated_ > 2; + if (txnType == ttVAULT_WITHDRAW) + return mptokensCreated_ > 1 || mptokensDeleted_ > 1; + } + return (mptokensCreated_ + mptokensDeleted_) > 1; + }; + if (mptokensExceedAuthorizeCap()) + { + JLOG(j.fatal()) << "Invariant failed: MPT authorize succeeded " + "but created/deleted bad number mptokens"; + return false; + } + if (submittedByIssuer && (mptokensCreated_ > 0 || mptokensDeleted_ > 0)) + { + JLOG(j.fatal()) << "Invariant failed: MPT authorize submitted by issuer " + "succeeded but created/deleted mptokens"; + return false; + } + if (!submittedByIssuer && hasPrivilege(tx, Privilege::MustAuthorizeMpt) && + (mptokensCreated_ + mptokensDeleted_ != 1)) + { + // if the holder submitted this tx, then a mptoken must be + // either created or deleted. + JLOG(j.fatal()) << "Invariant failed: MPT authorize submitted by holder " + "succeeded but created/deleted bad number of mptokens"; + return false; + } } return true; diff --git a/src/test/app/invariants/InvariantsMPT_test.cpp b/src/test/app/invariants/InvariantsMPT_test.cpp index 4692463baa..91984f2021 100644 --- a/src/test/app/invariants/InvariantsMPT_test.cpp +++ b/src/test/app/invariants/InvariantsMPT_test.cpp @@ -967,6 +967,75 @@ class InvariantsMPT_test : public InvariantsBase }); } + // LoanSet / VaultWithdraw MayAuthorizeMpt caps (fixCleanup3_4_0): + // LoanSet allows at most two creates and no deletes; VaultWithdraw + // allows at most one of each. Fabricate one extra mutation so a + // too-loose cap would miss these. + { + auto const insertHolderTokens = + [](Account const& issuer, Account const& holder, ApplyContext& ac, int n) { + auto const sle = ac.view().peek(keylet::account(issuer.id())); + if (!sle) + return false; + auto seq = sle->getFieldU32(sfSequence); + for (int i = 0; i < n; ++i) + { + MPTIssue const mpt{makeMptID(seq + i, issuer)}; + auto sleNew = + std::make_shared(keylet::mptoken(mpt.getMptID(), holder)); + (*sleNew)[sfAccount] = holder.id(); + (*sleNew)[sfMPTokenIssuanceID] = mpt.getMptID(); + ac.view().insert(sleNew); + } + return true; + }; + + std::array, 2> const createOverCap{ + {{ttLOAN_SET, 3}, {ttVAULT_WITHDRAW, 2}}}; + for (auto const& [txnType, nTokens] : createOverCap) + { + doInvariantCheck( + {{"MPT authorize succeeded but created/deleted bad number mptokens"}}, + [&](Account const& a1, Account const& a2, ApplyContext& ac) { + return insertHolderTokens(a1, a2, ac, nTokens); + }, + XRPAmount{}, + STTx{txnType, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}); + } + + MPTID id; + auto const precloseTwoHolders = [&id](Account const& a1, Account const& a2, Env& env) { + Account const gw("gw"); + env.fund(XRP(1'000), gw); + MPTTester const mpt({.env = env, .issuer = gw, .holders = {a1, a2}}); + id = mpt.issuanceID(); + return true; + }; + std::array, 2> const deleteOverCap{ + {{ttLOAN_SET, 1}, {ttVAULT_WITHDRAW, 2}}}; + for (auto const& [txnType, nTokens] : deleteOverCap) + { + doInvariantCheck( + {{"MPT authorize succeeded but created/deleted bad number mptokens"}}, + [&](Account const& a1, Account const& a2, ApplyContext& ac) { + std::array const holders{a1, a2}; + for (int i = 0; i < nTokens; ++i) + { + auto sle = ac.view().peek(keylet::mptoken(id, holders[i])); + if (!sle) + return false; + ac.view().erase(sle); + } + return true; + }, + XRPAmount{}, + STTx{txnType, [](STObject&) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + precloseTwoHolders); + } + } + // sfReferenceHolding can only be set on creation by VaultCreate. A // non-VaultCreate transaction that creates an MPTokenIssuance with // sfReferenceHolding present must trip the invariant. diff --git a/src/test/app/lending/LoanSet_test.cpp b/src/test/app/lending/LoanSet_test.cpp index 5eea6f83fe..469c1662ad 100644 --- a/src/test/app/lending/LoanSet_test.cpp +++ b/src/test/app/lending/LoanSet_test.cpp @@ -22,6 +22,7 @@ #include #include #include +#include #include #include #include @@ -597,6 +598,105 @@ private: nullptr); } + void + testLoanSetOriginationFeeTwoMptCreates(FeatureBitset features) + { + using namespace jtx; + using namespace loan; + + bool const fix340Enabled = features[fixCleanup3_4_0]; + testcase << "LoanSet: borrower and broker owner missing MPToken" + << (fix340Enabled ? "" : " pre-fixCleanup3_4_0"); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + Env env(*this, features); + env.fund(XRP(1'000'000), issuer, lender, borrower); + env.close(); + + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create({.flags = tfMPTCanTransfer | tfMPTCanLock}); + env.close(); + PrettyAsset const asset = mptt.issuanceID(); + mptt.authorize({.account = lender}); + mptt.authorize({.account = borrower}); + env.close(); + + env(pay(issuer, lender, asset(10'000'000))); + env.close(); + + auto const broker = createVaultAndBroker(env, asset, lender); + + // Delete borrower's asset MPToken. + mptt.authorize({.account = borrower, .flags = tfMPTUnauthorize}); + env.close(); + + // Pay out and delete the broker owner's asset MPToken. + auto const lenderMPToken = keylet::mptoken(mptt.issuanceID(), lender); + auto const sleLenderMPT = env.le(lenderMPToken); + if (!BEAST_EXPECT(sleLenderMPT)) + return; + env(pay(lender, issuer, asset(sleLenderMPT->at(sfMPTAmount)))); + env.close(); + mptt.authorize({.account = lender, .flags = tfMPTUnauthorize}); + env.close(); + + auto const borrowerMPToken = keylet::mptoken(mptt.issuanceID(), borrower); + auto const brokerKeylet = keylet::loanBroker(broker.brokerID); + auto const sleBrokerBefore = env.le(brokerKeylet); + if (!BEAST_EXPECT(sleBrokerBefore)) + return; + auto const loanSequence = sleBrokerBefore->at(sfLoanSequence); + auto const debtTotalBefore = sleBrokerBefore->at(sfDebtTotal); + auto const loanKeylet = keylet::loan(broker.brokerID, SeqProxy::rawSequence(loanSequence)); + + auto const sleVaultBefore = env.le(keylet::vault(broker.vaultID)); + if (!BEAST_EXPECT(sleVaultBefore)) + return; + auto const assetsAvailableBefore = sleVaultBefore->at(sfAssetsAvailable); + + env(set(borrower, broker.brokerID, asset(1'000).value()), + kLoanOriginationFee(asset(1).value()), + kCounterparty(lender), + Sig(sfCounterpartySignature, lender), + Fee(env.current()->fees().base * 5), + Ter{fix340Enabled ? TER{tesSUCCESS} : TER{tecINVARIANT_FAILED}}); + env.close(); + + auto const sleBorrowerAfter = env.le(borrowerMPToken); + auto const sleLenderAfter = env.le(lenderMPToken); + auto const sleLoanAfter = env.le(loanKeylet); + auto const sleBrokerAfter = env.le(brokerKeylet); + auto const sleVaultAfter = env.le(keylet::vault(broker.vaultID)); + if (!BEAST_EXPECT(sleVaultAfter)) + return; + if (fix340Enabled) + { + if (!BEAST_EXPECT(sleBorrowerAfter && sleLenderAfter && sleLoanAfter && sleBrokerAfter)) + return; + BEAST_EXPECT(sleBorrowerAfter->at(sfMPTAmount) == 999); + BEAST_EXPECT(sleLenderAfter->at(sfMPTAmount) == 1); + BEAST_EXPECT(sleLoanAfter->at(sfPrincipalOutstanding) == Number{1'000}); + BEAST_EXPECT(sleBrokerAfter->at(sfLoanSequence) == loanSequence + 1); + BEAST_EXPECT( + sleVaultAfter->at(sfAssetsAvailable) == assetsAvailableBefore - Number{1'000}); + } + else + { + // The whole transaction must roll back. + BEAST_EXPECT(!sleBorrowerAfter); + BEAST_EXPECT(!sleLenderAfter); + BEAST_EXPECT(!sleLoanAfter); + if (!BEAST_EXPECT(sleBrokerAfter)) + return; + BEAST_EXPECT(sleBrokerAfter->at(sfLoanSequence) == loanSequence); + BEAST_EXPECT(sleBrokerAfter->at(sfDebtTotal) == debtTotalBefore); + BEAST_EXPECT(sleVaultAfter->at(sfAssetsAvailable) == assetsAvailableBefore); + } + } + // LoanSet in a closed-ended vault — phase gating and maturity bound. void testLoanSetClosedEnded() @@ -838,6 +938,8 @@ public: testLoanSetClosedEnded(); testLoanSetExistingLineAfterIssuerClearsDefaultRipple(); + testLoanSetOriginationFeeTwoMptCreates(all_); + testLoanSetOriginationFeeTwoMptCreates(all_ - fixCleanup3_4_0); } }; diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index cc30bd6091..43f2b0354c 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -1586,14 +1586,10 @@ private: // 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. + // ValidMPTIssuance: pre-fixCleanup3_4_0, a VaultWithdraw that both + // creates and deletes an MPToken fails. Post-fixCleanup3_4_0 that is + // allowed. + // // Post-fixCleanup3_4_0, doWithdraw skips addEmptyHolding on a zero // payout and zeroDeltaIsLegitimate lets the vault-delta and diff --git a/src/test/app/vault/VaultLifecycle_test.cpp b/src/test/app/vault/VaultLifecycle_test.cpp index ce91ca857a..8d7afe67f2 100644 --- a/src/test/app/vault/VaultLifecycle_test.cpp +++ b/src/test/app/vault/VaultLifecycle_test.cpp @@ -795,6 +795,86 @@ private: }, {.requireAuth = false}); + auto const redeemAllNoAssetMpt = [this](TER expected) { + return [this, expected]( + Env& env, + Account const&, + Account const& owner, + Account const& depositor, + Asset const& asset, + Vault& vault, + MPTTester& mptt) { + testcase << "MPT non-owner redeems all shares with no asset MPToken" + << (isTesSuccess(expected) ? "" : " pre-fixCleanup3_4_0"); + + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + env(tx); + env.close(); + + tx = vault.deposit( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(1000)}); // all assets held by depositor + env(tx); + env.close(); + + auto const vaultSle = env.le(keylet); + if (!BEAST_EXPECT(vaultSle)) + return; + auto const shareMPTID = vaultSle->at(sfShareMPTID); + + // Depositor's asset MPToken balance is now zero; delete it. + mptt.authorize({.account = depositor, .flags = tfMPTUnauthorize}); + env.close(); + + auto const mptoken = keylet::mptoken(mptt.issuanceID(), depositor); + + auto const shareKeylet = keylet::mptoken(shareMPTID, depositor.id()); + auto const sleShareBefore = env.le(shareKeylet); + if (!BEAST_EXPECT(sleShareBefore)) + return; + auto const shareAmountBefore = sleShareBefore->at(sfMPTAmount); + auto const assetsTotalBefore = vaultSle->at(sfAssetsTotal); + auto const assetsAvailableBefore = vaultSle->at(sfAssetsAvailable); + + // Redeeming ALL shares in one transaction both erases the + // now-empty share MPToken and re-creates the asset MPToken. + tx = vault.withdraw( + {.depositor = depositor, .id = keylet.key, .amount = asset(1000)}); + env(tx, Ter{expected}); + env.close(); + + auto const sleAsset = env.le(mptoken); + auto const sleShare = env.le(shareKeylet); + auto const vaultAfter = env.le(keylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + if (isTesSuccess(expected)) + { + if (!BEAST_EXPECT(sleAsset)) + return; + BEAST_EXPECT(sleAsset->at(sfMPTAmount) == 1000); + BEAST_EXPECT(!sleShare); + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == beast::kZero); + BEAST_EXPECT(vaultAfter->at(sfAssetsAvailable) == beast::kZero); + } + else + { + BEAST_EXPECT(!sleAsset); + if (!BEAST_EXPECT(sleShare)) + return; + BEAST_EXPECT(sleShare->at(sfMPTAmount) == shareAmountBefore); + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore); + BEAST_EXPECT(vaultAfter->at(sfAssetsAvailable) == assetsAvailableBefore); + } + }; + }; + + testCase(redeemAllNoAssetMpt(tesSUCCESS), {.requireAuth = false}); + testCase( + redeemAllNoAssetMpt(tecINVARIANT_FAILED), + {.requireAuth = false, .features = testableAmendments() - fixCleanup3_4_0}); + auto const [acctReserve, incReserve] = [this]() -> std::pair { Env const env{*this, testableAmendments()}; return {