Skip to content

fix: Round vault deposit and withdraw conversions to prevent shareholder dilution#7824

Open
muhammadsalah wants to merge 1 commit into
XRPLF:developfrom
muhammadsalah:ci/fix-vault-deposit-rounding
Open

fix: Round vault deposit and withdraw conversions to prevent shareholder dilution#7824
muhammadsalah wants to merge 1 commit into
XRPLF:developfrom
muhammadsalah:ci/fix-vault-deposit-rounding

Conversation

@muhammadsalah

@muhammadsalah muhammadsalah commented Jul 17, 2026

Copy link
Copy Markdown

High Level Overview of Change

Under the fixCleanup3_3_0 amendment, the vault's share↔asset conversions are rounded in the pool's favor so that a single depositor or withdrawer can no longer dilute the other shareholders through round-to-nearest:

  • DepositsharesToAssetsDeposit() rounds the assets charged for freshly minted shares Upward, so the depositor pays at least the fair value of those shares.
  • WithdrawsharesToAssetsWithdraw() rounds the assets paid for burned shares Downward, so the withdrawer receives at most the fair value of those shares.

Both helpers take a rounding-mode parameter defaulting to ToNearest (pre-amendment behavior); the paired transactors pass the pool-favoring mode only when the amendment is enabled.

Context of Change

Both defects predate this PR and are symmetric. The conversions computed assetsTotal · shares / shareTotal and quantized to the asset's precision with the default ToNearest rounding:

  • On deposit, an inexact rate could round the charge down, so the depositor paid less than the fair value of the shares received — the shortfall accrues to NAV and dilutes existing holders.
  • On withdraw, an inexact rate could round the payout up, so the withdrawer took more assets than their burned shares were worth — dropping the assets-per-share price for the remaining holders.

Both are corrected only behind fixCleanup3_3_0, so pre-amendment ledgers replay identically. Part of the ongoing vault/lending rounding cleanup (cf. fixCleanup3_2_0, fixCleanup3_1_3).

Scope notes:

  • assetsToSharesDeposit already truncates (down) and assetsToSharesWithdraw needs no change — flooring the withdraw payout alone guarantees assetsWithdrawn ≤ fairValue(sharesBurned) regardless of how the shares were derived, so the "equal balance" invariant is preserved by construction (assetsWithdrawn drives both the vault decrement and the destination credit).
  • Clawback callers of sharesToAssetsWithdraw are intentionally left on ToNearest (different economic direction).
  • VaultWithdraw::preclaim passes the same Downward mode so the withdrawal-limit check estimates the same amount the apply will pay.

Bug discovery — Common Prefix formal verification. These fixes address findings from Common Prefix's public formal verification of the XRPL single-asset vault / lending protocol. Their testVaultDepositOvervaluedShares snippet states the deposit finding directly:

// Finding (C++ bug): a deposit charges round-to-nearest, so a deposit into a vault
// can be undercharged, overvaluing the new shares and diluting existing holders.

The withdraw payout corrected here is the exact symmetric case: round-to-nearest can overpay a withdrawer, diluting the holders who remain.

API Impact

  • Public API: New feature (new methods and/or new fields)
  • Public API: Breaking change (in general, breaking changes should only impact the next api_version)
  • libxrpl change — sharesToAssetsDeposit() and sharesToAssetsWithdraw() in include/xrpl/ledger/helpers/VaultHelpers.h each gain a defaulted Number::RoundingMode parameter (source-compatible; the default preserves prior behavior).
  • Peer protocol change (must be backward compatible or bump the peer protocol version)

Transaction-processing behavior changes only when fixCleanup3_3_0 is enabled, so no peer-protocol/version impact.

Before / After

Inexact rate, pool 100 assets / 3 shares:

Op Shares Fair value Before (ToNearest) After (amendment on) Effect of fix
Deposit charge mint 1 33.33… 33 (33·3=99 < 100 → dilution) 34 Upward (102 ≥ 100) charge favors pool
Withdraw payout burn 2 66.67… 67 (67·3=201 > 200 → dilution) 66 Downward (198 ≤ 200) payout favors pool

Exact rates, and cases where ToNearest already rounds the safe way, are unchanged — the fix only ever moves the amount toward the pool.

Test Plan

Direct unit tests of both conversion helpers (src/test/app/Vault_test.cpp) build minimal ltVAULT + ltMPTOKEN_ISSUANCE SLEs and call the helper with ToNearest vs the pool-favoring mode:

  • testDepositRoundingFavorsPool — asserts the undercharge/dilution case (33 vs 34) and the invariant charged · shareTotal ≥ assetsTotal · shares, plus no-op cases.
  • testWithdrawRoundingFavorsPool — asserts the overpay/dilution case (67 vs 66) and the invariant paid · shareTotal ≤ assetsTotal · shares, plus exact-rate and large awkward-rate cases.

Both target MPT assets deliberately: for IOU the fair value fits within STAmount's 16-digit quantization so the rounding difference is sub-ULP and unobservable; on integral (MPT/XRP) assets a wrong rounding is a full unit.

Local regression run (Debug): xrpl.app.Vault (409 cases / 11,358 tests), xrpl.tx.Loan (1,055 / 115,934 — the unrealized-loss withdraw path), and xrpl.app.Invariants (85 / 13,978) all pass with 0 failures.

@github-actions

Copy link
Copy Markdown

⚠️ This PR contains unsigned commits. To get your PR merged, please sign them. ⚠️

If only the most recent commit is unsigned, you can run:

  1. Amend the commit: git commit --amend --no-edit -n -S
  2. Overwrite the commit: git push --force-with-lease

If multiple commits are unsigned, you can run:

  1. Go into interactive rebase mode: git rebase --interactive HEAD~<NUM_OF_COMMITS>, where NUM_OF_COMMITS is the number of most recent commits that will be available to edit.
  2. Change "pick" to "edit" for the commits you need to sign, and then save and exit.
  3. For each commit, run: git commit --amend --no-edit -n -S
  4. Continue the rebase: git rebase --continue
  5. Overwrite the commit(s): git push --force-with-lease

If you're new to commit signing, there are different ways to set it up:

Sign commits with gpg

Follow the steps below to set up commit signing with gpg:

  1. Generate a GPG key
  2. Add the GPG key to your GitHub account
  3. Configure git to use your GPG key for commit signing
Sign commits with ssh-agent

Follow the steps below to set up commit signing with ssh-agent:

  1. Generate an SSH key and add it to ssh-agent
  2. Add the SSH key to your GitHub account
  3. Configure git to use your SSH key for commit signing
Sign commits with 1Password

You can also sign commits using 1Password, which lets you sign commits with biometrics without the signing key leaving the local 1Password process.
See use 1Password to sign your commits.

@muhammadsalah
muhammadsalah force-pushed the ci/fix-vault-deposit-rounding branch 2 times, most recently from 310dd72 to 81dbb1b Compare July 17, 2026 03:30
@muhammadsalah muhammadsalah reopened this Jul 17, 2026
…der dilution

Under the fixCleanup3_3_0 amendment, round the vault's share<->asset
conversions in the pool's favor so that a single depositor or withdrawer
can no longer dilute the other shareholders through round-to-nearest.

Deposit: sharesToAssetsDeposit() rounds the assets charged for freshly
minted shares Upward, so the depositor pays at least the fair value of
those shares. Round-to-nearest could undercharge, overvaluing the new
shares. VaultDeposit::doApply() passes Upward when the amendment is on.

Withdraw: sharesToAssetsWithdraw() rounds the assets paid for burned
shares Downward, so the withdrawer receives at most the fair value of
those shares. Round-to-nearest could overpay, dropping the assets-per-
share price for the remaining holders. VaultWithdraw::doApply() passes
Downward on both the fixed-asset and fixed-share branches, and preclaim
matches it so the withdrawal-limit check estimates the same amount.

Both helpers take a rounding mode defaulting to ToNearest to preserve
pre-amendment behavior; the quantization to the asset's precision honors
the mode for integral (XRP/MPT) and IOU assets alike. Clawback is left
unchanged. Direct unit tests on an MPT asset exercise each conversion
under ToNearest vs the pool-favoring mode.

This addresses catches found by the Common Prefix formal verification
public project.
@muhammadsalah
muhammadsalah force-pushed the ci/fix-vault-deposit-rounding branch from 81dbb1b to 0d2ac58 Compare July 17, 2026 04:22
@muhammadsalah muhammadsalah changed the title fix: Round vault deposit assets upward to prevent shareholder dilution fix: Round vault deposit and withdraw conversions to prevent shareholder dilution Jul 17, 2026
@muhammadsalah
muhammadsalah marked this pull request as ready for review July 17, 2026 04:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant