From 5e3d20b3ed7bc4086ff44298abd290a4fedce3af Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Tue, 25 Aug 2026 17:59:37 +0000 Subject: [PATCH] fix: Prevent vault clawback and withdraw overrun (#8075) --- .../tx/transactors/vault/VaultClawback.cpp | 9 +- .../tx/transactors/vault/VaultWithdraw.cpp | 13 +- src/test/app/vault/VaultBugs_test.cpp | 240 ++++++++++++++++++ src/test/app/vault/VaultScale_test.cpp | 105 ++++---- 4 files changed, 310 insertions(+), 57 deletions(-) diff --git a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp index b6dc9377c2..7348e1734b 100644 --- a/src/libxrpl/tx/transactors/vault/VaultClawback.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultClawback.cpp @@ -271,8 +271,15 @@ VaultClawback::assetsToClawback( } else { + // Pre-fixCleanup3_4_0: shares were rounded to nearest, so the + // round-trip back to assets could exceed clawbackAmount. + // Post-amendment: truncate shares so assetsRecovered <= + // clawbackAmount by construction (matches the clamp branch + // below). + auto const truncate = ctx_.view().rules().enabled(fixCleanup3_4_0) ? TruncateShares::Yes + : TruncateShares::No; auto const maybeShares = - assetsToSharesWithdraw(vault, sleShareIssuance, clawbackAmount); + assetsToSharesWithdraw(vault, sleShareIssuance, clawbackAmount, truncate); if (!maybeShares) return std::unexpected(tecINTERNAL); // LCOV_EXCL_LINE sharesDestroyed = *maybeShares; diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index cee03f3999..9e066304ed 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp @@ -305,9 +305,20 @@ VaultWithdraw::doApply() if (amount.asset() == vaultAsset) { // Fixed assets, variable shares. + // + // Pre-fixCleanup3_4_0: shares were rounded to nearest, so the + // round-trip back to assets could exceed the requested amount. + // That over-delivers to the depositor and can bypass the + // preclaim canWithdraw check on the destination, which was + // validated against the requested amount only. + // Post-amendment: truncate shares so assetsWithdrawn <= + // requested amount by construction. If truncation yields zero + // shares, the tecPRECISION_LOSS guard below fires. + auto const truncate = + view().rules().enabled(fixCleanup3_4_0) ? TruncateShares::Yes : TruncateShares::No; { auto const maybeShares = assetsToSharesWithdraw( - vault, sleIssuance, amount, TruncateShares::No, waiveUnrealizedLoss); + vault, sleIssuance, amount, truncate, waiveUnrealizedLoss); if (!maybeShares) return tecINTERNAL; // LCOV_EXCL_LINE sharesRedeemed = *maybeShares; diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index 0771d4a450..ad6fdcc8b8 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -32,6 +33,7 @@ #include #include #include +#include #include #include #include @@ -974,6 +976,242 @@ private: } } + // Shared setup for testBugClawbackRoundTripOvershoot and + // testBugWithdrawRoundTripOvershoot, which both need a vault at + // assetsTotal=7, sharesTotal=5 and differ only in what they do once + // that state is reached. + // + // The (7, 5) state is reached through ordinary transactions: a 5 USD + // deposit mints 5 shares 1:1, then a loan broker on the vault issues a + // single-payment bullet loan for the full 5 USD at 40% interest. When + // the borrower repays a year later, LoanPay books the 2 USD of accrued + // interest into sfAssetsTotal without minting shares, leaving + // assetsTotal=7 against sharesTotal=5 (see + // testBugDepositShareTruncationSubUlp for the same technique in more + // detail). + struct RoundTripOvershootVault + { + test::jtx::Account issuer; + test::jtx::Account holder; + PrettyAsset usd; + test::jtx::Vault vault; + Keylet vaultKeylet; + Number initialAssetsTotal; + Number initialAssetsAvailable; + }; + + std::optional + makeRoundTripOvershootVault(test::jtx::Env& env) + { + using namespace test::jtx; + using namespace loan_broker; + using namespace loan; + + Account const issuer{"issuer"}; + Account const owner{"owner"}; + Account const holder{"holder"}; + Account const borrower{"borrower"}; + + env.fund(XRP(10'000), issuer, owner, holder, borrower); + env.close(); + + env(fset(issuer, asfAllowTrustLineClawback)); + env.close(); + + PrettyAsset const usd = issuer["USD"]; + env.trust(usd(1'000), owner); + env.trust(usd(1'000), holder); + env.trust(usd(1'000), borrower); + env.close(); + + env(pay(issuer, holder, usd(100))); + env(pay(issuer, borrower, usd(100))); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = owner, .asset = usd}); + vaultTx[sfScale] = 0; + env(vaultTx); + env.close(); + + // Holder deposits 5 USD, minting 5 shares 1:1. + env(vault.deposit({.depositor = holder, .id = vaultKeylet.key, .amount = usd(5)})); + env.close(); + + // A loan broker on the vault, then a single bullet loan for the + // entire deposit at 40% interest, one payment, one year out. + auto const brokerKeylet = + keylet::loanBroker(owner.id(), SeqProxy::rawSequence(env.seq(owner))); + env(set(owner, vaultKeylet.key)); + env.close(); + + auto const loanKeylet = keylet::loan(brokerKeylet.key, SeqProxy::rawSequence(1)); + env(set(borrower, brokerKeylet.key, usd(5).value()), + loan::kInterestRate(percentageToTenthBips(40)), + kGracePeriod(60), + kPaymentInterval(365 * 24 * 60 * 60), + kPaymentTotal(1), + Sig(sfCounterpartySignature, owner), + Fee(env.current()->fees().base * 2), + Ter(tesSUCCESS)); + env.close(); + + // Advance to just before the single payment falls due and let the + // borrower repay principal plus interest. Share supply stays at 5, + // so assetsTotal/sharesTotal becomes 7/5. + env.close(std::chrono::seconds{(365 * 24 * 60 * 60) - 3600}); + env(pay(borrower, loanKeylet.key, usd(10).value()), Ter(tesSUCCESS)); + env.close(); + + auto const vaultSle = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultSle)) + return std::nullopt; + auto const mptIssuanceID = vaultSle->at(sfShareMPTID); + + Number const initialAssetsTotal = vaultSle->at(sfAssetsTotal); + Number const initialAssetsAvailable = vaultSle->at(sfAssetsAvailable); + BEAST_EXPECT(initialAssetsTotal == usd(7).number()); + BEAST_EXPECT(initialAssetsAvailable == usd(7).number()); + { + auto const sleIssuance = env.le(keylet::mptokenIssuance(mptIssuanceID)); + if (!BEAST_EXPECT(sleIssuance)) + return std::nullopt; + BEAST_EXPECT(sleIssuance->getFieldU64(sfOutstandingAmount) == 5); + } + + return RoundTripOvershootVault{ + .issuer = issuer, + .holder = holder, + .usd = usd, + .vault = vault, + .vaultKeylet = vaultKeylet, + .initialAssetsTotal = initialAssetsTotal, + .initialAssetsAvailable = initialAssetsAvailable}; + } + + // VaultClawback::assetsToClawback converts clawbackAmount to shares + // with round-to-nearest, then round-trips back to assets. When shares + // round up, assetsRecovered can exceed clawbackAmount. + // + // Repro: assetsTotal=7, sharesTotal=5, request 4: + // shares = round(20/7) = 3, assets = 7*3/5 = 4.2 > 4. + // + // Post-fixCleanup3_4_0: truncate shares so assetsRecovered <= + // clawbackAmount by construction. + void + testBugClawbackRoundTripOvershoot() + { + using namespace test::jtx; + + auto runScenario = [this](FeatureBitset features, bool withFix) { + Env env{*this, features}; + + auto const setup = makeRoundTripOvershootVault(env); + if (!BEAST_EXPECT(setup)) + return; + + auto const clawbackAmount = setup->usd(4); + env(setup->vault.clawback( + {.issuer = setup->issuer, + .id = setup->vaultKeylet.key, + .holder = setup->holder, + .amount = clawbackAmount.value()})); + + auto const vaultSleAfter = env.current()->read(setup->vaultKeylet); + if (!BEAST_EXPECT(vaultSleAfter)) + return; + Number const finalAssetsTotal = vaultSleAfter->at(sfAssetsTotal); + Number const assetsRecovered = setup->initialAssetsTotal - finalAssetsTotal; + Number const clawbackNum = clawbackAmount.number(); + + Number const expectedPost{28LL, -1}; + Number const expectedPre{42LL, -1}; + if (withFix) + { + BEAST_EXPECT(assetsRecovered <= clawbackNum); + BEAST_EXPECT(assetsRecovered == expectedPost); + } + else + { + BEAST_EXPECT(assetsRecovered > clawbackNum); + BEAST_EXPECT(assetsRecovered == expectedPre); + } + }; + + { + testcase( + "bug: VaultClawback round-trip overshoot lets issuer recover " + "more than requested (pre-fixCleanup3_4_0)"); + runScenario(testableAmendments() - fixCleanup3_4_0, false); + } + { + testcase( + "bug: VaultClawback round-trip overshoot is clamped so " + "assetsRecovered <= clawbackAmount (post-fixCleanup3_4_0)"); + runScenario(testableAmendments(), true); + } + } + + // Same root cause as testBugClawbackRoundTripOvershoot on the + // withdraw path. Also bypasses the preclaim canWithdraw check, which + // validates destination limits against the requested amount only. + // + // Repro: assetsTotal=7, sharesTotal=5, request 4: + // pre-fix : shares = round(20/7) = 3, assets = 7*3/5 = 4.2 > 4. + // post-fix: shares = floor(20/7) = 2, assets = 7*2/5 = 2.8 <= 4. + void + testBugWithdrawRoundTripOvershoot() + { + using namespace test::jtx; + + auto runScenario = [this](FeatureBitset features, bool withFix) { + Env env{*this, features}; + + auto const setup = makeRoundTripOvershootVault(env); + if (!BEAST_EXPECT(setup)) + return; + + auto const requested = setup->usd(4); + env(setup->vault.withdraw( + {.depositor = setup->holder, + .id = setup->vaultKeylet.key, + .amount = requested.value()})); + + auto const vaultSleAfter = env.current()->read(setup->vaultKeylet); + if (!BEAST_EXPECT(vaultSleAfter)) + return; + Number const finalAssetsTotal = vaultSleAfter->at(sfAssetsTotal); + Number const assetsWithdrawn = setup->initialAssetsTotal - finalAssetsTotal; + Number const requestedNum = requested.number(); + + Number const expectedPost{28LL, -1}; + Number const expectedPre{42LL, -1}; + if (withFix) + { + BEAST_EXPECT(assetsWithdrawn <= requestedNum); + BEAST_EXPECT(assetsWithdrawn == expectedPost); + } + else + { + BEAST_EXPECT(assetsWithdrawn > requestedNum); + BEAST_EXPECT(assetsWithdrawn == expectedPre); + } + }; + + { + testcase( + "bug: VaultWithdraw round-trip overshoot delivers more than " + "requested (pre-fixCleanup3_4_0)"); + runScenario(testableAmendments() - fixCleanup3_4_0, false); + } + { + testcase( + "bug: VaultWithdraw round-trip overshoot is clamped so " + "assetsWithdrawn <= requested (post-fixCleanup3_4_0)"); + runScenario(testableAmendments(), true); + } + } + void testCredentialPinsPseudoAccount() { @@ -1101,6 +1339,8 @@ public: testCredentialPinsPseudoAccount(); testCredentialPinOverflow(); testBug6LimitBypassWithShares(); + testBugClawbackRoundTripOvershoot(); + testBugWithdrawRoundTripOvershoot(); } }; diff --git a/src/test/app/vault/VaultScale_test.cpp b/src/test/app/vault/VaultScale_test.cpp index 94c594f674..28c9729d78 100644 --- a/src/test/app/vault/VaultScale_test.cpp +++ b/src/test/app/vault/VaultScale_test.cpp @@ -546,13 +546,13 @@ private: } { - testcase("Scale withdraw with rounding shares up"); - // assetsToSharesWithdraw: - // shares = sharesTotal * (assets / assetsTotal) - // shares = 875 * 3.75 / 87.5 = 875 * 0.042857... = 37.5 - // sharesToAssetsWithdraw: - // assets = assetsTotal * (shares / sharesTotal) - // assets = 87.5 * 38 / 875 = 87.5 * 0.043428... = 3.8 + testcase("Scale withdraw with rounding shares up (truncated post-fixCleanup3_4_0)"); + // Pre-fixCleanup3_4_0: + // shares = round(875 * 3.75 / 87.5) = 38 + // assets = 87.5 * 38 / 875 = 3.8 > 3.75 requested. + // Post-fixCleanup3_4_0: + // shares = floor(37.5) = 37 + // assets = 87.5 * 37 / 875 = 3.7 <= 3.75 requested. auto const start = env.balance(d.depositor, d.assets).number(); auto tx = d.vault.withdraw( @@ -561,26 +561,23 @@ private: .amount = STAmount(d.asset, Number(375, -2))}); env(tx); env.close(); - BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(875 - 38)); + BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(875 - 37)); BEAST_EXPECT( env.balance(d.depositor, d.assets) == - STAmount(d.asset, start + Number(38, -1))); + STAmount(d.asset, start + Number(37, -1))); BEAST_EXPECT( env.balance(d.vaultAccount, d.assets) == - STAmount(d.asset, Number(875 - 38, -1))); + STAmount(d.asset, Number(875 - 37, -1))); BEAST_EXPECT( env.balance(d.vaultAccount, d.shares) == - STAmount(d.share, -Number(875 - 38, 0))); + STAmount(d.share, -Number(875 - 37, 0))); } { testcase("Scale withdraw with rounding shares down"); - // assetsToSharesWithdraw: - // shares = sharesTotal * (assets / assetsTotal) - // shares = 837 * 3.72 / 83.7 = 837 * 0.04444... = 37.2 - // sharesToAssetsWithdraw: - // assets = assetsTotal * (shares / sharesTotal) - // assets = 83.7 * 37 / 837 = 83.7 * 0.044205... = 3.7 + // Chained state: 838 shares outstanding, 83.8 assets. + // shares = floor(838 * 3.72 / 83.8) = floor(37.199...) = 37 + // assets = 83.8 * 37 / 838 = 3.7 <= 3.72 requested. auto const start = env.balance(d.depositor, d.assets).number(); auto tx = d.vault.withdraw( @@ -589,37 +586,37 @@ private: .amount = STAmount(d.asset, Number(372, -2))}); env(tx); env.close(); - BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(837 - 37)); + BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(838 - 37)); BEAST_EXPECT( env.balance(d.depositor, d.assets) == STAmount(d.asset, start + Number(37, -1))); BEAST_EXPECT( env.balance(d.vaultAccount, d.assets) == - STAmount(d.asset, Number(837 - 37, -1))); + STAmount(d.asset, Number(838 - 37, -1))); BEAST_EXPECT( env.balance(d.vaultAccount, d.shares) == - STAmount(d.share, -Number(837 - 37, 0))); + STAmount(d.share, -Number(838 - 37, 0))); } { - testcase("Scale withdraw tiny amount"); + testcase("Scale withdraw tiny amount rejected post-fixCleanup3_4_0"); + // Chained state: 801 shares outstanding, 80.1 assets. + // shares = floor(801 * 0.09 / 80.1) = floor(0.9) = 0 + // Zero shares => tecPRECISION_LOSS. State is unchanged. auto const start = env.balance(d.depositor, d.assets).number(); auto tx = d.vault.withdraw( {.depositor = d.depositor, .id = d.keylet.key, .amount = STAmount(d.asset, Number(9, -2))}); - env(tx); + env(tx, Ter{tecPRECISION_LOSS}); env.close(); - BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(800 - 1)); + BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(801)); + BEAST_EXPECT(env.balance(d.depositor, d.assets) == STAmount(d.asset, start)); BEAST_EXPECT( - env.balance(d.depositor, d.assets) == STAmount(d.asset, start + Number(1, -1))); + env.balance(d.vaultAccount, d.assets) == STAmount(d.asset, Number(801, -1))); BEAST_EXPECT( - env.balance(d.vaultAccount, d.assets) == - STAmount(d.asset, Number(800 - 1, -1))); - BEAST_EXPECT( - env.balance(d.vaultAccount, d.shares) == - STAmount(d.share, -Number(800 - 1, 0))); + env.balance(d.vaultAccount, d.shares) == STAmount(d.share, -Number(801, 0))); } { @@ -738,13 +735,13 @@ private: } { - testcase("Scale clawback with rounding shares up"); - // assetsToSharesWithdraw: - // shares = sharesTotal * (assets / assetsTotal) - // shares = 875 * 3.75 / 87.5 = 875 * 0.042857... = 37.5 - // sharesToAssetsWithdraw: - // assets = assetsTotal * (shares / sharesTotal) - // assets = 87.5 * 38 / 875 = 87.5 * 0.043428... = 3.8 + testcase("Scale clawback with rounding shares up (truncated post-fixCleanup3_4_0)"); + // Pre-fixCleanup3_4_0: + // shares = round(875 * 3.75 / 87.5) = 38 + // assets = 87.5 * 38 / 875 = 3.8 > 3.75 requested. + // Post-fixCleanup3_4_0: + // shares = floor(37.5) = 37 + // assets = 87.5 * 37 / 875 = 3.7 <= 3.75 requested. auto const start = env.balance(d.depositor, d.assets).number(); auto tx = d.vault.clawback( @@ -754,24 +751,21 @@ private: .amount = STAmount(d.asset, Number(375, -2))}); env(tx); env.close(); - BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(875 - 38)); + BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(875 - 37)); BEAST_EXPECT(env.balance(d.depositor, d.assets) == STAmount(d.asset, start)); BEAST_EXPECT( env.balance(d.vaultAccount, d.assets) == - STAmount(d.asset, Number(875 - 38, -1))); + STAmount(d.asset, Number(875 - 37, -1))); BEAST_EXPECT( env.balance(d.vaultAccount, d.shares) == - STAmount(d.share, -Number(875 - 38, 0))); + STAmount(d.share, -Number(875 - 37, 0))); } { testcase("Scale clawback with rounding shares down"); - // assetsToSharesWithdraw: - // shares = sharesTotal * (assets / assetsTotal) - // shares = 837 * 3.72 / 83.7 = 837 * 0.04444... = 37.2 - // sharesToAssetsWithdraw: - // assets = assetsTotal * (shares / sharesTotal) - // assets = 83.7 * 37 / 837 = 83.7 * 0.044205... = 3.7 + // Chained state: 838 shares outstanding, 83.8 assets. + // shares = floor(838 * 3.72 / 83.8) = floor(37.199...) = 37 + // assets = 83.8 * 37 / 838 = 3.7 <= 3.72 requested. auto const start = env.balance(d.depositor, d.assets).number(); auto tx = d.vault.clawback( @@ -781,18 +775,21 @@ private: .amount = STAmount(d.asset, Number(372, -2))}); env(tx); env.close(); - BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(837 - 37)); + BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(838 - 37)); BEAST_EXPECT(env.balance(d.depositor, d.assets) == STAmount(d.asset, start)); BEAST_EXPECT( env.balance(d.vaultAccount, d.assets) == - STAmount(d.asset, Number(837 - 37, -1))); + STAmount(d.asset, Number(838 - 37, -1))); BEAST_EXPECT( env.balance(d.vaultAccount, d.shares) == - STAmount(d.share, -Number(837 - 37, 0))); + STAmount(d.share, -Number(838 - 37, 0))); } { - testcase("Scale clawback tiny amount"); + testcase("Scale clawback tiny amount rejected post-fixCleanup3_4_0"); + // Chained state: 801 shares outstanding, 80.1 assets. + // shares = floor(801 * 0.09 / 80.1) = floor(0.9) = 0 + // Zero shares => tecPRECISION_LOSS. State is unchanged. auto const start = env.balance(d.depositor, d.assets).number(); auto tx = d.vault.clawback( @@ -800,16 +797,14 @@ private: .id = d.keylet.key, .holder = d.depositor, .amount = STAmount(d.asset, Number(9, -2))}); - env(tx); + env(tx, Ter{tecPRECISION_LOSS}); env.close(); - BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(800 - 1)); + BEAST_EXPECT(env.balance(d.depositor, d.shares) == d.share(801)); BEAST_EXPECT(env.balance(d.depositor, d.assets) == STAmount(d.asset, start)); BEAST_EXPECT( - env.balance(d.vaultAccount, d.assets) == - STAmount(d.asset, Number(800 - 1, -1))); + env.balance(d.vaultAccount, d.assets) == STAmount(d.asset, Number(801, -1))); BEAST_EXPECT( - env.balance(d.vaultAccount, d.shares) == - STAmount(d.share, -Number(800 - 1, 0))); + env.balance(d.vaultAccount, d.shares) == STAmount(d.share, -Number(801, 0))); } {