From fbda53aa8b2453b074ce561af0ddc3156e7e8ad4 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Fri, 21 Aug 2026 11:14:31 +0200 Subject: [PATCH] fix: Guard debitIsNonZeroDust against overflow_error in VaultClawback Mirrors the equivalent guard already applied to VaultWithdraw: wrap the dust check in try/catch so a sufficiently abused sfScale pushing assetsTotal/assetsAvailable out of STAmount's representable range returns tecPATH_DRY instead of surfacing as a raw overflow_error. Also address other outstanding review nits: fix VaultTransactorPrecision_test's suite module (tx -> app, matching every other Vault suite), replace the "PR 2" comment in VaultPrecisionFixture.h with a stable description, and tighten the clampToAssetsTotalScale docstring in VaultHelpers.h. --- include/xrpl/ledger/helpers/VaultHelpers.h | 20 +++++---- .../tx/transactors/vault/VaultClawback.cpp | 41 ++++++++++++++++--- src/test/app/vault/VaultPrecisionFixture.h | 6 +-- .../vault/VaultTransactorPrecision_test.cpp | 2 +- 4 files changed, 51 insertions(+), 18 deletions(-) diff --git a/include/xrpl/ledger/helpers/VaultHelpers.h b/include/xrpl/ledger/helpers/VaultHelpers.h index 7824b98c34..9199f64b9f 100644 --- a/include/xrpl/ledger/helpers/VaultHelpers.h +++ b/include/xrpl/ledger/helpers/VaultHelpers.h @@ -44,14 +44,18 @@ assetsToSharesDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount co sharesToAssetsDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount const& shares); /** - * Rounds the magnitude of `delta` to the sfAssetsTotal STAmount scale and - * returns it as a non-negative STAmount. `delta` is positive when - * crediting the vault and negative when debiting it; only its sign is - * used to select the rounding direction. The caller adds the returned - * magnitude to sfAssetsTotal for credits, or subtracts it for debits, and - * applies it the same way to any related field (for example - * sfAssetsAvailable) so all rails stay in sync; the other fields' scale is - * at least as fine as sfAssetsTotal's. + * Returns the effective change to sfAssetsTotal after canonicalizing + * `sfAssetsTotal + delta` under `mode`, as a non-negative magnitude. + * `delta` is positive when crediting the vault and negative when debiting + * it; only its sign is used to select the rounding direction. This is not + * the same as simply rounding `delta`'s magnitude to sfAssetsTotal's + * scale: both the before and after totals are canonicalized under `mode` + * before subtracting, so the result also accounts for sfAssetsTotal + * itself sitting mid-grid. The caller adds the returned magnitude to + * sfAssetsTotal for credits, or subtracts it for debits, and applies it + * the same way to any related field (for example sfAssetsAvailable) so + * all rails stay in sync; the other fields' scale is at least as fine as + * sfAssetsTotal's. * * @param vault The vault SLE. * @param delta The signed amount by which sfAssetsTotal will change; only diff --git a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp index f697045489..fb7ef1601b 100644 --- a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp @@ -409,13 +409,42 @@ VaultClawback::doApply() // Even a non-zero recovery can be too small to change the stored sfAssetsTotal or // sfAssetsAvailable at STAmount's precision. Shares would still be burned, so ValidVault // would fail after apply with "clawback must decrease vault balance"; reject here instead. - if (view().rules().enabled(fixCleanup3_4_0) && - (debitIsNonZeroDust(vaultAsset, assetsTotal, assetsRecovered) || - debitIsNonZeroDust(vaultAsset, assetsAvailable, assetsRecovered))) + // On the issuer-clawback path this is effectively unreachable once fixCleanup3_4_0 is + // active, since assetsToClawback's own clampToAssetsTotalScale already snapped + // assetsRecovered to the grid; kept as defense in depth and to cover the owner-burn path, + // where assetsRecovered is not put through that clamp. + // + // Number arithmetic can throw overflow_error when Scale and totals are large. Caught + // below. debitIsNonZeroDust converts assetsTotal/assetsAvailable to STAmount, which is + // exactly what a sufficiently abused sfScale can push out of STAmount's representable + // range. + if (view().rules().enabled(fixCleanup3_4_0)) { - JLOG(j_.debug()) << "VaultClawback: clawback amount too small to change stored vault" - " balance"; - return tecPRECISION_LOSS; + try + { + if (debitIsNonZeroDust(vaultAsset, assetsTotal, assetsRecovered) || + debitIsNonZeroDust(vaultAsset, assetsAvailable, assetsRecovered)) + { + JLOG(j_.debug()) + << "VaultClawback: clawback amount too small to change stored vault" + " balance"; + return tecPRECISION_LOSS; + } + } + catch (std::overflow_error const&) + { + // It's easy to hit this exception from Number with large enough Scale + // so we avoid spamming the log and only use debug here. + JLOG(j_.debug()) // + << "VaultClawback: overflow error with" + << " scale=" << (int)vault->at(sfScale).value() // + << ", assetsTotal=" << vault->at(sfAssetsTotal).value() + << ", sharesTotal=" << sleIssuance->at(sfOutstandingAmount) + << ", amount=" << amount.value(); + // Overflow means this transaction cannot apply, but ledger state is still + // consistent. Return tecPATH_DRY rather than a hard internal error. + return tecPATH_DRY; + } } // Debit both rails by the same delta so sfAssetsTotal and sfAssetsAvailable stay in step, diff --git a/src/test/app/vault/VaultPrecisionFixture.h b/src/test/app/vault/VaultPrecisionFixture.h index 1eb60241fc..77438b104a 100644 --- a/src/test/app/vault/VaultPrecisionFixture.h +++ b/src/test/app/vault/VaultPrecisionFixture.h @@ -31,9 +31,9 @@ namespace xrpl::test { -// Shared fixture for VaultInvariantPrecision_test and -// VaultTransactorPrecision_test (PR 2). Both PRs land this file identically; -// whichever merges second will need to reconcile this copy at rebase time. +// Shared fixture for VaultInvariantPrecision_test and VaultTransactorPrecision_test. +// Also landed identically on the companion invariant-fix branch; reconcile this copy +// against that branch's version on rebase. // // Layout: // - A-1 (impairAndPaySibling=false): 1000 USD vault + one ordinary loan. diff --git a/src/test/app/vault/VaultTransactorPrecision_test.cpp b/src/test/app/vault/VaultTransactorPrecision_test.cpp index 2e77dbcee3..f88d1706c1 100644 --- a/src/test/app/vault/VaultTransactorPrecision_test.cpp +++ b/src/test/app/vault/VaultTransactorPrecision_test.cpp @@ -780,6 +780,6 @@ public: } }; -BEAST_DEFINE_TESTSUITE(VaultTransactorPrecision, tx, xrpl); +BEAST_DEFINE_TESTSUITE(VaultTransactorPrecision, app, xrpl); } // namespace xrpl::test