diff --git a/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp b/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp index a3c0a94eb5..5ee948bbba 100644 --- a/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultDeposit.cpp @@ -2,12 +2,15 @@ #include #include +#include #include #include +#include #include #include #include #include +#include #include #include #include @@ -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&) diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index 2dbd20f855..70a350a4f1 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -2,9 +2,12 @@ #include #include #include +#include #include +#include #include #include +#include #include #include #include @@ -15,13 +18,17 @@ #include #include #include +#include #include #include +#include #include #include +#include #include #include +#include #include #include #include @@ -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(&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();