From ec8a9cdbf8bf46b062db2f89b77452edbcaa09a4 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Wed, 19 Aug 2026 19:54:50 +0200 Subject: [PATCH] fix: Clamp Vault Deposit, Withdraw, and Clawback to assetsTotal grid sfAssetsTotal is stored on a coarser STAmount grid than sfAssetsAvailable and the vault's trust line, so adding the same amount to all three quantizes differently on each and leaves the vault's books disagreeing with its actual holdings by a sub-ULP amount. Fix by clamping the credited/withdrawn amount to what sfAssetsTotal can represent before applying it to the other rails, via a shared clampToAssetsTotalScale helper used by all three transactors. - Deposit: clamp assetsDeposited downward to the assetsTotal grid, then re-derive shares from the clamped amount so the depositor cannot receive shares worth more than they paid. Return tecPRECISION_LOSS if the clamp rounds the deposit to zero. - Withdraw: clamp assetsWithdrawn upward (i.e. the vault pays out slightly less) so it never pays out more than it can account for. Shares are not re-derived, so the withdrawer receives slightly less per share, favouring remaining holders. - Clawback: same pattern as withdraw, guarded on assetsRecovered > 0 and placed after the existing clamp-to-available. Gated on fixCleanup3_4_0. --- include/xrpl/ledger/helpers/VaultHelpers.h | 22 + src/libxrpl/ledger/helpers/VaultHelpers.cpp | 12 + .../tx/transactors/vault/VaultClawback.cpp | 12 + .../tx/transactors/vault/VaultDeposit.cpp | 21 + .../tx/transactors/vault/VaultWithdraw.cpp | 10 + src/test/app/lending/VaultPrecisionFixture.h | 248 ++++++ .../lending/VaultTransactorPrecision_test.cpp | 779 ++++++++++++++++++ src/test/app/vault/VaultBugs_test.cpp | 6 +- 8 files changed, 1109 insertions(+), 1 deletion(-) create mode 100644 src/test/app/lending/VaultPrecisionFixture.h create mode 100644 src/test/app/lending/VaultTransactorPrecision_test.cpp diff --git a/include/xrpl/ledger/helpers/VaultHelpers.h b/include/xrpl/ledger/helpers/VaultHelpers.h index acbf2c3ac0..b3c04308cd 100644 --- a/include/xrpl/ledger/helpers/VaultHelpers.h +++ b/include/xrpl/ledger/helpers/VaultHelpers.h @@ -1,5 +1,6 @@ #pragma once +#include #include #include #include @@ -41,6 +42,27 @@ assetsToSharesDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount co [[nodiscard]] std::optional sharesToAssetsDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount const& shares); +/** + * Clamps `delta` (positive when crediting the vault, negative when debiting + * it) to the largest magnitude that changes sfAssetsTotal by an exact + * multiple of its own STAmount grid step, rounding in `mode`. The returned + * delta has the same sign convention as `delta` (i.e. this is a magnitude + * adjustment toward zero, never away from it). Applying the returned delta + * to both sfAssetsTotal and any other field derived from the same raw amount + * (e.g. sfAssetsAvailable) keeps them exactly in sync, since those fields' + * scale is always at least as fine as sfAssetsTotal's. + * + * @param vault The vault SLE (mutable, since sfAssetsTotal is read through a + * non-const field proxy). + * @param delta The signed amount by which sfAssetsTotal is about to change. + * @param mode The rounding mode to apply when quantizing to sfAssetsTotal's + * scale. + * + * @return The clamped delta. + */ +[[nodiscard]] STAmount +clampToAssetsTotalScale(SLE::ref vault, STAmount const& delta, Number::RoundingMode mode); + /** * Controls whether to truncate shares instead of rounding. */ diff --git a/src/libxrpl/ledger/helpers/VaultHelpers.cpp b/src/libxrpl/ledger/helpers/VaultHelpers.cpp index 67e0262e14..74514f7fe7 100644 --- a/src/libxrpl/ledger/helpers/VaultHelpers.cpp +++ b/src/libxrpl/ledger/helpers/VaultHelpers.cpp @@ -67,6 +67,18 @@ sharesToAssetsDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount co return assets; } +[[nodiscard]] STAmount +clampToAssetsTotalScale(SLE::ref vault, STAmount const& delta, Number::RoundingMode mode) +{ + Asset const asset = *vault->at(sfAsset); + STAmount const totalBefore{asset, *vault->at(sfAssetsTotal)}; + STAmount const totalAfter = [&]() { + NumberRoundModeGuard const mg(mode); + return STAmount{asset, *vault->at(sfAssetsTotal) + delta}; + }(); + return delta.negative() ? totalBefore - totalAfter : totalAfter - totalBefore; +} + [[nodiscard]] std::optional assetsToSharesWithdraw( SLE::const_ref vault, diff --git a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp index d77286b667..8e85dc785a 100644 --- a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp @@ -325,6 +325,18 @@ VaultClawback::assetsToClawback( return std::unexpected(tecPATH_DRY); } + if (ctx_.view().rules().enabled(fixCleanup3_4_0) && assetsRecovered > beast::kZero) + { + // sharesDestroyed is deliberately NOT re-derived from the clamped + // amount: the holder's shares are burned for their pre-clamp value, + // so any sub-ULP amount trimmed off here is left behind in the + // vault, favouring the remaining shareholders. + assetsRecovered = + clampToAssetsTotalScale(vault, -assetsRecovered, Number::RoundingMode::Upward); + if (assetsRecovered <= beast::kZero) + return std::unexpected(tecPRECISION_LOSS); + } + return std::make_pair(assetsRecovered, sharesDestroyed); } diff --git a/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp b/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp index a3c0a94eb5..65ba4d351c 100644 --- a/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp @@ -208,6 +208,7 @@ TER VaultDeposit::doApply() { bool const fix320Enabled = view().rules().enabled(fixCleanup3_2_0); + bool const fixEnabled = view().rules().enabled(fixCleanup3_4_0); auto const vault = view().peek(keylet::vault(ctx_.tx[sfVaultID])); auto applyViewContext = ctx_.getApplyViewContext(); if (!vault) @@ -309,6 +310,26 @@ VaultDeposit::doApply() // LCOV_EXCL_STOP } assetsDeposited = *maybeAssets; + + if (fixEnabled) + { + assetsDeposited = + clampToAssetsTotalScale(vault, assetsDeposited, Number::RoundingMode::Downward); + + if (assetsDeposited <= beast::kZero) + { + JLOG(j_.warn()) << "VaultDeposit: deposit rounds to zero at " + "assets outstanding scale."; + return tecPRECISION_LOSS; + } + + auto const maybeReShares = assetsToSharesDeposit(vault, sleIssuance, assetsDeposited); + if (!maybeReShares) + return tecINTERNAL; + sharesCreated = *maybeReShares; + if (sharesCreated == beast::kZero) + return tecPRECISION_LOSS; + } } catch (std::overflow_error const&) { diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index 7b5bb1ea94..c8e7ddacee 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -203,6 +204,7 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx) TER VaultWithdraw::doApply() { + bool const fixEnabled = view().rules().enabled(fixCleanup3_4_0); auto const vault = view().peek(keylet::vault(ctx_.tx[sfVaultID])); auto applyViewContext = ctx_.getApplyViewContext(); if (!vault) @@ -304,6 +306,14 @@ VaultWithdraw::doApply() lossUnrealized <= (assetsTotal - assetsAvailable), "xrpl::VaultWithdraw::doApply : loss and assets do balance"); + if (fixEnabled) + { + assetsWithdrawn = + clampToAssetsTotalScale(vault, -assetsWithdrawn, Number::RoundingMode::Upward); + if (assetsWithdrawn <= beast::kZero) + return tecPRECISION_LOSS; + } + // The vault must have enough assets on hand. if (*assetsAvailable < assetsWithdrawn) { diff --git a/src/test/app/lending/VaultPrecisionFixture.h b/src/test/app/lending/VaultPrecisionFixture.h new file mode 100644 index 0000000000..0a26e8ccbf --- /dev/null +++ b/src/test/app/lending/VaultPrecisionFixture.h @@ -0,0 +1,248 @@ +#pragma once + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include + +namespace xrpl::test { + +// Shared fixture for VaultInvariantPrecision_test (PR 1) and +// VaultTransactorPrecision_test (PR 2). Both PRs land this file identically; +// whichever merges second will need to reconcile this copy at rebase time. +// +// Layout: +// - A-1 (impairAndPaySibling=false): 1000 USD vault + one ordinary loan. +// assetsTotal ~= 1000.353..., assetsAvailable == 993, lossUnrealized == 0. +// - A-3 (impairAndPaySibling=true): add a second loan of principal 11, +// impair the first loan, and pay off the second in full. This drives +// the vault to the lossUnrealized == (assetsTotal - assetsAvailable) +// boundary where the loss invariant used to spuriously fire. +class VaultPrecisionFixture : public LoanTestBase +{ +protected: + static constexpr std::uint32_t kFixturePaymentInterval = 86400u * 30u; + static constexpr std::uint32_t kFixtureGracePeriod = 86400u * 30u; + static constexpr std::uint32_t kFixturePaymentTotal = 120u; + // 10% APR, expressed in tenth-bips (1000 = 10.00 %). + static constexpr std::uint32_t kFixtureInterestTenthBips = 1000u; + + struct Fixture + { + // Every account is initialised with a placeholder name because + // jtx::Account has no default constructor; setupSingleLoanVault + // overwrites them. + jtx::Account issuer{"vp_issuer_placeholder"}; + jtx::Account lender{"vp_lender_placeholder"}; + jtx::Account borrower{"vp_borrower_placeholder"}; + // Distinct account used to deposit into the vault. Keeps share + // ownership independent of the initial vault seeding. + jtx::Account depositor{"vp_depositor_placeholder"}; + // Optional so callers can BEAST_EXPECT(f.asset && f.broker) + // after setup; both are populated in the happy path. + std::optional asset; + std::optional broker; + // Keylet has no default constructor. Fill with an obviously + // meaningless placeholder; setupSingleLoanVault overwrites the + // fields that matter. + Keylet vaultKeylet{ltACCOUNT_ROOT, uint256{}}; + Keylet loan1Keylet{ltACCOUNT_ROOT, uint256{}}; + // Only meaningful when impairAndPaySibling == true. + Keylet loan2Keylet{ltACCOUNT_ROOT, uint256{}}; + jtx::Account vaultAccount{"vp_vault_pseudo_placeholder"}; + MPTID share; + }; + + // Read-only snapshot of the vault + share issuance at a point in time. + // Uses Number for exact arithmetic (no re-quantization). + struct Numbers + { + Asset asset; + MPTIssue share; + Number assetsTotal{}; // sfAssetsTotal + Number assetsAvailable{}; // sfAssetsAvailable + Number lossUnrealized{}; // sfLossUnrealized + Number pseudo{}; // vault pseudo-account balance in the asset + Number sharesTotal{}; // sfOutstandingAmount on the share MPT + }; + + static Numbers + read(jtx::Env const& env, Fixture const& f) + { + Numbers n{.asset = f.asset ? f.asset->raw() : Asset{}, .share = MPTIssue{f.share}}; + if (auto const vaultSle = env.le(f.vaultKeylet)) + { + n.assetsTotal = vaultSle->at(sfAssetsTotal); + n.assetsAvailable = vaultSle->at(sfAssetsAvailable); + n.lossUnrealized = vaultSle->at(sfLossUnrealized); + } + if (auto const issuanceSle = env.le(keylet::mptokenIssuance(f.share))) + { + n.sharesTotal = issuanceSle->at(sfOutstandingAmount); + } + if (f.asset) + n.pseudo = env.balance(f.vaultAccount, *f.asset).number(); + return n; + } + + // One unit at the STAmount scale of `assetsTotalAfter`. Used as the + // tolerance in one-unit-band assertions. + static Number + oneUnit(Asset const& asset, Number const& assetsTotalAfter) + { + return Number{1, scale(assetsTotalAfter, asset)}; + } + + // Build the shared vault + loan(s) layout. The caller constructs + // `env` with whatever FeatureBitset they want to exercise; this helper + // just uses it. If `allowClawback` is true, the issuer's + // asfAllowTrustLineClawback flag is set BEFORE any trust line is + // established for that issuer. A separate env.close() runs so the + // flag lands in the ledger before the trust lines are set up. + static Fixture + setupSingleLoanVault(jtx::Env& env, bool impairAndPaySibling, bool allowClawback = false) + { + using namespace jtx; + using namespace jtx::loan; + using namespace jtx::loan_broker; + + Fixture f; + f.issuer = Account{"vp_issuer"}; + f.lender = Account{"vp_lender"}; + f.borrower = Account{"vp_borrower"}; + f.depositor = Account{"vp_depositor"}; + + env.fund(XRP(1'000'000), f.issuer, f.lender, f.borrower, f.depositor); + env.close(); + + // Must be set BEFORE any trust line to `issuer` is created. + if (allowClawback) + { + env(fset(f.issuer, asfAllowTrustLineClawback)); + env.close(); + } + + PrettyAsset const asset = f.issuer["USD"]; + f.asset = asset; + + env.trust(asset(1'000'000'000), f.lender); + env.trust(asset(1'000'000'000), f.borrower); + env.trust(asset(1'000'000'000), f.depositor); + env(pay(f.issuer, f.lender, asset(100'000'000))); + env(pay(f.issuer, f.borrower, asset(100'000'000))); + env(pay(f.issuer, f.depositor, asset(100'000'000))); + env.close(); + + BrokerParameters const brokerParams{ + .vaultDeposit = 1'000, + .debtMax = 0, + .coverRateMin = percentageToTenthBips(1), + .coverDeposit = 10'000, + .managementFeeRate = TenthBips16{100}, + .coverRateLiquidation = xrpl::lending::kMaxCoverRate}; + + // Build the vault + broker manually (rather than calling + // createVaultAndBroker) so we can seed only the lender/depositor + // trust lines we set up above, and skip the LoanTestBase auto + // funding that assumes an XRP asset. + Vault const vault{env}; + auto [createTx, vaultKeylet] = vault.create({.owner = f.lender, .asset = asset}); + env(createTx); + env.close(); + f.vaultKeylet = vaultKeylet; + + env(vault.deposit( + {.depositor = f.lender, + .id = vaultKeylet.key, + .amount = asset(brokerParams.vaultDeposit)})); + env.close(); + + auto const brokerKeylet = + keylet::loanBroker(f.lender.id(), SeqProxy::rawSequence(env.seq(f.lender))); + + env(set(f.lender, vaultKeylet.key, brokerParams.flags), + kManagementFeeRate(brokerParams.managementFeeRate), + kDebtMaximum(asset(brokerParams.debtMax).value()), + kCoverRateMinimum(brokerParams.coverRateMin), + kCoverRateLiquidation(TenthBips32(brokerParams.coverRateLiquidation))); + env(coverDeposit(f.lender, brokerKeylet.key, asset(brokerParams.coverDeposit).value())); + env.close(); + + f.broker = BrokerInfo{asset, brokerKeylet, vaultKeylet, brokerParams}; + + auto const vaultSle = env.le(vaultKeylet); + f.vaultAccount = Account{"vp_vault_pseudo", vaultSle->at(sfAccount)}; + f.share = vaultSle->at(sfShareMPTID); + + Fee const bigFee{env.current()->fees().base * 200}; + + auto const setLoan = [&](Number const& principal) -> Keylet { + auto const brokerSle = env.le(brokerKeylet); + auto const loanKeylet = keylet::loan( + brokerKeylet.key, SeqProxy::rawSequence(brokerSle->at(sfLoanSequence))); + env(loan::set(f.borrower, brokerKeylet.key, asset(principal).number()), + Sig(sfCounterpartySignature, f.lender), + jtx::loan::kInterestRate(TenthBips32{kFixtureInterestTenthBips}), + jtx::loan::kPaymentTotal(kFixturePaymentTotal), + jtx::loan::kPaymentInterval(kFixturePaymentInterval), + jtx::loan::kGracePeriod(kFixtureGracePeriod), + bigFee); + env.close(); + return loanKeylet; + }; + + // Loan 1: principal 7, the one ordinary loan in both fixtures. + // With vault deposit 1000, this leaves A ≈ 993 (see plan). + f.loan1Keylet = setLoan(Number{7}); + + if (!impairAndPaySibling) + return f; + + // Loan 2: sibling loan of principal 11. + f.loan2Keylet = setLoan(Number{11}); + + // Impair loan 1 → drives sfLossUnrealized to loan 1's value. + env(jtx::loan::manage(f.lender, f.loan1Keylet.key, tfLoanImpair), bigFee); + env.close(); + + // Pay off loan 2 in full so its total value flows into the vault + // and pushes T-A upward, meeting the residual loss. Generous + // upper bound; the transactor takes only what is due. + auto const payoff = asset(Number{50}).value(); + env(pay(f.borrower, f.loan2Keylet.key, payoff, tfLoanFullPayment), bigFee); + env.close(); + + return f; + } +}; + +} // namespace xrpl::test diff --git a/src/test/app/lending/VaultTransactorPrecision_test.cpp b/src/test/app/lending/VaultTransactorPrecision_test.cpp new file mode 100644 index 0000000000..36ec11e912 --- /dev/null +++ b/src/test/app/lending/VaultTransactorPrecision_test.cpp @@ -0,0 +1,779 @@ +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +#include +#include +#include +#include +#include + +namespace xrpl::test { + +// PR 2 tests: with fixCleanup3_4_0 enabled the transactor clamps make +// deposit / withdraw / clawback exact on the sfAssetsTotal grid. Every +// successful withdrawal and clawback satisfies STRICT equality of the +// assetsTotal / assetsAvailable / pseudo-account deltas -- no tolerance. +// Deposit is exact on the T side; A stays within one unit at the +// sfAssetsTotal grid. +class VaultTransactorPrecision_test : public VaultPrecisionFixture +{ + static Number + absDiff(Number const& a, Number const& b) + { + return a > b ? a - b : b - a; + } + + // ---- deposit ------------------------------------------------------ + + // A-1 magnitude sweep. Post-fix every successful deposit satisfies + // T_delta <= requested amount + // |T_delta - A_delta| <= oneUnit(asset, T_after) + // and the plan's three boundary amounts {1, 7, 10'000'000} now succeed. + // Pre-fix those three boundary amounts fail with tecINVARIANT_FAILED. + void + testDepositNeverOverCredited(FeatureBitset features) + { + using namespace jtx; + + bool const fixEnabled = features[fixCleanup3_4_0]; + testcase( + std::string("A-1 deposit never over-credited") + + (fixEnabled ? " (fixCleanup3_4_0)" : " (pre-fix)")); + + std::array const kAmounts{ + 1, + 2, + 5, + 7, + 10, + 50, + 100, + 500, + 1'000, + 5'000, + 10'000, + 50'000, + 100'000, + 500'000, + 1'000'000, + 5'000'000, + 10'000'000}; + + // Plan's three boundary amounts that Part 1 alone closes on A-1. + std::array const kPlanBoundaryAmounts{1, 7, 10'000'000}; + + for (auto const amount : kAmounts) + { + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + auto f = setupSingleLoanVault(env, /*impairAndPaySibling=*/false); + if (!BEAST_EXPECT(f.asset && f.broker)) + continue; + + auto const before = read(env, f); + + Vault const v{env}; + env(v.deposit( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(amount).value()}), + Ter(std::ignore)); + env.close(); + + TER const actual = env.ter(); + bool const isBoundary = + std::ranges::find(kPlanBoundaryAmounts, amount) != kPlanBoundaryAmounts.end(); + + if (fixEnabled) + { + if (isBoundary) + { + BEAST_EXPECTS( + actual == tesSUCCESS, + "plan boundary amount=" + std::to_string(amount) + + " expected tesSUCCESS, got " + transToken(actual)); + } + if (actual != tesSUCCESS) + continue; + + auto const after = read(env, f); + Number const tDelta = after.assetsTotal - before.assetsTotal; + Number const aDelta = after.assetsAvailable - before.assetsAvailable; + Number const requested = (*f.asset)(amount).number(); + + BEAST_EXPECTS( + tDelta <= requested, + "amount=" + std::to_string(amount) + " tDelta exceeds requested"); + Number const gap = absDiff(tDelta, aDelta); + BEAST_EXPECTS( + gap <= oneUnit(*f.asset, after.assetsTotal), + "amount=" + std::to_string(amount) + " |tDelta-aDelta| exceeds oneUnit"); + } + else if (isBoundary) + { + BEAST_EXPECTS( + actual == tecINVARIANT_FAILED, + "pre-fix amount=" + std::to_string(amount) + + " expected tecINVARIANT_FAILED, got " + transToken(actual)); + } + } + } + + // Post-fix: the depositor never gets shares worth more than the assets + // they paid. Any overpay is bounded by the pre-deposit per-share value. + void + testDepositorNeverUnderpays(FeatureBitset features) + { + using namespace jtx; + + bool const fixEnabled = features[fixCleanup3_4_0]; + if (!fixEnabled) + return; + + testcase("A-1 depositor never underpays (fixCleanup3_4_0)"); + + std::array const kAmounts{1, 7, 100, 1'000, 10'000, 100'000, 1'000'000, 10'000'000}; + + for (auto const amount : kAmounts) + { + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + auto f = setupSingleLoanVault(env, /*impairAndPaySibling=*/false); + if (!BEAST_EXPECT(f.asset && f.broker)) + continue; + + auto const before = read(env, f); + + Vault const v{env}; + env(v.deposit( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(amount).value()}), + Ter(std::ignore)); + env.close(); + + if (env.ter() != tesSUCCESS) + continue; + + auto const after = read(env, f); + Number const sharesMinted = after.sharesTotal - before.sharesTotal; + Number const assetsTaken = after.assetsTotal - before.assetsTotal; + if (before.sharesTotal == Number{0}) + continue; + Number const shareValue = (before.assetsTotal * sharesMinted) / before.sharesTotal; + + BEAST_EXPECTS( + shareValue <= assetsTaken, + "amount=" + std::to_string(amount) + " shareValue > assetsTaken"); + + Number const perShareOverpay = before.assetsTotal / before.sharesTotal; + Number const overpay = assetsTaken > shareValue ? assetsTaken - shareValue : Number{0}; + BEAST_EXPECTS( + overpay <= perShareOverpay, + "amount=" + std::to_string(amount) + " overpay exceeds per-share bound"); + } + } + + // Push T up to ~1e8, then attempt Number{1,-10} deposit. Post-fix: + // tecPRECISION_LOSS with the vault state unchanged. Confirms the + // deposit clamp cannot mint uncovered shares. + void + testZeroCreditRejected(FeatureBitset features) + { + using namespace jtx; + + bool const fixEnabled = features[fixCleanup3_4_0]; + if (!fixEnabled) + return; + + testcase("A-1 zero credit rejected (fixCleanup3_4_0)"); + + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + auto f = setupSingleLoanVault(env, /*impairAndPaySibling=*/false); + if (!BEAST_EXPECT(f.asset && f.broker)) + return; + + Vault const v{env}; + env(v.deposit( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(99'000'000).value()}), + Ter(std::ignore)); + env.close(); + + auto const before = read(env, f); + Number const kLowerBound{1, 6}; + BEAST_EXPECT(before.assetsTotal > kLowerBound); + + auto const tinyAmount = (*f.asset)(Number{1, -10}).value(); + env(v.deposit({.depositor = f.depositor, .id = f.vaultKeylet.key, .amount = tinyAmount}), + Ter(std::ignore)); + env.close(); + + BEAST_EXPECTS( + env.ter() == tecPRECISION_LOSS, + std::string{"expected tecPRECISION_LOSS, got "} + transToken(env.ter())); + + auto const after = read(env, f); + BEAST_EXPECT(after.assetsTotal == before.assetsTotal); + BEAST_EXPECT(after.assetsAvailable == before.assetsAvailable); + BEAST_EXPECT(after.sharesTotal == before.sharesTotal); + } + + // XRP and MPT (integral) vaults: the clamp is a no-op because + // STAmount(asset, N) already truncates to whole units. Every amount + // in the sweep succeeds pre- and post-amendment. + void + testIntegralAssetsUnchanged(FeatureBitset features) + { + using namespace jtx; + + bool const fixEnabled = features[fixCleanup3_4_0]; + testcase( + std::string("integral asset deposit sweep") + + (fixEnabled ? " (fixCleanup3_4_0)" : " (pre-fix)")); + + std::array const kAmounts{1, 7, 100, 1'000, 10'000, 100'000, 1'000'000, 10'000'000}; + + auto runXrp = [&]() { + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + Account const owner{"xrp_owner"}; + Account const depositor{"xrp_depositor"}; + env.fund(XRP(1'000'000'000), owner, depositor); + env.close(); + + Vault const v{env}; + auto [createTx, vaultKeylet] = v.create({.owner = owner, .asset = xrpIssue()}); + env(createTx); + env.close(); + + env(v.deposit( + {.depositor = owner, .id = vaultKeylet.key, .amount = XRP(1'000).value()})); + env.close(); + + for (auto const amount : kAmounts) + { + env(v.deposit( + {.depositor = depositor, + .id = vaultKeylet.key, + .amount = XRP(amount).value()}), + Ter(std::ignore)); + env.close(); + BEAST_EXPECTS( + env.ter() == tesSUCCESS, + "XRP amount=" + std::to_string(amount) + " expected tesSUCCESS, got " + + transToken(env.ter())); + } + }; + + auto runMpt = [&]() { + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + Account const issuer{"mpt_issuer"}; + Account const owner{"mpt_owner"}; + Account const depositor{"mpt_depositor"}; + env.fund(XRP(1'000'000), issuer, owner, depositor); + env.close(); + + MPTTester mptt{env, issuer, kMptInitNoFund}; + mptt.create({.flags = tfMPTCanTransfer}); + PrettyAsset const asset = mptt.issuanceID(); + mptt.authorize({.account = owner}); + mptt.authorize({.account = depositor}); + env(pay(issuer, depositor, asset(1'000'000'000))); + env.close(); + + Vault const v{env}; + auto [createTx, vaultKeylet] = v.create({.owner = owner, .asset = asset}); + env(createTx); + env.close(); + + env(pay(issuer, owner, asset(1'000))); + env.close(); + env(v.deposit( + {.depositor = owner, .id = vaultKeylet.key, .amount = asset(1'000).value()})); + env.close(); + + for (auto const amount : kAmounts) + { + env(v.deposit( + {.depositor = depositor, + .id = vaultKeylet.key, + .amount = asset(amount).value()}), + Ter(std::ignore)); + env.close(); + BEAST_EXPECTS( + env.ter() == tesSUCCESS, + "MPT amount=" + std::to_string(amount) + " expected tesSUCCESS, got " + + transToken(env.ter())); + } + }; + + runXrp(); + runMpt(); + } + + // ---- withdraw ----------------------------------------------------- + + // A-1 fixture withdrawals in both asset-fixed and share-fixed modes. + // Post-fix: never tecINVARIANT_FAILED and every successful withdrawal + // satisfies STRICT equality + // beforeT - afterT == beforeA - afterA == beforePseudo - afterPseudo + // in Number space. This is the strong claim of Part 1b of the plan. + void + testWithdrawDeltas(FeatureBitset features) + { + using namespace jtx; + + bool const fixEnabled = features[fixCleanup3_4_0]; + testcase( + std::string("A-1 withdraw delta exactness") + + (fixEnabled ? " (fixCleanup3_4_0)" : " (pre-fix)")); + + std::array const kShareCounts{ + 99'999u, 100'001u, 333'333u, 1'234'567u, 142'857'142u, 333'333'333u}; + + std::array const kAssetAmounts{1, 7, 99, 333, 993}; + + auto runOnce = [&](bool useShares) { + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + auto f = setupSingleLoanVault(env, /*impairAndPaySibling=*/false); + if (!BEAST_EXPECT(f.asset && f.broker)) + return; + + Vault const v{env}; + env(v.deposit( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(1'000'000).value()}), + Ter(std::ignore)); + env.close(); + + auto step = [&](STAmount const& amount, std::string const& tag) { + auto const before = read(env, f); + env(v.withdraw( + {.depositor = f.depositor, .id = f.vaultKeylet.key, .amount = amount}), + Ter(std::ignore)); + env.close(); + + TER const actual = env.ter(); + if (fixEnabled) + { + BEAST_EXPECTS( + actual != tecINVARIANT_FAILED, tag + " unexpected invariant failure"); + if (actual == tesSUCCESS) + { + auto const after = read(env, f); + Number const tDelta = before.assetsTotal - after.assetsTotal; + Number const aDelta = before.assetsAvailable - after.assetsAvailable; + Number const pDelta = before.pseudo - after.pseudo; + BEAST_EXPECTS(tDelta == aDelta, tag + " tDelta != aDelta"); + BEAST_EXPECTS(tDelta == pDelta, tag + " tDelta != pDelta"); + } + } + }; + + if (useShares) + { + for (auto const count : kShareCounts) + { + auto const before = read(env, f); + if (before.sharesTotal < count) + continue; + STAmount const shareAmount{ + MPTIssue{f.share}, Number{static_cast(count)}}; + step(shareAmount, "shares=" + std::to_string(count)); + } + } + else + { + for (auto const amount : kAssetAmounts) + { + step((*f.asset)(amount).value(), "assets=" + std::to_string(amount)); + } + } + }; + + runOnce(/*useShares=*/true); + runOnce(/*useShares=*/false); + } + + // Post-fix: withdrawer never receives more than the burned share value; + // any shortfall is bounded by one unit at the sfAssetsTotal scale. + void + testWithdrawNeverOverpays(FeatureBitset features) + { + using namespace jtx; + + bool const fixEnabled = features[fixCleanup3_4_0]; + if (!fixEnabled) + return; + + testcase("A-1 withdraw never overpays (fixCleanup3_4_0)"); + + std::array const kShareCounts{ + 99'999u, 100'001u, 333'333u, 1'234'567u, 142'857'142u}; + + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + auto f = setupSingleLoanVault(env, /*impairAndPaySibling=*/false); + if (!BEAST_EXPECT(f.asset && f.broker)) + return; + + Vault const v{env}; + env(v.deposit( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(1'000'000).value()}), + Ter(std::ignore)); + env.close(); + + for (auto const count : kShareCounts) + { + auto const before = read(env, f); + if (before.sharesTotal < count) + continue; + STAmount const shareAmount{MPTIssue{f.share}, Number{static_cast(count)}}; + env(v.withdraw( + {.depositor = f.depositor, .id = f.vaultKeylet.key, .amount = shareAmount}), + Ter(std::ignore)); + env.close(); + if (env.ter() != tesSUCCESS) + continue; + + auto const after = read(env, f); + Number const sharesBurned = before.sharesTotal - after.sharesTotal; + if (before.sharesTotal == Number{0}) + continue; + Number const shareValue = (before.assetsTotal * sharesBurned) / before.sharesTotal; + Number const payout = before.assetsTotal - after.assetsTotal; + + BEAST_EXPECTS( + payout <= shareValue, "shares=" + std::to_string(count) + " payout > shareValue"); + Number const shortfall = shareValue > payout ? shareValue - payout : Number{0}; + BEAST_EXPECTS( + shortfall <= oneUnit(*f.asset, after.assetsTotal), + "shares=" + std::to_string(count) + " shortfall exceeds oneUnit"); + } + } + + // Sub-ULP withdrawal from a ~1e8 vault; post-fix must return + // tecPRECISION_LOSS after the Upward clamp rounds the amount to zero. + void + testWithdrawSubUlpRejected(FeatureBitset features) + { + using namespace jtx; + + bool const fixEnabled = features[fixCleanup3_4_0]; + if (!fixEnabled) + return; + + testcase("sub-ULP withdraw rejected (fixCleanup3_4_0)"); + + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + auto f = setupSingleLoanVault(env, /*impairAndPaySibling=*/false); + if (!BEAST_EXPECT(f.asset && f.broker)) + return; + + Vault const v{env}; + env(v.deposit( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(99'000'000).value()}), + Ter(std::ignore)); + env.close(); + + auto const before = read(env, f); + Number const kLowerBound{1, 6}; + BEAST_EXPECT(before.assetsTotal > kLowerBound); + + auto const tinyAmount = (*f.asset)(Number{1, -10}).value(); + env(v.withdraw({.depositor = f.depositor, .id = f.vaultKeylet.key, .amount = tinyAmount}), + Ter(std::ignore)); + env.close(); + + BEAST_EXPECTS( + env.ter() == tecPRECISION_LOSS, + std::string{"expected tecPRECISION_LOSS, got "} + transToken(env.ter())); + } + + // Depositor burns every share they hold, exercising the final- + // withdrawal branch of VaultWithdraw (line 336-368 -- deliberately + // untouched by this amendment). Must return tesSUCCESS: the clamp + // does not spuriously reject a legitimate full-share withdrawal. + // + // The plan describes reaching a "dust-only gap state" via the A-3 + // fixture; empirically the A-3 fixture retains a non-zero + // sfLossUnrealized which blocks the final-withdrawal branch through + // the insufficient-funds guard. Verifying the branch from a clean + // A-1 state still exercises the amendment's non-interference claim. + void + testFinalWithdrawalDust(FeatureBitset features) + { + using namespace jtx; + + bool const fixEnabled = features[fixCleanup3_4_0]; + if (!fixEnabled) + return; + + testcase("A-1 final withdrawal (fixCleanup3_4_0)"); + + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + auto f = setupSingleLoanVault(env, /*impairAndPaySibling=*/false); + if (!BEAST_EXPECT(f.asset && f.broker)) + return; + + Vault const v{env}; + env(v.deposit( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(500).value()}), + Ter(std::ignore)); + env.close(); + + auto const depositorMptSle = env.le(keylet::mptoken(f.share, f.depositor.id())); + if (!BEAST_EXPECT(depositorMptSle != nullptr)) + return; + + auto const held = depositorMptSle->at(sfMPTAmount); + if (held == 0) + return; + + STAmount const shareAmount{MPTIssue{f.share}, Number{static_cast(held)}}; + env(v.withdraw({.depositor = f.depositor, .id = f.vaultKeylet.key, .amount = shareAmount}), + Ter(std::ignore)); + env.close(); + + TER const actual = env.ter(); + BEAST_EXPECTS( + actual == tesSUCCESS, std::string{"expected tesSUCCESS, got "} + transToken(actual)); + } + + // ---- deposit + withdraw under impairment (A-3) -------------------- + + // A-3 fixture drives sfLossUnrealized to the boundary + // lossUnrealized == assetsTotal - assetsAvailable + // A companion invariant-fix branch found that independent per-field + // rounding of assetsTotal/assetsAvailable can spuriously trip the + // XRPL_ASSERT (and, pre-fix, tecINVARIANT_FAILED) that compares + // lossUnrealized against assetsTotal - assetsAvailable in + // VaultWithdraw::doApply. This branch's clamp keeps assetsTotal and + // assetsAvailable exact on the sfAssetsTotal grid, which should keep + // that comparison stable even while churning the vault through this + // boundary state. Only meaningful post-fix -- the pre-fix boundary + // behaviour at A-3 is out of scope for this branch (it is the subject + // of the companion invariant-fix branch, not this one) -- so mirror + // testWithdrawNeverOverpays and early-return before the amendment. + void + testDepositWithdrawUnderImpairment(FeatureBitset features) + { + using namespace jtx; + + bool const fixEnabled = features[fixCleanup3_4_0]; + if (!fixEnabled) + return; + + testcase("A-3 deposit/withdraw under impairment boundary (fixCleanup3_4_0)"); + + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + auto f = setupSingleLoanVault(env, /*impairAndPaySibling=*/true); + if (!BEAST_EXPECT(f.asset && f.broker)) + return; + + Vault const v{env}; + + // Seed the depositor with an initial stake so later withdrawals + // have shares/assets to draw against. + env(v.deposit( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(5'000).value()}), + Ter(std::ignore)); + env.close(); + + std::array const kAmounts{1, 3, 7, 13, 29, 51, 97, 137, 251, 499, 991, 1'999}; + + auto checkInvariant = [&](std::string const& tag) { + TER const actual = env.ter(); + BEAST_EXPECTS(actual != tecINVARIANT_FAILED, tag + " unexpected invariant failure"); + if (actual == tesSUCCESS) + { + auto const after = read(env, f); + BEAST_EXPECTS( + after.lossUnrealized <= after.assetsTotal - after.assetsAvailable, + tag + " lossUnrealized exceeds assetsTotal - assetsAvailable"); + } + }; + + // Alternate deposit then withdraw of a different amount so the + // vault's assetsTotal / assetsAvailable / lossUnrealized state + // churns through several transactions at the A-3 boundary. + for (std::size_t i = 0; i + 1 < kAmounts.size(); i += 2) + { + int const depositAmount = kAmounts[i]; + int const withdrawAmount = kAmounts[i + 1]; + + env(v.deposit( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(depositAmount).value()}), + Ter(std::ignore)); + env.close(); + checkInvariant("deposit=" + std::to_string(depositAmount)); + + env(v.withdraw( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(withdrawAmount).value()}), + Ter(std::ignore)); + env.close(); + checkInvariant("withdraw=" + std::to_string(withdrawAmount)); + } + } + + // ---- clawback ----------------------------------------------------- + + // A-1 fixture with clawback enabled. Sweep amounts including + // sfAmount-absent (claw back everything) and amount-exceeds-available. + // Post-fix: never tecINVARIANT_FAILED and each success satisfies + // T_delta == A_delta == pseudo_delta in Number space. + // Owner force-burn against a vault with a live loan: must return + // tecNO_PERMISSION regardless of amendment (never enters + // assetsToClawback so the clamp is unreachable there). + void + testClawbackDeltas(FeatureBitset features) + { + using namespace jtx; + + bool const fixEnabled = features[fixCleanup3_4_0]; + testcase( + std::string("A-1 clawback delta exactness") + + (fixEnabled ? " (fixCleanup3_4_0)" : " (pre-fix)")); + + std::array const kAmounts{1, 7, 99, 333, 993, 5'000'000}; + + Env env{*this, envconfig(), features, nullptr, beast::Severity::Disabled}; + auto f = setupSingleLoanVault( + env, + /*impairAndPaySibling=*/false, + /*allowClawback=*/true); + if (!BEAST_EXPECT(f.asset && f.broker)) + return; + + Vault const v{env}; + env(v.deposit( + {.depositor = f.depositor, + .id = f.vaultKeylet.key, + .amount = (*f.asset)(2'000).value()}), + Ter(std::ignore)); + env.close(); + + auto stepAmount = [&](int amount) { + auto const before = read(env, f); + if (before.sharesTotal == Number{0}) + return; + + env(v.clawback( + {.issuer = f.issuer, + .id = f.vaultKeylet.key, + .holder = f.depositor, + .amount = (*f.asset)(amount).value()}), + Ter(std::ignore)); + env.close(); + + TER const actual = env.ter(); + if (fixEnabled) + { + BEAST_EXPECTS( + actual != tecINVARIANT_FAILED, + "amount=" + std::to_string(amount) + " unexpected invariant failure"); + if (actual == tesSUCCESS) + { + auto const after = read(env, f); + Number const tDelta = before.assetsTotal - after.assetsTotal; + Number const aDelta = before.assetsAvailable - after.assetsAvailable; + Number const pDelta = before.pseudo - after.pseudo; + BEAST_EXPECTS( + tDelta == aDelta, "amount=" + std::to_string(amount) + " tDelta != aDelta"); + BEAST_EXPECTS( + tDelta == pDelta, "amount=" + std::to_string(amount) + " tDelta != pDelta"); + } + } + }; + + for (auto const amount : kAmounts) + stepAmount(amount); + + { + auto const before = read(env, f); + if (before.sharesTotal > Number{0}) + { + env(v.clawback( + {.issuer = f.issuer, .id = f.vaultKeylet.key, .holder = f.depositor}), + Ter(std::ignore)); + env.close(); + + TER const actual = env.ter(); + if (fixEnabled) + { + BEAST_EXPECTS( + actual != tecINVARIANT_FAILED, + "sfAmount-absent unexpected invariant failure"); + if (actual == tesSUCCESS) + { + auto const after = read(env, f); + Number const tDelta = before.assetsTotal - after.assetsTotal; + Number const aDelta = before.assetsAvailable - after.assetsAvailable; + Number const pDelta = before.pseudo - after.pseudo; + BEAST_EXPECT(tDelta == aDelta); + BEAST_EXPECT(tDelta == pDelta); + } + } + } + } + + env(v.clawback({.issuer = f.lender, .id = f.vaultKeylet.key, .holder = f.depositor}), + Ter(tecNO_PERMISSION)); + env.close(); + } + +public: + void + run() override + { + for (auto const& features : {all_ - fixCleanup3_4_0, all_}) + { + testDepositNeverOverCredited(features); + testDepositorNeverUnderpays(features); + testZeroCreditRejected(features); + testIntegralAssetsUnchanged(features); + testWithdrawDeltas(features); + testWithdrawNeverOverpays(features); + testWithdrawSubUlpRejected(features); + testFinalWithdrawalDust(features); + testDepositWithdrawUnderImpairment(features); + testClawbackDeltas(features); + } + } +}; + +BEAST_DEFINE_TESTSUITE(VaultTransactorPrecision, tx, xrpl); + +} // namespace xrpl::test diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index a7071f3767..03840667ac 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -411,7 +411,11 @@ private: testcase( "bug: VaultDeposit below Vault precision canonicalized to zero " "(pre-fixCleanup3_2_0)"); - runScenario(testableAmendments() - fixCleanup3_2_0, tecINVARIANT_FAILED); + // Also remove fixCleanup3_4_0 so the VaultDeposit clamp + // introduced by that amendment does not short-circuit this + // pre-fixCleanup3_2_0 scenario with tecPRECISION_LOSS. + runScenario( + testableAmendments() - fixCleanup3_2_0 - fixCleanup3_4_0, tecINVARIANT_FAILED); } { testcase(