From c979643d01be9ffd923bf972ff6db7053c412174 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Fri, 3 Apr 2026 19:41:45 +0200 Subject: [PATCH] fix: Add assorted Lending Protocol fixes (#6678) Co-authored-by: Shawn Xie <35279399+shawnxie999@users.noreply.github.com> --- src/test/app/Invariants_test.cpp | 121 +++++++++++++++------ src/test/app/Loan_test.cpp | 18 ++- src/xrpld/app/tx/detail/InvariantCheck.cpp | 29 +++-- src/xrpld/app/tx/detail/LoanManage.cpp | 35 +++--- src/xrpld/app/tx/detail/LoanPay.cpp | 4 +- 5 files changed, 153 insertions(+), 54 deletions(-) diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index 3237e61e35..25fa9db7c4 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -1939,39 +1939,41 @@ class Invariants_test : public beast::unit_test::suite { // Initialize with a placeholder value because there's no default // ctor - Keylet loanBrokerKeylet = keylet::amendments(); - Preclose createLoanBroker = [&, this]( - Account const& alice, - Account const& issuer, - Env& env) { - PrettyAsset const asset = [&]() { - switch (assetType) - { - case Asset::IOU: { - PrettyAsset const iouAsset = issuer["IOU"]; - env(trust(alice, iouAsset(1000))); - env(pay(issuer, alice, iouAsset(1000))); - env.close(); - return iouAsset; - } - - case Asset::MPT: { - MPTTester mptt{env, issuer, mptInitNoFund}; - mptt.create( - {.flags = tfMPTCanClawback | tfMPTCanTransfer | - tfMPTCanLock}); - PrettyAsset const mptAsset = mptt.issuanceID(); - mptt.authorize({.account = alice}); - env(pay(issuer, alice, mptAsset(1000))); - env.close(); - return mptAsset; - } - - case Asset::XRP: - default: - return PrettyAsset{xrpIssue(), 1'000'000}; + auto const setupAsset = [&](Account const& alice, + Account const& issuer, + Env& env) -> PrettyAsset { + switch (assetType) + { + case Asset::IOU: { + PrettyAsset const iouAsset = issuer["IOU"]; + env(trust(alice, iouAsset(1000))); + env(pay(issuer, alice, iouAsset(1000))); + env.close(); + return iouAsset; } - }(); + case Asset::MPT: { + MPTTester mptt{env, issuer, mptInitNoFund}; + mptt.create( + {.flags = tfMPTCanClawback | tfMPTCanTransfer | + tfMPTCanLock}); + PrettyAsset const mptAsset = mptt.issuanceID(); + mptt.authorize({.account = alice}); + env(pay(issuer, alice, mptAsset(1000))); + env.close(); + return mptAsset; + } + case Asset::XRP: + default: + return PrettyAsset{xrpIssue(), 1'000'000}; + } + }; + + Keylet loanBrokerKeylet = keylet::amendments(); + Preclose const createLoanBroker = [&, this]( + Account const& alice, + Account const& issuer, + Env& env) { + auto const asset = setupAsset(alice, issuer, env); loanBrokerKeylet = this->createLoanBroker(alice, env, asset); return BEAST_EXPECT(env.le(loanBrokerKeylet)); }; @@ -2159,6 +2161,61 @@ class Invariants_test : public beast::unit_test::suite STTx{ttLOAN_BROKER_SET, [](STObject& tx) {}}, {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, createLoanBroker); + + // Test: cover available less than pseudo-account asset balance + { + Keylet brokerKeylet = keylet::amendments(); + Preclose const createBrokerWithCover = + [&, this]( + Account const& alice, Account const& issuer, Env& env) { + auto const asset = setupAsset(alice, issuer, env); + brokerKeylet = + this->createLoanBroker(alice, env, asset); + if (!BEAST_EXPECT(env.le(brokerKeylet))) + return false; + env(loanBroker::coverDeposit( + alice, brokerKeylet.key, asset(10))); + env.close(); + return BEAST_EXPECT(env.le(brokerKeylet)); + }; + + doInvariantCheck( + {{"Loan Broker cover available is less than pseudo-account " + "asset balance"}}, + [&](Account const&, Account const&, ApplyContext& ac) { + auto sle = ac.view().peek(brokerKeylet); + if (!BEAST_EXPECT(sle)) + return false; + // Pseudo-account holds 10 units, set cover to 5 + sle->at(sfCoverAvailable) = Number(5); + ac.view().update(sle); + return true; + }, + XRPAmount{}, + STTx{ttLOAN_BROKER_SET, [](STObject& tx) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + createBrokerWithCover); + } + + // Test: cover available greater than pseudo-account asset balance + // (requires fixSecurity3_1_3) + doInvariantCheck( + {{"Loan Broker cover available is greater than pseudo-account " + "asset balance"}}, + [&](Account const&, Account const&, ApplyContext& ac) { + auto sle = ac.view().peek(loanBrokerKeylet); + if (!BEAST_EXPECT(sle)) + return false; + // Pseudo-account has no cover deposited; set cover + // higher than any incidental balance + sle->at(sfCoverAvailable) = Number(1'000'000); + ac.view().update(sle); + return true; + }, + XRPAmount{}, + STTx{ttLOAN_BROKER_SET, [](STObject& tx) {}}, + {tecINVARIANT_FAILED, tefINVARIANT_FAILED}, + createLoanBroker); } } diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index f2253cd0e7..f8d9940803 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -2273,7 +2273,23 @@ protected: fee(XRPAmount{ baseFee * (Number{15, -1} / loanPaymentsPerFeeIncrement + 1)}), - ter(temINVALID_FLAG)); + ter(tecNO_PERMISSION)); + + { + env.disableFeature(fixSecurity3_1_3); + env(pay(borrower, + loanKeylet.key, + STAmount{ + broker.asset, + state.periodicPayment * Number{15, -1}}, + tfLoanOverpayment), + fee(XRPAmount{ + baseFee * + (Number{15, -1} / loanPaymentsPerFeeIncrement + + 1)}), + ter(temINVALID_FLAG)); + env.enableFeature(fixSecurity3_1_3); + } } // Try to send a payment marked as multiple mutually exclusive // payment types. Do not include `txFlags`, so we don't duplicate diff --git a/src/xrpld/app/tx/detail/InvariantCheck.cpp b/src/xrpld/app/tx/detail/InvariantCheck.cpp index 6da1cc0f05..2d33636457 100644 --- a/src/xrpld/app/tx/detail/InvariantCheck.cpp +++ b/src/xrpld/app/tx/detail/InvariantCheck.cpp @@ -2532,18 +2532,33 @@ ValidLoanBroker::finalize( return false; } auto const& vaultAsset = vault->at(sfAsset); - if (after->at(sfCoverAvailable) < accountHolds( - view, - after->at(sfAccount), - vaultAsset, - FreezeHandling::fhIGNORE_FREEZE, - AuthHandling::ahIGNORE_AUTH, - j)) + auto const pseudoBalance = accountHolds( + view, + after->at(sfAccount), + vaultAsset, + FreezeHandling::fhIGNORE_FREEZE, + AuthHandling::ahIGNORE_AUTH, + j); + if (after->at(sfCoverAvailable) < pseudoBalance) { JLOG(j.fatal()) << "Invariant failed: Loan Broker cover available " "is less than pseudo-account asset balance"; return false; } + + if (view.rules().enabled(fixSecurity3_1_3)) + { + // Don't check the balance when LoanBroker is deleted, + // sfCoverAvailable is not zeroed + if (tx.getTxnType() != ttLOAN_BROKER_DELETE && + after->at(sfCoverAvailable) > pseudoBalance) + { + JLOG(j.fatal()) << "Invariant failed: Loan Broker cover " + "available is greater " + "than pseudo-account asset balance"; + return false; + } + } } return true; } diff --git a/src/xrpld/app/tx/detail/LoanManage.cpp b/src/xrpld/app/tx/detail/LoanManage.cpp index 6e04da7b5d..50db7d23cc 100644 --- a/src/xrpld/app/tx/detail/LoanManage.cpp +++ b/src/xrpld/app/tx/detail/LoanManage.cpp @@ -418,21 +418,30 @@ LoanManage::doApply() return tefBAD_LEDGER; // LCOV_EXCL_LINE auto const vaultAsset = vaultSle->at(sfAsset); - // Valid flag combinations are checked in preflight. No flags is valid - - // just a noop. - if (tx.isFlag(tfLoanDefault)) - return defaultLoan(view, loanSle, brokerSle, vaultSle, vaultAsset, j_); - if (tx.isFlag(tfLoanImpair)) - return impairLoan(view, loanSle, vaultSle, vaultAsset, j_); - if (tx.isFlag(tfLoanUnimpair)) - return unimpairLoan(view, loanSle, vaultSle, vaultAsset, j_); - // Noop, as described above. + auto const result = [&]() -> TER { + // Valid flag combinations are checked in preflight. No flags is valid - + // just a noop. + if (tx.isFlag(tfLoanDefault)) + return defaultLoan( + view, loanSle, brokerSle, vaultSle, vaultAsset, j_); + if (tx.isFlag(tfLoanImpair)) + return impairLoan(view, loanSle, vaultSle, vaultAsset, j_); + if (tx.isFlag(tfLoanUnimpair)) + return unimpairLoan(view, loanSle, vaultSle, vaultAsset, j_); + // Noop, as described above. + return tesSUCCESS; + }(); - associateAsset(*loanSle, vaultAsset); - associateAsset(*brokerSle, vaultAsset); - associateAsset(*vaultSle, vaultAsset); + // Pre-amendment, associateAsset was only called on the noop (no flags) + // path. Post-amendment, we call associateAsset on all successful paths. + if (view.rules().enabled(fixSecurity3_1_3) && isTesSuccess(result)) + { + associateAsset(*loanSle, vaultAsset); + associateAsset(*brokerSle, vaultAsset); + associateAsset(*vaultSle, vaultAsset); + } - return tesSUCCESS; + return result; } //------------------------------------------------------------------------------ diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index 675cf10da9..f606ab91cb 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -149,7 +149,9 @@ LoanPay::preclaim(PreclaimContext const& ctx) { JLOG(ctx.j.warn()) << "Requested overpayment on a loan that doesn't allow it"; - return temINVALID_FLAG; + return ctx.view.rules().enabled(fixSecurity3_1_3) + ? TER{tecNO_PERMISSION} + : temINVALID_FLAG; } auto const principalOutstanding = loanSle->at(sfPrincipalOutstanding);