diff --git a/src/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index bfd78ea82b..ef93f0dc1b 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -141,6 +141,23 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) NumberRoundModeGuard const mg( tx.isFlag(tfLoanOverpayment) ? Number::RoundingMode::Upward : Number::RoundingMode::Downward); + + static_assert(kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION % kLOAN_PAYMENTS_PER_FEE_INCREMENT == 0); + std::int64_t constexpr kMAX_FEE_INCREMENTS = + kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION / kLOAN_PAYMENTS_PER_FEE_INCREMENT; + + if (view.rules().enabled(fixSecurity3_1_3) && + amount >= regularPayment * kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION) + { + // 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 kMAX_FEE_INCREMENTS * normalCost; + } + // Estimate how many payments will be made Number const numPaymentEstimate = static_cast(amount / regularPayment); @@ -149,6 +166,10 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) auto const feeIncrements = std::max( std::int64_t(1), static_cast(numPaymentEstimate / kLOAN_PAYMENTS_PER_FEE_INCREMENT)); + XRPL_ASSERT( + !view.rules().enabled(fixSecurity3_1_3) || feeIncrements <= kMAX_FEE_INCREMENTS, + "xrpl::LoanPay::calculateBaseFee : number of fee increments is in " + "range"); return feeIncrements * normalCost; } diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index f189196a82..713168a4ee 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -2797,8 +2797,8 @@ protected: pseudoAcct, tfLoanOverpayment, [&](Keylet const& loanKeylet, VerifyLoanStatus const& verifyLoanStatus) { - // Estimate optimal values for loanPaymentsPerFeeIncrement and - // loanMaximumPaymentsPerTransaction. + // Estimate optimal values for kLOAN_PAYMENTS_PER_FEE_INCREMENT and + // kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION. using namespace loan; auto const state = getCurrentState(env, broker, verifyLoanStatus.keylet); @@ -2816,7 +2816,8 @@ protected: // 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)}; + XRPAmount const bigFee{ + baseFee * (numPayments / kLOAN_PAYMENTS_PER_FEE_INCREMENT + 1)}; time("ten payments", [&]() { env(pay(borrower, loanKeylet.key, bigPayment), Fee(bigFee)); }); @@ -4753,15 +4754,17 @@ 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"}; @@ -4770,6 +4773,8 @@ 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))); @@ -4782,52 +4787,117 @@ 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), + kCLOSE_PAYMENT_FEE(0), + kGRACE_PERIOD(60), + kINTEREST_RATE(TenthBips32(20930)), + kLATE_INTEREST_RATE(TenthBips32(77049)), + kLATE_PAYMENT_FEE(0), + kLOAN_SERVICE_FEE(0), + kOVERPAYMENT_FEE(TenthBips32(7)), + kOVERPAYMENT_INTEREST_RATE(TenthBips32(66653)), + kPAYMENT_INTERVAL(60), + kPAYMENT_TOTAL(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 > kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION); + auto const roundedPayment = [&]() { + auto const stateBefore = getCurrentState(env, broker, keylet); + BEAST_EXPECT(stateBefore.paymentRemaining == 3239184); + BEAST_EXPECT(stateBefore.paymentRemaining > kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION); - 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 / kLOAN_PAYMENTS_PER_FEE_INCREMENT + 1)}; - env(loanPayTx, Ter(tesSUCCESS), Fee(payFee)); - env.close(); + return roundToAsset( + iouAsset, + stateBefore.periodicPayment, + stateBefore.loanScale, + Number::RoundingMode::Upward); + }(); - auto const stateAfter = getCurrentState(env, broker, keylet); - BEAST_EXPECT( - stateAfter.paymentRemaining == - stateBefore.paymentRemaining - kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION); + 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 > kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION); + + 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(kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION, payFactor) + : 0; + + auto const stateAfter = getCurrentState(env, broker, keylet); + BEAST_EXPECT( + stateAfter.paymentRemaining == stateBefore.paymentRemaining - expectedChange); + }; + + std::int64_t constexpr kMAX_FEE_INCREMENTS = + kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION / kLOAN_PAYMENTS_PER_FEE_INCREMENT; + + 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, kMAX_FEE_INCREMENTS, failWithoutFix); + + // * Amount exactly at threshold -> capped fee + test(kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION, kMAX_FEE_INCREMENTS); + // More fee is always ok + test(kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION, kMAX_FEE_INCREMENTS + 10); + + // * Amount below threshold -> normal calculation + test(1, 1); + test(kLOAN_PAYMENTS_PER_FEE_INCREMENT * 2, 2); + test(0, 0, temBAD_AMOUNT); + test(0, 1, temBAD_AMOUNT); + // Fee difference rounds evenly + test( + kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION - 10, + ((kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION - 10) / kLOAN_PAYMENTS_PER_FEE_INCREMENT) - 1, + telINSUF_FEE_P); + test( + kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION - 10, + ((kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION - 10) / kLOAN_PAYMENTS_PER_FEE_INCREMENT)); + // More fee is always ok + test( + kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION - 10, + ((kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION - 10) / kLOAN_PAYMENTS_PER_FEE_INCREMENT) + 3); + // Fee rounds up + for (int under = 1; under < kLOAN_PAYMENTS_PER_FEE_INCREMENT; ++under) + { + test( + kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION - under, + kMAX_FEE_INCREMENTS - 1, + telINSUF_FEE_P); + test(kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION - under, kMAX_FEE_INCREMENTS); + } + // Only when you get one less fee increment can you pay less + test( + kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION - kLOAN_PAYMENTS_PER_FEE_INCREMENT, + kMAX_FEE_INCREMENTS - 1); + // And again, more fee is always ok. + test( + kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION - kLOAN_PAYMENTS_PER_FEE_INCREMENT, + kMAX_FEE_INCREMENTS); } void @@ -7248,7 +7318,8 @@ public: testLoanPayDebtDecreaseInvariant(); testWrongMaxDebtBehavior(); testLoanPayComputePeriodicPaymentValidTotalInterestInvariant(); - testDosLoanPay(); + testDosLoanPay(all | fixSecurity3_1_3); + testDosLoanPay(all - fixSecurity3_1_3); testLoanPayComputePeriodicPaymentValidTotalPrincipalPaidInvariant(); testLoanPayComputePeriodicPaymentValidTotalInterestPaidInvariant(); testLoanNextPaymentDueDateOverflow();