Compare commits

..

4 Commits

Author SHA1 Message Date
Bart
04249ae86e Apply clang-tidy diff 2026-04-28 13:39:39 -04:00
Bart
7962ed3c22 Merge branch 'develop' into bthomee/logic 2026-04-28 13:39:18 -04:00
Bart
58beb2d9ba Remove unused function 2026-04-28 11:16:59 -04:00
Bart
0d6f9b8428 refactor: Revert certain Throws by LogicErrors 2026-04-27 15:49:02 -04:00
5 changed files with 46 additions and 383 deletions

View File

@@ -229,19 +229,6 @@ public:
std::shared_ptr<Serializer const> const& txn,
std::shared_ptr<Serializer const> const& metaData) override;
// Insert the transaction, and return the hash of the SHAMap leaf node
// holding the transaction. The hash can be used to fetch the transaction
// directly, instead of traversing the SHAMap
// @param key transaction ID
// @param txn transaction
// @param metaData transaction metadata
// @return hash of SHAMap leaf node that holds the transaction
uint256
rawTxInsertWithHash(
uint256 const& key,
std::shared_ptr<Serializer const> const& txn,
std::shared_ptr<Serializer const> const& metaData);
//--------------------------------------------------------------------------
void

View File

@@ -14,7 +14,6 @@
#include <xrpl/nodestore/NodeObject.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Fees.h>
#include <xrpl/protocol/HashPrefix.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/KeyType.h>
#include <xrpl/protocol/Keylet.h>
@@ -31,7 +30,6 @@
#include <xrpl/protocol/Seed.h>
#include <xrpl/protocol/Serializer.h>
#include <xrpl/protocol/SystemParameters.h>
#include <xrpl/protocol/digest.h>
#include <xrpl/shamap/Family.h>
#include <xrpl/shamap/SHAMap.h>
#include <xrpl/shamap/SHAMapItem.h>
@@ -43,7 +41,6 @@
#include <exception>
#include <memory>
#include <optional>
#include <stdexcept>
#include <utility>
#include <vector>
@@ -493,14 +490,14 @@ void
Ledger::rawErase(std::shared_ptr<SLE> const& sle)
{
if (!stateMap_.delItem(sle->key()))
Throw<std::logic_error>("Ledger::rawErase: key not found");
LogicError("Ledger::rawErase: key not found");
}
void
Ledger::rawErase(uint256 const& key)
{
if (!stateMap_.delItem(key))
Throw<std::logic_error>("Ledger::rawErase: key not found");
LogicError("Ledger::rawErase: key not found");
}
void
@@ -510,7 +507,7 @@ Ledger::rawInsert(std::shared_ptr<SLE> const& sle)
sle->add(ss);
if (!stateMap_.addGiveItem(
SHAMapNodeType::tnACCOUNT_STATE, make_shamapitem(sle->key(), ss.slice())))
Throw<std::logic_error>("Ledger::rawInsert: key already exists");
LogicError("Ledger::rawInsert: key already exists");
}
void
@@ -520,7 +517,7 @@ Ledger::rawReplace(std::shared_ptr<SLE> const& sle)
sle->add(ss);
if (!stateMap_.updateGiveItem(
SHAMapNodeType::tnACCOUNT_STATE, make_shamapitem(sle->key(), ss.slice())))
Throw<std::logic_error>("Ledger::rawReplace: key not found");
LogicError("Ledger::rawReplace: key not found");
}
void
@@ -536,27 +533,7 @@ Ledger::rawTxInsert(
s.addVL(txn->peekData());
s.addVL(metaData->peekData());
if (!txMap_.addGiveItem(SHAMapNodeType::tnTRANSACTION_MD, make_shamapitem(key, s.slice())))
Throw<std::logic_error>("duplicate_tx: " + to_string(key));
}
uint256
Ledger::rawTxInsertWithHash(
uint256 const& key,
std::shared_ptr<Serializer const> const& txn,
std::shared_ptr<Serializer const> const& metaData)
{
XRPL_ASSERT(metaData, "xrpl::Ledger::rawTxInsertWithHash : non-null metadata input");
// low-level - just add to table
Serializer s(txn->getDataLength() + metaData->getDataLength() + 16);
s.addVL(txn->peekData());
s.addVL(metaData->peekData());
auto item = make_shamapitem(key, s.slice());
auto hash = sha512Half(HashPrefix::txNode, item->slice(), item->key());
if (!txMap_.addGiveItem(SHAMapNodeType::tnTRANSACTION_MD, std::move(item)))
Throw<std::logic_error>("duplicate_tx: " + to_string(key));
return hash;
LogicError("duplicate_tx: " + to_string(key));
}
bool

View File

@@ -1857,18 +1857,8 @@ 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::towards_zero)
: amount;
if (paymentType == LoanPaymentType::overpayment && loan->isFlag(lsfLoanOverpayment) &&
paymentRemainingProxy > 0 && totalPaid < roundedAmount &&
paymentRemainingProxy > 0 && totalPaid < amount &&
numPayments < loanMaximumPaymentsPerTransaction)
{
TenthBips32 const overpaymentInterestRate{loan->at(sfOverpaymentInterestRate)};
@@ -1877,7 +1867,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(roundedAmount - totalPaid, *totalValueOutstandingProxy);
Number const overpayment = std::min(amount - totalPaid, *totalValueOutstandingProxy);
detail::ExtendedPaymentComponents const overpaymentComponents =
detail::computeOverpaymentComponents(

View File

@@ -30,7 +30,6 @@
#include <bit>
#include <cstdint>
#include <memory>
#include <vector>
namespace xrpl {
@@ -144,7 +143,6 @@ 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::upward);
auto const feeIncrements = std::max(
std::int64_t(1),
@@ -442,10 +440,9 @@ 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,
@@ -469,6 +466,16 @@ 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
@@ -494,68 +501,12 @@ LoanPay::doApply()
associateAsset(*vaultSle, asset);
// Duplicate some checks after rounding
Number const assetsAvailableAfter = *assetsAvailableProxy;
Number const assetsTotalAfter = *assetsTotalProxy;
XRPL_ASSERT_PARTS(
assetsAvailableAfter <= assetsTotalAfter,
*assetsAvailableProxy <= *assetsTotalProxy,
"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::zero && 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::zero && 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
}
// These three values are used to check that funds are conserved after the transfers
#if !NDEBUG
auto const accountBalanceBefore = accountHolds(
view,
account_,
@@ -584,6 +535,7 @@ LoanPay::doApply()
ahIGNORE_AUTH,
j_,
SpendableHandling::shFULL_BALANCE);
#endif
if (totalPaidToVaultRounded != beast::zero)
{
@@ -619,22 +571,19 @@ LoanPay::doApply()
return ter;
#if !NDEBUG
{
Number const pseudoAccountBalanceAfter = accountHolds(
view,
vaultPseudoAccount,
asset,
FreezeHandling::fhIGNORE_FREEZE,
AuthHandling::ahIGNORE_AUTH,
j_);
XRPL_ASSERT_PARTS(
assetsAvailableAfter == pseudoAccountBalanceAfter,
"xrpl::LoanPay::doApply",
"vault pseudo balance agrees after");
}
#endif
Number const assetsAvailableAfter = *assetsAvailableProxy;
Number const pseudoAccountBalanceAfter = accountHolds(
view,
vaultPseudoAccount,
asset,
FreezeHandling::fhIGNORE_FREEZE,
AuthHandling::ahIGNORE_AUTH,
j_);
XRPL_ASSERT_PARTS(
assetsAvailableAfter == pseudoAccountBalanceAfter,
"xrpl::LoanPay::doApply",
"vault pseudo balance agrees after");
// Check that funds are conserved
auto const accountBalanceAfter = accountHolds(
view,
account_,
@@ -663,121 +612,14 @@ LoanPay::doApply()
ahIGNORE_AUTH,
j_,
SpendableHandling::shFULL_BALANCE);
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::cMaxOffset, just in case the numbers are all very large.
std::vector<int> exponents;
for (auto const& a : {
accountBalanceBefore,
vaultBalanceBefore,
brokerBalanceBefore,
accountBalanceAfter,
vaultBalanceAfter,
brokerBalanceAfter,
})
{
// Exclude zeroes
if (a != beast::zero)
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::cMaxOffset);
}();
// No object changes are made below this point
XRPL_ASSERT_PARTS(
Number::getround() == Number::to_nearest,
"xrpl::LoanPay::doApply",
"Number rounding to_nearest");
NumberRoundModeGuard const mg(Number::to_nearest);
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) << ")";
if (totalBalanceBeforeRounded != totalBalanceAfterRounded)
{
JLOG(j_.warn()) << "Total rounded balances don't match"
<< (totalBalanceChangeRounded == beast::zero ? ", but total changes do"
: "");
}
if (totalBalanceChangeRounded != beast::zero)
{
JLOG(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(
totalBalanceBeforeRounded == totalBalanceAfterRounded ||
totalBalanceChangeRounded == beast::zero,
accountBalanceBefore + vaultBalanceBefore + brokerBalanceBefore ==
accountBalanceAfter + vaultBalanceAfter + brokerBalanceAfter,
"xrpl::LoanPay::doApply",
"funds are conserved (with rounding)");
XRPL_ASSERT_PARTS(
accountBalanceAfter >= beast::zero, "xrpl::LoanPay::doApply", "positive account balance");
XRPL_ASSERT_PARTS(
accountBalanceAfter < accountBalanceBefore || account_ == asset.getIssuer(),
"xrpl::LoanPay::doApply",
@@ -798,6 +640,7 @@ LoanPay::doApply()
vaultBalanceAfter > vaultBalanceBefore || brokerBalanceAfter > brokerBalanceBefore,
"xrpl::LoanPay::doApply",
"vault and/or broker balance increased");
#endif
return tesSUCCESS;
}

View File

@@ -370,11 +370,16 @@ protected:
env.balance(vaultPseudo, broker.asset).number());
if (ownerCount == 0)
{
// The Vault must be perfectly balanced if there
// are no loans outstanding
// Allow some slop for rounding IOUs
// TODO: This needs to be an exact match once all the
// other rounding issues are worked out.
auto const total = vaultSle->at(sfAssetsTotal);
auto const available = vaultSle->at(sfAssetsAvailable);
env.test.BEAST_EXPECT(total == available);
env.test.BEAST_EXPECT(
total == available ||
(!broker.asset.integral() && available != 0 &&
((total - available) / available < Number(1, -6))));
env.test.BEAST_EXPECT(vaultSle->at(sfLossUnrealized) == 0);
}
}
@@ -7063,140 +7068,6 @@ 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));
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::interestRate(interestRate),
loan::paymentInterval(paymentInterval),
loan::paymentTotal(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::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::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::zero && borrowerDelta <= roundedPayment) ||
(delta > beast::zero && borrowerDelta > roundedPayment));
// If tx succeeded but Assets Total didn't change, interest was
// stolen.
if (delta == beast::zero && 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.
@@ -7335,11 +7206,6 @@ public:
testLoanPayLateFullPaymentBypassesPenalties();
testLoanCoverMinimumRoundingExploit();
#endif
for (auto const flags : {0u, tfLoanOverpayment})
{
testYieldTheftRounding(flags);
}
testWithdrawReflectsUnrealizedLoss();
testInvalidLoanSet();