ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-06-03 00:36:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

Limit how many payments can be made in a single LoanPay

Browse Source 
- 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.
This commit is contained in:
Ed Hennis
2025-10-26 12:56:13 -04:00
parent fe4269cf8b
commit 40ef91e7e0
5 changed files with 278 additions and 107 deletions
Download Patch File Download Diff File Expand all files Collapse all files

35
include/xrpl/protocol/Protocol.h
View File

@@ -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;

223
src/test/app/Loan_test.cpp
View File

@@ -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<void()> 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<duration_type>(
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();
}
};

61
src/xrpld/app/misc/detail/LendingHelpers.cpp
View File

@@ -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(

39
src/xrpld/app/tx/detail/LoanPay.cpp
View File

@@ -23,6 +23,7 @@
#include <xrpld/app/tx/detail/LoanManage.h>
#include <xrpl/json/to_string.h>
#include <xrpl/protocol/Protocol.h>
#include <xrpl/protocol/TxFlags.h>
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<std::int64_t>(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<std::int64_t>(
numPaymentEstimate / loanPaymentsPerFeeIncrement));
return feeIncrements * normalCost;
}

27
src/xrpld/app/tx/detail/LoanSet.cpp
View File

@@ -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