fix: Cap the base fee for LoanPay (#6969)

src/test/app/Loan_test.cpp

@@ -5126,15 +5126,18 @@ protected:
     }
 
     void
-    testDosLoanPay()
+    testDosLoanPay(FeatureBitset features)
     {
+        bool const feeCapped = features[fixSecurity3_1_3];
+
         // From FIND-005
-        testcase << "DoS LoanPay";
+        testcase << "DoS LoanPay: fee calculation "
+                 << (feeCapped ? "capped" : "uncapped");
 
         using namespace jtx;
         using namespace std::chrono_literals;
         using namespace Lending;
-        Env env(*this, all);
+        Env env(*this, features);
 
         Account const issuer{"issuer"};
         Account const lender{"lender"};
@@ -5143,6 +5146,9 @@ protected:
         env.fund(XRP(1'000'000), issuer, lender, borrower);
         env.close();
 
+        BEAST_EXPECT(
+            feeCapped == env.current()->rules().enabled(fixSecurity3_1_3));
+
         PrettyAsset const iouAsset = issuer[iouCurrency];
         env(trust(lender, iouAsset(100'000'000)));
         env(trust(borrower, iouAsset(100'000'000)));
@@ -5155,57 +5161,127 @@ protected:
         using namespace loan;
 
         auto const loanSetFee = fee(env.current()->fees().base * 2);
-        Number const principalRequest{1, 3};
+        Number const principalRequest{3959'37, -2};
         auto const baseFee = env.current()->fees().base;
 
-        auto createJson = env.json(
+        auto const createJson = env.json(
             set(borrower, broker.brokerID, principalRequest),
             fee(loanSetFee),
-            json(sfCounterpartySignature, Json::objectValue));
-
-        createJson["ClosePaymentFee"] = "0";
-        createJson["GracePeriod"] = 60;
-        createJson["InterestRate"] = 20930;
-        createJson["LateInterestRate"] = 77049;
-        createJson["LatePaymentFee"] = "0";
-        createJson["LoanServiceFee"] = "0";
-        createJson["OverpaymentFee"] = 7;
-        createJson["OverpaymentInterestRate"] = 66653;
-        createJson["PaymentInterval"] = 60;
-        createJson["PaymentTotal"] = 3239184;
-        createJson["PrincipalRequested"] = "3959.37";
+            json(sfCounterpartySignature, Json::objectValue),
+            closePaymentFee(0),
+            gracePeriod(60),
+            interestRate(TenthBips32(20930)),
+            lateInterestRate(TenthBips32(77049)),
+            latePaymentFee(0),
+            loanServiceFee(0),
+            overpaymentFee(TenthBips32(7)),
+            overpaymentInterestRate(TenthBips32(66653)),
+            paymentInterval(60),
+            paymentTotal(3239184));
 
+        // There are enough payments due on this loan that it only needs to be
+        // created once, and can be paid on multiple times. Just don't create a
+        // gazillion test cases.
         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(createJson, sig(sfCounterpartySignature, lender));
         env.close();
 
-        auto const stateBefore = getCurrentState(env, broker, keylet);
-        BEAST_EXPECT(stateBefore.paymentRemaining == 3239184);
-        BEAST_EXPECT(
-            stateBefore.paymentRemaining > loanMaximumPaymentsPerTransaction);
+        auto const roundedPayment = [&]() {
+            auto const stateBefore = getCurrentState(env, broker, keylet);
+            BEAST_EXPECT(stateBefore.paymentRemaining == 3239184);
 
-        auto loanPayTx = env.json(
-            pay(borrower, keylet.key, STAmount{broker.asset, Number{}}));
-        Number const amount{395937, -2};
-        loanPayTx["Amount"]["value"] = to_string(amount);
-        XRPAmount const payFee{
-            baseFee *
-            std::int64_t(
-                amount / stateBefore.periodicPayment /
+            return roundToAsset(
+                iouAsset,
+                stateBefore.periodicPayment,
+                stateBefore.loanScale,
+                Number::upward);
+        }();
+
+        auto test = [&](int const payFactor,
+                        int const feeFactor,
+                        TER const expectedTer = tesSUCCESS) {
+            auto const stateBefore = getCurrentState(env, broker, keylet);
+            BEAST_EXPECT(stateBefore.paymentRemaining <= 3239184);
+            BEAST_EXPECT(
+                stateBefore.paymentRemaining >
+                loanMaximumPaymentsPerTransaction);
+
+            Number const amount = roundedPayment * payFactor;
+            auto loanPayTx = env.json(
+                pay(borrower, keylet.key, STAmount{broker.asset, amount}));
+            XRPAmount const payFee{baseFee * feeFactor};
+            env(loanPayTx, ter(expectedTer), fee(payFee));
+            env.close();
+            auto const expectedChange = isTesSuccess(expectedTer)
+                ? std::min(loanMaximumPaymentsPerTransaction, payFactor)
+                : 0;
+
+            auto const stateAfter = getCurrentState(env, broker, keylet);
+            BEAST_EXPECT(
+                stateAfter.paymentRemaining ==
+                stateBefore.paymentRemaining - expectedChange);
+        };
+
+        std::int64_t constexpr maxFeeIncrements =
+            loanMaximumPaymentsPerTransaction / loanPaymentsPerFeeIncrement;
+
+        TER const failWithoutFix =
+            feeCapped ? (TER)tesSUCCESS : (TER)telINSUF_FEE_P;
+
+        // * Amount well above threshold -> capped fee
+        // The original test case - way over the limit - more fee is always ok
+        test(1819878, 363976);
+        // The capped fee is only sufficient if the amendment is enabled.
+        test(1819878, maxFeeIncrements, failWithoutFix);
+
+        // * Amount exactly at threshold -> capped fee
+        test(loanMaximumPaymentsPerTransaction, maxFeeIncrements);
+        // More fee is always ok
+        test(loanMaximumPaymentsPerTransaction, maxFeeIncrements + 10);
+
+        // * Amount below threshold -> normal calculation
+        test(1, 1);
+        test(loanPaymentsPerFeeIncrement * 2, 2);
+        test(0, 0, temBAD_AMOUNT);
+        test(0, 1, temBAD_AMOUNT);
+        // Fee difference rounds evenly
+        test(
+            loanMaximumPaymentsPerTransaction - 10,
+            (loanMaximumPaymentsPerTransaction - 10) /
+                    loanPaymentsPerFeeIncrement -
+                1,
+            telINSUF_FEE_P);
+        test(
+            loanMaximumPaymentsPerTransaction - 10,
+            (loanMaximumPaymentsPerTransaction - 10) /
+                loanPaymentsPerFeeIncrement);
+        // More fee is always ok
+        test(
+            loanMaximumPaymentsPerTransaction - 10,
+            (loanMaximumPaymentsPerTransaction - 10) /
                     loanPaymentsPerFeeIncrement +
-                1)};
-        env(loanPayTx, ter(tesSUCCESS), fee(payFee));
-        env.close();
-
-        auto const stateAfter = getCurrentState(env, broker, keylet);
-        BEAST_EXPECT(
-            stateAfter.paymentRemaining ==
-            stateBefore.paymentRemaining - loanMaximumPaymentsPerTransaction);
+                3);
+        // Fee rounds up
+        for (int under = 1; under < loanPaymentsPerFeeIncrement; ++under)
+        {
+            test(
+                loanMaximumPaymentsPerTransaction - under,
+                maxFeeIncrements - 1,
+                telINSUF_FEE_P);
+            test(loanMaximumPaymentsPerTransaction - under, maxFeeIncrements);
+        }
+        // Only when you get one less fee increment can you pay less
+        test(
+            loanMaximumPaymentsPerTransaction - loanPaymentsPerFeeIncrement,
+            maxFeeIncrements - 1);
+        // And again, more fee is always ok.
+        test(
+            loanMaximumPaymentsPerTransaction - loanPaymentsPerFeeIncrement,
+            maxFeeIncrements);
     }
 
     void
@@ -7972,7 +8048,8 @@ public:
         testLoanPayDebtDecreaseInvariant();
         testWrongMaxDebtBehavior();
         testLoanPayComputePeriodicPaymentValidTotalInterestInvariant();
-        testDosLoanPay();
+        testDosLoanPay(all | fixSecurity3_1_3);
+        testDosLoanPay(all - fixSecurity3_1_3);
         testLoanPayComputePeriodicPaymentValidTotalPrincipalPaidInvariant();
         testLoanPayComputePeriodicPaymentValidTotalInterestPaidInvariant();
         testLoanNextPaymentDueDateOverflow();

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

@@ -110,6 +110,24 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx)
     // about the same amount of work, if not more.
     NumberRoundModeGuard mg(
         tx.isFlag(tfLoanOverpayment) ? Number::upward : Number::downward);
+
+    static_assert(
+        loanMaximumPaymentsPerTransaction % loanPaymentsPerFeeIncrement == 0);
+    std::int64_t constexpr maxFeeIncrements =
+        loanMaximumPaymentsPerTransaction / loanPaymentsPerFeeIncrement;
+
+    if (view.rules().enabled(fixSecurity3_1_3) &&
+        amount >= regularPayment * loanMaximumPaymentsPerTransaction)
+    {
+        // The payment handler will never process more than
+        // loanMaximumPaymentsPerTransaction payments (including overpayments),
+        // and one fee increment is charged for every
+        // loanPaymentsPerFeeIncrement, so don't charge more than
+        // loanMaximumPaymentsPerTransaction / loanPaymentsPerFeeIncrement fee
+        // increments.
+        return maxFeeIncrements * normalCost;
+    }
+
     // Estimate how many payments will be made
     Number const numPaymentEstimate =
         static_cast<std::int64_t>(amount / regularPayment);
@@ -120,6 +138,11 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx)
         std::int64_t(1),
         static_cast<std::int64_t>(
             numPaymentEstimate / loanPaymentsPerFeeIncrement));
+    XRPL_ASSERT(
+        !view.rules().enabled(fixSecurity3_1_3) ||
+            feeIncrements <= maxFeeIncrements,
+        "ripple::LoanPay::calculateBaseFee : number of fee increments is in "
+        "range");
 
     return feeIncrements * normalCost;
 }