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.
This commit is contained in:
Vito
2026-08-21 11:14:31 +02:00
parent 59d4aff3b2
commit fbda53aa8b
4 changed files with 51 additions and 18 deletions

View File

@@ -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

View File

@@ -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,

View File

@@ -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.

View File

@@ -780,6 +780,6 @@ public:
}
};
BEAST_DEFINE_TESTSUITE(VaultTransactorPrecision, tx, xrpl);
BEAST_DEFINE_TESTSUITE(VaultTransactorPrecision, app, xrpl);
} // namespace xrpl::test