Skip to content

Commit 1bf68f7

Browse files
ximinezximinez
committed
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.
1 parent 9847025 commit 1bf68f7

File tree

3 files changed

+188
-53
lines changed

3 files changed

+188
-53
lines changed

‎src/test/app/Loan_test.cpp‎

Lines changed: 116 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1824,7 +1824,10 @@ class Loan_test : public beast::unit_test::suite
18241824
// The LoanSet json can be created without a counterparty signature, but
18251825
// it is malformed.
18261826
auto createJson = env.json(
1827-
set(lender, broker.brokerID, principalRequest, startDate),
1827+
set(lender,
1828+
broker.brokerID,
1829+
broker.asset(principalRequest).value(),
1830+
startDate),
18281831
fee(loanSetFee));
18291832

18301833
env(createJson, ter(temMALFORMED));
@@ -1868,7 +1871,7 @@ class Loan_test : public beast::unit_test::suite
18681871
BEAST_EXPECT(types[type] == 1);
18691872
}
18701873
}
1871-
{
1874+
auto const loanID = [&]() {
18721875
Json::Value params(Json::objectValue);
18731876
params[jss::account] = lender.human();
18741877
params[jss::type] = "Loan";
@@ -1879,7 +1882,7 @@ class Loan_test : public beast::unit_test::suite
18791882
BEAST_EXPECT(objects.size() == 1);
18801883

18811884
auto const loan = objects[0u];
1882-
BEAST_EXPECT(loan[sfAssetsAvailable] == "1000");
1885+
BEAST_EXPECT(loan[sfAssetsAvailable] == "1000000000");
18831886
BEAST_EXPECT(loan[sfBorrower] == lender.human());
18841887
BEAST_EXPECT(loan[sfCloseInterestRate] == 0);
18851888
BEAST_EXPECT(loan[sfClosePaymentFee] == "0");
@@ -1899,12 +1902,23 @@ class Loan_test : public beast::unit_test::suite
18991902
BEAST_EXPECT(loan[sfPaymentInterval] == 60);
19001903
BEAST_EXPECT(loan[sfPaymentRemaining] == 1);
19011904
BEAST_EXPECT(loan[sfPreviousPaymentDate] == 0);
1902-
BEAST_EXPECT(loan[sfPrincipalOutstanding] == "1000");
1903-
BEAST_EXPECT(loan[sfPrincipalRequested] == "1000");
1905+
BEAST_EXPECT(loan[sfPrincipalOutstanding] == "1000000000");
1906+
BEAST_EXPECT(loan[sfPrincipalRequested] == "1000000000");
19041907
BEAST_EXPECT(
19051908
loan[sfStartDate].asUInt() ==
19061909
startDate.time_since_epoch().count());
1907-
}
1910+
1911+
return loan["index"].asString();
1912+
}();
1913+
auto const loanKeylet{keylet::loan(uint256{std::string_view(loanID)})};
1914+
1915+
env.close(startDate);
1916+
1917+
// Draw the loan
1918+
env(draw(lender, loanKeylet.key, broker.asset(1000)));
1919+
env.close();
1920+
// Make a payment
1921+
env(pay(lender, loanKeylet.key, broker.asset(1000)));
19081922
}
19091923

19101924
void
@@ -2028,6 +2042,101 @@ class Loan_test : public beast::unit_test::suite
20282042
env.close();
20292043
}
20302044

2045+
void
2046+
testLoanPayComputePeriodicPaymentValidRateInvariant()
2047+
{
2048+
testcase
2049+
<< "LoanPay ripple::detail::computePeriodicPayment : valid rate";
2050+
2051+
using namespace jtx;
2052+
using namespace std::chrono_literals;
2053+
Env env(*this, all);
2054+
2055+
Account const issuer{"issuer"};
2056+
Account const lender{"lender"};
2057+
Account const borrower{"borrower"};
2058+
2059+
env.fund(XRP(1'000'000), issuer, lender, borrower);
2060+
env.close();
2061+
2062+
PrettyAsset const xrpAsset{xrpIssue(), 1'000'000};
2063+
BrokerInfo broker{createVaultAndBroker(env, xrpAsset, lender)};
2064+
2065+
using namespace loan;
2066+
2067+
auto const loanSetFee = fee(env.current()->fees().base * 2);
2068+
Number const principalRequest{640562, -5};
2069+
auto const startDate = env.now() + 60s;
2070+
2071+
Number const serviceFee{2462611968};
2072+
std::uint32_t const numPayments{4294967295};
2073+
2074+
auto createJson = env.json(
2075+
set(borrower, broker.brokerID, principalRequest, startDate),
2076+
fee(loanSetFee),
2077+
loanServiceFee(serviceFee),
2078+
paymentTotal(numPayments),
2079+
json(sfCounterpartySignature, Json::objectValue));
2080+
2081+
createJson["CloseInterestRate"] = 55374;
2082+
createJson["ClosePaymentFee"] = "3825205248";
2083+
createJson["GracePeriod"] = 0;
2084+
createJson["LatePaymentFee"] = "237";
2085+
createJson["LoanOriginationFee"] = "0";
2086+
createJson["OverpaymentFee"] = 35167;
2087+
createJson["OverpaymentInterestRate"] = 1360;
2088+
createJson["PaymentInterval"] = 727;
2089+
2090+
Number const actualPrincipal{6};
2091+
2092+
auto const brokerStateBefore =
2093+
env.le(keylet::loanbroker(broker.brokerID));
2094+
auto const loanSequence = brokerStateBefore->at(sfLoanSequence);
2095+
auto const keylet = keylet::loan(broker.brokerID, loanSequence);
2096+
2097+
createJson = env.json(createJson, sig(sfCounterpartySignature, lender));
2098+
env(createJson, ter(tesSUCCESS));
2099+
env.close(startDate);
2100+
2101+
if (auto const loan = env.le(keylet); BEAST_EXPECT(loan))
2102+
{
2103+
// Verify the payment decreased the principal
2104+
BEAST_EXPECT(loan->at(sfPaymentRemaining) == numPayments);
2105+
BEAST_EXPECT(loan->at(sfPrincipalRequested) == actualPrincipal);
2106+
BEAST_EXPECT(loan->at(sfPrincipalOutstanding) == actualPrincipal);
2107+
BEAST_EXPECT(loan->at(sfAssetsAvailable) == actualPrincipal);
2108+
}
2109+
2110+
auto loanDrawTx = env.json(
2111+
draw(borrower, keylet.key, STAmount{broker.asset, Number{6}}));
2112+
env(loanDrawTx, ter(tesSUCCESS));
2113+
env.close();
2114+
2115+
if (auto const loan = env.le(keylet); BEAST_EXPECT(loan))
2116+
{
2117+
// Verify the payment decreased the principal
2118+
BEAST_EXPECT(loan->at(sfPaymentRemaining) == numPayments);
2119+
BEAST_EXPECT(loan->at(sfPrincipalRequested) == actualPrincipal);
2120+
BEAST_EXPECT(loan->at(sfPrincipalOutstanding) == actualPrincipal);
2121+
BEAST_EXPECT(loan->at(sfAssetsAvailable) == actualPrincipal - 6);
2122+
}
2123+
2124+
auto loanPayTx = env.json(
2125+
pay(borrower, keylet.key, STAmount{broker.asset, serviceFee + 6}));
2126+
env(loanPayTx, ter(tesSUCCESS));
2127+
env.close();
2128+
2129+
if (auto const loan = env.le(keylet); BEAST_EXPECT(loan))
2130+
{
2131+
// Verify the payment decreased the principal
2132+
BEAST_EXPECT(loan->at(sfPaymentRemaining) == numPayments - 1);
2133+
BEAST_EXPECT(loan->at(sfPrincipalRequested) == actualPrincipal);
2134+
BEAST_EXPECT(
2135+
loan->at(sfPrincipalOutstanding) == actualPrincipal - 1);
2136+
BEAST_EXPECT(loan->at(sfAssetsAvailable) == actualPrincipal - 6);
2137+
}
2138+
}
2139+
20312140
public:
20322141
void
20332142
run() override
@@ -2037,6 +2146,7 @@ class Loan_test : public beast::unit_test::suite
20372146
testLifecycle();
20382147
testBatchBypassCounterparty();
20392148
testWrongMaxDebtBehavior();
2149+
testLoanPayComputePeriodicPaymentValidRateInvariant();
20402150
}
20412151
};
20422152

‎src/xrpld/app/misc/LendingHelpers.h‎

Lines changed: 71 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -153,48 +153,6 @@ loanAccruedInterest(
153153
std::uint32_t prevPaymentDate,
154154
std::uint32_t paymentInterval);
155155

156-
struct PeriodicPayment
157-
{
158-
Number interest;
159-
Number principal;
160-
};
161-
162-
template <AssetType A>
163-
PeriodicPayment
164-
computePeriodicPaymentParts(
165-
A const& asset,
166-
Number const& originalPrincipal,
167-
Number const& principalOutstanding,
168-
Number const& periodicPaymentAmount,
169-
Number const& periodicRate,
170-
std::uint32_t paymentRemaining)
171-
{
172-
if (paymentRemaining == 1)
173-
{
174-
// If there's only one payment left, we need to pay off the principal.
175-
Number const interest = roundToAsset(
176-
asset,
177-
periodicPaymentAmount - principalOutstanding,
178-
originalPrincipal);
179-
return {interest, principalOutstanding};
180-
}
181-
Number const interest = roundToAsset(
182-
asset, principalOutstanding * periodicRate, originalPrincipal);
183-
XRPL_ASSERT(
184-
interest >= 0,
185-
"ripple::detail::computePeriodicPayment : valid interest");
186-
187-
auto const roundedPayment =
188-
roundToAsset(asset, periodicPaymentAmount, originalPrincipal);
189-
Number const principal =
190-
roundToAsset(asset, roundedPayment - interest, originalPrincipal);
191-
XRPL_ASSERT(
192-
principal > 0 && principal <= principalOutstanding,
193-
"ripple::detail::computePeriodicPayment : valid principal");
194-
195-
return {interest, principal};
196-
}
197-
198156
inline Number
199157
minusManagementFee(Number value, TenthBips32 managementFeeRate)
200158
{
@@ -283,6 +241,72 @@ loanLatePaymentInterest(
283241
originalPrincipal);
284242
}
285243

244+
struct PeriodicPaymentParts
245+
{
246+
Number interest;
247+
Number principal;
248+
};
249+
250+
template <AssetType A>
251+
PeriodicPaymentParts
252+
computePeriodicPaymentParts(
253+
A const& asset,
254+
Number const& originalPrincipal,
255+
Number const& principalOutstanding,
256+
Number const& periodicPaymentAmount,
257+
Number const& periodicRate,
258+
std::uint32_t paymentRemaining)
259+
{
260+
if (paymentRemaining == 1)
261+
{
262+
// If there's only one payment left, we need to pay off the principal.
263+
Number const interest = roundToAsset(
264+
asset,
265+
periodicPaymentAmount - principalOutstanding,
266+
originalPrincipal);
267+
return {interest, principalOutstanding};
268+
}
269+
Number const interest = roundToAsset(
270+
asset, principalOutstanding * periodicRate, originalPrincipal);
271+
XRPL_ASSERT(
272+
interest >= 0,
273+
"ripple::detail::computePeriodicPayment : valid interest");
274+
275+
auto const roundedPayment = [&]() {
276+
auto roundedPayment =
277+
roundToAsset(asset, periodicPaymentAmount, originalPrincipal);
278+
if (roundedPayment > interest)
279+
return roundedPayment;
280+
auto newPayment = roundedPayment;
281+
if (asset.native() || !asset.holds<Issue>())
282+
{
283+
// integral types, just add one
284+
++newPayment;
285+
}
286+
else
287+
{
288+
// Non-integral types: IOU. Add "dust" that will not be lost in
289+
// rounding.
290+
auto const epsilon = Number{1, originalPrincipal.exponent() - 14};
291+
newPayment += epsilon;
292+
}
293+
roundedPayment = roundToAsset(asset, newPayment, originalPrincipal);
294+
XRPL_ASSERT_PARTS(
295+
roundedPayment == newPayment,
296+
"ripple::computePeriodicPaymentParts",
297+
"epsilon preserved in rounding");
298+
return roundedPayment;
299+
}();
300+
Number const principal =
301+
roundToAsset(asset, roundedPayment - interest, originalPrincipal);
302+
XRPL_ASSERT_PARTS(
303+
principal > 0 && principal <= principalOutstanding,
304+
"ripple::computePeriodicPaymentParts",
305+
"valid principal");
306+
307+
return {interest, principal};
308+
}
309+
286310
struct LoanPaymentParts
287311
{
288312
Number principalPaid;
@@ -335,7 +359,8 @@ loanComputePaymentParts(
335359
Number const periodicRate =
336360
detail::loanPeriodicRate(interestRate, paymentInterval);
337361
XRPL_ASSERT(
338-
periodicRate > 0, "ripple::loanComputePaymentParts : valid rate");
362+
interestRate == 0 || periodicRate > 0,
363+
"ripple::loanComputePaymentParts : valid rate");
339364

340365
// Don't round the payment amount. Only round the final computations using
341366
// it.
@@ -345,7 +370,7 @@ loanComputePaymentParts(
345370
periodicPaymentAmount > 0,
346371
"ripple::computePeriodicPayment : valid payment");
347372

348-
auto const periodic = detail::computePeriodicPaymentParts(
373+
auto const periodic = computePeriodicPaymentParts(
349374
asset,
350375
originalPrincipalRequested,
351376
principalOutstandingField,
@@ -514,12 +539,12 @@ loanComputePaymentParts(
514539
Number totalInterestPaid = 0;
515540
Number loanValueChange = 0;
516541

517-
std::optional<detail::PeriodicPayment> future = periodic;
542+
std::optional<PeriodicPaymentParts> future = periodic;
518543
for (int i = 0; i < fullPeriodicPayments; ++i)
519544
{
520545
// Only do the work if we need to
521546
if (!future)
522-
future = detail::computePeriodicPaymentParts(
547+
future = computePeriodicPaymentParts(
523548
asset,
524549
originalPrincipalRequested,
525550
principalOutstandingField,

‎src/xrpld/app/tx/detail/LoanPay.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -170,7 +170,7 @@ LoanPay::doApply()
170170
if (!vaultSle)
171171
return tefBAD_LEDGER; // LCOV_EXCL_LINE
172172
auto const vaultPseudoAccount = vaultSle->at(sfAccount);
173-
auto const asset = vaultSle->at(sfAsset);
173+
auto const asset = *vaultSle->at(sfAsset);
174174

175175
//------------------------------------------------------
176176
// Loan object state changes

0 commit comments

Comments
 (0)