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

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

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 <AssetType A>
-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 <AssetType A>
+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<Issue>())
+        {
+            // 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<detail::PeriodicPayment> future = periodic;
+    std::optional<PeriodicPaymentParts> 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,

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