From 162875616d03729e1bc618688b7a7eb65e5e0e60 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 5 Aug 2025 21:05:26 -0400 Subject: [PATCH] Correct more issues related to 0-interest loans and some rounding issues - Addresses FIND-012 from audit. - If computePeriodicPaymentParts rounds the principal part to 0, add a small amount so that some principal is paid regardless of how extreme the loan parameters are. For XRP and MPTs, this just adds 1. For IOUs, compute an epsilon based on the scale of the original loan. (IOUs untested.) - Also move this function out of the detail namespace so direct unit tests can be written. (Pending.) - Adds the testLoanPayComputePeriodicPaymentValidRateInvariant from auditors with some minor modifications. - Fixes an assert that the periodic rate > 0, which won't be true if the loan interest rate is 0. --- src/test/app/Loan_test.cpp | 122 ++++++++++++++++++++++++++-- src/xrpld/app/misc/LendingHelpers.h | 117 +++++++++++++++----------- src/xrpld/app/tx/detail/LoanPay.cpp | 2 +- 3 files changed, 188 insertions(+), 53 deletions(-) diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 6fb603bfd6..9146f23dd9 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -1824,7 +1824,10 @@ class Loan_test : public beast::unit_test::suite // The LoanSet json can be created without a counterparty signature, but // it is malformed. auto createJson = env.json( - set(lender, broker.brokerID, principalRequest, startDate), + set(lender, + broker.brokerID, + broker.asset(principalRequest).value(), + startDate), fee(loanSetFee)); env(createJson, ter(temMALFORMED)); @@ -1868,7 +1871,7 @@ class Loan_test : public beast::unit_test::suite BEAST_EXPECT(types[type] == 1); } } - { + auto const loanID = [&]() { Json::Value params(Json::objectValue); params[jss::account] = lender.human(); params[jss::type] = "Loan"; @@ -1879,7 +1882,7 @@ class Loan_test : public beast::unit_test::suite BEAST_EXPECT(objects.size() == 1); auto const loan = objects[0u]; - BEAST_EXPECT(loan[sfAssetsAvailable] == "1000"); + BEAST_EXPECT(loan[sfAssetsAvailable] == "1000000000"); BEAST_EXPECT(loan[sfBorrower] == lender.human()); BEAST_EXPECT(loan[sfCloseInterestRate] == 0); BEAST_EXPECT(loan[sfClosePaymentFee] == "0"); @@ -1899,12 +1902,23 @@ class Loan_test : public beast::unit_test::suite BEAST_EXPECT(loan[sfPaymentInterval] == 60); BEAST_EXPECT(loan[sfPaymentRemaining] == 1); BEAST_EXPECT(loan[sfPreviousPaymentDate] == 0); - BEAST_EXPECT(loan[sfPrincipalOutstanding] == "1000"); - BEAST_EXPECT(loan[sfPrincipalRequested] == "1000"); + BEAST_EXPECT(loan[sfPrincipalOutstanding] == "1000000000"); + BEAST_EXPECT(loan[sfPrincipalRequested] == "1000000000"); BEAST_EXPECT( loan[sfStartDate].asUInt() == startDate.time_since_epoch().count()); - } + + return loan["index"].asString(); + }(); + auto const loanKeylet{keylet::loan(uint256{std::string_view(loanID)})}; + + env.close(startDate); + + // Draw the loan + env(draw(lender, loanKeylet.key, broker.asset(1000))); + env.close(); + // Make a payment + env(pay(lender, loanKeylet.key, broker.asset(1000))); } void @@ -2028,6 +2042,101 @@ class Loan_test : public beast::unit_test::suite env.close(); } + void + testLoanPayComputePeriodicPaymentValidRateInvariant() + { + testcase + << "LoanPay ripple::detail::computePeriodicPayment : valid rate"; + + 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(); + + PrettyAsset const xrpAsset{xrpIssue(), 1'000'000}; + BrokerInfo broker{createVaultAndBroker(env, xrpAsset, lender)}; + + using namespace loan; + + auto const loanSetFee = fee(env.current()->fees().base * 2); + Number const principalRequest{640562, -5}; + auto const startDate = env.now() + 60s; + + Number const serviceFee{2462611968}; + std::uint32_t const numPayments{4294967295}; + + auto createJson = env.json( + set(borrower, broker.brokerID, principalRequest, startDate), + fee(loanSetFee), + loanServiceFee(serviceFee), + paymentTotal(numPayments), + json(sfCounterpartySignature, Json::objectValue)); + + createJson["CloseInterestRate"] = 55374; + createJson["ClosePaymentFee"] = "3825205248"; + createJson["GracePeriod"] = 0; + createJson["LatePaymentFee"] = "237"; + createJson["LoanOriginationFee"] = "0"; + createJson["OverpaymentFee"] = 35167; + createJson["OverpaymentInterestRate"] = 1360; + createJson["PaymentInterval"] = 727; + + Number const actualPrincipal{6}; + + 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(tesSUCCESS)); + env.close(startDate); + + if (auto const loan = env.le(keylet); BEAST_EXPECT(loan)) + { + // Verify the payment decreased the principal + BEAST_EXPECT(loan->at(sfPaymentRemaining) == numPayments); + BEAST_EXPECT(loan->at(sfPrincipalRequested) == actualPrincipal); + BEAST_EXPECT(loan->at(sfPrincipalOutstanding) == actualPrincipal); + BEAST_EXPECT(loan->at(sfAssetsAvailable) == actualPrincipal); + } + + auto loanDrawTx = env.json( + draw(borrower, keylet.key, STAmount{broker.asset, Number{6}})); + env(loanDrawTx, ter(tesSUCCESS)); + env.close(); + + if (auto const loan = env.le(keylet); BEAST_EXPECT(loan)) + { + // Verify the payment decreased the principal + BEAST_EXPECT(loan->at(sfPaymentRemaining) == numPayments); + BEAST_EXPECT(loan->at(sfPrincipalRequested) == actualPrincipal); + BEAST_EXPECT(loan->at(sfPrincipalOutstanding) == actualPrincipal); + BEAST_EXPECT(loan->at(sfAssetsAvailable) == actualPrincipal - 6); + } + + auto loanPayTx = env.json( + pay(borrower, keylet.key, STAmount{broker.asset, serviceFee + 6})); + env(loanPayTx, ter(tesSUCCESS)); + env.close(); + + if (auto const loan = env.le(keylet); BEAST_EXPECT(loan)) + { + // Verify the payment decreased the principal + BEAST_EXPECT(loan->at(sfPaymentRemaining) == numPayments - 1); + BEAST_EXPECT(loan->at(sfPrincipalRequested) == actualPrincipal); + BEAST_EXPECT( + loan->at(sfPrincipalOutstanding) == actualPrincipal - 1); + BEAST_EXPECT(loan->at(sfAssetsAvailable) == actualPrincipal - 6); + } + } + public: void run() override @@ -2037,6 +2146,7 @@ public: testLifecycle(); testBatchBypassCounterparty(); testWrongMaxDebtBehavior(); + testLoanPayComputePeriodicPaymentValidRateInvariant(); } }; diff --git a/src/xrpld/app/misc/LendingHelpers.h b/src/xrpld/app/misc/LendingHelpers.h index 2b51f3d14d..022e9557e9 100644 --- a/src/xrpld/app/misc/LendingHelpers.h +++ b/src/xrpld/app/misc/LendingHelpers.h @@ -153,48 +153,6 @@ loanAccruedInterest( std::uint32_t prevPaymentDate, std::uint32_t paymentInterval); -struct PeriodicPayment -{ - Number interest; - Number principal; -}; - -template -PeriodicPayment -computePeriodicPaymentParts( - A const& asset, - Number const& originalPrincipal, - Number const& principalOutstanding, - Number const& periodicPaymentAmount, - Number const& periodicRate, - std::uint32_t paymentRemaining) -{ - if (paymentRemaining == 1) - { - // If there's only one payment left, we need to pay off the principal. - Number const interest = roundToAsset( - asset, - periodicPaymentAmount - principalOutstanding, - originalPrincipal); - return {interest, principalOutstanding}; - } - Number const interest = roundToAsset( - asset, principalOutstanding * periodicRate, originalPrincipal); - XRPL_ASSERT( - interest >= 0, - "ripple::detail::computePeriodicPayment : valid interest"); - - auto const roundedPayment = - roundToAsset(asset, periodicPaymentAmount, originalPrincipal); - Number const principal = - roundToAsset(asset, roundedPayment - interest, originalPrincipal); - XRPL_ASSERT( - principal > 0 && principal <= principalOutstanding, - "ripple::detail::computePeriodicPayment : valid principal"); - - return {interest, principal}; -} - inline Number minusManagementFee(Number value, TenthBips32 managementFeeRate) { @@ -283,6 +241,72 @@ loanLatePaymentInterest( originalPrincipal); } +struct PeriodicPaymentParts +{ + Number interest; + Number principal; +}; + +template +PeriodicPaymentParts +computePeriodicPaymentParts( + A const& asset, + Number const& originalPrincipal, + Number const& principalOutstanding, + Number const& periodicPaymentAmount, + Number const& periodicRate, + std::uint32_t paymentRemaining) +{ + if (paymentRemaining == 1) + { + // If there's only one payment left, we need to pay off the principal. + Number const interest = roundToAsset( + asset, + periodicPaymentAmount - principalOutstanding, + originalPrincipal); + return {interest, principalOutstanding}; + } + Number const interest = roundToAsset( + asset, principalOutstanding * periodicRate, originalPrincipal); + XRPL_ASSERT( + interest >= 0, + "ripple::detail::computePeriodicPayment : valid interest"); + + auto const roundedPayment = [&]() { + auto roundedPayment = + roundToAsset(asset, periodicPaymentAmount, originalPrincipal); + if (roundedPayment > interest) + return roundedPayment; + auto newPayment = roundedPayment; + if (asset.native() || !asset.template holds()) + { + // integral types, just add one + ++newPayment; + } + else + { + // Non-integral types: IOU. Add "dust" that will not be lost in + // rounding. + auto const epsilon = Number{1, originalPrincipal.exponent() - 14}; + newPayment += epsilon; + } + roundedPayment = roundToAsset(asset, newPayment, originalPrincipal); + XRPL_ASSERT_PARTS( + roundedPayment == newPayment, + "ripple::computePeriodicPaymentParts", + "epsilon preserved in rounding"); + return roundedPayment; + }(); + Number const principal = + roundToAsset(asset, roundedPayment - interest, originalPrincipal); + XRPL_ASSERT_PARTS( + principal > 0 && principal <= principalOutstanding, + "ripple::computePeriodicPaymentParts", + "valid principal"); + + return {interest, principal}; +} + struct LoanPaymentParts { Number principalPaid; @@ -335,7 +359,8 @@ loanComputePaymentParts( Number const periodicRate = detail::loanPeriodicRate(interestRate, paymentInterval); XRPL_ASSERT( - periodicRate > 0, "ripple::loanComputePaymentParts : valid rate"); + interestRate == 0 || periodicRate > 0, + "ripple::loanComputePaymentParts : valid rate"); // Don't round the payment amount. Only round the final computations using // it. @@ -345,7 +370,7 @@ loanComputePaymentParts( periodicPaymentAmount > 0, "ripple::computePeriodicPayment : valid payment"); - auto const periodic = detail::computePeriodicPaymentParts( + auto const periodic = computePeriodicPaymentParts( asset, originalPrincipalRequested, principalOutstandingField, @@ -514,12 +539,12 @@ loanComputePaymentParts( Number totalInterestPaid = 0; Number loanValueChange = 0; - std::optional future = periodic; + std::optional future = periodic; for (int i = 0; i < fullPeriodicPayments; ++i) { // Only do the work if we need to if (!future) - future = detail::computePeriodicPaymentParts( + future = computePeriodicPaymentParts( asset, originalPrincipalRequested, principalOutstandingField, diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index 866a5d9914..6ed405aa6d 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -170,7 +170,7 @@ LoanPay::doApply() if (!vaultSle) return tefBAD_LEDGER; // LCOV_EXCL_LINE auto const vaultPseudoAccount = vaultSle->at(sfAccount); - auto const asset = vaultSle->at(sfAsset); + auto const asset = *vaultSle->at(sfAsset); //------------------------------------------------------ // Loan object state changes