From 40ef91e7e08d10f0bdf4300cf93856159b3d9584 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Sun, 26 Oct 2025 12:56:13 -0400 Subject: [PATCH] Limit how many payments can be made in a single LoanPay - Addresses FIND-005 from audit. - Tuning values defined in Protocol.h. Optimal values TBD. - loanPaymentsPerFeeIncrement: calculateBaseFee estimates the number of payments included in the Amount and charges "baseFee * number / loanPaymentsPerFeeIncrement". - loanMaximumPaymentsPerTransaction: If the number of payments (including overpayments if applicable) hits this limit, stop processing more payments, but DO NOT FAIL. - Fix the rounding in LoanSet for Guard 4 (sufficient computed payments) - Tweak several test parameters to account for the new limits. - Change payment component rounding for IOUs to "towards_zero". - Add some safety limits to loan calculations to prevent nonsensical values. --- include/xrpl/protocol/Protocol.h | 35 +++ src/test/app/Loan_test.cpp | 223 ++++++++++++++----- src/xrpld/app/misc/detail/LendingHelpers.cpp | 61 +++-- src/xrpld/app/tx/detail/LoanPay.cpp | 39 ++-- src/xrpld/app/tx/detail/LoanSet.cpp | 27 +-- 5 files changed, 278 insertions(+), 107 deletions(-) diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 503d5a5693..95f654e5f1 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -177,6 +177,41 @@ static_assert(maxCloseInterestRate == TenthBips32(100'000u)); TenthBips32 constexpr maxOverpaymentInterestRate = percentageToTenthBips(100); static_assert(maxOverpaymentInterestRate == TenthBips32(100'000u)); +/** LoanPay transaction cost will be one base fee per X combined payments + * + * The number of payments is estimated based on the Amount paid and the Loan's + * Fixed Payment size. Overpayments (indicated with the tfLoanOverpayment flag) + * count as one more payment. + * + * This number was chosen arbitrarily, but should not be changed once released + * without an amendment + */ +static constexpr int loanPaymentsPerFeeIncrement = 5; + +/** Maximum number of combined payments that a LoanPay transaction will process + * + * This limit is enforced during the loan payment process, and thus is not + * estimated. If the limit is hit, no further payments or overpayments will be + * processed, no matter how much of the transation Amount is left, but the + * transaction will succeed with the payments that have been processed up to + * that point. + * + * This limit is independent of loanPaymentsPerFeeIncrement, so a transaction + * could potentially be charged for many more payments than actually get + * processed. Users should take care not to submit a transaction paying more + * than loanMaximumPaymentsPerTransaction * Loan.PeriodicPayment. Because + * overpayments are charged as a payment, if submitting + * loanMaximumPaymentsPerTransaction * Loan.PeriodicPayment, users should not + * set the tfLoanOverpayment flag. + * + * Even though they're independent, loanMaximumPaymentsPerTransaction should be + * a multiple of loanPaymentsPerFeeIncrement. + * + * This number was chosen arbitrarily, but should not be changed once released + * without an amendment + */ +static constexpr int loanMaximumPaymentsPerTransaction = 100; + /** The maximum length of a URI inside an NFT */ std::size_t constexpr maxTokenURILength = 256; diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 752d50f104..b5f3923207 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -371,20 +371,20 @@ class Loan_test : public beast::unit_test::suite return {asset, keylet.key}; } + /// Get the state without checking anything LoanState getCurrentState( jtx::Env const& env, BrokerInfo const& broker, - Keylet const& loanKeylet, - VerifyLoanStatus const& verifyLoanStatus) + Keylet const& loanKeylet) { - using namespace std::chrono_literals; using d = NetClock::duration; using tp = NetClock::time_point; + // Lookup the current loan state if (auto loan = env.le(loanKeylet); BEAST_EXPECT(loan)) { - LoanState state{ + return LoanState{ .previousPaymentDate = loan->at(sfPreviousPaymentDate), .startDate = tp{d{loan->at(sfStartDate)}}, .nextPaymentDate = loan->at(sfNextPaymentDueDate), @@ -399,40 +399,52 @@ class Loan_test : public beast::unit_test::suite .paymentInterval = loan->at(sfPaymentInterval), .interestRate = TenthBips32{loan->at(sfInterestRate)}, }; - BEAST_EXPECT(state.previousPaymentDate == 0); - BEAST_EXPECT( - tp{d{state.nextPaymentDate}} == state.startDate + 600s); - BEAST_EXPECT(state.paymentRemaining == 12); - BEAST_EXPECT( - state.principalOutstanding == broker.asset(1000).value()); - BEAST_EXPECT( - state.loanScale == - (broker.asset.integral() - ? 0 - : state.principalOutstanding.exponent())); - BEAST_EXPECT(state.paymentInterval == 600); - BEAST_EXPECT( - state.totalValue == - roundToAsset( - broker.asset, - state.periodicPayment * state.paymentRemaining, - state.loanScale)); - BEAST_EXPECT( - state.managementFeeOutstanding == - computeFee( - broker.asset, - state.totalValue - state.principalOutstanding, - managementFeeRateParameter, - state.loanScale)); - - verifyLoanStatus(state); - - return state; } - return LoanState{}; } + /// Get the state and check the values against the parameters used in + /// `lifecycle` + LoanState + getCurrentState( + jtx::Env const& env, + BrokerInfo const& broker, + Keylet const& loanKeylet, + VerifyLoanStatus const& verifyLoanStatus) + { + using namespace std::chrono_literals; + using d = NetClock::duration; + using tp = NetClock::time_point; + + auto const state = getCurrentState(env, broker, loanKeylet); + BEAST_EXPECT(state.previousPaymentDate == 0); + BEAST_EXPECT(tp{d{state.nextPaymentDate}} == state.startDate + 600s); + BEAST_EXPECT(state.paymentRemaining == 12); + BEAST_EXPECT(state.principalOutstanding == broker.asset(1000).value()); + BEAST_EXPECT( + state.loanScale == + (broker.asset.integral() ? 0 + : state.principalOutstanding.exponent())); + BEAST_EXPECT(state.paymentInterval == 600); + BEAST_EXPECT( + state.totalValue == + roundToAsset( + broker.asset, + state.periodicPayment * state.paymentRemaining, + state.loanScale)); + BEAST_EXPECT( + state.managementFeeOutstanding == + computeFee( + broker.asset, + state.totalValue - state.principalOutstanding, + managementFeeRateParameter, + state.loanScale)); + + verifyLoanStatus(state); + + return state; + } + bool canImpairLoan( jtx::Env const& env, @@ -851,6 +863,8 @@ class Loan_test : public beast::unit_test::suite return Account("Broker pseudo-account", brokerPseudo); }(); + auto const baseFee = env.current()->fees().base; + auto badKeylet = keylet::vault(lender.id(), env.seq(lender)); // Try some failure cases // flags are checked first @@ -1379,11 +1393,13 @@ class Loan_test : public beast::unit_test::suite broker.asset(10), txFlags), ter(temINVALID)); - env(pay(borrower, loanKeylet.key, broker.asset(-100), txFlags), + // broker.asset(80) is less than a single payment, but all these + // checks fail before that matters + env(pay(borrower, loanKeylet.key, broker.asset(-80), txFlags), ter(temBAD_AMOUNT)); - env(pay(borrower, broker.brokerID, broker.asset(100), txFlags), + env(pay(borrower, broker.brokerID, broker.asset(80), txFlags), ter(tecNO_ENTRY)); - env(pay(evan, loanKeylet.key, broker.asset(500), txFlags), + env(pay(evan, loanKeylet.key, broker.asset(80), txFlags), ter(tecNO_PERMISSION)); // TODO: Write a general "isFlag" function? See STObject::isFlag. @@ -1395,8 +1411,13 @@ class Loan_test : public beast::unit_test::suite // don't end up duplicating the next test transaction. env(pay(borrower, loanKeylet.key, - broker.asset(state.periodicPayment * 2), + STAmount{ + broker.asset, + state.periodicPayment * Number{15, -1}}, tfLoanOverpayment), + fee(XRPAmount{ + baseFee * + (Number{15, -1} / loanPaymentsPerFeeIncrement + 1)}), ter(temINVALID_FLAG)); } // Try to send a payment marked as both full payment and @@ -1435,23 +1456,34 @@ class Loan_test : public beast::unit_test::suite // Send a transaction that tries to pay more than the borrowers's // balance + XRPAmount const badFee{ + baseFee * + (borrowerBalanceBeforePayment.number() * 2 / + state.periodicPayment / loanPaymentsPerFeeIncrement + + 1)}; env(pay(borrower, loanKeylet.key, STAmount{ broker.asset, borrowerBalanceBeforePayment.number() * 2}, txFlags), + fee(badFee), ter(tecINSUFFICIENT_FUNDS)); - env(pay(borrower, loanKeylet.key, transactionAmount, txFlags)); + XRPAmount const goodFee{ + baseFee * (numPayments / loanPaymentsPerFeeIncrement + 1)}; + env(pay(borrower, loanKeylet.key, transactionAmount, txFlags), + fee(goodFee)); env.close(); + // log << env.meta()->getJson() << std::endl; + // Need to account for fees if the loan is in XRP PrettyAmount adjustment = broker.asset(0); if (broker.asset.native()) { - adjustment = env.current()->fees().base * 2; + adjustment = badFee + goodFee; } state.paymentRemaining = 0; @@ -1781,27 +1813,28 @@ class Loan_test : public beast::unit_test::suite << "\tLoan starting state: " << state.paymentRemaining << ", " << raw.interestDue << ", " << raw.principalOutstanding << ", " - << raw.managementFeeDue << ", " << rounded.interestDue - << ", " << rounded.principalOutstanding << ", " + << raw.managementFeeDue << ", " + << rounded.valueOutstanding << ", " + << rounded.principalOutstanding << ", " << rounded.managementFeeDue; } + // Try to pay a little extra to show that it's _not_ + // taken + STAmount const transactionAmount = + STAmount{broker.asset, totalDue} + broker.asset(10); + // Only check the first payment since the rounding + // may drift as payments are made + BEAST_EXPECT( + transactionAmount == + roundToScale( + broker.asset( + Number(9533457001162141, -14), Number::upward), + state.loanScale, + Number::upward)); + while (state.paymentRemaining > 0) { - // Try to pay a little extra to show that it's _not_ - // taken - STAmount const transactionAmount = - STAmount{broker.asset, totalDue} + broker.asset(10); - // Only check the first payment since the rounding - // may drift as payments are made - BEAST_EXPECT( - transactionAmount == - roundToScale( - broker.asset( - Number(9533457001162141, -14), Number::upward), - state.loanScale, - Number::upward)); - // Compute the expected principal amount auto const paymentComponents = detail::computePaymentComponents( @@ -1954,6 +1987,78 @@ class Loan_test : public beast::unit_test::suite #if LOANCOMPLETE // TODO + auto time = [&](std::string label, std::function timed) { + if (!BEAST_EXPECT(timed)) + return; + + using clock_type = std::chrono::steady_clock; + using duration_type = std::chrono::milliseconds; + + auto const start = clock_type::now(); + timed(); + auto const duration = std::chrono::duration_cast( + clock_type::now() - start); + + log << label << " took " << duration.count() << "ms" << std::endl; + + return duration; + }; + + lifecycle( + caseLabel, + "timing", + env, + loanAmount, + interestExponent, + lender, + borrower, + evan, + broker, + pseudoAcct, + tfLoanOverpayment, + [&](Keylet const& loanKeylet, + VerifyLoanStatus const& verifyLoanStatus) { + // Estimate optimal values for loanPaymentsPerFeeIncrement and + // loanMaximumPaymentsPerTransaction. + using namespace loan; + + auto const state = + getCurrentState(env, broker, verifyLoanStatus.keylet); + auto const serviceFee = broker.asset(2).value(); + + STAmount const totalDue{ + broker.asset, + roundPeriodicPayment( + broker.asset, + state.periodicPayment + serviceFee, + state.loanScale)}; + + // Make a single payment + time("single payment", [&]() { + env(pay(borrower, loanKeylet.key, totalDue)); + }); + env.close(); + + // Make all but the final payment + auto const numPayments = (state.paymentRemaining - 2); + STAmount const bigPayment{broker.asset, totalDue * numPayments}; + XRPAmount const bigFee{ + baseFee * (numPayments / loanPaymentsPerFeeIncrement + 1)}; + time("ten payments", [&]() { + env(pay(borrower, loanKeylet.key, bigPayment), fee(bigFee)); + }); + env.close(); + + time("final payment", [&]() { + // Make the final payment + env( + pay(borrower, + loanKeylet.key, + totalDue + STAmount{broker.asset, 1})); + }); + env.close(); + }); + lifecycle( caseLabel, "Loan overpayment allowed - Explicit overpayment", @@ -3661,8 +3766,6 @@ class Loan_test : public beast::unit_test::suite 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)); @@ -3673,8 +3776,6 @@ public: void run() override { - testAccountSendMptMinAmountInvariant(); - testIssuerLoan(); testDisabled(); testSelfLoan(); @@ -3692,6 +3793,8 @@ public: testInvalidLoanManage(); testInvalidLoanPay(); testInvalidLoanSet(); + + testAccountSendMptMinAmountInvariant(); } }; diff --git a/src/xrpld/app/misc/detail/LendingHelpers.cpp b/src/xrpld/app/misc/detail/LendingHelpers.cpp index b0f747d9fb..edf3dd9646 100644 --- a/src/xrpld/app/misc/detail/LendingHelpers.cpp +++ b/src/xrpld/app/misc/detail/LendingHelpers.cpp @@ -228,7 +228,7 @@ computeRoundedPrincipalComponent( asset, principalOutstanding - rawPrincipalOutstanding, scale, - Number::downward); + asset.integral() ? Number::downward : Number::towards_zero); // If the rounded principal outstanding is greater than the true // principal outstanding, we need to pay more principal to reduce @@ -308,7 +308,7 @@ computeRoundedInterestComponent( asset, interestOutstanding - rawInterestOutstanding, scale, - Number::downward); + asset.integral() ? Number::downward : Number::towards_zero); roundedInterest += diff; } @@ -358,7 +358,7 @@ computeRoundedInterestAndFeeComponents( asset, managementFeeOutstanding - rawManagementFeeOutstanding, scale, - Number::downward); + asset.integral() ? Number::downward : Number::towards_zero); roundedFee += diff; @@ -383,18 +383,27 @@ computeRoundedInterestAndFeeComponents( roundedInterest = std::min(interestOutstanding, roundedInterest); // Make sure the parts don't add up to too much - Number excess = roundedPeriodicPayment - roundedPrincipal - - roundedInterest - roundedFee; + auto const initialTotal = roundedPrincipal + roundedInterest + roundedFee; + Number excess = roundedPeriodicPayment - initialTotal; XRPL_ASSERT_PARTS( isRounded(asset, excess, scale), "ripple::detail::computeRoundedInterestAndFeeComponents", "excess is rounded"); +#if LOANCOMPLETE + if (excess != beast::zero) + std::cout << "computeRoundedInterestAndFeeComponents excess is " + << excess << std::endl; +#endif + if (excess < beast::zero) { // Take as much of the excess as we can out of the interest - auto part = std::min(roundedInterest, abs(excess)); +#if LOANCOMPLETE + std::cout << "\tApplying excess to interest\n"; +#endif + auto part = std::min(roundedInterest, -excess); roundedInterest -= part; excess += part; @@ -407,7 +416,10 @@ computeRoundedInterestAndFeeComponents( { // If there's any left, take as much of the excess as we can out of the // fee - auto part = std::min(roundedFee, abs(excess)); +#if LOANCOMPLETE + std::cout << "\tApplying excess to fee\n"; +#endif + auto part = std::min(roundedFee, -excess); roundedFee -= part; excess += part; } @@ -422,7 +434,7 @@ computeRoundedInterestAndFeeComponents( ((asset.integral() && excess < 3) || (roundedPeriodicPayment.exponent() - excess.exponent() > 6))), "ripple::detail::computeRoundedInterestAndFeeComponents", - "excess is zero (fee)"); + "excess is extremely small (fee)"); XRPL_ASSERT_PARTS( roundedFee >= beast::zero, @@ -596,8 +608,9 @@ tryOverpayment( auto const principalError = principalOutstanding - raw.principalOutstanding; auto const feeError = managementFeeOutstanding - raw.managementFeeDue; - auto const newRawPrincipal = - raw.principalOutstanding - overpaymentComponents.trackedPrincipalDelta; + auto const newRawPrincipal = std::max( + raw.principalOutstanding - overpaymentComponents.trackedPrincipalDelta, + Number{0}); auto newLoanProperties = computeLoanProperties( asset, @@ -1136,7 +1149,8 @@ computeOverpaymentComponents( { XRPL_ASSERT( overpayment > 0 && isRounded(asset, overpayment, loanScale), - "ripple::loanMakePayment : valid overpayment amount"); + "ripple::detail::computeOverpaymentComponents : valid overpayment " + "amount"); Number const fee = roundToAsset( asset, tenthBipsOfValue(overpayment, overpaymentFeeRate), loanScale); @@ -1237,6 +1251,15 @@ calculateRawLoanState( std::uint32_t const paymentRemaining, TenthBips16 const managementFeeRate) { + if (paymentRemaining == 0) + { + return LoanState{ + .valueOutstanding = 0, + .principalOutstanding = 0, + .interestOutstanding = 0, + .interestDue = 0, + .managementFeeDue = 0}; + } Number const rawValueOutstanding = periodicPayment * paymentRemaining; Number const rawPrincipalOutstanding = detail::loanPrincipalFromPeriodicPayment( @@ -1556,11 +1579,9 @@ loanMakePayment( Number const serviceFee = loan->at(sfLoanServiceFee); Number const latePaymentFee = loan->at(sfLatePaymentFee); - Number const closePaymentFee = - roundToAsset(asset, loan->at(sfClosePaymentFee), loanScale); TenthBips16 const managementFeeRate{brokerSle->at(sfManagementFeeRate)}; - auto const periodicPayment = loan->at(sfPeriodicPayment); + Number const periodicPayment = loan->at(sfPeriodicPayment); auto prevPaymentDateProxy = loan->at(sfPreviousPaymentDate); @@ -1655,7 +1676,8 @@ loanMakePayment( std::size_t numPayments = 1; - while (totalPaid < amount && paymentRemainingProxy > 0) + while (totalPaid < amount && paymentRemainingProxy > 0 && + numPayments < loanMaximumPaymentsPerTransaction) { // Try to make more payments detail::PaymentComponentsPlus const nextPayment{ @@ -1718,13 +1740,18 @@ loanMakePayment( // ------------------------------------------------------------- // overpayment handling if (overpaymentAllowed && loan->isFlag(lsfLoanOverpayment) && - paymentRemainingProxy > 0 && nextDueDateProxy && totalPaid < amount) + paymentRemainingProxy > 0 && nextDueDateProxy && totalPaid < amount && + numPayments < loanMaximumPaymentsPerTransaction) { TenthBips32 const overpaymentInterestRate{ loan->at(sfOverpaymentInterestRate)}; TenthBips32 const overpaymentFeeRate{loan->at(sfOverpaymentFee)}; - Number const overpayment = amount - totalPaid; + // It shouldn't be possible for the overpayment to be greater than + // totalValueOutstanding, because that would have been processed as + // another normal payment. But cap it just in case. + Number const overpayment = + std::min(amount - totalPaid, *totalValueOutstandingProxy); detail::PaymentComponentsPlus const overpaymentComponents = detail::computeOverpaymentComponents( diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index 6bf9560afa..ead8bd2784 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -23,6 +23,7 @@ #include #include +#include #include namespace ripple { @@ -65,8 +66,6 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) // payment return normalCost; - auto const paymentsPerFeeIncrement = 20; - // The fee is based on the potential number of payments, unless the loan is // being fully paid off. auto const amount = tx[sfAmount]; @@ -77,10 +76,10 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) // Let preclaim worry about the error for this return normalCost; - if (loanSle->at(sfPaymentRemaining) <= paymentsPerFeeIncrement) + if (loanSle->at(sfPaymentRemaining) <= loanPaymentsPerFeeIncrement) { - // If there are fewer than paymentsPerFeeIncrement payments left, we can - // skip the computations. + // If there are fewer than loanPaymentsPerFeeIncrement payments left to + // pay, we can skip the computations. return normalCost; } @@ -93,30 +92,36 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) if (!brokerSle) // Let preclaim worry about the error for this return normalCost; - auto const vaultSle = view.read(keylet::vault(loanSle->at(sfVaultID))); + auto const vaultSle = view.read(keylet::vault(brokerSle->at(sfVaultID))); if (!vaultSle) // Let preclaim worry about the error for this return normalCost; auto const asset = vaultSle->at(sfAsset); + + if (asset != amount.asset()) + // Let preclaim worry about the error for this + return normalCost; + auto const scale = loanSle->at(sfLoanScale); auto const regularPayment = roundPeriodicPayment(asset, loanSle->at(sfPeriodicPayment), scale) + loanSle->at(sfLoanServiceFee); - if (amount < regularPayment * paymentsPerFeeIncrement) - // This is definitely paying fewer than paymentsPerFeeIncrement payments - return normalCost; - - NumberRoundModeGuard mg(Number::downward); - // Figure out how many payments will be made - auto const numPaymentEstimate = + // If making an overpayment, count it as a full payment because it will do + // about the same amount of work, if not more. + NumberRoundModeGuard mg( + tx.isFlag(tfLoanOverpayment) ? Number::upward : Number::downward); + // Estimate how many payments will be made + Number const numPaymentEstimate = static_cast(amount / regularPayment); - // Charge one base fee per paymentsPerFeeIncrement payments - use integer - // math (round down), then add one to ensure all this extra math is worth - // it. - auto const feeIncrements = numPaymentEstimate / paymentsPerFeeIncrement + 1; + // Charge one base fee per paymentsPerFeeIncrement payments, rounding up. + Number::setround(Number::upward); + auto const feeIncrements = std::max( + 1ll, + static_cast( + numPaymentEstimate / loanPaymentsPerFeeIncrement)); return feeIncrements * normalCost; } diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index e207d2089a..41e4b7d4ba 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -411,20 +411,21 @@ LoanSet::doApply() // Guard 4: if the rounded periodic payment is large enough that the loan // can't be amortized in the specified number of payments, raise an error - if (auto const computedPayments = roundToAsset( - vaultAsset, - properties.totalValueOutstanding / roundedPayment, - properties.loanScale, - Number::upward); - computedPayments < paymentTotal) { - JLOG(j_.warn()) - << "Loan Periodic payment (" << properties.periodicPayment - << ") rounding (" << roundedPayment - << ") will complete the " - "loan in less than the specified number of payments (" - << computedPayments << " < " << paymentTotal << ")"; - return tecPRECISION_LOSS; + NumberRoundModeGuard mg(Number::upward); + + if (std::int64_t const computedPayments{ + properties.totalValueOutstanding / roundedPayment}; + computedPayments < paymentTotal) + { + JLOG(j_.warn()) + << "Loan Periodic payment (" << properties.periodicPayment + << ") rounding (" << roundedPayment + << ") will complete the " + "loan in less than the specified number of payments (" + << computedPayments << " < " << paymentTotal << ")"; + return tecPRECISION_LOSS; + } } // Check that the other computed values are valid