fix: Reject vault deposits that move nothing from the depositor - #8014
fix: Reject vault deposits that move nothing from the depositor#8014tyalymov wants to merge 2 commits into
Conversation
A deposit does not transfer the requested amount. It transfers that amount truncated to a whole number of shares and converted back, which can be smaller, and the gap grows once the vault's assets-to-shares ratio stops being a power of ten. LoanPay does exactly that when it books accrued interest into AssetsTotal without minting shares. If the transferred value lands below half a ULP at the depositor's own trust-line scale, the debit rounds away to nothing: the depositor keeps their balance while the vault books the assets and mints shares, and the vault invariant fails the transaction after the fact. Under fixCleanup3_4_0, check the post-truncation value against the depositor's scale and reject with tecPRECISION_LOSS before any state moves. preclaim recomputes the round-trip and rejects up front; doApply repeats the check on the value it computed itself, so the transfer can never be a no-op for the depositor. The asset issuer is exempt, since accountHolds reports the maximum representable value for them. The existing pre-fixCleanup3_2_0 deposit-canonicalization case now disables fixCleanup3_4_0 as well: the new check rejects that deposit too, so the invariant is only reachable with neither guard in place. Co-authored-by: Cursor <cursoragent@cursor.com>
![xrplf-ai-reviewer[bot]](https://avatars.githubusercontent.com/u/67929741?s=60&v=4){kind=small}
There was a problem hiding this comment.
This change adds a depositor-side rounding guard for VaultDeposit under fixCleanup3_4_0, gated in both preclaim and doApply, with a well-targeted regression test (testBugDepositShareTruncationSubUlp) that reproduces the truncation-to-zero scenario via a real loan/interest flow. The logic in roundsToZeroForDepositor correctly short-circuits for integral assets (XRP/MPT) and for the issuer account (whose accountHolds balance is unbounded), and the preclaim/doApply checks mirror each other using the actual transferred value rather than the raw requested amount, closing the gap left by the fixCleanup3_2_0 guard. The existing testVaultDepositCanonicalizeToZero case is correctly updated to also disable fixCleanup3_4_0 so the invariant-failure path stays reachable. I did not find correctness, security, or resource-management issues in the added/changed lines; the exception handling (catching std::overflow_error only) and warn-level logging are consistent with the described design intent and don't appear to be bugs.
clang-tidy's bugprone-empty-catch flags the empty handler, and fairly so. There is nothing to decide in it: an amount that overflows Number is unusable for a different reason than the one this check is about, and doApply still reports that case as tecPATH_DRY. Say as much in the log. The message sits at debug level, like the one doApply writes for the same overflow, because a large enough Scale runs into it easily and the line would otherwise be noise.
There was a problem hiding this comment.
The change is a well-scoped, well-tested precision guard: it recomputes the actual (post-truncation) transferred amount and rejects deposits where the depositor's own debit would round away to nothing, both in preclaim (early rejection) and doApply (authoritative check before vault state is touched). The reasoning, exception handling, and regression test (testBugDepositShareTruncationSubUlp) are solid. One area worth double-checking before merge: the new roundsToZeroForDepositor helper derives its rounding-scale from the depositor's current balance via accountHolds, and none of the added tests (or, seemingly, existing deposit tests) exercise a depositor whose balance in the asset is exactly zero (i.e. a genuinely first-time depositor) at deposit time.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
gregtatcam
left a comment
gregtatcam
left a comment
There was a problem hiding this comment.
Add a scenario where spendable and balance diverge. The current cases have no reverse limit on bob's line, so spendable equals the balance and they pass whether the guard reads FullBalance or SimpleBalance. Add a depositor in debt on the line — same vault setup, default IOU scale:
env(trust(gw, depositor["USD"](10'000'000'001))); // gw accepts depositor's own USD
env(pay(depositor, bob, usd(10'000'000'000))); // sfBalance = -1e10, spendable = 1
env(vault.deposit({.depositor = depositor, .id = vaultKeylet.key,
.amount = STAmount{usd.raw(), Number{4, -6}}}), Ter(expected));
4e-6 truncates to 3 shares worth 3.72e-6, under half a ULP of -1e10. Expect tecINVARIANT_FAILED today with fixCleanup3_4_0 either on or off, tecPRECISION_LOSS once the guard reads the raw balance. That case is what pins the fix.
| FreezeHandling::ZeroIfFrozen, | ||
| AuthHandling::ZeroIfUnauthorized, | ||
| j, | ||
| SpendableHandling::FullBalance); |
There was a problem hiding this comment.
This reads the balance with SpendableHandling::FullBalance, which for a trust line returns balance + counterparty's limit, not the balance. The rounding of the debit is governed by the magnitude of the stored sfBalance. When the two diverge — negative balance plus a large opposite limit — scale(balance, …) is far too fine and the check passes on an amount that still rounds away.
Suggested fix, which also removes the scale reasoning entirely: compare the balance against itself post-debit rather than inferring a ULP.
auto const balance = accountHolds(view, account, assets.asset(), ..., SpendableHandling::SimpleBalance);
if (balance - assets != balance)
return false;
| // 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(); |
There was a problem hiding this comment.
Pin the ratio the test depends on. After the LoanPay, assert the setup actually produced what the comment claims, so the test can't silently stop exercising the bug:
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);
Tapanito
left a comment
Tapanito
left a comment
There was a problem hiding this comment.
All in all LGTM, left a few nitpicks.
| if (fix340Enabled) | ||
| { | ||
| try | ||
| { | ||
| if (auto const shares = assetsToSharesDeposit(vault, sleIssuance, roundedAmount)) | ||
| { | ||
| auto const assets = sharesToAssetsDeposit(vault, sleIssuance, *shares); | ||
| if (assets && roundsToZeroForDepositor(ctx.view, account, *assets, ctx.j)) | ||
| return tecPRECISION_LOSS; | ||
| } | ||
| } | ||
| catch (std::overflow_error const&) | ||
| { | ||
| // A large enough Scale overflows Number easily, so this stays at debug to avoid | ||
| // spamming the log. Nothing to decide here: the amount is unusable for a different | ||
| // reason than the one this check is about, and doApply reports it as tecPATH_DRY. | ||
| JLOG(ctx.j.debug()) << "VaultDeposit: overflow error computing deposited assets" | ||
| << " with scale=" << static_cast<int>(vault->at(sfScale)) | ||
| << ", amount=" << roundedAmount; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The rule of thumb is to perform as much validation as possible in preclaim. However, in this case I'd argue it is okay to make an exception, and perform the entire validation in doApply. We already have a check there to check if we are issuing zero shares.
| // rather than recomputed, so the transfer below can never be a no-op for the depositor. | ||
| if (fix340Enabled && roundsToZeroForDepositor(view(), accountID_, assetsDeposited, j_)) | ||
| return tecPRECISION_LOSS; // LCOV_EXCL_LINE | ||
|
|
There was a problem hiding this comment.
nit: Let's keep the deposit amount validation in a single place. That way, when we'll get to moving this code into a single helper, it'll be much easier to reason through. Please move this check to line 364.
There was a problem hiding this comment.
Pull request overview
This PR addresses a consensus-critical VaultDeposit edge case where share truncation can result in the depositor’s trust-line debit rounding to zero (no value actually transferred) while the vault still books assets and mints shares, ultimately triggering tecINVARIANT_FAILED. Behind fixCleanup3_4_0, the transaction is now rejected early with tecPRECISION_LOSS to prevent any inconsistent state transitions.
Changes:
- Add a new depositor-side “rounds-to-zero” guard for the post share-truncation transferred amount in
VaultDeposit(checked in bothpreclaimanddoApply) underfixCleanup3_4_0. - Update and extend unit tests to cover the share-truncation + sub-ULP rounding scenario across amendment combinations, and adjust an existing invariant-reachability test to disable
fixCleanup3_4_0as well.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/test/app/Vault_test.cpp | Adds a new regression test for share-truncation deposits that round away at depositor scale; updates an existing test scenario to reflect the new guard under fixCleanup3_4_0. |
| src/libxrpl/tx/transactors/vault/VaultDeposit.cpp | Introduces roundsToZeroForDepositor and uses it to reject deposits where the actually-transferred amount would not change the depositor’s balance under fixCleanup3_4_0. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // True if debiting `assets` would leave the depositor's balance untouched, because the value is | ||
| // below half a ULP at the scale that balance is held at. Such a deposit would mint shares against | ||
| // a transfer that never happened. |
| if (auto const shares = assetsToSharesDeposit(vault, sleIssuance, roundedAmount)) | ||
| { | ||
| auto const assets = sharesToAssetsDeposit(vault, sleIssuance, *shares); | ||
| if (assets && roundsToZeroForDepositor(ctx.view, account, *assets, ctx.j)) | ||
| return tecPRECISION_LOSS; |
|
This PR has conflicts, please resolve them in order for the PR to be reviewed. |
High Level Overview of Change
A
VaultDepositcan mint shares against a transfer that never happened, which trips the vaultinvariant and fails the transaction with
tecINVARIANT_FAILED. BehindfixCleanup3_4_0, such adeposit is now rejected with
tecPRECISION_LOSSbefore any state changes.Context of Change
A deposit does not transfer the requested amount. It transfers that amount truncated to a whole
number of shares and converted back (
assetsToSharesDeposit, thensharesToAssetsDeposit), whichcan be smaller. The gap opens up once the vault's assets-to-shares ratio stops being a power of
ten, and that happens through ordinary use:
LoanPaybooks accrued interest intoAssetsTotalwhile the share supply stays put.
When the transferred value lands below half a ULP at the depositor's own trust-line scale, the
debit rounds away to nothing. The depositor's balance does not change, the vault still books the
assets and mints the shares, and
ValidVaultcatches the mismatch at finalize time ("deposit mustdecrease depositor balance").
The precision guard added by
fixCleanup3_2_0does not prevent this. It tests the raw requestedamount, which is large enough to survive rounding, rather than the smaller value that is actually
transferred, so the failure is reachable both with and without that amendment.
Under
fixCleanup3_4_0,preclaimrecomputes the share round-trip and rejects the deposit upfront, and
doApplyrepeats the check on the value it computed itself, before the vault fields aretouched. The asset issuer is exempt:
accountHoldsreports the maximum representable value for theissuer, which would make every amount look sub-ULP.
One existing case moves with the amendment. The pre-
fixCleanup3_2_0run oftestVaultDepositCanonicalizeToZeronow disablesfixCleanup3_4_0as well, because the newdepositor-side check rejects that deposit too; the invariant is only reachable with neither guard
active.
API Impact
libxrplchange (any change that may affectlibxrplor dependents oflibxrpl)The change is consensus-affecting and gated on
fixCleanup3_4_0. Before activation, behavior isunchanged.
Test Plan
testBugDepositShareTruncationSubUlpinVault_testbuilds the ratio through real transactions: a1000 USD deposit, a loan broker with a one-year bullet loan at 24%, then a
LoanPaythat leaves thevault holding 1240 assets against 1000 shares. A depositor sitting on the 1e16 IOU precision cusp,
where one ULP is 10, then deposits 6 USD, which truncates to 4 shares worth 4.96.
It runs four times:
fixCleanup3_4_0on and off, each crossed withfixCleanup3_2_0on and off.With the amendment off the deposit still ends in
tecINVARIANT_FAILED; with it on the deposit isrejected with
tecPRECISION_LOSS.