From 2ebc2ca885ffa90fa6c7e8cf421dc0a12a34c3a8 Mon Sep 17 00:00:00 2001
From: Bronek Kozicki <brok@incorrekt.com>
Date: Tue, 11 Nov 2025 19:39:09 +0000
Subject: [PATCH] fix: floating point representation errors in vault (#5997)

This change fixes floating point errors in conversion of shares to assets and other way, used in `VaultDeposit`, `VaultWithdraw` and `VaultClawback`. In the floating point calculations the division introduces a larger error than multiplication. If we do division first, then the error introduced will be increased by the multiplication that follows, which is therefore the wrong order to perform these two operations. This change flips the order of arithmetic operations, which minimizes the error.
---
 src/libxrpl/ledger/View.cpp |  8 ++--
 src/test/app/Vault_test.cpp | 83 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 86 insertions(+), 5 deletions(-)

diff --git a/src/libxrpl/ledger/View.cpp b/src/libxrpl/ledger/View.cpp
index 0175b099ea..069bd3a4d8 100644
--- a/src/libxrpl/ledger/View.cpp
+++ b/src/libxrpl/ledger/View.cpp
@@ -2885,7 +2885,7 @@ assetsToSharesDeposit(
                 .truncate()};
 
     Number const shareTotal = issuance->at(sfOutstandingAmount);
-    shares = (shareTotal * (assets / assetTotal)).truncate();
+    shares = ((shareTotal * assets) / assetTotal).truncate();
     return shares;
 }
 
@@ -2914,7 +2914,7 @@ sharesToAssetsDeposit(
             false};
 
     Number const shareTotal = issuance->at(sfOutstandingAmount);
-    assets = assetTotal * (shares / shareTotal);
+    assets = (assetTotal * shares) / shareTotal;
     return assets;
 }
 
@@ -2940,7 +2940,7 @@ assetsToSharesWithdraw(
     if (assetTotal == 0)
         return shares;
     Number const shareTotal = issuance->at(sfOutstandingAmount);
-    Number result = shareTotal * (assets / assetTotal);
+    Number result = (shareTotal * assets) / assetTotal;
     if (truncate == TruncateShares::yes)
         result = result.truncate();
     shares = result;
@@ -2968,7 +2968,7 @@ sharesToAssetsWithdraw(
     if (assetTotal == 0)
         return assets;
     Number const shareTotal = issuance->at(sfOutstandingAmount);
-    assets = assetTotal * (shares / shareTotal);
+    assets = (assetTotal * shares) / shareTotal;
     return assets;
 }
 
diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp
index ccbf0fed42..2ca525c036 100644
--- a/src/test/app/Vault_test.cpp
+++ b/src/test/app/Vault_test.cpp
@@ -2454,6 +2454,7 @@ class Vault_test : public beast::unit_test::suite
         struct CaseArgs
         {
             int initialXRP = 1000;
+            Number initialIOU = 200;
             double transferRate = 1.0;
         };
 
@@ -2481,7 +2482,7 @@ class Vault_test : public beast::unit_test::suite
                 PrettyAsset const asset = issuer["IOU"];
                 env.trust(asset(1000), owner);
                 env.trust(asset(1000), charlie);
-                env(pay(issuer, owner, asset(200)));
+                env(pay(issuer, owner, asset(args.initialIOU)));
                 env(rate(issuer, args.transferRate));
                 env.close();
 
@@ -2859,6 +2860,86 @@ class Vault_test : public beast::unit_test::suite
             env(tx1);
         });
 
+        testCase(
+            [&, this](
+                Env& env,
+                Account const& owner,
+                Account const& issuer,
+                Account const& charlie,
+                auto const& vaultAccount,
+                Vault& vault,
+                PrettyAsset const& asset,
+                auto&&...) {
+                testcase("IOU calculation rounding");
+
+                auto [tx, keylet] =
+                    vault.create({.owner = owner, .asset = asset});
+                tx[sfScale] = 1;
+                env(tx);
+                env.close();
+
+                auto const startingOwnerBalance = env.balance(owner, asset);
+                BEAST_EXPECT(
+                    (startingOwnerBalance.value() ==
+                     STAmount{asset, 11875, -2}));
+
+                // This operation (first deposit 100, then 3.75 x 5) is known to
+                // have triggered calculation rounding errors in Number
+                // (addition and division), causing the last deposit to be
+                // blocked by Vault invariants.
+                env(vault.deposit(
+                    {.depositor = owner,
+                     .id = keylet.key,
+                     .amount = asset(100)}));
+
+                auto const tx1 = vault.deposit(
+                    {.depositor = owner,
+                     .id = keylet.key,
+                     .amount = asset(Number(375, -2))});
+                for (auto i = 0; i < 5; ++i)
+                {
+                    env(tx1);
+                }
+                env.close();
+
+                {
+                    STAmount const xfer{asset, 1185, -1};
+                    BEAST_EXPECT(
+                        env.balance(owner, asset) ==
+                        startingOwnerBalance.value() - xfer);
+                    BEAST_EXPECT(
+                        env.balance(vaultAccount(keylet), asset) == xfer);
+
+                    auto const vault = env.le(keylet);
+                    BEAST_EXPECT(vault->at(sfAssetsAvailable) == xfer);
+                    BEAST_EXPECT(vault->at(sfAssetsTotal) == xfer);
+                }
+
+                // Total vault balance should be 118.5 IOU. Withdraw and delete
+                // the vault to verify this exact amount was deposited and the
+                // owner has matching shares
+                env(vault.withdraw(
+                    {.depositor = owner,
+                     .id = keylet.key,
+                     .amount = asset(Number(1000 + 37 * 5, -1))}));
+
+                {
+                    BEAST_EXPECT(
+                        env.balance(owner, asset) ==
+                        startingOwnerBalance.value());
+                    BEAST_EXPECT(
+                        env.balance(vaultAccount(keylet), asset) ==
+                        beast::zero);
+                    auto const vault = env.le(keylet);
+                    BEAST_EXPECT(vault->at(sfAssetsAvailable) == beast::zero);
+                    BEAST_EXPECT(vault->at(sfAssetsTotal) == beast::zero);
+                }
+
+                env(vault.del({.owner = owner, .id = keylet.key}));
+                env.close();
+            },
+            {.initialIOU = Number(11875, -2)});
+
         auto const [acctReserve, incReserve] = [this]() -> std::pair<int, int> {
             Env env{*this, testable_amendments()};
             return {