fix: Allow zero-value MPT vault withdraw when the asset holding is missing (#8153)

This commit is contained in:
Vito Tumas
2026-09-02 13:45:52 +00:00
committed by GitHub
parent 8809bdf3f0
commit 346ea40f69
3 changed files with 505 additions and 7 deletions

View File

@@ -543,12 +543,19 @@ doWithdraw(
{
auto const dstSle = ctx.view.read(keylet::account(dstAcct));
// Create trust line or MPToken for the receiving account
// Create a trust line or MPToken for a self-destination only when there
// is a payout to credit. Post-fixCleanup3_4_0, a zero-value withdraw
// (e.g. share redemption from a fully impaired vault) must not insert
// an empty holding: that records a one-sided zero delta and can also
// create+delete MPTokens in the same transaction.
if (dstAcct == senderAcct)
{
if (auto const ter = addEmptyHolding(ctx, senderAcct, priorBalance, amount.asset(), j);
!isTesSuccess(ter) && ter != tecDUPLICATE)
return ter;
if (amount > beast::kZero || !ctx.view.rules().enabled(fixCleanup3_4_0))
{
if (auto const ter = addEmptyHolding(ctx, senderAcct, priorBalance, amount.asset(), j);
!isTesSuccess(ter) && ter != tecDUPLICATE)
return ter;
}
}
else
{

View File

@@ -1129,9 +1129,7 @@ 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. Because they are integer-exact, a zero
// destination delta indicates a true accounting bug, not a rounding
// artifact.
// XRP and MPT remain strict for rounding artifacts.
bool const tolerateZeroDelta =
view.rules().enabled(fixCleanup3_2_0) && !vaultAsset.integral();
auto const invalidBalanceChange = tolerateZeroDelta

View File

@@ -7,6 +7,7 @@
#include <test/jtx/credentials.h>
#include <test/jtx/fee.h>
#include <test/jtx/flags.h>
#include <test/jtx/mpt.h>
#include <test/jtx/pay.h>
#include <test/jtx/permissioned_domains.h>
#include <test/jtx/sig.h>
@@ -1677,6 +1678,495 @@ private:
}
}
// Bug: a fully impaired vault may pay zero assets for a share burn.
// Sending zero MPT is a no-op, so the vault pseudo-account's asset
// MPToken is never written and ValidVault, which only records deltas for
// created, modified or deleted entries, sees no vault delta at all.
//
// Pre-fixCleanup3_4_0 that alone makes the withdrawal impossible:
// zeroDeltaIsLegitimate is gated on the amendment, so the absent vault
// delta fails "withdrawal must change vault balance". Every pre-amendment
// arm below dies there, before any destination-side check runs.
//
// The destination side differs per arm, and only the vault-delta return
// hides that pre-amendment. With Alice's asset MPToken already present
// nothing touches it, so she has no delta either. With it missing,
// doWithdraw still called addEmptyHolding for a self-destination on a
// zero payout and created her MPToken at amount 0; a created MPToken is
// recorded even at zero, so she arrives with a present-and-zero delta,
// which for an integral MPT asset the destination check would reject if
// it were reached.
//
// ValidMPTIssuance is a separate checker and still runs. It only trips on
// the one arm that both creates and deletes an MPToken: Alice's last
// share with the asset MPToken missing, where addEmptyHolding creates the
// asset token while her share token is deleted (created + deleted > 1).
// Leftover shares with the token missing is create-only, and a last share
// with the token present is delete-only; neither exceeds one. Bob still
// owns shares throughout, so this is never the vault's final outstanding
// share.
//
// Post-fixCleanup3_4_0, doWithdraw skips addEmptyHolding on a zero
// payout and zeroDeltaIsLegitimate lets the vault-delta and
// missing-recipient-delta checks accept the transfer. A present
// destination delta of zero is still rejected.
void
testBugMptZeroWithdrawMissingHolding()
{
using namespace test::jtx;
using namespace loan_broker;
using namespace loan;
using namespace std::chrono_literals;
auto runScenario = [this](
FeatureBitset features,
bool removeAssetToken,
bool withdrawAllAliceShares,
TER expected) {
testcase(
std::string{"bug: MPT vault zero-value withdraw "} +
(removeAssetToken ? "without asset MPToken" : "with asset MPToken") +
(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);
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();
MPTTester mptt{env, issuer, kMptInitNoFund};
mptt.create({.flags = tfMPTCanTransfer});
PrettyAsset const asset = mptt.issuanceID();
mptt.authorize({.account = owner});
mptt.authorize({.account = alice});
mptt.authorize({.account = bob});
mptt.authorize({.account = 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();
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 issuanceBefore = env.le(keylet::mptokenIssuance(shareId));
if (!BEAST_EXPECT(issuanceBefore))
return;
std::uint64_t const outstandingBefore =
issuanceBefore->getFieldU64(sfOutstandingAmount);
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);
std::uint64_t const sharesToRedeem = withdrawAllAliceShares ? sharesBefore : 1;
STAmount const redeemShares{MPTIssue{shareId}, Number(sharesToRedeem)};
auto const assetTokenKeylet = keylet::mptoken(mptt.issuanceID(), alice.id());
if (removeAssetToken)
{
mptt.authorize({.account = alice, .flags = tfMPTUnauthorize});
env.close();
BEAST_EXPECT(!env.le(assetTokenKeylet));
}
else
{
auto const existing = env.le(assetTokenKeylet);
if (!BEAST_EXPECT(existing))
return;
BEAST_EXPECT(existing->getFieldU64(sfMPTAmount) == 0);
}
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(expected));
env.close();
if (expected != tesSUCCESS)
return;
if (removeAssetToken)
{
BEAST_EXPECT(!env.le(assetTokenKeylet));
}
else
{
auto const assetAfter = env.le(assetTokenKeylet);
if (!BEAST_EXPECT(assetAfter))
return;
BEAST_EXPECT(assetAfter->getFieldU64(sfMPTAmount) == 0);
}
auto const shareAfter = env.le(keylet::mptoken(shareId, alice.id()));
if (withdrawAllAliceShares)
{
BEAST_EXPECT(!shareAfter);
}
else if (BEAST_EXPECT(shareAfter))
{
BEAST_EXPECT(shareAfter->getFieldU64(sfMPTAmount) == sharesBefore - sharesToRedeem);
}
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());
auto const issuanceAfter = env.le(keylet::mptokenIssuance(shareId));
if (!BEAST_EXPECT(issuanceAfter))
return;
BEAST_EXPECT(
issuanceAfter->getFieldU64(sfOutstandingAmount) ==
outstandingBefore - sharesToRedeem);
};
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,
false /* removeAssetToken */,
false /* withdrawAllAliceShares */,
tecINVARIANT_FAILED);
runScenario(
all_ - fixCleanup3_4_0,
false /* removeAssetToken */,
true /* withdrawAllAliceShares */,
tecINVARIANT_FAILED);
runScenario(
all_ - fixCleanup3_4_0,
true /* removeAssetToken */,
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 unit from her
// scaled share balance after the vault is fully impaired. Bob's share
// balance keeps 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);
// Default IOU vault scale is 6, so 2 USD mints 2e6 shares. Redeem one
// leftover share; do not require 1:1 like the MPT case.
BEAST_EXPECT(sharesBefore > 1);
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
// the vault asset is XRP. addEmptyHolding is a no-op for native assets.
// Sequence processing still touches the sender AccountRoot; a sponsored
// fee leaves that XRP balance economically unchanged. After the
// sponsored-withdraw fee-payer fix, deltaAssetsForParty collapses that
// economically-zero XRP delta to absence, so tesSUCCESS takes the
// missing-recipient-delta arm gated by zeroDeltaIsLegitimate. This test
// covers that live SUCCESS path. Pre-fixCleanup3_4_0 still fails the
// invariant.
void
testBugXrpZeroWithdrawSponsoredFee()
{
using namespace test::jtx;
using namespace loan_broker;
using namespace loan;
using namespace std::chrono_literals;
auto runScenario = [this](FeatureBitset features, TER expected) {
testcase(
std::string{"bug: XRP vault zero-value withdraw with sponsored fee"} +
(features[fixCleanup3_4_0] ? " (post-fixCleanup3_4_0)" : " (pre-fixCleanup3_4_0)"));
Env env(*this, features);
Account const owner{"owner"};
Account const alice{"alice"};
Account const bob{"bob"};
Account const borrower{"borrower"};
Account const sponsor{"sponsor"};
env.fund(XRP(100'000), owner, alice, bob, borrower, sponsor);
env.close();
PrettyAsset const asset{xrpIssue()};
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();
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);
auto const aliceBalanceBefore = env.balance(alice);
auto const sponsorBalanceBefore = env.balance(sponsor);
auto const fee = env.current()->fees().base;
env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = redeemShares}),
Fee(fee),
sponsor::As(sponsor, spfSponsorFee),
Sig(sfSponsorSignature, sponsor),
Ter(expected));
env.close();
BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore - fee);
BEAST_EXPECT(env.balance(alice) == aliceBalanceBefore);
if (expected != tesSUCCESS)
return;
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());
};
runScenario(all_, tesSUCCESS);
runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED);
}
// addEmptyHolding() used to check isGlobalFrozen(issuer) and
// !lsfDefaultRipple before the "line already exists" tecDUPLICATE
// short circuit. doWithdraw() calls addEmptyHolding() for a
@@ -2383,6 +2873,9 @@ public:
testBugClawbackRoundTripOvershoot();
testBugWithdrawRoundTripOvershoot();
testBugClawbackAfterLoanImpair();
testBugMptZeroWithdrawMissingHolding();
testBugIouZeroWithdrawMissingTrustLine();
testBugXrpZeroWithdrawSponsoredFee();
testBugSelfWithdrawAfterIssuerClearsDefaultRipple();
testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient();
testBugSponsorAsDestinationFeeMisappliedToPayout();