diff --git a/include/xrpl/ledger/helpers/VaultHelpers.h b/include/xrpl/ledger/helpers/VaultHelpers.h index 5681cc57e86..b059ef53c89 100644 --- a/include/xrpl/ledger/helpers/VaultHelpers.h +++ b/include/xrpl/ledger/helpers/VaultHelpers.h @@ -1,7 +1,9 @@ #pragma once +#include #include #include +#include #include #include #include @@ -52,6 +54,37 @@ enum class TruncateShares : bool { No = false, Yes = true }; */ enum class WaiveUnrealizedLoss : bool { No = false, Yes = true }; +/** + * Returns the effective total of assets backing outstanding shares, i.e. + * sfAssetsTotal, discounted by sfLossUnrealized unless waived. This is the + * numerator used by both withdraw conversion helpers (assetsToSharesWithdraw + * and sharesToAssetsWithdraw) to compute the share/asset exchange rate. + * + * @param vault The vault SLE. + * @param waive Whether to waive (i.e. not subtract) the vault's unrealized + * loss. + */ +[[nodiscard]] Number +effectiveAssetsTotalWithdraw(SLE::const_ref vault, WaiveUnrealizedLoss waive); + +/** + * Returns whether debiting `amount` from `total` — the current value of a + * vault's sfAssetsTotal or sfAssetsAvailable field — would canonicalize back + * to the exact same STAmount value it started at. This happens when a + * genuinely non-zero debit is dust relative to a `total` large enough to + * exceed STAmount's significant-digit precision: the shares still move, but + * the stored total doesn't change, which otherwise trips the ValidVault + * invariant after the fact instead of failing cleanly upfront. + * + * @param asset The vault's underlying asset, used to canonicalize both sides + * the same way the ledger will when the field is stored. + * @param total The field's current value. + * @param amount The amount to debit. A value of zero always returns false; + * that case is rejected separately and unconditionally. + */ +[[nodiscard]] bool +debitRoundsToNoOp(Asset const& asset, Number const& total, Number const& amount); + /** * From the perspective of a vault, return the number of shares to demand from * the depositor when they ask to withdraw a fixed amount of assets. Since diff --git a/src/libxrpl/ledger/helpers/VaultHelpers.cpp b/src/libxrpl/ledger/helpers/VaultHelpers.cpp index 78f64d20774..9f2bd04ad82 100644 --- a/src/libxrpl/ledger/helpers/VaultHelpers.cpp +++ b/src/libxrpl/ledger/helpers/VaultHelpers.cpp @@ -65,6 +65,23 @@ sharesToAssetsDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount co return assets; } +[[nodiscard]] Number +effectiveAssetsTotalWithdraw(SLE::const_ref vault, WaiveUnrealizedLoss waive) +{ + Number assetTotal = vault->at(sfAssetsTotal); + if (waive == WaiveUnrealizedLoss::No) + assetTotal -= vault->at(sfLossUnrealized); + return assetTotal; +} + +[[nodiscard]] bool +debitRoundsToNoOp(Asset const& asset, Number const& total, Number const& amount) +{ + if (amount == 0) + return false; + return STAmount{asset, total - amount} == STAmount{asset, total}; +} + [[nodiscard]] std::optional assetsToSharesWithdraw( SLE::const_ref vault, @@ -80,9 +97,7 @@ assetsToSharesWithdraw( if (assets.negative() || assets.asset() != vault->at(sfAsset)) return std::nullopt; // LCOV_EXCL_LINE - Number assetTotal = vault->at(sfAssetsTotal); - if (waive == WaiveUnrealizedLoss::No) - assetTotal -= vault->at(sfLossUnrealized); + Number const assetTotal = effectiveAssetsTotalWithdraw(vault, waive); STAmount shares{vault->at(sfShareMPTID)}; if (assetTotal == 0) return shares; @@ -108,9 +123,7 @@ sharesToAssetsWithdraw( if (shares.negative() || shares.asset() != vault->at(sfShareMPTID)) return std::nullopt; // LCOV_EXCL_LINE - Number assetTotal = vault->at(sfAssetsTotal); - if (waive == WaiveUnrealizedLoss::No) - assetTotal -= vault->at(sfLossUnrealized); + Number const assetTotal = effectiveAssetsTotalWithdraw(vault, waive); STAmount assets{vault->at(sfAsset)}; if (assetTotal == 0) return assets; diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index eca50eb8093..946cffde606 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -805,19 +805,36 @@ ValidVault::finalize( auto const& beforeVault = beforeVault_[0]; auto const maybeVaultDeltaAssets = deltaAssets(afterVault.pseudoId); - if (!maybeVaultDeltaAssets) + + // Post-fixCleanup3_4_0: a withdrawal that redeems shares from a + // pool with no effective value left to back them (e.g. fully + // impaired/insolvent) legitimately moves zero assets on both + // sides — VaultWithdraw::doApply does not touch either + // balance-holding entry for a zero-value transfer, so no delta + // is recorded. VaultWithdraw::doApply separately rejects + // (tecPRECISION_LOSS) the case where a *positive* per-share + // value merely rounds down to zero, so a missing delta while + // the pool still held positive effective value indicates a + // real accounting bug, not this exception. + bool const zeroDeltaIsLegitimate = view.rules().enabled(fixCleanup3_4_0) && + !maybeVaultDeltaAssets && beforeVault.assetsTotal <= beforeVault.lossUnrealized; + + if (!maybeVaultDeltaAssets && !zeroDeltaIsLegitimate) { JLOG(j.fatal()) << "Invariant failed: withdrawal must change vault balance"; return false; // That's all we can do } + DeltaInfo const vaultDeltaAssets = maybeVaultDeltaAssets.value_or( + DeltaInfo{.delta = kNumZero, .scale = std::nullopt}); + // Get the posterior scale to round calculations to - auto const minScale = computeVaultMinScale(*maybeVaultDeltaAssets, view.rules()); + auto const minScale = computeVaultMinScale(vaultDeltaAssets, view.rules()); auto const vaultPseudoDeltaAssets = - roundToAsset(vaultAsset, maybeVaultDeltaAssets->delta, minScale); + roundToAsset(vaultAsset, vaultDeltaAssets.delta, minScale); - if (vaultPseudoDeltaAssets >= kZero) + if (!zeroDeltaIsLegitimate && vaultPseudoDeltaAssets >= kZero) { JLOG(j.fatal()) << "Invariant failed: withdrawal must decrease vault balance"; result = false; @@ -844,63 +861,76 @@ ValidVault::finalize( if (maybeAccDelta.has_value() == maybeOtherAccDelta.has_value()) { - JLOG(j.fatal()) << // - "Invariant failed: withdrawal must change one destination balance"; - return false; + // Both changed is always a bug. Neither changed is + // consistent only with a legitimate zero-value + // withdrawal, which moves nothing on either side — + // there is nothing left to cross-check. + if (!zeroDeltaIsLegitimate || maybeAccDelta.has_value()) + { + JLOG(j.fatal()) << // + "Invariant failed: withdrawal must change one destination balance"; + return false; + } } - - auto const destinationDelta = // - maybeAccDelta ? *maybeAccDelta : *maybeOtherAccDelta; - - // the scale of destinationDelta can be coarser than - // minScale, so we take that into account when rounding - auto const destinationScale = computeCoarsestScale({destinationDelta}); - auto const localMinScale = std::max(minScale, destinationScale); - - auto const roundedDestinationDelta = - roundToAsset(vaultAsset, destinationDelta.delta, localMinScale); - - // Post-fixCleanup3_2_0: Tolerate zero-rounded destination deltas for IOUs only. - // If the receiver's trust line sits at a coarser scale, the inflow may - // safely round down to zero. - // - // XRP and MPT remain strict. Because they are integer-exact, a zero - // destination delta indicates a true accounting bug, not a rounding artifact. - bool const tolerateZeroDelta = - view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral(); - auto const invalidBalanceChange = tolerateZeroDelta - ? roundedDestinationDelta < kZero - : roundedDestinationDelta <= kZero; - if (invalidBalanceChange) + else { - JLOG(j.fatal()) << // - "Invariant failed: withdrawal must increase destination balance"; - result = false; - } - - auto const localPseudoDeltaAssets = - roundToAsset(vaultAsset, vaultPseudoDeltaAssets, localMinScale); - // For IOU assets near a precision boundary the destination's STAmount - // exponent can shift, making part of the sent value unrepresentable at the - // receiver's new scale — that portion is irreversibly absorbed by the IOU - // rail. Tolerate the mismatch only when the destroyed amount (vault outflow - // minus destination inflow, in Number space) is itself sub-ULP at the - // destination's scale. Floor rounding is used so that values exactly at the - // step boundary are not mistakenly dismissed. Any representable discrepancy - // indicates a real accounting bug and must be caught. - auto const destroyedIsSubUlp = tolerateZeroDelta && - roundToAsset( - vaultAsset, - maybeVaultDeltaAssets->delta * -1 - destinationDelta.delta, - destinationScale, - Number::RoundingMode::Downward) == kZero; - if (!destroyedIsSubUlp && - localPseudoDeltaAssets * -1 != roundedDestinationDelta) - { - JLOG(j.fatal()) << "Invariant failed: " << // - "withdrawal must change vault and destination balance by equal " - "amount"; - result = false; + // A one-sided change is cross-checked even for a + // legitimate zero vault delta: the destination must + // then have moved by (rounded) zero as well. + auto const destinationDelta = // + maybeAccDelta ? *maybeAccDelta : *maybeOtherAccDelta; + + // the scale of destinationDelta can be coarser than + // minScale, so we take that into account when rounding + auto const destinationScale = computeCoarsestScale({destinationDelta}); + auto const localMinScale = std::max(minScale, destinationScale); + + auto const roundedDestinationDelta = + roundToAsset(vaultAsset, destinationDelta.delta, localMinScale); + + // Post-fixCleanup3_2_0: Tolerate zero-rounded destination deltas for IOUs + // only. If the receiver's trust line sits at a coarser scale, the inflow + // may safely round down to zero. + // + // XRP and MPT remain strict. Because they are integer-exact, a zero + // destination delta indicates a true accounting bug, not a rounding + // artifact. + bool const tolerateZeroDelta = + view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral(); + auto const invalidBalanceChange = tolerateZeroDelta + ? roundedDestinationDelta < kZero + : roundedDestinationDelta <= kZero; + if (invalidBalanceChange) + { + JLOG(j.fatal()) << // + "Invariant failed: withdrawal must increase destination balance"; + result = false; + } + + auto const localPseudoDeltaAssets = + roundToAsset(vaultAsset, vaultPseudoDeltaAssets, localMinScale); + // For IOU assets near a precision boundary the destination's STAmount + // exponent can shift, making part of the sent value unrepresentable at + // the receiver's new scale — that portion is irreversibly absorbed by the + // IOU rail. Tolerate the mismatch only when the destroyed amount (vault + // outflow minus destination inflow, in Number space) is itself sub-ULP at + // the destination's scale. Floor rounding is used so that values exactly + // at the step boundary are not mistakenly dismissed. Any representable + // discrepancy indicates a real accounting bug and must be caught. + auto const destroyedIsSubUlp = tolerateZeroDelta && + roundToAsset( + vaultAsset, + vaultDeltaAssets.delta * -1 - destinationDelta.delta, + destinationScale, + Number::RoundingMode::Downward) == kZero; + if (!destroyedIsSubUlp && + localPseudoDeltaAssets * -1 != roundedDestinationDelta) + { + JLOG(j.fatal()) << "Invariant failed: " << // + "withdrawal must change vault and destination balance by equal " + "amount"; + result = false; + } } } diff --git a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp index d77286b667b..1a0626c74e4 100644 --- a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp @@ -383,6 +383,20 @@ VaultClawback::doApply() if (sharesDestroyed == beast::kZero) return tecPRECISION_LOSS; + // A recovered amount can be genuinely non-zero yet still be dust relative to a + // sfAssetsTotal/sfAssetsAvailable large enough to exceed STAmount's significant-digit + // precision: subtracting it below rounds the stored total right back to where it started. + // The shares still move, so ValidVault would fail after the fact with "clawback must + // decrease vault balance" instead of a clean upfront rejection. + if (view().rules().enabled(fixCleanup3_4_0) && + (debitRoundsToNoOp(vaultAsset, assetsTotal, assetsRecovered) || + debitRoundsToNoOp(vaultAsset, assetsAvailable, assetsRecovered))) + { + JLOG(j_.debug()) << "VaultClawback: clawback amount too small to change stored vault" + " balance"; + return tecPRECISION_LOSS; + } + assetsTotal -= assetsRecovered; assetsAvailable -= assetsRecovered; view().update(vault); diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index 353b72c30d1..63e1c6763f2 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -273,6 +273,44 @@ VaultWithdraw::doApply() return tecPATH_DRY; } + // The "final withdrawal" rule below handles its own zero-value case using + // sfAssetsAvailable directly, so it is exempt from the checks below. + bool const isFinalWithdrawal = + sharesRedeemed == STAmount{share, sleIssuance->at(sfOutstandingAmount)}; + + auto assetsAvailable = vault->at(sfAssetsAvailable); + auto assetsTotal = vault->at(sfAssetsTotal); + auto const lossUnrealized = vault->at(sfLossUnrealized); + XRPL_ASSERT( + lossUnrealized <= (assetsTotal - assetsAvailable), + "xrpl::VaultWithdraw::doApply : loss and assets do balance"); + + if (view().rules().enabled(fixCleanup3_4_0) && !isFinalWithdrawal) + { + // A withdrawal for a fixed share amount (variable assets) has no requested-asset + // amount to check for rounding, unlike the fixed-assets branch above: a small enough + // share amount can round down to an exact zero even though the vault still holds + // positive effective value backing outstanding shares. + if (amount.asset() == share && assetsWithdrawn == beast::kZero && + effectiveAssetsTotalWithdraw(vault, waiveUnrealizedLoss) != beast::kZero) + { + JLOG(j_.debug()) << "VaultWithdraw: fixed-share withdrawal rounds to zero assets"; + return tecPRECISION_LOSS; + } + + // assetsWithdrawn can also be genuinely non-zero and still too small to move + // sfAssetsTotal or sfAssetsAvailable once canonicalized to STAmount's precision. Either + // way the shares still move, so ValidVault would otherwise fail after the fact instead + // of a clean upfront rejection. + if (debitRoundsToNoOp(vaultAsset, assetsTotal, assetsWithdrawn) || + debitRoundsToNoOp(vaultAsset, assetsAvailable, assetsWithdrawn)) + { + JLOG(j_.debug()) << "VaultWithdraw: withdrawal amount too small to change stored" + " vault balance"; + return tecPRECISION_LOSS; + } + } + // Post-fixCleanup3_3_0: preclaim already validated all freeze conditions // (checkWithdrawFreeze), so IgnoreFreeze avoids a redundant check that // would incorrectly return zero for vault pseudo-accounts whose shares @@ -287,13 +325,6 @@ VaultWithdraw::doApply() return tecINSUFFICIENT_FUNDS; } - auto assetsAvailable = vault->at(sfAssetsAvailable); - auto assetsTotal = vault->at(sfAssetsTotal); - auto const lossUnrealized = vault->at(sfLossUnrealized); - XRPL_ASSERT( - lossUnrealized <= (assetsTotal - assetsAvailable), - "xrpl::VaultWithdraw::doApply : loss and assets do balance"); - // The vault must have enough assets on hand. if (*assetsAvailable < assetsWithdrawn) { @@ -309,8 +340,6 @@ VaultWithdraw::doApply() // When the rule applies, the payout is the remaining sfAssetsAvailable; in a clean vault // the helper result should already equal that value, and any mismatch is a rounding artifact // worth logging. - bool const isFinalWithdrawal = - sharesRedeemed == STAmount{share, sleIssuance->at(sfOutstandingAmount)}; if (view().rules().enabled(fixCleanup3_2_0) && isFinalWithdrawal) { // Unreachable: a final withdrawal with lossUnrealized > 0 has diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 791cf216c3a..a5269cdf4dd 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -6947,6 +6947,102 @@ class Vault_test : public beast::unit_test::Suite } } + // Bug: a debit can be genuinely non-zero yet still be dust relative to a + // sfAssetsTotal/sfAssetsAvailable large enough to exceed STAmount's precision, e.g. + // AssetsTotal 2e12 minus a 1e-6 debit needs 19 significant digits and rounds straight + // back to 2e12. The shares still move, so ValidVault later fails with "must decrease + // vault balance" instead of a clean upfront rejection. + // + // Fix (fixCleanup3_4_0): reject upfront with tecPRECISION_LOSS if the debit would + // canonicalize back to the prior stored value. + // + // With a single depositor AssetsTotal == AssetsAvailable, so both + // debitRoundsToNoOp operands trip together here. LoanRounding_test's + // "dust debit vs AssetsTotal only" case isolates the AssetsTotal operand + // via a heavily-loaned vault. + void + testBugVaultDustDebitCanonicalizesToNoOp() + { + using namespace test::jtx; + + // Fund a single depositor and have them deposit `total` USD in one shot (default + // scale 6, so shares mint at exactly total*1e6). + auto const seedVault = [](Env& env, Number const& total) { + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Account const holder{"holder"}; + + env.fund(XRP(1'000'000), issuer, owner, holder); + env.close(); + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + env(trust(holder, usd(100'000'000'000'000LL))); + env.close(); + env(pay(issuer, holder, usd(total))); + env.close(); + + Vault const vault{env}; + auto const [tx, keylet] = vault.create({.owner = owner, .asset = usd.raw()}); + env(tx); + env.close(); + env(vault.deposit({.depositor = holder, .id = keylet.key, .amount = usd(total)}), + Ter(tesSUCCESS)); + env.close(); + + return keylet; + }; + + { + auto runScenario = [&](FeatureBitset features, TER expected) { + Env env(*this, features); + Number const total{2, 12}; + auto const keylet = seedVault(env, total); + + Account const issuer{"issuer"}; + PrettyAsset const usd{issuer["USD"]}; + + // 1 share's worth of assets: 1e-6, below AssetsTotal's storage precision. + env(Vault::clawback( + {.issuer = issuer, + .id = keylet.key, + .holder = Account{"holder"}, + .amount = usd(Number{1, -6}).value()}), + Ter(expected)); + env.close(); + }; + + testcase("bug: VaultClawback dust debit fires invariant (pre-fixCleanup3_4_0)"); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); + testcase("bug: VaultClawback dust debit rejected cleanly (post-fixCleanup3_4_0)"); + runScenario(all_, tecPRECISION_LOSS); + } + + { + auto runScenario = [&](FeatureBitset features, TER expected) { + Env env(*this, features); + Number const total{2, 12}; + auto const keylet = seedVault(env, total); + + MPTIssue const share{env.le(keylet)->at(sfShareMPTID)}; + + // Redeem 1 share, worth 1e-6 assets, below AssetsTotal's storage precision. + env(Vault::withdraw( + {.depositor = Account{"holder"}, + .id = keylet.key, + .amount = STAmount{share, 1}}), + Ter(expected)); + env.close(); + }; + + testcase("bug: VaultWithdraw dust debit fires invariant (pre-fixCleanup3_4_0)"); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); + testcase("bug: VaultWithdraw dust debit rejected cleanly (post-fixCleanup3_4_0)"); + runScenario(all_, tecPRECISION_LOSS); + } + } + // Bug: when a depositor's IOU trustline balance is very large (e.g. // ~1e17), adding a small deposit (e.g. 1 USD) leaves sfAssetsTotal // unchanged at IOU precision because the increment is sub-ULP at the @@ -8375,6 +8471,7 @@ class Vault_test : public beast::unit_test::Suite testBugMakeDeltaAnteriorScale(); testVaultDepositCanonicalizeToZero(); testVaultWithdrawCanonicalizeToZero(); + testBugVaultDustDebitCanonicalizesToNoOp(); testVaultDepositNegativeBalanceFromOppositeLimit(); testSequences(); testPreflight(); diff --git a/src/test/app/lending/LoanRounding_test.cpp b/src/test/app/lending/LoanRounding_test.cpp index b914961e786..139c2c36549 100644 --- a/src/test/app/lending/LoanRounding_test.cpp +++ b/src/test/app/lending/LoanRounding_test.cpp @@ -885,6 +885,258 @@ class LoanRounding_test : public LoanTestBase env.close(); } + // Pre-fixCleanup3_4_0 bug: VaultWithdraw for a fixed *share* amount that + // rounds to zero assets trips tecINVARIANT_FAILED instead of failing + // cleanly or succeeding, depending on why it's zero. The fixed-shares + // branch had no zero guard, unlike the fixed-assets branch. + // XRP case: pool value is nonzero (2,000,000) but 1 share's worth (0.5 + // drops) truncates to zero drops -> real precision loss -> tecPRECISION_LOSS. + // IOU case: loan drew 100% of the vault and is fully impaired, so + // AssetsTotal == LossUnrealized exactly -> pool value is genuinely zero + // -> legitimate zero-value withdrawal -> tesSUCCESS. + void + testBugVaultWithdrawFixedSharesRoundsToZero(FeatureBitset features) + { + testcase("bug: VaultWithdraw fixed shares round down to zero assets"); + + using namespace jtx; + using namespace loan; + + bool const fixed = features[fixCleanup3_4_0]; + + Env env(*this, features); + + Account const lender{"lender"}; + Account const depositorB{"depositorB"}; + Account const borrower{"borrower"}; + + env.fund(XRP(10'000'000), lender, depositorB, borrower); + env.close(); + + // asset(n) == n drops. + PrettyAsset const xrpAsset{xrpIssue(), 1}; + + auto const broker = createVaultAndBroker( + env, + xrpAsset, + lender, + {.vaultDeposit = 1'000'000, .debtMax = 3'000'000, .coverDeposit = 1'000'000}); + + Vault const v{env}; + env(v.deposit( + {.depositor = depositorB, + .id = broker.vaultKeylet().key, + .amount = xrpAsset(3'000'000)})); + env.close(); + + auto const brokerSle = env.le(broker.brokerKeylet()); + if (!BEAST_EXPECT(brokerSle)) + return; + auto const loanKeylet = keylet::loan(broker.brokerID, brokerSle->at(sfLoanSequence)); + + env(set(borrower, broker.brokerID, Number{2'000'000}), + Sig(sfCounterpartySignature, lender), + kPaymentTotal(2), + kPaymentInterval(600), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + // Impair the loan so LossUnrealized > 0. + env(manage(lender, loanKeylet.key, tfLoanImpair), Ter(tesSUCCESS)); + env.close(); + + auto const vaultSle = env.le(broker.vaultKeylet()); + if (!BEAST_EXPECT(vaultSle)) + return; + BEAST_EXPECT(vaultSle->at(sfLossUnrealized) > beast::kZero); + + // (AssetsTotal 4M - LossUnrealized 2M) * 1 share / 4M shares = 0.5, + // rounds down to zero drops. + auto const shareAsset = vaultSle->at(sfShareMPTID); + STAmount const oneShare{MPTIssue{shareAsset}, Number(1)}; + + env(v.withdraw({.depositor = lender, .id = broker.vaultKeylet().key, .amount = oneShare}), + Ter(fixed ? tecPRECISION_LOSS : tecINVARIANT_FAILED)); + env.close(); + + // Same bug, IOU asset. Needs a 2nd, minimal depositor: a sole + // shareholder would waive the loss subtraction (fixCleanup3_2_0), + // returning full value instead of zero. + { + Account const issuer{"issuer"}; + Account const iouLender{"iouLender"}; + Account const iouDepositorB{"iouDepositorB"}; + Account const iouBorrower{"iouBorrower"}; + + env.fund(XRP(10'000'000), issuer, iouLender, iouDepositorB, iouBorrower); + env.close(); + + PrettyAsset const iouAsset = issuer[iouCurrency_]; + env(trust(iouLender, iouAsset(10'000'000))); + env(trust(iouDepositorB, iouAsset(10'000'000))); + env(trust(iouBorrower, iouAsset(10'000'000))); + // iouLender funds the vault deposit and the broker's cover deposit. + env(pay(issuer, iouLender, iouAsset(9'000'000))); + env(pay(issuer, iouDepositorB, iouAsset(1))); + env.close(); + + // No management fee -> LossUnrealized ends up == AssetsTotal. + auto const iouBroker = createVaultAndBroker( + env, + iouAsset, + iouLender, + {.vaultDeposit = 3'999'999, + .debtMax = 4'000'000, + .coverDeposit = 4'000'000, + .managementFeeRate = TenthBips16{0}}); + + env(v.deposit( + {.depositor = iouDepositorB, + .id = iouBroker.vaultKeylet().key, + .amount = iouAsset(1)})); + env.close(); + + auto const iouBrokerSle = env.le(iouBroker.brokerKeylet()); + if (!BEAST_EXPECT(iouBrokerSle)) + return; + auto const iouLoanKeylet = + keylet::loan(iouBroker.brokerID, iouBrokerSle->at(sfLoanSequence)); + + // Draw the entire vault out as a single loan. + env(set(iouBorrower, iouBroker.brokerID, Number{4'000'000}), + Sig(sfCounterpartySignature, iouLender), + kPaymentTotal(2), + kPaymentInterval(600), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + env(manage(iouLender, iouLoanKeylet.key, tfLoanImpair), Ter(tesSUCCESS)); + env.close(); + + auto const iouVaultSle = env.le(iouBroker.vaultKeylet()); + if (!BEAST_EXPECT(iouVaultSle)) + return; + BEAST_EXPECT(iouVaultSle->at(sfLossUnrealized) == iouVaultSle->at(sfAssetsTotal)); + + auto const iouShareAsset = iouVaultSle->at(sfShareMPTID); + STAmount const oneIouShare{MPTIssue{iouShareAsset}, Number(1)}; + + auto const iouLenderBalanceBefore = env.balance(iouLender, iouAsset); + auto const iouVaultAvailableBefore = iouVaultSle->at(sfAssetsAvailable); + // Env::balance can't be used for shares: it resolves the issuer + // name, and the share issuer is the vault pseudo-account, which + // Env doesn't know. + auto const lenderShares = [&]() -> std::uint64_t { + auto const sle = env.le(keylet::mptoken(iouShareAsset, iouLender.id())); + return sle ? sle->at(sfMPTAmount) : 0; + }; + auto const iouLenderSharesBefore = lenderShares(); + auto const iouIssuanceBefore = env.le(keylet::mptokenIssuance(iouShareAsset)); + if (!BEAST_EXPECT(iouIssuanceBefore)) + return; + auto const iouSharesOutstandingBefore = iouIssuanceBefore->at(sfOutstandingAmount); + env(v.withdraw( + {.depositor = iouLender, + .id = iouBroker.vaultKeylet().key, + .amount = oneIouShare}), + fixed ? Ter(tesSUCCESS) : Ter(tecINVARIANT_FAILED)); + env.close(); + + if (fixed) + { + // Confirm this was a true zero-value transfer: balances + // unchanged even though a share was burned. + BEAST_EXPECT(env.balance(iouLender, iouAsset) == iouLenderBalanceBefore); + BEAST_EXPECT(lenderShares() == iouLenderSharesBefore - 1); + auto const iouIssuanceAfter = env.le(keylet::mptokenIssuance(iouShareAsset)); + if (BEAST_EXPECT(iouIssuanceAfter)) + { + BEAST_EXPECT( + iouIssuanceAfter->at(sfOutstandingAmount) == + iouSharesOutstandingBefore - 1); + } + auto const iouVaultAfter = env.le(iouBroker.vaultKeylet()); + if (BEAST_EXPECT(iouVaultAfter)) + { + BEAST_EXPECT(iouVaultAfter->at(sfAssetsAvailable) == iouVaultAvailableBefore); + } + } + } + } + + // Companion to the Vault_test dust-debit tests, which use a single + // depositor so AssetsTotal == AssetsAvailable and both debitRoundsToNoOp + // operands in VaultWithdraw::doApply trip together. Here a loan draws + // almost the entire vault, leaving AssetsTotal (1e7) far above + // AssetsAvailable (100): redeeming 1 share moves 1e-10 assets, which is + // dust against AssetsTotal but representable against AssetsAvailable, so + // the AssetsTotal operand alone carries the rejection. + void + testBugVaultWithdrawDustVsAssetsTotal(FeatureBitset features) + { + testcase("bug: VaultWithdraw dust debit vs AssetsTotal only"); + + using namespace jtx; + using namespace loan; + + bool const fixed = features[fixCleanup3_4_0]; + + Env env(*this, features); + + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(10'000'000), issuer, lender, borrower); + env.close(); + + PrettyAsset const iouAsset = issuer[iouCurrency_]; + env(trust(lender, iouAsset(100'000'000))); + env(trust(borrower, iouAsset(100'000'000))); + env(pay(issuer, lender, iouAsset(20'000'000))); + env.close(); + + // Scale 10 so 1 share is worth 1e-10 assets against the 1e7 pool. + auto const broker = createVaultAndBroker( + env, + iouAsset, + lender, + {.vaultDeposit = 10'000'000, + .debtMax = 10'000'000, + .coverDeposit = 1'000'000, + .vaultScale = 10}); + + // Draw all but 100 units: AssetsAvailable drops to 100 while + // AssetsTotal stays at 1e7 (the loan is still an asset of the vault). + env(set(borrower, broker.brokerID, Number{9'999'900}), + Sig(sfCounterpartySignature, lender), + kPaymentTotal(2), + kPaymentInterval(600), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + auto const vaultSle = env.le(broker.vaultKeylet()); + if (!BEAST_EXPECT(vaultSle)) + return; + BEAST_EXPECT(vaultSle->at(sfAssetsTotal) == Number{10'000'000}); + BEAST_EXPECT(vaultSle->at(sfAssetsAvailable) == Number{100}); + + // 1 share redeems 1e7 * 1 / 1e17 = 1e-10 assets. Subtracting that + // from AssetsTotal needs 18 significant digits and canonicalizes + // straight back to 1e7 (no-op), while AssetsAvailable would become + // 99.9999999999 — perfectly representable. + auto const shareAsset = vaultSle->at(sfShareMPTID); + STAmount const oneShare{MPTIssue{shareAsset}, Number(1)}; + + Vault const v{env}; + env(v.withdraw({.depositor = lender, .id = broker.vaultKeylet().key, .amount = oneShare}), + Ter(fixed ? tecPRECISION_LOSS : tecINVARIANT_FAILED)); + env.close(); + } + // A near-zero interest rate on a 100 USD loan // produces total interest of ~6 units at loanScale -9. Numerical error // in the amortization formula pushes the theoretical principal above @@ -962,6 +1214,10 @@ class LoanRounding_test : public LoanTestBase testYieldTheftRounding(flags); testBugOverpaymentPrincipalChange(); testBugOverpayUnroundedAmount(); + testBugVaultWithdrawFixedSharesRoundsToZero(all_ - fixCleanup3_4_0); + testBugVaultWithdrawFixedSharesRoundsToZero(all_); + testBugVaultWithdrawDustVsAssetsTotal(all_ - fixCleanup3_4_0); + testBugVaultWithdrawDustVsAssetsTotal(all_); testBugInterestDueDeltaCrash(); }