From bad77604ed9df1a42cf10d01a757099b87c2150c Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Wed, 22 Jul 2026 17:52:38 +0200 Subject: [PATCH] fix: Conserve funds correctly when LoanPay fee payee is below reserve LoanPay's fund-conservation check read XRP balances via accountHolds(SpendableHandling::FullBalance), which for XRP always defers to xrpLiquid (balance minus reserve, clamped at zero). When the broker fee landed on a payee sitting below its own reserve, the payee's clamped balance stayed zero and the fee vanished from the conservation sum, tripping the "funds are conserved" assert on an otherwise valid payment. --- .../tx/transactors/lending/LoanPay.cpp | 85 ++++++--------- src/test/app/Loan_test.cpp | 103 ++++++++++++++++++ 2 files changed, 139 insertions(+), 49 deletions(-) diff --git a/src/libxrpl/tx/transactors/lending/LoanPay.cpp b/src/libxrpl/tx/transactors/lending/LoanPay.cpp index 54ee85b186..8a23670768 100644 --- a/src/libxrpl/tx/transactors/lending/LoanPay.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanPay.cpp @@ -33,6 +33,34 @@ namespace xrpl { +namespace { +// Returns the account's true, unclamped balance in `asset`, for use only in +// fund-conservation checks. accountHolds(..., SpendableHandling::FullBalance) +// cannot be used for this: for XRP it always defers to xrpLiquid, which +// subtracts the account's reserve, so a payee sitting below its own reserve +// would appear to receive nothing even though its raw ledger balance grew. +// That mismatch is exactly what a conservation check must not see. +STAmount +conservationBalance(ReadView const& view, AccountID const& id, Asset const& asset, beast::Journal j) +{ + if (isXRP(asset)) + { + auto const sle = view.read(keylet::account(id)); + if (!sle) + return STAmount{}; + return view.balanceHookIOU(id, xrpAccount(), sle->getFieldAmount(sfBalance)); + } + return accountHolds( + view, + id, + asset, + FreezeHandling::IgnoreFreeze, + AuthHandling::IgnoreAuth, + j, + SpendableHandling::FullBalance); +} +} // namespace + bool LoanPay::checkExtraFeatures(PreflightContext const& ctx) { @@ -584,34 +612,13 @@ LoanPay::doApply() } // These three values are used to check that funds are conserved after the transfers - auto const accountBalanceBefore = accountHolds( - view, - accountID_, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + auto const accountBalanceBefore = conservationBalance(view, accountID_, asset, j_); auto const vaultBalanceBefore = accountID_ == vaultPseudoAccount ? STAmount{asset, 0} - : accountHolds( - view, - vaultPseudoAccount, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + : conservationBalance(view, vaultPseudoAccount, asset, j_); auto const brokerBalanceBefore = accountID_ == brokerPayee ? STAmount{asset, 0} - : accountHolds( - view, - brokerPayee, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + : conservationBalance(view, brokerPayee, asset, j_); if (totalPaidToVaultRounded != beast::kZero) { @@ -667,33 +674,13 @@ LoanPay::doApply() #endif // Check that funds are conserved - auto const accountBalanceAfter = accountHolds( - view, - accountID_, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + auto const accountBalanceAfter = conservationBalance(view, accountID_, asset, j_); auto const vaultBalanceAfter = accountID_ == vaultPseudoAccount ? STAmount{asset, 0} - : accountHolds( - view, - vaultPseudoAccount, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); - auto const brokerBalanceAfter = accountID_ == brokerPayee ? STAmount{asset, 0} - : accountHolds( - view, - brokerPayee, - asset, - FreezeHandling::IgnoreFreeze, - AuthHandling::IgnoreAuth, - j_, - SpendableHandling::FullBalance); + : conservationBalance(view, vaultPseudoAccount, asset, j_); + auto const brokerBalanceAfter = accountID_ == brokerPayee + ? STAmount{asset, 0} + : conservationBalance(view, brokerPayee, asset, j_); auto const balanceScale = [&]() { // Find a reasonable scale to use for the balance comparisons. // diff --git a/src/test/app/Loan_test.cpp b/src/test/app/Loan_test.cpp index 231a3b405a..17c6e34242 100644 --- a/src/test/app/Loan_test.cpp +++ b/src/test/app/Loan_test.cpp @@ -8547,6 +8547,107 @@ protected: }); } + void + testLoanPayFundsConservedPayeeBelowReserve(FeatureBitset features) + { + // Regression test: LoanPay::doApply's fund-conservation check used to + // read XRP balances via accountHolds(..., SpendableHandling:: + // FullBalance), which for XRP always defers to xrpLiquid (balance + // minus reserve, clamped at zero). When the broker fee landed on a + // payee sitting below its own reserve, that payee's clamped balance + // stayed zero and the fee vanished from the conservation sum, + // tripping "funds are conserved (with rounding)". + testcase << "LoanPay funds conserved: broker fee payee below reserve"; + + using namespace jtx; + + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + // Broker defaults match the fuzz workload: ManagementFeeRate = 100 + // tenth-bips. The service fee guarantees feePaid > 0 on the first + // regular payment. + BrokerParameters const brokerParams; + Number const serviceFeeValue{2}; + LoanParameters const loanParams{ + .account = borrower, + .counter = lender, + .principalRequest = 1000, + .serviceFee = serviceFeeValue, + .interest = TenthBips32{percentageToTenthBips(12)}, + .payTotal = 12, + .payInterval = 3600}; + + auto const loanOpt = + createLoan(env, AssetType::XRP, brokerParams, loanParams, issuer, lender, borrower); + if (!BEAST_EXPECT(loanOpt)) + return; + auto const& [broker, loanKeylet, brokerPseudo] = *loanOpt; + + auto const vaultPseudo = [&]() { + auto const vaultSle = env.le(keylet::vault(broker.vaultID)); + BEAST_EXPECT(vaultSle); + return vaultSle ? vaultSle->at(sfAccount) : AccountID{}; + }(); + + // Raw AccountRoot balance, matching LoanPay::doApply's conservation + // check (not the reserve-clamped accountHolds()/xrpLiquid() value). + auto rawBalance = [&](AccountID const& id) -> STAmount { + auto const sle = env.le(keylet::account(id)); + return sle ? sle->getFieldAmount(sfBalance) : STAmount{}; + }; + auto lenderReserve = [&] { + return env.current()->fees().accountReserve(ownerCount(env, lender), 1); + }; + + STAmount const baseFee{env.current()->fees().base}; + + // Park the lender (broker owner, fee payee) exactly at its reserve, + // then burn part of the reserve with an oversized transaction fee. + // Fees are exempt from the reserve check, so the balance ends up + // below the reserve. + env(pay(lender, issuer, rawBalance(lender.id()) - lenderReserve() - baseFee)); + env(noop(lender), Fee(XRP(100))); + env.close(); + BEAST_EXPECT(env.balance(lender) < lenderReserve()); + + // First regular payment, exactly the amount due. + auto const state = getCurrentState(env, broker, loanKeylet); + STAmount const serviceFee = broker.asset(serviceFeeValue); + STAmount const roundedPeriodicPayment{ + broker.asset, + roundPeriodicPayment(broker.asset, state.periodicPayment, state.loanScale)}; + STAmount const totalDue = roundToScale( + roundedPeriodicPayment + serviceFee, state.loanScale, Number::RoundingMode::Upward); + + auto const borrowerBefore = rawBalance(borrower.id()); + auto const vaultBefore = rawBalance(vaultPseudo); + auto const lenderBefore = rawBalance(lender.id()); + + // Before the fix, this aborted inside LoanPay::doApply on + // XRPL_ASSERT_PARTS(goodRounding, "xrpl::LoanPay::doApply", "funds + // are conserved (with rounding)"). + env(loan::pay(borrower, loanKeylet.key, totalDue)); + env.close(); + + auto const borrowerAfter = rawBalance(borrower.id()); + auto const vaultAfter = rawBalance(vaultPseudo); + auto const lenderAfter = rawBalance(lender.id()); + + // The broker fee reached the lender's AccountRoot, even though the + // lender's balance remains below its reserve. + BEAST_EXPECT(lenderAfter > lenderBefore); + BEAST_EXPECT(lenderAfter < lenderReserve()); + + // Total funds conserved across the payer, vault, and fee payee. + BEAST_EXPECT( + borrowerBefore - baseFee + vaultBefore + lenderBefore == + borrowerAfter + vaultAfter + lenderAfter); + } + void runAmendmentIndependent() { @@ -8580,6 +8681,8 @@ protected: testLoanPayLateFullPaymentBypassesPenalties(features); testLoanCoverMinimumRoundingExploit(features); #endif + testLoanPayFundsConservedPayeeBelowReserve(features); + // Lifecycle testLifecycle(features); testLoanSet(features);