From fe4269cf8bcd673407a43f28b9c7c6079571ff5c Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Fri, 24 Oct 2025 18:33:57 -0400 Subject: [PATCH] Avoid negative payments: limit origination fee to the loan principal - Addresses FIND-006 from audit. - Removes the "minimum" check for sfLoanOriginationFee, and replaces it with a "valid range" check with the max value being sfPrincipalRequested. - Reuses the test from the report, with some modifications. - Also adds some more test cases for existing interest rate tests. --- src/test/app/Loan_test.cpp | 108 +++++++++++++++++++++++++++ src/xrpld/app/tx/detail/LoanSet.cpp | 10 +-- src/xrpld/app/tx/detail/Transactor.h | 2 +- 3 files changed, 114 insertions(+), 6 deletions(-) diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index d053530236..752d50f104 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -893,12 +893,23 @@ class Loan_test : public beast::unit_test::suite interestRate(maxInterestRate), loanSetFee, ter(tefBAD_AUTH)); + env(set(evan, broker.brokerID, principalRequest), + sig(sfCounterpartySignature, borrower), + interestRate(TenthBips32(0)), + loanSetFee, + ter(tefBAD_AUTH)); // sfInterestRate: too big env(set(evan, broker.brokerID, principalRequest), sig(sfCounterpartySignature, lender), interestRate(maxInterestRate + 1), loanSetFee, ter(temINVALID)); + // sfInterestRate: too small + env(set(evan, broker.brokerID, principalRequest), + sig(sfCounterpartySignature, lender), + interestRate(TenthBips32(-1)), + loanSetFee, + ter(temINVALID)); // sfLateInterestRate: good value, bad account env(set(evan, broker.brokerID, principalRequest), @@ -906,12 +917,23 @@ class Loan_test : public beast::unit_test::suite lateInterestRate(maxLateInterestRate), loanSetFee, ter(tefBAD_AUTH)); + env(set(evan, broker.brokerID, principalRequest), + sig(sfCounterpartySignature, borrower), + lateInterestRate(TenthBips32(0)), + loanSetFee, + ter(tefBAD_AUTH)); // sfLateInterestRate: too big env(set(evan, broker.brokerID, principalRequest), sig(sfCounterpartySignature, lender), lateInterestRate(maxLateInterestRate + 1), loanSetFee, ter(temINVALID)); + // sfLateInterestRate: too small + env(set(evan, broker.brokerID, principalRequest), + sig(sfCounterpartySignature, lender), + lateInterestRate(TenthBips32(-1)), + loanSetFee, + ter(temINVALID)); // sfCloseInterestRate: good value, bad account env(set(evan, broker.brokerID, principalRequest), @@ -919,12 +941,22 @@ class Loan_test : public beast::unit_test::suite closeInterestRate(maxCloseInterestRate), loanSetFee, ter(tefBAD_AUTH)); + env(set(evan, broker.brokerID, principalRequest), + sig(sfCounterpartySignature, borrower), + closeInterestRate(TenthBips32(0)), + loanSetFee, + ter(tefBAD_AUTH)); // sfCloseInterestRate: too big env(set(evan, broker.brokerID, principalRequest), sig(sfCounterpartySignature, lender), closeInterestRate(maxCloseInterestRate + 1), loanSetFee, ter(temINVALID)); + env(set(evan, broker.brokerID, principalRequest), + sig(sfCounterpartySignature, lender), + closeInterestRate(TenthBips32(-1)), + loanSetFee, + ter(temINVALID)); // sfOverpaymentInterestRate: good value, bad account env(set(evan, broker.brokerID, principalRequest), @@ -932,12 +964,22 @@ class Loan_test : public beast::unit_test::suite overpaymentInterestRate(maxOverpaymentInterestRate), loanSetFee, ter(tefBAD_AUTH)); + env(set(evan, broker.brokerID, principalRequest), + sig(sfCounterpartySignature, borrower), + overpaymentInterestRate(TenthBips32(0)), + loanSetFee, + ter(tefBAD_AUTH)); // sfOverpaymentInterestRate: too big env(set(evan, broker.brokerID, principalRequest), sig(sfCounterpartySignature, lender), overpaymentInterestRate(maxOverpaymentInterestRate + 1), loanSetFee, ter(temINVALID)); + env(set(evan, broker.brokerID, principalRequest), + sig(sfCounterpartySignature, lender), + overpaymentInterestRate(TenthBips32(-1)), + loanSetFee, + ter(temINVALID)); // sfPaymentTotal: good value, bad account env(set(evan, broker.brokerID, principalRequest), @@ -3563,10 +3605,76 @@ class Loan_test : public beast::unit_test::suite }); } + void + testAccountSendMptMinAmountInvariant() + { + // (From FIND-006) + testcase << "LoanSet trigger ripple::accountSendMPT : minimum amount " + "and MPT"; + + using namespace jtx; + using namespace std::chrono_literals; + Env env(*this, all); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(1'000'000), issuer, lender, borrower); + env.close(); + + MPTTester mptt{env, issuer, mptInitNoFund}; + mptt.create( + {.flags = tfMPTCanClawback | tfMPTCanTransfer | tfMPTCanLock}); + PrettyAsset const mptAsset = mptt.issuanceID(); + mptt.authorize({.account = lender}); + mptt.authorize({.account = borrower}); + env(pay(issuer, lender, mptAsset(2'000'000))); + env(pay(issuer, borrower, mptAsset(1'000))); + env.close(); + + BrokerInfo broker{createVaultAndBroker(env, mptAsset, lender)}; + + using namespace loan; + + auto const loanSetFee = fee(env.current()->fees().base * 2); + Number const principalRequest{1, 3}; + + auto createJson = env.json( + set(borrower, broker.brokerID, principalRequest), + fee(loanSetFee), + json(sfCounterpartySignature, Json::objectValue)); + + createJson["CloseInterestRate"] = 76671; + createJson["ClosePaymentFee"] = "2061925410"; + createJson["GracePeriod"] = 434; + createJson["InterestRate"] = 50302; + createJson["LateInterestRate"] = 30322; + createJson["LatePaymentFee"] = "294427911"; + createJson["LoanOriginationFee"] = "3250635102"; + createJson["LoanServiceFee"] = "9557386"; + createJson["OverpaymentFee"] = 51249; + createJson["OverpaymentInterestRate"] = 14304; + createJson["PaymentInterval"] = 434; + createJson["PaymentTotal"] = "2891743748"; + createJson["PrincipalRequested"] = "8516.98"; + + auto const brokerStateBefore = + env.le(keylet::loanbroker(broker.brokerID)); + auto const loanSequence = brokerStateBefore->at(sfLoanSequence); + auto const keylet = keylet::loan(broker.brokerID, loanSequence); + + createJson = env.json(createJson, sig(sfCounterpartySignature, lender)); + env(createJson, ter(temINVALID)); + env.close(); + } + public: void run() override { + testAccountSendMptMinAmountInvariant(); + testIssuerLoan(); testDisabled(); testSelfLoan(); diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index 4f69e3c5a8..e207d2089a 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -76,15 +76,15 @@ LoanSet::preflight(PreflightContext const& ctx) !validDataLength(tx[~sfData], maxDataPayloadLength)) return temINVALID; for (auto const& field : - {&sfLoanOriginationFee, - &sfLoanServiceFee, - &sfLatePaymentFee, - &sfClosePaymentFee}) + {&sfLoanServiceFee, &sfLatePaymentFee, &sfClosePaymentFee}) { if (!validNumericMinimum(tx[~*field])) return temINVALID; } - if (auto const p = tx[~sfPrincipalRequested]; p && p <= 0) + // Principal Requested is required + if (auto const p = tx[sfPrincipalRequested]; p <= 0) + return temINVALID; + else if (!validNumericRange(tx[~sfLoanOriginationFee], p)) return temINVALID; if (!validNumericRange(tx[~sfInterestRate], maxInterestRate)) return temINVALID; diff --git a/src/xrpld/app/tx/detail/Transactor.h b/src/xrpld/app/tx/detail/Transactor.h index 044f604a11..0e67ba70c4 100644 --- a/src/xrpld/app/tx/detail/Transactor.h +++ b/src/xrpld/app/tx/detail/Transactor.h @@ -307,7 +307,7 @@ protected: template static bool - validNumericRange(std::optional value, T max, T min = {}); + validNumericRange(std::optional value, T max, T min = T{}); template static bool