ARCHIVE/rippled
1
0
Fork 0
mirror of https://github.com/XRPLF/rippled.git synced 2026-09-28 15:58:07 +00:00
Code Issues Packages Projects Releases Wiki Activity

fix: Keep zero-withdraw destination deltas strict

Browse Source 
Remove the redundant present-delta exemption so an unsolicited empty holding cannot pass the vault invariant, and cover missing IOU trust lines plus the relevant pre-amendment MPT path.
This commit is contained in:
Vito
2026-09-02 10:37:12 +02:00
parent 81ace99a75
commit 5acab52b09
2 changed files with 151 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

8
src/libxrpl/tx/invariants/VaultInvariant.cpp
View File

@@ -1129,14 +1129,10 @@ ValidVault::finalize(
// only. If the receiver's trust line sits at a coarser scale, the inflow
// may safely round down to zero.
//
// XRP and MPT remain strict for rounding artifacts. A zero destination
// delta is still allowed when zeroDeltaIsLegitimate, to tolerate a
// one-sided rounded-zero change during a legitimate zero-value
// withdrawal.
// XRP and MPT remain strict for rounding artifacts.
bool const tolerateZeroDelta =
view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral();
auto const invalidBalanceChange =
(tolerateZeroDelta || zeroDeltaIsLegitimate)
auto const invalidBalanceChange = tolerateZeroDelta
? roundedDestinationDelta < kZero
: roundedDestinationDelta <= kZero;
if (invalidBalanceChange)

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

@@ -1680,9 +1680,10 @@ private:
// doWithdraw used to call addEmptyHolding for a self-destination even
// when the payout was zero, so a missing asset MPToken was created at
// amount 0. ValidVault then saw a one-sided zero destination delta and
// fired for integral assets; redeeming the last share in the same tx
// also created that token while deleting the share MPToken, which
// ValidMPTIssuance rejects (created + deleted > 1).
// fired for integral assets; redeeming Alice's last share in the same tx
// also created that token while deleting Alice's share MPToken, which
// ValidMPTIssuance rejects (created + deleted > 1). Bob still owns shares,
// so this is not the vault's final outstanding share.
//
// Post-fixCleanup3_4_0, doWithdraw skips addEmptyHolding on a zero
// payout. ValidVault also treats a one-sided zero destination delta as
@@ -1698,12 +1699,12 @@ private:
auto runScenario = [this](
FeatureBitset features,
bool removeAssetToken,
bool withdrawAllShares,
bool withdrawAllAliceShares,
TER expected) {
testcase(
std::string{"bug: MPT vault zero-value withdraw "} +
(removeAssetToken ? "without asset MPToken" : "with asset MPToken (control)") +
(withdrawAllShares ? ", last share" : ", leftover shares") +
(withdrawAllAliceShares ? ", Alice's last share" : ", Alice has leftover shares") +
(features[fixCleanup3_4_0] ? " (post-fixCleanup3_4_0)" : " (pre-fixCleanup3_4_0)"));
Env env(*this, features);
@@ -1792,7 +1793,7 @@ private:
return;
std::uint64_t const sharesBefore = tokenAlice->getFieldU64(sfMPTAmount);
BEAST_EXPECT(sharesBefore == 2);
std::uint64_t const sharesToRedeem = withdrawAllShares ? sharesBefore : 1;
std::uint64_t const sharesToRedeem = withdrawAllAliceShares ? sharesBefore : 1;
STAmount const redeemShares{MPTIssue{shareId}, Number(sharesToRedeem)};
auto const assetTokenKeylet = keylet::mptoken(mptt.issuanceID(), alice.id());
@@ -1832,7 +1833,7 @@ private:
}
auto const shareAfter = env.le(keylet::mptoken(shareId, alice.id()));
if (withdrawAllShares)
if (withdrawAllAliceShares)
{
BEAST_EXPECT(!shareAfter);
}
@@ -1856,15 +1857,150 @@ private:
outstandingBefore - sharesToRedeem);
};
runScenario(all_, false /* removeAssetToken */, false /* withdrawAllShares */, tesSUCCESS);
runScenario(all_, false /* removeAssetToken */, true /* withdrawAllShares */, tesSUCCESS);
runScenario(all_, true /* removeAssetToken */, false /* withdrawAllShares */, tesSUCCESS);
runScenario(all_, true /* removeAssetToken */, true /* withdrawAllShares */, tesSUCCESS);
runScenario(
all_, false /* removeAssetToken */, false /* withdrawAllAliceShares */, tesSUCCESS);
runScenario(
all_, false /* removeAssetToken */, true /* withdrawAllAliceShares */, tesSUCCESS);
runScenario(
all_, true /* removeAssetToken */, false /* withdrawAllAliceShares */, tesSUCCESS);
runScenario(
all_, true /* removeAssetToken */, true /* withdrawAllAliceShares */, tesSUCCESS);
runScenario(
all_ - fixCleanup3_4_0,
true /* removeAssetToken */,
true /* withdrawAllShares */,
false /* withdrawAllAliceShares */,
tecINVARIANT_FAILED);
runScenario(
all_ - fixCleanup3_4_0,
true /* removeAssetToken */,
true /* withdrawAllAliceShares */,
tecINVARIANT_FAILED);
}
// IOU analogue of the missing-MPToken case above. Alice removes her
// zero-balance trust line after depositing, then burns one of her two
// shares after the vault is fully impaired. Bob's eight shares keep this
// out of the sole-shareholder loss-waiver and final-outstanding-share
// paths. A zero payout must not recreate Alice's unsolicited trust line.
void
testBugIouZeroWithdrawMissingTrustLine()
{
using namespace test::jtx;
using namespace loan_broker;
using namespace loan;
using namespace std::chrono_literals;
Env env(*this, all_);
Account const issuer{"issuer"};
Account const owner{"owner"};
Account const alice{"alice"};
Account const bob{"bob"};
Account const borrower{"borrower"};
env.fund(XRP(100'000), issuer, owner, alice, bob, borrower);
env.close();
env(fset(issuer, asfDefaultRipple));
env.close();
PrettyAsset const asset = issuer["USD"];
env.trust(asset(100), owner);
env.trust(asset(100), alice);
env.trust(asset(100), bob);
env.trust(asset(100), borrower);
env.close();
env(pay(issuer, alice, asset(2)));
env(pay(issuer, bob, asset(8)));
env.close();
Vault const vault{env};
auto const [createTx, vaultKeylet, subscriptionDate] =
vault.createClosedEnded({.owner = owner, .asset = asset, .subscriptionOffset = 60s});
env(createTx);
env.close();
env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = asset(2)}));
env(vault.deposit({.depositor = bob, .id = vaultKeylet.key, .amount = asset(8)}));
env.close();
auto const assetLine = keylet::trustLine(alice, asset.raw().get<Issue>());
if (!BEAST_EXPECT(env.le(assetLine)))
return;
env.trust(asset(0), alice);
env.close();
BEAST_EXPECT(!env.le(assetLine));
vault.closePastSubscription(subscriptionDate);
auto const brokerKeylet =
keylet::loanBroker(owner.id(), SeqProxy::rawSequence(env.seq(owner)));
env(set(owner, vaultKeylet.key));
env.close();
auto const sleBroker = env.le(brokerKeylet);
if (!BEAST_EXPECT(sleBroker))
return;
auto const loanKeylet =
keylet::loan(brokerKeylet.key, SeqProxy::rawSequence(sleBroker->at(sfLoanSequence)));
env(set(borrower, brokerKeylet.key, asset(10).value()),
kInterestRate(percentageToTenthBips(0)),
kGracePeriod(60),
kPaymentInterval(120),
kPaymentTotal(10),
Sig(sfCounterpartySignature, owner),
Fee(env.current()->fees().base * 2),
Ter(tesSUCCESS));
env.close();
auto const loanBefore = env.le(loanKeylet);
if (!BEAST_EXPECT(loanBefore))
return;
std::uint32_t const dueDate = loanBefore->at(sfNextPaymentDueDate);
env.close(NetClock::time_point{NetClock::duration{dueDate}} + 1s);
env(manage(owner, loanKeylet.key, tfLoanImpair), Ter(tesSUCCESS));
env.close();
auto const vaultImpaired = env.le(vaultKeylet);
if (!BEAST_EXPECT(vaultImpaired))
return;
BEAST_EXPECT(vaultImpaired->at(sfAssetsAvailable) == asset(0).value());
BEAST_EXPECT(vaultImpaired->at(sfAssetsTotal) == vaultImpaired->at(sfLossUnrealized));
Number const totalBefore = vaultImpaired->at(sfAssetsTotal);
Number const lossBefore = vaultImpaired->at(sfLossUnrealized);
MPTID const shareId = vaultImpaired->at(sfShareMPTID);
auto const tokenAlice = env.le(keylet::mptoken(shareId, alice.id()));
if (!BEAST_EXPECT(tokenAlice))
return;
std::uint64_t const sharesBefore = tokenAlice->getFieldU64(sfMPTAmount);
BEAST_EXPECT(sharesBefore == 2);
STAmount const redeemShares{MPTIssue{shareId}, Number(1)};
std::uint32_t const redemptionDate = vaultImpaired->at(sfRedemptionDate);
env.close(NetClock::time_point{NetClock::duration{redemptionDate}} + 1s);
env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = redeemShares}),
Ter(tesSUCCESS));
env.close();
// A regression in the View guard would recreate this line even though
// no asset value was paid.
BEAST_EXPECT(!env.le(assetLine));
auto const shareAfter = env.le(keylet::mptoken(shareId, alice.id()));
if (!BEAST_EXPECT(shareAfter))
return;
BEAST_EXPECT(shareAfter->getFieldU64(sfMPTAmount) == sharesBefore - 1);
auto const vaultAfter = env.le(vaultKeylet);
if (!BEAST_EXPECT(vaultAfter))
return;
BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == totalBefore);
BEAST_EXPECT(vaultAfter->at(sfLossUnrealized) == lossBefore);
BEAST_EXPECT(vaultAfter->at(sfAssetsAvailable) == asset(0).value());
}
// Same zero-payout withdrawal as testBugMptZeroWithdrawMissingHolding, but
@@ -2353,6 +2489,7 @@ public:
testBugWithdrawRoundTripOvershoot();
testBugClawbackAfterLoanImpair();
testBugMptZeroWithdrawMissingHolding();
testBugIouZeroWithdrawMissingTrustLine();
testBugXrpZeroWithdrawSponsoredFee();
testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient();
testBugSponsorAsDestinationFeeMisappliedToPayout();