From 8012b5d34f533c559a7f9fababbe38bfedaa6b7d Mon Sep 17 00:00:00 2001 From: Ed Hennis Date: Tue, 12 May 2026 15:25:11 -0400 Subject: [PATCH] fix: Fix touchy "funds are conserved" assertion in LoanPay (#6231) (#6967) Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- src/libxrpl/ledger/helpers/LendingHelpers.cpp | 14 +- .../tx/transactors/lending/LoanPay.cpp | 219 +++++++++++++++--- src/test/app/Loan_test.cpp | 153 +++++++++++- 3 files changed, 345 insertions(+), 41 deletions(-) diff --git a/src/libxrpl/ledger/helpers/LendingHelpers.cpp b/src/libxrpl/ledger/helpers/LendingHelpers.cpp index 89d5ed8e35..a8bee7aecb 100644 --- a/src/libxrpl/ledger/helpers/LendingHelpers.cpp +++ b/src/libxrpl/ledger/helpers/LendingHelpers.cpp @@ -1963,8 +1963,18 @@ loanMakePayment( // ------------------------------------------------------------- // overpayment handling + // + // If the "fixSecurity3_1_3" amendment is enabled, truncate "amount", + // at the loan scale. If the raw value is used, the overpayment + // amount could be meaningless dust. Trying to process such a small + // amount will, at best, waste time when all the result values round + // to zero. At worst, it can cause logical errors with tiny amounts + // of interest that don't add up correctly. + auto const roundedAmount = view.rules().enabled(fixSecurity3_1_3) + ? roundToAsset(asset, amount, loanScale, Number::RoundingMode::TowardsZero) + : amount; if (paymentType == LoanPaymentType::Overpayment && loan->isFlag(lsfLoanOverpayment) && - paymentRemainingProxy > 0 && totalPaid < amount && + paymentRemainingProxy > 0 && totalPaid < roundedAmount && numPayments < kLOAN_MAXIMUM_PAYMENTS_PER_TRANSACTION) { TenthBips32 const overpaymentInterestRate{loan->at(sfOverpaymentInterestRate)}; @@ -1973,7 +1983,7 @@ loanMakePayment( // It shouldn't be possible for the overpayment to be greater than // totalValueOutstanding, because that would have been processed as // another normal payment. But cap it just in case. - Number const overpayment = std::min(amount - totalPaid, *totalValueOutstandingProxy); + Number const overpayment = std::min(roundedAmount - totalPaid, *totalValueOutstandingProxy); detail::ExtendedPaymentComponents const overpaymentComponents = detail::computeOverpaymentComponents( diff --git a/src/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index ef93f0dc1b..d7147128cc 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -30,6 +30,7 @@ #include #include #include +#include namespace xrpl { @@ -162,6 +163,7 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx) Number const numPaymentEstimate = static_cast(amount / regularPayment); // Charge one base fee per paymentsPerFeeIncrement payments, rounding up. + // This set round is safe because there's a mode guard just above Number::setround(Number::RoundingMode::Upward); auto const feeIncrements = std::max( std::int64_t(1), @@ -463,9 +465,10 @@ LoanPay::doApply() // Vault object state changes view.update(vaultSle); + Number const assetsAvailableBefore = *assetsAvailableProxy; + Number const assetsTotalBefore = *assetsTotalProxy; #if !NDEBUG { - Number const assetsAvailableBefore = *assetsAvailableProxy; Number const pseudoAccountBalanceBefore = accountHolds( view, vaultPseudoAccount, @@ -489,16 +492,6 @@ LoanPay::doApply() "xrpl::LoanPay::doApply", "assets available must not be greater than assets outstanding"); - if (*assetsAvailableProxy > *assetsTotalProxy) - { - // LCOV_EXCL_START - JLOG(j_.fatal()) << "Vault assets available must not be greater " - "than assets outstanding. Available: " - << *assetsAvailableProxy << ", Total: " << *assetsTotalProxy; - return tecINTERNAL; - // LCOV_EXCL_STOP - } - JLOG(j_.debug()) << "total paid to vault raw: " << totalPaidToVaultRaw << ", total paid to vault rounded: " << totalPaidToVaultRounded << ", total paid to broker: " << totalPaidToBroker @@ -524,12 +517,68 @@ LoanPay::doApply() associateAsset(*vaultSle, asset); // Duplicate some checks after rounding + Number const assetsAvailableAfter = *assetsAvailableProxy; + Number const assetsTotalAfter = *assetsTotalProxy; + XRPL_ASSERT_PARTS( - *assetsAvailableProxy <= *assetsTotalProxy, + assetsAvailableAfter <= assetsTotalAfter, "xrpl::LoanPay::doApply", "assets available must not be greater than assets outstanding"); + if (assetsAvailableAfter == assetsAvailableBefore) + { + // An unchanged assetsAvailable indicates that the amount paid to the + // vault was zero, or rounded to zero. That should be impossible, but I + // can't rule it out for extreme edge cases, so fail gracefully if it + // happens. + // + // LCOV_EXCL_START + JLOG(j_.warn()) << "LoanPay: Vault assets available unchanged after rounding: " // + << "Before: " << assetsAvailableBefore // + << ", After: " << assetsAvailableAfter; + return tecPRECISION_LOSS; + // LCOV_EXCL_STOP + } + if (paymentParts->valueChange != beast::kZERO && assetsTotalAfter == assetsTotalBefore) + { + // Non-zero valueChange with an unchanged assetsTotal indicates that the + // actual value change rounded to zero. That should be impossible, but I + // can't rule it out for extreme edge cases, so fail gracefully if it + // happens. + // + // LCOV_EXCL_START + JLOG(j_.warn()) + << "LoanPay: Vault assets expected change, but unchanged after rounding: " // + << "Before: " << assetsTotalBefore // + << ", After: " << assetsTotalAfter // + << ", ValueChange: " << paymentParts->valueChange; + return tecPRECISION_LOSS; + // LCOV_EXCL_STOP + } + if (paymentParts->valueChange == beast::kZERO && assetsTotalAfter != assetsTotalBefore) + { + // A change in assetsTotal when there was no valueChange indicates that + // something really weird happened. That should be flat out impossible. + // + // LCOV_EXCL_START + JLOG(j_.fatal()) << "LoanPay: Vault assets changed unexpectedly after rounding: " // + << "Before: " << assetsTotalBefore // + << ", After: " << assetsTotalAfter // + << ", ValueChange: " << paymentParts->valueChange; + return tecINTERNAL; + // LCOV_EXCL_STOP + } + if (assetsAvailableAfter > assetsTotalAfter) + { + // Assets available are not allowed to be larger than assets total. + // LCOV_EXCL_START + JLOG(j_.fatal()) << "LoanPay: Vault assets available must not be greater " + "than assets outstanding. Available: " + << assetsAvailableAfter << ", Total: " << assetsTotalAfter; + return tecINTERNAL; + // LCOV_EXCL_STOP + } -#if !NDEBUG + // These three values are used to check that funds are conserved after the transfers auto const accountBalanceBefore = accountHolds( view, account_, @@ -557,7 +606,6 @@ LoanPay::doApply() AuthHandling::IgnoreAuth, j_, SpendableHandling::FullBalance); -#endif if (totalPaidToVaultRounded != beast::kZERO) { @@ -593,19 +641,22 @@ LoanPay::doApply() return ter; #if !NDEBUG - Number const assetsAvailableAfter = *assetsAvailableProxy; - Number const pseudoAccountBalanceAfter = accountHolds( - view, - vaultPseudoAccount, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_); - XRPL_ASSERT_PARTS( - assetsAvailableAfter == pseudoAccountBalanceAfter, - "xrpl::LoanPay::doApply", - "vault pseudo balance agrees after"); + { + Number const pseudoAccountBalanceAfter = accountHolds( + view, + vaultPseudoAccount, + asset, + FreezeHandling::IgnoreFreeze, + AuthHandling::IgnoreAuth, + j_); + XRPL_ASSERT_PARTS( + assetsAvailableAfter == pseudoAccountBalanceAfter, + "xrpl::LoanPay::doApply", + "vault pseudo balance agrees after"); + } +#endif + // Check that funds are conserved auto const accountBalanceAfter = accountHolds( view, account_, @@ -633,14 +684,121 @@ LoanPay::doApply() AuthHandling::IgnoreAuth, j_, SpendableHandling::FullBalance); + auto const balanceScale = [&]() { + // Find a reasonable scale to use for the balance comparisons. + // + // First find the minimum and maximum exponent of all the non-zero balances, before and + // after. If min and max are equal, use that value. If they are not, use "max + 1" to reduce + // rounding discrepancies without making the result meaningless. Cap the scale at + // STAmount::kMAX_OFFSET, just in case the numbers are all very large. + std::vector exponents; + exponents.reserve(6); + for (auto const& a : { + accountBalanceBefore, + vaultBalanceBefore, + brokerBalanceBefore, + accountBalanceAfter, + vaultBalanceAfter, + brokerBalanceAfter, + }) + { + // Exclude zeroes + if (a != beast::kZERO) + exponents.push_back(a.exponent()); + } + if (exponents.empty()) + { + UNREACHABLE("xrpl::LoanPay::doApply : all zeroes"); + return 0; + } + auto const [minItr, maxItr] = std::ranges::minmax_element(exponents); + auto const min = *minItr; + auto const max = *maxItr; + JLOG(j_.trace()) << "Min scale: " << min << ", max scale: " << max; + // IOU rounding can be interesting. We want all the balance checks to agree, but don't want + // to round to such an extreme that it becomes meaningless. e.g. Everything rounds to one + // digit. So add 1 to the max (reducing the number of digits after the decimal point by 1) + // if the scales are not already all the same. + return std::min(min == max ? max : max + 1, STAmount::kMAX_OFFSET); + }(); + + // No object changes are made below this point XRPL_ASSERT_PARTS( - accountBalanceBefore + vaultBalanceBefore + brokerBalanceBefore == - accountBalanceAfter + vaultBalanceAfter + brokerBalanceAfter, + Number::getround() == Number::RoundingMode::ToNearest, "xrpl::LoanPay::doApply", - "funds are conserved (with rounding)"); + "Number rounding ToNearest"); + NumberRoundModeGuard const mg(Number::RoundingMode::ToNearest); + + auto const accountBalanceBeforeRounded = roundToScale(accountBalanceBefore, balanceScale); + auto const vaultBalanceBeforeRounded = roundToScale(vaultBalanceBefore, balanceScale); + auto const brokerBalanceBeforeRounded = roundToScale(brokerBalanceBefore, balanceScale); + + auto const totalBalanceBefore = accountBalanceBefore + vaultBalanceBefore + brokerBalanceBefore; + auto const totalBalanceBeforeRounded = roundToScale(totalBalanceBefore, balanceScale); + + JLOG(j_.trace()) << "Before: " // + << "account " << Number(accountBalanceBeforeRounded) << " (" + << Number(accountBalanceBefore) << ")" + << ", vault " << Number(vaultBalanceBeforeRounded) << " (" + << Number(vaultBalanceBefore) << ")" + << ", broker " << Number(brokerBalanceBeforeRounded) << " (" + << Number(brokerBalanceBefore) << ")" + << ", total " << Number(totalBalanceBeforeRounded) << " (" + << Number(totalBalanceBefore) << ")"; + + auto const accountBalanceAfterRounded = roundToScale(accountBalanceAfter, balanceScale); + auto const vaultBalanceAfterRounded = roundToScale(vaultBalanceAfter, balanceScale); + auto const brokerBalanceAfterRounded = roundToScale(brokerBalanceAfter, balanceScale); + + auto const totalBalanceAfter = accountBalanceAfter + vaultBalanceAfter + brokerBalanceAfter; + auto const totalBalanceAfterRounded = roundToScale(totalBalanceAfter, balanceScale); + + JLOG(j_.trace()) << "After: " // + << "account " << Number(accountBalanceAfterRounded) << " (" + << Number(accountBalanceAfter) << ")" + << ", vault " << Number(vaultBalanceAfterRounded) << " (" + << Number(vaultBalanceAfter) << ")" + << ", broker " << Number(brokerBalanceAfterRounded) << " (" + << Number(brokerBalanceAfter) << ")" + << ", total " << Number(totalBalanceAfterRounded) << " (" + << Number(totalBalanceAfter) << ")"; + + auto const accountBalanceChange = accountBalanceAfter - accountBalanceBefore; + auto const vaultBalanceChange = vaultBalanceAfter - vaultBalanceBefore; + auto const brokerBalanceChange = brokerBalanceAfter - brokerBalanceBefore; + + auto const totalBalanceChange = accountBalanceChange + vaultBalanceChange + brokerBalanceChange; + auto const totalBalanceChangeRounded = roundToScale(totalBalanceChange, balanceScale); + + JLOG(j_.trace()) << "Changes: " // + << "account " << to_string(accountBalanceChange) // + << ", vault " << to_string(vaultBalanceChange) // + << ", broker " << to_string(brokerBalanceChange) // + << ", total " << to_string(totalBalanceChangeRounded) << " (" + << Number(totalBalanceChange) << ")"; + + bool const goodRounding = totalBalanceBeforeRounded == totalBalanceAfterRounded || + totalBalanceChangeRounded == beast::kZERO; + if (totalBalanceBeforeRounded != totalBalanceAfterRounded) + { + JLOG((goodRounding ? j_.debug() : j_.warn())) + << "Total rounded balances don't match" + << (totalBalanceChangeRounded == beast::kZERO ? ", but total changes do" : ""); + } + if (totalBalanceChangeRounded != beast::kZERO) + { + JLOG((goodRounding ? j_.debug() : j_.warn())) + << "Total balance changes don't match" + << (totalBalanceBeforeRounded == totalBalanceAfterRounded ? ", but total balances do" + : ""); + } + + // Rounding for IOUs can be weird, so check a few different ways to show + // that funds are conserved. XRPL_ASSERT_PARTS( - accountBalanceAfter >= beast::kZERO, "xrpl::LoanPay::doApply", "positive account balance"); + goodRounding, "xrpl::LoanPay::doApply", "funds are conserved (with rounding)"); + XRPL_ASSERT_PARTS( accountBalanceAfter < accountBalanceBefore || account_ == asset.getIssuer(), "xrpl::LoanPay::doApply", @@ -661,7 +819,6 @@ LoanPay::doApply() vaultBalanceAfter > vaultBalanceBefore || brokerBalanceAfter > brokerBalanceBefore, "xrpl::LoanPay::doApply", "vault and/or broker balance increased"); -#endif return tesSUCCESS; } diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 67780414c0..70997b1dcd 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -370,16 +370,11 @@ protected: env.balance(vaultPseudo, broker.asset).number()); if (ownerCount == 0) { - // Allow some slop for rounding IOUs - - // TODO: This needs to be an exact match once all the - // other rounding issues are worked out. + // The Vault must be perfectly balanced if there + // are no loans outstanding auto const total = vaultSle->at(sfAssetsTotal); auto const available = vaultSle->at(sfAssetsAvailable); - env.test.BEAST_EXPECT( - total == available || - (!broker.asset.integral() && available != 0 && - ((total - available) / available < Number(1, -6)))); + env.test.BEAST_EXPECT(total == available); env.test.BEAST_EXPECT(vaultSle->at(sfLossUnrealized) == 0); } } @@ -7177,6 +7172,144 @@ protected: BEAST_EXPECT(afterSecondCoverAvailable == 0); } + void + testYieldTheftRounding(std::uint32_t flags) + { + testcase("Rounding manipulation does not permit yield theft"); + using namespace jtx; + using namespace loan; + + // 1. Setup Environment + Env env(*this, all_); + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(1000), issuer, lender, borrower); + env.close(); + + // 2. Asset Selection + PrettyAsset const iou = issuer["USD"]; + env(trust(lender, iou(100'000'000))); + env(trust(borrower, iou(100'000'000))); + env(pay(issuer, lender, iou(100'000'000))); + env(pay(issuer, borrower, iou(100'000'000))); + env.close(); + + // 3. Create Vault and Broker with High Debt Limit (100M) + auto const brokerInfo = createVaultAndBroker( + env, + iou, + lender, + { + .vaultDeposit = 5'000'000, + .debtMax = Number{100'000'000}, + .coverDeposit = 500'000, + }); + auto const [currentSeq, vaultKeylet] = [&]() { + auto const brokerSle = env.le(keylet::loanbroker(brokerInfo.brokerID)); + if (!BEAST_EXPECT(brokerSle)) + return std::make_tuple(0u, keylet::unchecked(beast::kZERO)); + auto const currentSeq = brokerSle->at(sfLoanSequence); + auto const vaultKeylet = keylet::vault(brokerSle->at(sfVaultID)); + return std::make_tuple(currentSeq, vaultKeylet); + }(); + + // 4. Loan Parameters (Attack Vector) + Number const principal = 1'000'000; + TenthBips32 const interestRate = TenthBips32{1}; // 0.001% + std::uint32_t const paymentInterval = 86400; + std::uint32_t const paymentTotal = 3650; + + auto const loanSetFee = Fee(env.current()->fees().base * 2); + env(set(borrower, brokerInfo.brokerID, iou(principal).value(), flags), + Sig(sfCounterpartySignature, lender), + loan::kINTEREST_RATE(interestRate), + loan::kPAYMENT_INTERVAL(paymentInterval), + loan::kPAYMENT_TOTAL(paymentTotal), + Fee(loanSetFee)); + env.close(); + + // --- RETRIEVE OBJECTS & SETUP ATTACK --- + + auto borrowerBalance = [&]() { return env.balance(borrower, iou); }; + auto const borrowerScale = static_cast(borrowerBalance()).exponent(); + + auto const loanKeylet = keylet::loan(brokerInfo.brokerID, currentSeq); + auto const maybePeriodicPayment = [&]() -> std::optional { + auto const loanSle = env.le(loanKeylet); + if (!BEAST_EXPECT(loanSle)) + return std::nullopt; + // Construct Payment + return STAmount{iou, loanSle->at(sfPeriodicPayment)}; + }(); + if (!maybePeriodicPayment) + return; + auto const periodicPayment = *maybePeriodicPayment; + auto const roundedPayment = + roundToScale(periodicPayment, borrowerScale, Number::RoundingMode::Upward); + + // ATTACK: Add dust buffer (1e-9) to force 'excess' logic execution + STAmount const paymentBuffer{iou, Number(1, -9)}; + STAmount const attackPayment = periodicPayment + paymentBuffer; + + auto const maybeInitialVaultAssets = [&]() -> std::optional { + auto const vault = env.le(vaultKeylet); + if (!BEAST_EXPECT(vault)) + return std::nullopt; + return vault->at(sfAssetsTotal); + }(); + if (!maybeInitialVaultAssets) + return; + auto const initialVaultAssets = *maybeInitialVaultAssets; + + // 5. Execution Loop + int yieldTheftCount = 0; + auto previousAssetsTotal = initialVaultAssets; + + for (int i = 0; i < 100; ++i) + { + auto const balanceBefore = borrowerBalance(); + env(pay(borrower, loanKeylet.key, attackPayment, flags)); + env.close(); + auto const borrowerDelta = balanceBefore - borrowerBalance(); + BEAST_EXPECT(borrowerDelta.signum() == roundedPayment.signum()); + + auto const loanSle = env.le(loanKeylet); + if (!BEAST_EXPECT(loanSle)) + break; + auto const updatedPayment = STAmount{iou, loanSle->at(sfPeriodicPayment)}; + BEAST_EXPECT( + (roundToScale(updatedPayment, borrowerScale, Number::RoundingMode::Upward) == + roundedPayment)); + BEAST_EXPECT( + (updatedPayment == periodicPayment) || + (flags == tfLoanOverpayment && i >= 2 && updatedPayment < periodicPayment)); + + auto const currentVaultSle = env.le(vaultKeylet); + if (!BEAST_EXPECT(currentVaultSle)) + break; + + auto const currentAssetsTotal = currentVaultSle->at(sfAssetsTotal); + auto const delta = currentAssetsTotal - previousAssetsTotal; + + BEAST_EXPECT( + (delta == beast::kZERO && borrowerDelta <= roundedPayment) || + (delta > beast::kZERO && borrowerDelta > roundedPayment)); + + // If tx succeeded but Assets Total didn't change, interest was + // stolen. + if (delta == beast::kZERO && borrowerDelta > roundedPayment) + { + yieldTheftCount++; + } + + previousAssetsTotal = currentAssetsTotal; + } + + BEAST_EXPECTS(yieldTheftCount == 0, std::to_string(yieldTheftCount)); + } + // Tests that vault withdrawals work correctly when the vault has unrealized // loss from an impaired loan, ensuring the invariant check properly // accounts for the loss. @@ -7497,6 +7630,10 @@ public: testLoanPayLateFullPaymentBypassesPenalties(); testLoanCoverMinimumRoundingExploit(); #endif + for (auto const flags : {0u, tfLoanOverpayment}) + { + testYieldTheftRounding(flags); + } testBugInterestDueDeltaCrash(); testFullLifecycleVaultPnLNearZeroRate();