ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-10-04 02:38:02 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Prevent vault clawback and withdraw overrun (#8075)

Browse Source
This commit is contained in:
Vito Tumas
2026-08-25 17:59:37 +00:00
committed by GitHub
parent 9e2aaf6f60
commit 5e3d20b3ed
4 changed files with 310 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

9
src/libxrpl/tx/transactors/vault/VaultClawback.cpp
View File

@@ -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;

13
src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp
View File

@@ -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;

240
src/test/app/vault/VaultBugs_test.cpp
View File

@@ -21,6 +21,7 @@
#include <xrpl/protocol/Feature.h>
#include <xrpl/protocol/Indexes.h>
#include <xrpl/protocol/Issue.h>
#include <xrpl/protocol/Keylet.h>
#include <xrpl/protocol/MPTIssue.h>
#include <xrpl/protocol/Protocol.h>
#include <xrpl/protocol/SField.h>
@@ -32,6 +33,7 @@
#include <chrono>
#include <cstdint>
#include <memory>
#include <optional>
#include <string>
#include <tuple>
#include <utility>
@@ -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<RoundTripOvershootVault>
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();
}
};

105
src/test/app/vault/VaultScale_test.cpp
View File

@@ -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)));
}
{