From 7e94916d08e5b9deafb6c0fda042ce6ee0028c06 Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Mon, 10 Nov 2025 17:59:10 -0500 Subject: [PATCH] Always round cover minimum calculations up - Addresses RIPD-4016. - Add and update testRoundingAllowsUndercoverage() unit test from ticket. --- src/test/app/Loan_test.cpp | 91 +++++++++++++++++++ .../app/tx/detail/LoanBrokerCoverClawback.cpp | 12 ++- .../app/tx/detail/LoanBrokerCoverWithdraw.cpp | 16 +++- src/xrpld/app/tx/detail/LoanManage.cpp | 32 ++++--- src/xrpld/app/tx/detail/LoanPay.cpp | 16 ++-- src/xrpld/app/tx/detail/LoanSet.cpp | 13 ++- 6 files changed, 148 insertions(+), 32 deletions(-) diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 57da3e28e7..78e44d0f13 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -86,6 +86,7 @@ protected: Number maxCoveredLoanValue(Number const& currentDebt) const { + NumberRoundModeGuard mg(Number::downward); auto debtLimit = coverDeposit * tenthBipsPerUnity.value() / coverRateMin.value(); @@ -2059,6 +2060,7 @@ protected: : std::max( broker.vaultScale(env), state.principalOutstanding.exponent()))); + NumberRoundModeGuard mg(Number::upward); auto const defaultAmount = roundToAsset( broker.asset, std::min( @@ -6499,6 +6501,94 @@ protected: } } + void + testRoundingAllowsUndercoverage() + { + testcase("Minimum cover rounding allows undercoverage (XRP)"); + + using namespace jtx; + using namespace loanBroker; + + Env env(*this, all); + + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + auto const asset = xrpIssue(); + + env.fund(XRP(200'000), lender, borrower); + env.close(); + + // Vault with XRP asset + Vault vault{env}; + auto [vaultCreate, vaultKeylet] = + vault.create({.owner = lender, .asset = xrpIssue()}); + env(vaultCreate); + env.close(); + BEAST_EXPECT(env.le(vaultKeylet)); + + // Seed the vault with XRP so it can fund the loan principal + PrettyAsset const xrpAsset{xrpIssue(), 1}; + + BrokerParameters const brokerParams{ + .vaultDeposit = 1'000, + .debtMax = Number{0}, + .coverRateMin = TenthBips32{10'000}, + .coverDeposit = 82, + }; + + auto const brokerInfo = + createVaultAndBroker(env, xrpAsset, lender, brokerParams); + // Create a loan with principal 804 XRP and 0% interest (so + // DebtTotal increases by exactly 804) + env(loan::set(borrower, brokerInfo.brokerID, xrpAsset(804).value()), + loan::interestRate(TenthBips32(0)), + sig(sfCounterpartySignature, lender), + fee(env.current()->fees().base * 2)); + BEAST_EXPECT(env.ter() == tesSUCCESS); + env.close(); + + // Verify DebtTotal is exactly 804 + if (auto const brokerSle = + env.le(keylet::loanbroker(brokerInfo.brokerID)); + BEAST_EXPECT(brokerSle)) + { + std::cout << *brokerSle << std::endl; + BEAST_EXPECT(brokerSle->at(sfDebtTotal) == Number(804)); + } + + // Attempt to withdraw 2 XRP to self, leaving 80 XRP CoverAvailable. + // The minimum is 80.4 XRP, which rounds up to 81 XRP, so this fails. + env(coverWithdraw(lender, brokerInfo.brokerID, xrpAsset(2).value()), + ter(tecINSUFFICIENT_FUNDS)); + BEAST_EXPECT(env.ter() == tecINSUFFICIENT_FUNDS); + env.close(); + + // Attempt to withdraw 1 XRP to self, leaving 81 XRP CoverAvailable. + // because that leaves sufficient cover, this succeeds + env(coverWithdraw(lender, brokerInfo.brokerID, xrpAsset(1).value())); + BEAST_EXPECT(env.ter() == tesSUCCESS); + env.close(); + + // Validate CoverAvailable == 80 XRP and DebtTotal remains 804 + if (auto const brokerSle = + env.le(keylet::loanbroker(brokerInfo.brokerID)); + BEAST_EXPECT(brokerSle)) + { + std::cout << *brokerSle << std::endl; + BEAST_EXPECT( + brokerSle->at(sfCoverAvailable) == xrpAsset(81).value()); + BEAST_EXPECT(brokerSle->at(sfDebtTotal) == Number(804)); + + // Also demonstrate that the true minimum (804 * 10%) exceeds 80 + auto const theoreticalMin = + tenthBipsOfValue(Number(804), TenthBips32(10'000)); + std::cout << "Theoretical min cover: " << theoreticalMin + << std::endl; + BEAST_EXPECT(Number(804, -1) == theoreticalMin); + } + } + public: void run() override @@ -6542,6 +6632,7 @@ public: testRIPD3831(); testRIPD3459(); testRIPD3901(); + testRoundingAllowsUndercoverage(); } }; diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverClawback.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverClawback.cpp index f4a6f6ba20..f31520e42b 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverClawback.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerCoverClawback.cpp @@ -133,9 +133,15 @@ determineClawAmount( Asset const& vaultAsset, std::optional const& amount) { - auto const maxClawAmount = sleBroker[sfCoverAvailable] - - tenthBipsOfValue(sleBroker[sfDebtTotal], - TenthBips32(sleBroker[sfCoverRateMinimum])); + auto const maxClawAmount = [&]() { + // Always round the minimum required up + NumberRoundModeGuard mg1(Number::upward); + auto const minRequiredCover = tenthBipsOfValue( + sleBroker[sfDebtTotal], TenthBips32(sleBroker[sfCoverRateMinimum])); + // The subtraction probably won't round, but round down if it does. + NumberRoundModeGuard mg2(Number::downward); + return sleBroker[sfCoverAvailable] - minRequiredCover; + }(); if (maxClawAmount <= beast::zero) return Unexpected(tecINSUFFICIENT_FUNDS); diff --git a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp index 97d8c48b74..aebda09e0b 100644 --- a/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp +++ b/src/xrpld/app/tx/detail/LoanBrokerCoverWithdraw.cpp @@ -99,11 +99,17 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx) auto const coverAvail = sleBroker->at(sfCoverAvailable); // Cover Rate is in 1/10 bips units auto const currentDebtTotal = sleBroker->at(sfDebtTotal); - auto const minimumCover = roundToAsset( - vaultAsset, - tenthBipsOfValue( - currentDebtTotal, TenthBips32(sleBroker->at(sfCoverRateMinimum))), - currentDebtTotal.exponent()); + auto const minimumCover = [&]() { + // Always round the minimum required up. + // Applies to `tenthBipsOfValue` as well as `roundToAsset`. + NumberRoundModeGuard mg(Number::upward); + return roundToAsset( + vaultAsset, + tenthBipsOfValue( + currentDebtTotal, + TenthBips32(sleBroker->at(sfCoverRateMinimum))), + currentDebtTotal.exponent()); + }(); if (coverAvail < amount) return tecINSUFFICIENT_FUNDS; if ((coverAvail - amount) < minimumCover) diff --git a/src/xrpld/app/tx/detail/LoanManage.cpp b/src/xrpld/app/tx/detail/LoanManage.cpp index 75d7b9552c..c45d317dea 100644 --- a/src/xrpld/app/tx/detail/LoanManage.cpp +++ b/src/xrpld/app/tx/detail/LoanManage.cpp @@ -148,20 +148,24 @@ LoanManage::defaultLoan( TenthBips32 const coverRateMinimum{brokerSle->at(sfCoverRateMinimum)}; TenthBips32 const coverRateLiquidation{ brokerSle->at(sfCoverRateLiquidation)}; - auto const defaultCovered = roundToAsset( - vaultAsset, - /* - * This formula is from the XLS-66 spec, section 3.2.3.2 (State - * Changes), specifically "if the `tfLoanDefault` flag is set" / "Apply - * the First-Loss Capital to the Default Amount" - */ - std::min( - tenthBipsOfValue( - tenthBipsOfValue( - brokerDebtTotalProxy.value(), coverRateMinimum), - coverRateLiquidation), - totalDefaultAmount), - loanScale); + auto const defaultCovered = [&]() { + // Always round the minimum required up. + NumberRoundModeGuard mg(Number::upward); + auto const minimumCover = + tenthBipsOfValue(brokerDebtTotalProxy.value(), coverRateMinimum); + // Round the liquidation amount up, too + return roundToAsset( + vaultAsset, + /* + * This formula is from the XLS-66 spec, section 3.2.3.2 (State + * Changes), specifically "if the `tfLoanDefault` flag is set" / + * "Apply the First-Loss Capital to the Default Amount" + */ + std::min( + tenthBipsOfValue(minimumCover, coverRateLiquidation), + totalDefaultAmount), + loanScale); + }(); auto const vaultDefaultAmount = totalDefaultAmount - defaultCovered; diff --git a/src/xrpld/app/tx/detail/LoanPay.cpp b/src/xrpld/app/tx/detail/LoanPay.cpp index 4cad02d762..fe19ed1c0d 100644 --- a/src/xrpld/app/tx/detail/LoanPay.cpp +++ b/src/xrpld/app/tx/detail/LoanPay.cpp @@ -253,12 +253,16 @@ LoanPay::doApply() // // Normally freeze status is checked in preflight, but we do it here to // avoid duplicating the check. It'll claim a fee either way. - bool const sendBrokerFeeToOwner = coverAvailableProxy >= - roundToAsset(asset, - tenthBipsOfValue( - debtTotalProxy.value(), coverRateMinimum), - loanScale) && - !isDeepFrozen(view, brokerOwner, asset); + bool const sendBrokerFeeToOwner = [&]() { + // Always round the minimum required up. + NumberRoundModeGuard mg(Number::upward); + return coverAvailableProxy >= + roundToAsset( + asset, + tenthBipsOfValue(debtTotalProxy.value(), coverRateMinimum), + loanScale) && + !isDeepFrozen(view, brokerOwner, asset); + }(); auto const brokerPayee = sendBrokerFeeToOwner ? brokerOwner : brokerPseudoAccount; diff --git a/src/xrpld/app/tx/detail/LoanSet.cpp b/src/xrpld/app/tx/detail/LoanSet.cpp index 1ef78b11b6..c129673222 100644 --- a/src/xrpld/app/tx/detail/LoanSet.cpp +++ b/src/xrpld/app/tx/detail/LoanSet.cpp @@ -514,11 +514,16 @@ LoanSet::doApply() return tecLIMIT_EXCEEDED; } TenthBips32 const coverRateMinimum{brokerSle->at(sfCoverRateMinimum)}; - if (brokerSle->at(sfCoverAvailable) < - tenthBipsOfValue(newDebtTotal, coverRateMinimum)) { - JLOG(j_.warn()) << "Insufficient first-loss capital to cover the loan."; - return tecINSUFFICIENT_FUNDS; + // Always round the minimum required up. + NumberRoundModeGuard mg(Number::upward); + if (brokerSle->at(sfCoverAvailable) < + tenthBipsOfValue(newDebtTotal, coverRateMinimum)) + { + JLOG(j_.warn()) + << "Insufficient first-loss capital to cover the loan."; + return tecINSUFFICIENT_FUNDS; + } } adjustOwnerCount(view, borrowerSle, 1, j_);