ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-06-03 00:36:48 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Fix touchy "funds are conserved" assertion in LoanPay (#6231) (#6967)

Browse Source 
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
This commit is contained in:
Ed Hennis
2026-05-12 15:25:11 -04:00
committed by GitHub
parent 6c2266c5c7
commit 8012b5d34f
3 changed files with 345 additions and 41 deletions
Download Patch File Download Diff File Expand all files Collapse all files

14
src/libxrpl/ledger/helpers/LendingHelpers.cpp
View File

@@ -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(

219
src/libxrpl/tx/transactors/lending/LoanPay.cpp
View File

@@ -30,6 +30,7 @@
#include <bit>
#include <cstdint>
#include <memory>
#include <vector>
namespace xrpl {
@@ -162,6 +163,7 @@ LoanPay::calculateBaseFee(ReadView const& view, STTx const& tx)
Number const numPaymentEstimate = static_cast<std::int64_t>(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<int> 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;
}

153
src/test/app/Loan_test.cpp
View File

@@ -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<STAmount const&>(borrowerBalance()).exponent();
auto const loanKeylet = keylet::loan(brokerInfo.brokerID, currentSeq);
auto const maybePeriodicPayment = [&]() -> std::optional<STAmount> {
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<Number> {
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();