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

include/xrpl/ledger/helpers/VaultHelpers.h

@@ -28,14 +28,25 @@ assetsToSharesDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount co
  * depositor when they receive a fixed amount of shares. Note, since shares are
  * MPT, they are always an integral number.
  *
+ * The result is quantized to the vault asset's precision using @p rounding.
+ * Callers charging a depositor for freshly minted shares must round Upward, so
+ * the depositor pays at least the fair value of those shares and existing
+ * shareholders cannot be diluted. The ToNearest default preserves the
+ * pre-amendment behavior.
+ *
  * @param vault The vault SLE.
  * @param issuance The MPTokenIssuance SLE for the vault's shares.
  * @param shares The amount of shares to convert.
+ * @param rounding How to round the resulting assets to the asset's precision.
  *
  * @return The number of assets, or nullopt on error.
  */
 [[nodiscard]] std::optional<STAmount>
-sharesToAssetsDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount const& shares);
+sharesToAssetsDeposit(
+    SLE::const_ref vault,
+    SLE::const_ref issuance,
+    STAmount const& shares,
+    Number::RoundingMode rounding = Number::RoundingMode::ToNearest);
 
 /**
  * Controls whether to truncate shares instead of rounding.
@@ -79,11 +90,18 @@ assetsToSharesWithdraw(
  * depositor when they redeem a fixed amount of shares. Note, since shares are
  * MPT, they are always an integral number.
  *
+ * The result is quantized to the vault asset's precision using @p rounding.
+ * Callers paying a withdrawing shareholder must round Downward, so the
+ * shareholder receives at most the fair value of the shares they burn and the
+ * remaining shareholders cannot be diluted. The ToNearest default preserves the
+ * pre-amendment behavior.
+ *
  * @param vault The vault SLE.
  * @param issuance The MPTokenIssuance SLE for the vault's shares.
  * @param shares The amount of shares to convert.
  * @param waive Whether to waive (i.e. not subtract) the vault's unrealized
  *              loss when computing the exchange rate.
+ * @param rounding How to round the resulting assets to the asset's precision.
  *
  * @return The number of assets, or nullopt on error.
  */
@@ -92,7 +110,8 @@ sharesToAssetsWithdraw(
     SLE::const_ref vault,
     SLE::const_ref issuance,
     STAmount const& shares,
-    WaiveUnrealizedLoss waive = WaiveUnrealizedLoss::No);
+    WaiveUnrealizedLoss waive = WaiveUnrealizedLoss::No,
+    Number::RoundingMode rounding = Number::RoundingMode::ToNearest);
 
 /**
  * Returns true iff `account` holds all of the vault's outstanding shares —

src/libxrpl/ledger/helpers/VaultHelpers.cpp

@@ -41,7 +41,11 @@ assetsToSharesDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount co
 }
 
 [[nodiscard]] std::optional<STAmount>
-sharesToAssetsDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount const& shares)
+sharesToAssetsDeposit(
+    SLE::const_ref vault,
+    SLE::const_ref issuance,
+    STAmount const& shares,
+    Number::RoundingMode rounding)
 {
     XRPL_ASSERT(!shares.negative(), "xrpl::sharesToAssetsDeposit : non-negative shares");
     XRPL_ASSERT(
@@ -59,6 +63,10 @@ sharesToAssetsDeposit(SLE::const_ref vault, SLE::const_ref issuance, STAmount co
     }
 
     Number const shareTotal = issuance->at(sfOutstandingAmount);
+    // Quantizing to the asset's precision honors the thread-local rounding mode
+    // for both integral (XRP/MPT) and IOU assets. Charging a depositor must
+    // round Upward so they pay at least the fair value of the minted shares.
+    NumberRoundModeGuard const rg(rounding);
     assets = (assetTotal * shares) / shareTotal;
     return assets;
 }
@@ -97,7 +105,8 @@ sharesToAssetsWithdraw(
     SLE::const_ref vault,
     SLE::const_ref issuance,
     STAmount const& shares,
-    WaiveUnrealizedLoss waive)
+    WaiveUnrealizedLoss waive,
+    Number::RoundingMode rounding)
 {
     XRPL_ASSERT(!shares.negative(), "xrpl::sharesToAssetsWithdraw : non-negative shares");
     XRPL_ASSERT(
@@ -113,6 +122,12 @@ sharesToAssetsWithdraw(
     if (assetTotal == 0)
         return assets;
     Number const shareTotal = issuance->at(sfOutstandingAmount);
+    // Paying a withdrawing shareholder must round Downward so they receive at
+    // most the fair value of the shares they burn; otherwise the assets-per-
+    // share price would drop and dilute the remaining shareholders. Quantizing
+    // to the asset's precision honors the thread-local rounding mode for both
+    // integral (XRP/MPT) and IOU assets.
+    NumberRoundModeGuard const rg(rounding);
     assets = (assetTotal * shares) / shareTotal;
     return assets;
 }

src/libxrpl/tx/transactors/vault/VaultDeposit.cpp

@@ -196,6 +196,7 @@ TER
 VaultDeposit::doApply()
 {
     bool const fix320Enabled = view().rules().enabled(fixCleanup3_2_0);
+    bool const fix330Enabled = view().rules().enabled(fixCleanup3_3_0);
     auto const vault = view().peek(keylet::vault(ctx_.tx[sfVaultID]));
     auto applyViewContext = ctx_.getApplyViewContext();
     if (!vault)
@@ -284,7 +285,15 @@ VaultDeposit::doApply()
         if (sharesCreated == beast::kZero)
             return tecPRECISION_LOSS;
 
-        auto const maybeAssets = sharesToAssetsDeposit(vault, sleIssuance, sharesCreated);
+        // Post-fixCleanup3_3_0: round the charged assets Upward so the depositor
+        // pays at least the fair value of the freshly minted shares. Round-to-
+        // nearest could undercharge, overvaluing the new shares and diluting
+        // existing shareholders.
+        auto const maybeAssets = sharesToAssetsDeposit(
+            vault,
+            sleIssuance,
+            sharesCreated,
+            fix330Enabled ? Number::RoundingMode::Upward : Number::RoundingMode::ToNearest);
         if (!maybeAssets)
         {
             return tecINTERNAL;  // LCOV_EXCL_LINE

src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp

@@ -1,6 +1,7 @@
 #include <xrpl/tx/transactors/vault/VaultWithdraw.h>
 
 #include <xrpl/basics/Log.h>
+#include <xrpl/basics/Number.h>
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/utility/Zero.h>
 #include <xrpl/beast/utility/instrumentation.h>
@@ -124,8 +125,14 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
         auto const waiveUnrealizedLoss = shouldWaiveWithdrawal(ctx.view, account, sleIssuance);
         try
         {
-            auto const maybeAssets =
-                sharesToAssetsWithdraw(vault, sleIssuance, amount, waiveUnrealizedLoss);
+            // Match the Downward payout rounding used in doApply so the limit
+            // check estimates the same asset amount the withdrawal will pay.
+            auto const maybeAssets = sharesToAssetsWithdraw(
+                vault,
+                sleIssuance,
+                amount,
+                waiveUnrealizedLoss,
+                fix330Enabled ? Number::RoundingMode::Downward : Number::RoundingMode::ToNearest);
             if (!maybeAssets)
                 return tefINTERNAL;  // LCOV_EXCL_LINE
 
@@ -193,6 +200,7 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
 TER
 VaultWithdraw::doApply()
 {
+    bool const fix330Enabled = view().rules().enabled(fixCleanup3_3_0);
     auto const vault = view().peek(keylet::vault(ctx_.tx[sfVaultID]));
     auto applyViewContext = ctx_.getApplyViewContext();
     if (!vault)
@@ -224,6 +232,13 @@ VaultWithdraw::doApply()
     // We waive the unrealized-loss subtraction in this case to avoid user withdrawing all of their
     // shares but keeping future value in the vault.
     auto const waiveUnrealizedLoss = shouldWaiveWithdrawal(view(), accountID_, sleIssuance);
+
+    // Post-fixCleanup3_3_0: round the assets paid to the withdrawing shareholder
+    // Downward, so they receive at most the fair value of the shares they burn.
+    // Round-to-nearest could overpay, dropping the assets-per-share price and
+    // diluting the remaining shareholders.
+    auto const withdrawRounding =
+        fix330Enabled ? Number::RoundingMode::Downward : Number::RoundingMode::ToNearest;
     try
     {
         if (amount.asset() == vaultAsset)
@@ -239,8 +254,8 @@ VaultWithdraw::doApply()
 
             if (sharesRedeemed == beast::kZero)
                 return tecPRECISION_LOSS;
-            auto const maybeAssets =
-                sharesToAssetsWithdraw(vault, sleIssuance, sharesRedeemed, waiveUnrealizedLoss);
+            auto const maybeAssets = sharesToAssetsWithdraw(
+                vault, sleIssuance, sharesRedeemed, waiveUnrealizedLoss, withdrawRounding);
             if (!maybeAssets)
                 return tecINTERNAL;  // LCOV_EXCL_LINE
             assetsWithdrawn = *maybeAssets;
@@ -249,8 +264,8 @@ VaultWithdraw::doApply()
         {
             // Fixed shares, variable assets.
             sharesRedeemed = amount;
-            auto const maybeAssets =
-                sharesToAssetsWithdraw(vault, sleIssuance, sharesRedeemed, waiveUnrealizedLoss);
+            auto const maybeAssets = sharesToAssetsWithdraw(
+                vault, sleIssuance, sharesRedeemed, waiveUnrealizedLoss, withdrawRounding);
             if (!maybeAssets)
                 return tecINTERNAL;  // LCOV_EXCL_LINE
             assetsWithdrawn = *maybeAssets;
@@ -277,9 +292,8 @@ VaultWithdraw::doApply()
     // (checkWithdrawFreeze), so IgnoreFreeze avoids a redundant check that
     // would incorrectly return zero for vault pseudo-accounts whose shares
     // are frozen via a transitively frozen underlying asset.
-    auto const freezeHandling = view().rules().enabled(fixCleanup3_3_0)
-        ? FreezeHandling::IgnoreFreeze
-        : FreezeHandling::ZeroIfFrozen;
+    auto const freezeHandling =
+        fix330Enabled ? FreezeHandling::IgnoreFreeze : FreezeHandling::ZeroIfFrozen;
     if (accountHolds(view(), accountID_, share, freezeHandling, AuthHandling::IgnoreAuth, j_) <
         sharesRedeemed)
     {