From c0ee81366674cd7ca395139d3bc2ea2cc0f943b2 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> --- .../tx/invariants/LoanBrokerInvariant.cpp | 28 +++-- .../tx/transactors/lending/LoanManage.cpp | 34 +++--- .../tx/transactors/lending/LoanPay.cpp | 2 +- src/test/app/Invariants_test.cpp | 104 +++++++++++++----- src/test/app/Loan_test.cpp | 14 ++- 5 files changed, 133 insertions(+), 49 deletions(-) diff --git a/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp b/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp index 2bc9e622ad..8ee0a0deb8 100644 --- a/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp +++ b/src/libxrpl/tx/invariants/LoanBrokerInvariant.cpp @@ -175,18 +175,32 @@ 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/libxrpl/tx/transactors/lending/LoanManage.cpp b/src/libxrpl/tx/transactors/lending/LoanManage.cpp index adef5374b9..8c3e625963 100644 --- a/src/libxrpl/tx/transactors/lending/LoanManage.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanManage.cpp @@ -386,21 +386,29 @@ 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/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index f748a670fd..d400fb3630 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -155,7 +155,7 @@ LoanPay::preclaim(PreclaimContext const& ctx) if (tx.isFlag(tfLoanOverpayment) && !loanSle->isFlag(lsfLoanOverpayment)) { 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); diff --git a/src/test/app/Invariants_test.cpp b/src/test/app/Invariants_test.cpp index ef0624b481..72ed7ba57b 100644 --- a/src/test/app/Invariants_test.cpp +++ b/src/test/app/Invariants_test.cpp @@ -2046,36 +2046,36 @@ class Invariants_test : public beast::unit_test::suite { // Initialize with a placeholder value because there's no default // ctor + 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) { - 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 asset = setupAsset(alice, issuer, env); loanBrokerKeylet = this->createLoanBroker(alice, env, asset); return BEAST_EXPECT(env.le(loanBrokerKeylet)); }; @@ -2249,6 +2249,56 @@ 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 a63d31f030..05123c11c0 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -2071,7 +2071,19 @@ protected: STAmount{broker.asset, state.periodicPayment * Number{15, -1}}, tfLoanOverpayment), 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