ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-08-21 22:30:57 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Reject vault deposits that move nothing from the depositor (#8014)

Browse Source 
Co-authored-by: Cursor <cursoragent@cursor.com>
This commit is contained in:
Timur Yalymov
2026-08-21 12:30:17 +00:00
committed by GitHub
parent d27beef500
commit 046d4dd4af
2 changed files with 225 additions and 1 deletions
Download Patch File Download Diff File Expand all files Collapse all files

43
src/libxrpl/tx/transactors/vault/VaultDeposit.cpp
View File

@@ -2,12 +2,15 @@
#include <xrpl/basics/Log.h>
#include <xrpl/basics/Number.h>
#include <xrpl/beast/utility/Journal.h>
#include <xrpl/beast/utility/Zero.h>
#include <xrpl/beast/utility/instrumentation.h>
#include <xrpl/ledger/ReadView.h>
#include <xrpl/ledger/helpers/CredentialHelpers.h>
#include <xrpl/ledger/helpers/MPTokenHelpers.h>
#include <xrpl/ledger/helpers/TokenHelpers.h>
#include <xrpl/ledger/helpers/VaultHelpers.h>
#include <xrpl/protocol/AccountID.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/Issue.h>
@@ -47,6 +50,39 @@ roundToVaultScale(STAmount const& amount, SLE::const_ref vault)
return roundToScale(amount, postScale, Number::RoundingMode::Downward);
}
// True if debiting `assets` would leave the depositor's balance where it started, so the deposit
// would mint shares against a transfer that never happened. Asking the balance directly whether it
// notices the debit avoids having to infer the rounding step: it has to be the stored balance that
// answers, because that magnitude is what governs the rounding, and it is not the same as the
// spendable amount, which also counts what the counterparty's limit allows.
[[nodiscard]]
static bool
roundsToZeroForDepositor(
ReadView const& view,
AccountID const& account,
STAmount const& assets,
beast::Journal j)
{
if (assets.integral())
return false;
auto const balance = accountHolds(
view,
account,
assets.asset(),
FreezeHandling::ZeroIfFrozen,
AuthHandling::ZeroIfUnauthorized,
j,
SpendableHandling::SimpleBalance);
if (balance - assets != balance)
return false;
JLOG(j.warn()) << "VaultDeposit: amount " << assets.getFullText()
<< " leaves the depositor's balance " << balance.getFullText() << " unchanged";
return true;
}
NotTEC
VaultDeposit::preflight(PreflightContext const& ctx)
{
@@ -208,6 +244,7 @@ TER
VaultDeposit::doApply()
{
bool const fix320Enabled = view().rules().enabled(fixCleanup3_2_0);
bool const fix340Enabled = view().rules().enabled(fixCleanup3_4_0);
auto const vault = view().peek(keylet::vault(ctx_.tx[sfVaultID]));
auto applyViewContext = ctx_.getApplyViewContext();
if (!vault)
@@ -308,6 +345,12 @@ VaultDeposit::doApply()
return tecINTERNAL;
// LCOV_EXCL_STOP
}
// What a deposit transfers is not the requested amount but that amount truncated to a
// whole number of shares and converted back, which can be smaller. Only here is that
// value known rather than recomputed, so this is where it can be checked against the
// depositor's balance before anything moves.
if (fix340Enabled && roundsToZeroForDepositor(view(), accountID_, *maybeAssets, j_))
return tecPRECISION_LOSS;
assetsDeposited = *maybeAssets;
}
catch (std::overflow_error const&)

183
src/test/app/vault/VaultBugs_test.cpp
View File

@@ -2,9 +2,12 @@
#include <test/jtx/Account.h>
#include <test/jtx/CaptureLogs.h>
#include <test/jtx/Env.h>
#include <test/jtx/TestHelpers.h>
#include <test/jtx/amount.h>
#include <test/jtx/fee.h>
#include <test/jtx/flags.h>
#include <test/jtx/pay.h>
#include <test/jtx/sig.h>
#include <test/jtx/ter.h>
#include <test/jtx/trust.h>
#include <test/jtx/vault.h>
@@ -15,13 +18,17 @@
#include <xrpl/json/json_value.h>
#include <xrpl/protocol/Asset.h>
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/Issue.h>
#include <xrpl/protocol/MPTIssue.h>
#include <xrpl/protocol/Protocol.h>
#include <xrpl/protocol/SField.h>
#include <xrpl/protocol/STAmount.h>
#include <xrpl/protocol/SeqProxy.h>
#include <xrpl/protocol/TER.h>
#include <xrpl/protocol/TxFlags.h>
#include <chrono>
#include <memory>
#include <string>
#include <tuple>
@@ -408,10 +415,14 @@ private:
};
{
// fixCleanup3_4_0 has to be off as well: its depositor-side check
// rejects alice's deposit for the same reason, so the invariant is
// only reachable with neither guard in place.
testcase(
"bug: VaultDeposit below Vault precision canonicalized to zero "
"(pre-fixCleanup3_2_0)");
runScenario(testableAmendments() - fixCleanup3_2_0, tecINVARIANT_FAILED);
runScenario(
testableAmendments() - fixCleanup3_2_0 - fixCleanup3_4_0, tecINVARIANT_FAILED);
}
{
testcase(
@@ -421,6 +432,175 @@ private:
}
}
// A deposit does not transfer the requested amount. It transfers the
// request truncated to a whole number of shares and converted back, which
// can be strictly smaller. When that smaller value is below half a ULP at
// the depositor's own trust-line scale, the debit rounds away to nothing:
// the depositor pays nothing, while the vault books the assets and mints
// shares. ValidVault catches the desync at finalize time.
//
// Only a non-power-of-ten assets-to-shares ratio is needed, and that
// happens through ordinary use: LoanPay books accrued interest into
// sfAssetsTotal without minting shares.
//
// The fixCleanup3_2_0 guard in preclaim does not help, because it tests the
// raw requested amount, which is large enough to survive the rounding.
// Post-fixCleanup3_4_0 the post-truncation value is checked as well and the
// deposit is rejected with tecPRECISION_LOSS before anything moves.
void
testBugDepositShareTruncationSubUlp()
{
using namespace test::jtx;
using namespace loan_broker;
using namespace loan;
// How bob's trust line is set up before he deposits. Holding is the plain case: a large
// positive balance whose ULP swallows the debit. InDebt is the case where the stored
// balance and the spendable amount diverge: bob owes the issuer 1e16, and the issuer's
// limit on the same line lets him spend 1000 anyway. Reading the spendable amount there
// reports a small, finely scaled number, while the rounding of the debit is still governed
// by the 1e16 he actually holds.
enum class Line { Holding, InDebt };
auto runScenario = [this](FeatureBitset features, Line line, TER expected) {
std::string logs;
Env env(*this, features, std::make_unique<test::CaptureLogs>(&logs));
Account const issuer{"issuer"};
Account const alice{"alice"};
Account const carol{"carol"};
Account const bob{"bob"};
env.fund(XRP(100'000), issuer, alice, carol, bob);
env.close();
env(fset(issuer, asfDefaultRipple));
env.close();
PrettyAsset const usd{issuer["USD"]};
PrettyAsset const bobUsd{bob["USD"]};
STAmount const trustLimit{usd.raw(), Number{99'999'999'999'999'999LL}};
// Bob's balance sits exactly on a multiple-of-10 boundary at the
// 1e16 IOU precision cusp, where one ULP is 10.
STAmount const bobEdge{usd.raw(), Number{10'000'000'000'000'010LL}};
STAmount const bobDebt{bobUsd.raw(), Number{10'000'000'000'000'000LL}};
STAmount const oppositeLimit{bobUsd.raw(), Number{10'000'000'000'001'000LL}};
env(trust(alice, trustLimit));
env(trust(carol, trustLimit));
env(trust(bob, trustLimit));
env.close();
env(pay(issuer, alice, usd(1'000)));
env(pay(issuer, carol, usd(1'000)));
if (line == Line::Holding)
{
env(pay(issuer, bob, bobEdge));
}
else
{
// The issuer trusts bob's own USD, so bob can issue 1e16 back and still have
// 1000 of spendable room left on the same line.
env(trust(issuer, oppositeLimit));
env.close();
env(pay(bob, issuer, bobDebt));
}
env.close();
Vault const vault{env};
auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = usd});
vaultTx[sfScale] = 0;
env(vaultTx);
env.close();
// Alice deposits 1000 USD, minting 1000 shares 1:1.
env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(1'000)}));
env.close();
// A loan broker on the vault, then a bullet loan at 24% interest:
// a single payment, one year out.
auto const brokerKeylet =
keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice)));
env(set(alice, vaultKeylet.key));
env.close();
auto const loanKeylet = keylet::loan(brokerKeylet.key, SeqProxy::rawSequence(1));
env(set(carol, brokerKeylet.key, usd(1'000).value()),
loan::kInterestRate(percentageToTenthBips(24)),
kGracePeriod(60),
kPaymentInterval(365 * 24 * 60 * 60),
kPaymentTotal(1),
Sig(sfCounterpartySignature, alice),
Fee(env.current()->fees().base * 2),
Ter(tesSUCCESS));
env.close();
// Advance to just before the single payment falls due and let carol
// repay principal plus interest. LoanPay is what books the accrued
// interest into sfAssetsTotal; under cash-basis accounting LoanSet
// alone does not. Share supply stays at 1000, so
// assetsTotal/sharesTotal becomes 1240/1000.
env.close(std::chrono::seconds{(365 * 24 * 60 * 60) - 3600});
env(pay(carol, loanKeylet.key, usd(2'000).value()), Ter(tesSUCCESS));
env.close();
// Pin the ratio the rest of the scenario reasons about, so the test cannot quietly
// stop exercising the bug if the setup drifts.
auto const sleVault = env.le(vaultKeylet);
BEAST_EXPECT(sleVault && sleVault->at(sfAssetsTotal) == Number{1'240});
auto const sleIssuance = env.le(keylet::mptokenIssuance(sleVault->at(sfShareMPTID)));
BEAST_EXPECT(sleIssuance && sleIssuance->at(sfOutstandingAmount) == 1'000);
// Bob deposits 6 USD, which rounds to 10 at his own trust-line
// scale and so clears the fixCleanup3_2_0 guard. But
// floor(1000 * 6 / 1240) is 4 shares, worth 4 * 1240 / 1000 = 4.96,
// and that is below half a ULP of his balance, so it rounds away to
// nothing when subtracted.
env(vault.deposit({.depositor = bob, .id = vaultKeylet.key, .amount = usd(6)}),
Ter(expected));
env.close();
};
{
testcase(
"bug: VaultDeposit share truncation lets depositor debit "
"round away to zero (pre-fixCleanup3_4_0)");
runScenario(testableAmendments() - fixCleanup3_4_0, Line::Holding, tecINVARIANT_FAILED);
}
{
testcase(
"bug: VaultDeposit share truncation lets depositor debit "
"round away to zero (pre-fixCleanup3_2_0 and pre-fixCleanup3_4_0)");
runScenario(
testableAmendments() - fixCleanup3_2_0 - fixCleanup3_4_0,
Line::Holding,
tecINVARIANT_FAILED);
}
{
testcase(
"bug: VaultDeposit share truncation rejected with "
"tecPRECISION_LOSS (post-fixCleanup3_4_0)");
runScenario(testableAmendments(), Line::Holding, tecPRECISION_LOSS);
}
{
testcase(
"bug: VaultDeposit share truncation rejected with "
"tecPRECISION_LOSS (post-fixCleanup3_4_0, pre-fixCleanup3_2_0)");
runScenario(testableAmendments() - fixCleanup3_2_0, Line::Holding, tecPRECISION_LOSS);
}
{
testcase(
"bug: VaultDeposit share truncation against a debt balance "
"round away to zero (pre-fixCleanup3_4_0)");
runScenario(testableAmendments() - fixCleanup3_4_0, Line::InDebt, tecINVARIANT_FAILED);
}
{
testcase(
"bug: VaultDeposit share truncation against a debt balance rejected with "
"tecPRECISION_LOSS (post-fixCleanup3_4_0)");
runScenario(testableAmendments(), Line::InDebt, tecPRECISION_LOSS);
}
}
// Bug: ValidVault::visitEntry computes destinationDelta.scale as
// max(before_exponent, after_exponent) for RippleState entries. When a
// withdrawal credits a destination whose IOU balance sits just below a
@@ -801,6 +981,7 @@ public:
testBugMakeDeltaPosteriorScale();
testBugMakeDeltaAnteriorScale();
testVaultDepositCanonicalizeToZero();
testBugDepositShareTruncationSubUlp();
testVaultWithdrawCanonicalizeToZero();
testBugVaultDustDebitCanonicalizesToNoOp();
testVaultDepositNegativeBalanceFromOppositeLimit();