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);