From ccd5dc5e06b3fcf36b45ee370707355290ee8d9d Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:40:53 +0000 Subject: [PATCH 1/2] fix: Keep LoanBrokerDelete valid for auth-required MPT cover (#8144) --- include/xrpl/tx/invariants/MPTInvariant.h | 7 ++ src/libxrpl/tx/invariants/MPTInvariant.cpp | 21 ++++- src/test/app/lending/LoanBroker_test.cpp | 101 ++++++++++++++++++++- 3 files changed, 125 insertions(+), 4 deletions(-) diff --git a/include/xrpl/tx/invariants/MPTInvariant.h b/include/xrpl/tx/invariants/MPTInvariant.h index 5740cd5be2..ddda348d2e 100644 --- a/include/xrpl/tx/invariants/MPTInvariant.h +++ b/include/xrpl/tx/invariants/MPTInvariant.h @@ -215,6 +215,13 @@ class ValidMPTTransfer // Deleted MPToken // MPToken key: true if MPTAuthorized is set hash_map deletedAuthorized_; + // Every touched AccountRoot (not only pseudos): + // AccountID -> whether it was a pseudo-account BEFORE this transaction + // applied. Needed because a transaction may erase a pseudo-account and + // move MPT out of it in the same transaction; by finalize() time the + // view no longer shows it as a pseudo-account (or as existing at all). + // False entries freeze the pre-tx classification for touched non-pseudos. + hash_map pseudoAccountsBefore_; public: /** diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 66b9028ed2..09b4308165 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/src/libxrpl/tx/invariants/MPTInvariant.cpp @@ -832,6 +832,14 @@ ValidMPTTransfer::visitEntry( if (after) update(*after, false); + + // Record whether every touched AccountRoot was a pseudo-account BEFORE + // the transaction applied (true and false). A transaction that erases a + // pseudo-account (and moves MPT out of it) in the same transaction leaves + // no trace of its pseudo-account status in the post-transaction view + // isAuthorized() sees at finalize() time. + if (before && before->getType() == ltACCOUNT_ROOT) + pseudoAccountsBefore_[before->at(sfAccount)] = isPseudoAccount(before); } bool @@ -844,10 +852,19 @@ ValidMPTTransfer::isAuthorized( // Pseudo-accounts (Vault, LoanBroker, AMM) hold assets on behalf of their // participants and are implicitly authorized for any MPT they hold, // including vault shares whose underlying asset would otherwise require - // auth. Exempt them here rather than relying on requireAuth: the recursive + // auth. Exempt them here rather than relying on requireAuth: the recursive // share -> underlying descent in requireAuth fails for a pseudo-account // that holds the share but not the underlying. - if (isPseudoAccount(view, holder)) + // + // Use the pre-transaction classification for any account this + // transaction touched (pseudoAccountsBefore_): the post-transaction view + // is wrong for an account this same transaction erased. Untouched + // accounts aren't in the map, so fall back to the current view, which is + // still accurate for them since nothing changed. + auto const pseudoIt = pseudoAccountsBefore_.find(holder); + bool const isPseudo = + pseudoIt != pseudoAccountsBefore_.end() ? pseudoIt->second : isPseudoAccount(view, holder); + if (isPseudo) return true; auto const key = keylet::mptoken(mptid, holder); diff --git a/src/test/app/lending/LoanBroker_test.cpp b/src/test/app/lending/LoanBroker_test.cpp index d75b359868..5b3ea854f8 100644 --- a/src/test/app/lending/LoanBroker_test.cpp +++ b/src/test/app/lending/LoanBroker_test.cpp @@ -1849,6 +1849,96 @@ class LoanBroker_test : public beast::unit_test::Suite BEAST_EXPECT(aliceBalanceAfter == aliceBalanceBefore); } + void + testLoanBrokerDeleteRequireAuthMPT(FeatureBitset features) + { + testcase << "LoanBrokerDelete - auth-required broker pseudo-account MPT " + << (features[fixCleanup3_4_0] ? "post-fix" : "pre-fix"); + using namespace jtx; + using namespace loan_broker; + + Account const issuer("issuer"); + Account const alice("alice"); + + Env env(*this, features); + env.fund(XRP(100'000), issuer, alice); + env.close(); + + // Create an auth-required MPT and authorize alice as a holder. The + // broker pseudo-account's cover MPToken is auto-created later + // (addEmptyHolding -> authorizeMPToken) with lsfMPTAuthorized clear; + // the pseudo-account is implicitly authorized to hold any MPT + // regardless of that flag. + auto tester = MPTTester( + {.env = env, + .issuer = issuer, + .holders = {alice}, + .pay = 20'000, + .flags = tfMPTRequireAuth | tfMPTCanTransfer, + .authHolder = true}); + + PrettyAsset const mpt{tester.issuanceID()}; + + // Create vault + Vault const vault{env}; + auto [tx, vaultKeylet] = vault.create({.owner = alice, .asset = mpt}); + env(tx); + env.close(); + + // Deposit into vault + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = mpt(10'000)})); + env.close(); + + // Create loan broker + auto const brokerKeylet = + keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice))); + env(set(alice, vaultKeylet.key)); + env.close(); + + // Deposit cover + env(coverDeposit(alice, brokerKeylet.key, mpt(5'000).value())); + env.close(); + + // Verify cover is deposited + auto const broker = env.le(brokerKeylet); + if (!BEAST_EXPECT(broker)) + return; + BEAST_EXPECT(broker->at(sfCoverAvailable) > 0); + + // Get the broker pseudo-account + auto const brokerPseudoID = broker->at(sfAccount); + + // Verify the broker pseudo-account has an MPToken, and that it was + // never explicitly authorized (issuer cannot authorize a + // pseudo-account holder; see MPTokenAuthorize::preclaim). + auto const pseudoMptKey = keylet::mptoken(tester.issuanceID(), brokerPseudoID); + auto const pseudoMpt = env.le(pseudoMptKey); + if (!BEAST_EXPECT(pseudoMpt)) + return; + BEAST_EXPECT(!pseudoMpt->isFlag(lsfMPTAuthorized)); + + // Record alice's balance before deletion + auto const aliceBalanceBefore = env.balance(alice, mpt); + + // LoanBrokerDelete sends the remaining cover out of the broker pseudo-account, deletes its + // now-empty MPToken, and erases the pseudo AccountRoot. Before the fix, + // ValidMPTTransfer::isAuthorized evaluates isPseudoAccount() on the post-transaction view + // (where the pseudo-account is already gone) and falls back to the MPToken's + // lsfMPTAuthorized flag, which was never set, so the invariant treats the broker as an + // unauthorized sender and the whole transaction fails once fixCleanup3_4_0 makes the check + // enforcing. + env(del(alice, brokerKeylet.key), Ter(tesSUCCESS)); + env.close(); + + // Broker and its pseudo-account MPToken are gone + BEAST_EXPECT(env.le(brokerKeylet) == nullptr); + BEAST_EXPECT(env.le(pseudoMptKey) == nullptr); + + // Alice received the cover + auto const aliceBalanceAfter = env.balance(alice, mpt); + BEAST_EXPECT(aliceBalanceAfter > aliceBalanceBefore); + } + void testCoverDepositFreezes() { @@ -2550,7 +2640,7 @@ class LoanBroker_test : public beast::unit_test::Suite using namespace jtx; using namespace std::chrono_literals; - bool const fixEnabled = features[fixCleanup3_4_0]; + bool const fix340Enabled = features[fixCleanup3_4_0]; Env env(*this, features); @@ -2611,7 +2701,7 @@ class LoanBroker_test : public beast::unit_test::Suite env(coverWithdrawToDest(), loan_broker::kDestination(dest), Ter{tecNO_PERMISSION}); env.close(); - if (!fixEnabled) + if (!fix340Enabled) { // Pre-fix: sfCredentialIDs in LoanBrokerCoverWithdraw is disabled env(coverWithdrawToDest(), @@ -3035,6 +3125,13 @@ public: testLoanBrokerDeleteFrozenIOU(all_); testLoanBrokerDeleteFrozenIOU(all_ - fixCleanup3_2_0); + + // featureMPTokensV2 independently makes ValidMPTTransfer enforcing, + // but it's Supported::No (never enabled on real networks); exclude + // it here so fixCleanup3_4_0 alone is the deciding amendment, as it + // would be on mainnet. + testLoanBrokerDeleteRequireAuthMPT(all_ - featureMPTokensV2); + testLoanBrokerDeleteRequireAuthMPT(all_ - featureMPTokensV2 - fixCleanup3_4_0); // TODO: Write clawback failure tests with an issuer / MPT that doesn't // have the right flags set. } From b3b38e4416c59b57060af701e76212b939ea7527 Mon Sep 17 00:00:00 2001 From: Vito Tumas <5780819+Tapanito@users.noreply.github.com> Date: Tue, 1 Sep 2026 14:43:11 +0000 Subject: [PATCH 2/2] fix: Correct fee-payer XRP delta in ValidVault for sponsored VaultWithdraw (#8141) --- include/xrpl/tx/Transactor.h | 10 +- include/xrpl/tx/invariants/VaultInvariant.h | 55 ++- src/libxrpl/tx/invariants/VaultInvariant.cpp | 103 ++++-- src/test/app/Sponsor_test.cpp | 6 +- src/test/app/vault/VaultBugs_test.cpp | 339 +++++++++++++++++++ 5 files changed, 474 insertions(+), 39 deletions(-) diff --git a/include/xrpl/tx/Transactor.h b/include/xrpl/tx/Transactor.h index 96ad7e00bc..aabde69ff9 100644 --- a/include/xrpl/tx/Transactor.h +++ b/include/xrpl/tx/Transactor.h @@ -259,6 +259,13 @@ public: static XRPAmount calculateBaseFee(ReadView const& view, STTx const& tx, std::uint32_t extraBaseFeeMultiplier); + // Exposed for invariant checks (e.g. ValidVault) that need to know which + // ledger entry actually pays a transaction's fee, distinguishing an + // ordinary sender, a delegate, and pre-funded vs. co-signed fee + // sponsorship. + static FeePayer + getFeePayer(ReadView const& view, STTx const& tx); + /* Do NOT define an invokePreflight function in a derived class. Instead, define: @@ -525,9 +532,6 @@ private: std::pair reset(XRPAmount fee); - static FeePayer - getFeePayer(ReadView const& view, STTx const& tx); - TER consumeSeqProxy(SLE::pointer const& sleAccount); TER diff --git a/include/xrpl/tx/invariants/VaultInvariant.h b/include/xrpl/tx/invariants/VaultInvariant.h index efeec7fda6..8297b941d1 100644 --- a/include/xrpl/tx/invariants/VaultInvariant.h +++ b/include/xrpl/tx/invariants/VaultInvariant.h @@ -131,20 +131,57 @@ private: deltaAssets(AccountID const& id) const; /** - * @brief Return the vault-asset delta for the transaction's sending - * account, adjusted for the fee. + * @brief Return the AccountRoot whose XRP balance actually absorbed a + * transaction's fee, if any. * - * Calls @c deltaAssets for @c tx[sfAccount] and, for non-delegated XRP - * transactions, adds the consumed fee back so the invariant sees the net - * asset movement rather than the fee-reduced balance change. + * Mirrors @c Transactor::getFeePayer, but resolves to @c std::nullopt for + * a pre-funded sponsorship: that fee is drawn from the @c ltSponsorship + * object's @c sfFeeAmount, never from the sponsor's own AccountRoot, so + * there is no balance to add back there. * - * @param tx The transaction being applied. - * @param fee Fee charged by this transaction. + * @param view Read-only view of the ledger after the transaction. + * @param tx The transaction being applied. + * @return The fee-paying AccountRoot's id, or @c std::nullopt when the + * fee was not drawn from any AccountRoot balance. + */ + [[nodiscard]] static std::optional + feePayerAccountRoot(ReadView const& view, STTx const& tx); + + /** + * @brief Return the vault-asset delta for a party inspected as a + * withdrawal/deposit counterparty, adjusted for the fee. + * + * Calls @c deltaAssets for @p id and, for XRP transactions, adds the + * consumed fee back only when @p id is the AccountRoot that actually + * paid it (per @c feePayerAccountRoot) -- so the invariant sees the net + * asset movement rather than a fee-reduced balance change, regardless of + * whether @p id is the sender, a distinct destination, a delegate, or a + * co-signed fee sponsor. Post-@c fixCleanup3_4_0, any resulting + * economically-zero delta is always normalized to absence. + * + * Pre-@c fixCleanup3_4_0 this replicates the legacy behaviour exactly: + * only @c tx[sfAccount] could ever receive a fee correction (and only + * when it was itself, per @c STTx::getFeePayerID, the fee payer). After + * that sender-only correction a zero delta is collapsed to absence; if + * the correction does not apply, a present-zero delta is kept as-is. + * + * @param view Read-only view of the ledger after the transaction. + * @param id Account being inspected as sender or destination. + * @param tx The transaction being applied. + * @param fee Fee charged by this transaction. + * @param fix340Enabled Whether @c fixCleanup3_4_0 is enabled, as already + * determined once by @c finalize. * @return The fee-adjusted delta, or @c std::nullopt if the net delta is - * zero or the account entry was not touched. + * zero (always post-amendment; pre-amendment only after the + * sender-only fee correction) or the entry was not touched. */ [[nodiscard]] std::optional - deltaAssetsTxAccount(STTx const& tx, XRPAmount fee) const; + deltaAssetsForParty( + ReadView const& view, + AccountID const& id, + STTx const& tx, + XRPAmount fee, + bool fix340Enabled) const; /** * @brief Return the vault-share balance-change delta for an account. diff --git a/src/libxrpl/tx/invariants/VaultInvariant.cpp b/src/libxrpl/tx/invariants/VaultInvariant.cpp index c0f98a8128..ff3a20c8ff 100644 --- a/src/libxrpl/tx/invariants/VaultInvariant.cpp +++ b/src/libxrpl/tx/invariants/VaultInvariant.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include @@ -235,21 +236,57 @@ ValidVault::deltaAssets(AccountID const& id) const vaultAsset.value()); } +std::optional +ValidVault::feePayerAccountRoot(ReadView const& view, STTx const& tx) +{ + auto const feePayer = Transactor::getFeePayer(view, tx); + if (feePayer.type == FeePayerType::SponsorPreFunded) + return std::nullopt; + return feePayer.id; +} + std::optional -ValidVault::deltaAssetsTxAccount(STTx const& tx, XRPAmount fee) const +ValidVault::deltaAssetsForParty( + ReadView const& view, + AccountID const& id, + STTx const& tx, + XRPAmount fee, + bool fix340Enabled) const { auto const& vaultAsset = afterVault_[0].asset; - auto ret = deltaAssets(tx[sfAccount]); + auto ret = deltaAssets(id); if (!ret.has_value() || !vaultAsset.native()) return ret; - // Only add the fee back if tx[sfAccount] actually paid it. When the fee is - // paid by someone else (a delegate or a fee sponsor), the - // account's XRP balance moved only by the vault amount. - if (tx.getFeePayerID() != tx[sfAccount]) - return ret; + if (!fix340Enabled) + { + // Legacy behaviour: only tx[sfAccount] was ever considered for a fee + // correction, and only when STTx::getFeePayerID identified it as the + // fee payer (which is never true for a sponsor, since + // self-sponsorship is disallowed). After that sender-only correction + // a zero delta is collapsed to absence; if the correction does not + // apply, a present-zero is returned as-is. + if (id != tx[sfAccount] || tx.getFeePayerID() != id) + return ret; - ret->delta += fee.drops(); + ret->delta += fee.drops(); + if (ret->delta == kZero) + return std::nullopt; + + return ret; + } + + // Add the fee back only onto the AccountRoot that actually paid it: an + // ordinary sender, a delegate, or a co-signed fee sponsor -- but never a + // pre-funded sponsorship, whose fee is drawn from the ltSponsorship + // object rather than the sponsor's own XRP balance. + if (auto const payer = feePayerAccountRoot(view, tx); payer && *payer == id) + ret->delta += fee.drops(); + + // Normalize an economically zero delta to absence regardless of who (if + // anyone) paid the fee, so a touched-but-unchanged AccountRoot (e.g. the + // sender in a third-party withdrawal, touched only for sequence/ticket + // processing) is never misread as a second payout recipient. if (ret->delta == kZero) return std::nullopt; @@ -860,7 +897,8 @@ ValidVault::finalize( if (!issuerDeposit) { - auto const maybeAccDeltaAssets = deltaAssetsTxAccount(tx, fee); + auto const maybeAccDeltaAssets = + deltaAssetsForParty(view, tx[sfAccount], tx, fee, fix340Enabled); if (!maybeAccDeltaAssets) { JLOG(j.fatal()) @@ -1033,21 +1071,39 @@ ValidVault::finalize( if (!issuerWithdrawal) { - auto const maybeAccDelta = deltaAssetsTxAccount(tx, fee); - auto const maybeOtherAccDelta = [&]() -> std::optional { - if (auto const destination = tx[~sfDestination]; - destination && *destination != tx[sfAccount]) - return deltaAssets(*destination); - return std::nullopt; - }(); + // Identify the intended recipient explicitly from + // sfDestination (falling back to sfAccount for a + // self-withdrawal), rather than inferring it from which + // side happens to show a delta. When a distinct + // destination is named, the sending account must not + // also show a real economic delta -- that would mean two + // accounts were paid, which is always a bug, regardless + // of what (if anything) the named destination received. + auto const destinationField = tx[~sfDestination]; + AccountID const recipient = destinationField.value_or(tx[sfAccount]); + bool const distinctDestination = + destinationField.has_value() && *destinationField != tx[sfAccount]; - if (maybeAccDelta.has_value() == maybeOtherAccDelta.has_value()) + // Intentionally ungated: `fix340Enabled &&` here would let the + // pre-amendment sponsored case succeed and change consensus. + if (distinctDestination && + deltaAssetsForParty(view, tx[sfAccount], tx, fee, fix340Enabled) + .has_value()) { - // Both changed is always a bug. Neither changed is - // consistent only with a legitimate zero-value - // withdrawal, which moves nothing on either side — - // there is nothing left to cross-check. - if (!zeroDeltaIsLegitimate || maybeAccDelta.has_value()) + JLOG(j.fatal()) << // + "Invariant failed: withdrawal must change one destination balance"; + return false; + } + + auto const maybeRecipientDelta = + deltaAssetsForParty(view, recipient, tx, fee, fix340Enabled); + + if (!maybeRecipientDelta.has_value()) + { + // A legitimate zero-value withdrawal moves nothing to + // the recipient either; there is nothing left to + // cross-check. + if (!zeroDeltaIsLegitimate) { JLOG(j.fatal()) << // "Invariant failed: withdrawal must change one destination balance"; @@ -1059,8 +1115,7 @@ ValidVault::finalize( // A one-sided change is cross-checked even for a // legitimate zero vault delta: the destination must // then have moved by (rounded) zero as well. - auto const destinationDelta = - *maybeAccDelta.or_else([&] { return maybeOtherAccDelta; }); + auto const destinationDelta = *maybeRecipientDelta; // the scale of destinationDelta can be coarser than // minScale, so we take that into account when rounding diff --git a/src/test/app/Sponsor_test.cpp b/src/test/app/Sponsor_test.cpp index e58d8c9f8f..593edfe0a4 100644 --- a/src/test/app/Sponsor_test.cpp +++ b/src/test/app/Sponsor_test.cpp @@ -5585,9 +5585,9 @@ public: Ter(tesSUCCESS)); env.close(); - // The same helper (deltaAssetsTxAccount) drives the withdraw path, so a - // fee-sponsored withdrawal back to the depositor's own account also - // passes on the destination side. + // The same fee-correction logic (ValidVault::deltaAssetsForParty) + // drives the withdraw path, so a fee-sponsored withdrawal back to + // the depositor's own account also passes on the destination side. env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = xrpAsset(50)}), Fee(XRP(1)), sponsor::As(sponsor, spfSponsorFee), diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index 04f31c9526..dc75371f3e 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -1674,6 +1675,340 @@ private: } } + // Bug 1: a sponsored XRP VaultWithdraw to a distinct destination is + // rejected because the vault invariant treats the holder's touched + // but economically unchanged AccountRoot as a second payout + // recipient. Sequence/ticket processing still touches the holder + // while the sponsor pays the fee, so the holder's XRP delta is + // present-zero and is not normalized away. If this happens on the + // last Subscription ledger of a closed-ended vault, the holder + // cannot retry until Redemption (tecTOO_SOON during Investment). + // + // Fixed by ValidVault::deltaAssetsForParty always collapsing an + // economically-zero XRP delta to absence, regardless of who paid the + // fee. + void + testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient() + { + using namespace test::jtx; + + auto runScenario = [this](FeatureBitset features, TER expected) { + Env env{*this, features}; + Account const owner{"owner"}; + Account const holder{"holder"}; + Account const destination{"destination"}; + Account const sponsor{"sponsor"}; + env.fund(XRP(10'000), owner, holder, destination, sponsor); + env.close(); + + constexpr std::uint32_t investmentPeriod = 14u * 24u * 60u * 60u; + auto const [vault, vaultKeylet, subscriptionDate, redemptionDate] = + makeClosedEndedVault(env, owner, xrpIssue(), 120u, investmentPeriod); + BEAST_EXPECT(redemptionDate - subscriptionDate == investmentPeriod); + + env(vault.deposit( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()})); + env.close(); + + // Inclusive SubscriptionDate boundary: still Subscription, so an + // ordinary withdrawal is allowed. + closeToTime(env, tp{d{subscriptionDate}}); + + auto const vaultBefore = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultBefore)) + return; + auto const assetsTotalBefore = vaultBefore->at(sfAssetsTotal); + auto const holderBalanceBefore = env.balance(holder); + auto const destinationBalanceBefore = env.balance(destination); + auto const sponsorBalanceBefore = env.balance(sponsor); + auto const fee = env.current()->fees().base; + + auto withdraw = vault.withdraw( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()}); + withdraw[sfDestination] = destination.human(); + env(withdraw, + Fee(fee), + sponsor::As(sponsor, spfSponsorFee), + Sig(sfSponsorSignature, sponsor), + Ter(expected)); + env.close(); + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore - fee); + + if (expected == tesSUCCESS) + { + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore - XRP(100).value()); + BEAST_EXPECT(env.balance(holder) == holderBalanceBefore); + BEAST_EXPECT(env.balance(destination) == destinationBalanceBefore + XRP(100)); + return; + } + + // Invariant rollback: the payout and share burn are undone, but + // sequence processing and the sponsored fee charge remain. + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore); + BEAST_EXPECT(env.balance(holder) == holderBalanceBefore); + BEAST_EXPECT(env.balance(destination) == destinationBalanceBefore); + + // Once the ledger advances into Investment, the same holder + // cannot retry until Redemption. + auto retry = vault.withdraw( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()}); + retry[sfDestination] = destination.human(); + env(retry, Ter(tecTOO_SOON)); + }; + + testcase( + "bug: sponsored XRP withdrawal to a distinct destination misreads a " + "touched-but-zero sender delta as a second recipient " + "(pre-fixCleanup3_4_0)"); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); + + testcase( + "bug: sponsored XRP withdrawal to a distinct destination succeeds " + "(post-fixCleanup3_4_0)"); + runScenario(all_, tesSUCCESS); + } + + // Bug 2: a co-signed fee sponsor named as the withdrawal's own + // destination pays its fee from the same AccountRoot it is paid into, + // so its net XRP delta is (payout - fee). The invariant never fee- + // corrected the destination side at all, so this always failed the + // equal-amount check against the vault's outflow (payout). + // + // Fixed by ValidVault::deltaAssetsForParty adding the fee back onto + // whichever inspected party's AccountRoot actually paid it -- the + // sender, or a distinct destination -- not just the sender. + void + testBugSponsorAsDestinationFeeMisappliedToPayout() + { + using namespace test::jtx; + + auto runScenario = [this](FeatureBitset features, TER expected) { + Env env{*this, features}; + Account const owner{"owner"}; + Account const holder{"holder"}; + Account const sponsor{"sponsor"}; + env.fund(XRP(10'000), owner, holder, sponsor); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = owner, .asset = xrpIssue()}); + env(vaultTx); + env.close(); + + env(vault.deposit( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()})); + env.close(); + + auto const vaultBefore = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultBefore)) + return; + auto const assetsTotalBefore = vaultBefore->at(sfAssetsTotal); + auto const sponsorBalanceBefore = env.balance(sponsor); + auto const fee = env.current()->fees().base; + + // The sponsor both receives the withdrawal (as sfDestination) + // and pays its own fee (co-signed) from the same AccountRoot. + auto withdraw = vault.withdraw( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()}); + withdraw[sfDestination] = sponsor.human(); + env(withdraw, + Fee(fee), + sponsor::As(sponsor, spfSponsorFee), + Sig(sfSponsorSignature, sponsor), + Ter(expected)); + env.close(); + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + + if (expected == tesSUCCESS) + { + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore - XRP(100).value()); + // Paid the withdrawal, then separately debited for the fee + // it chose to cover; net effect is payout minus fee. + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore + XRP(100) - fee); + return; + } + + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore); + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore - fee); + }; + + testcase( + "bug: co-signed sponsor named as withdrawal destination has its " + "own fee debit misread as breaking the payout equality " + "(pre-fixCleanup3_4_0)"); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED); + + testcase( + "bug: co-signed sponsor named as withdrawal destination succeeds " + "(post-fixCleanup3_4_0)"); + runScenario(all_, tesSUCCESS); + } + + // Pre-funded fee sponsorship draws the fee from ltSponsorship.sfFeeAmount, + // so feePayerAccountRoot must return nullopt rather than the sponsor's + // AccountRoot. A bystander sponsor leaves that branch unexercised: the + // result is only consulted by deltaAssetsForParty via `payer && *payer == + // id`. Naming the sponsor as sfDestination makes the early return + // load-bearing -- returning the sponsor's id instead of nullopt would add + // the fee back onto a balance that never paid it, and the equal-amount + // check against the vault outflow would fail. + // + // Contrast testBugSponsorAsDestinationFeeMisappliedToPayout, where the + // sponsor co-signs and so really does pay from its own AccountRoot. + void + testPrefundedFeeWithdraw() + { + using namespace test::jtx; + + auto runScenario = [this]( + FeatureBitset features, + TER expected, + bool const sponsorIsDestination) { + Env env{*this, features}; + Account const owner{"owner"}; + Account const holder{"holder"}; + Account const destination{"destination"}; + Account const sponsor{"sponsor"}; + env.fund(XRP(10'000), owner, holder, destination, sponsor); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = owner, .asset = xrpIssue()}); + env(vaultTx); + env.close(); + + env(vault.deposit( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()})); + env.close(); + + auto const fee = env.current()->fees().base; + env(sponsor::set_fee(sponsor, 0, fee), sponsor::SponseeAcc(holder)); + env.close(); + + auto const vaultBefore = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultBefore)) + return; + auto const assetsTotalBefore = vaultBefore->at(sfAssetsTotal); + auto const holderBalanceBefore = env.balance(holder); + auto const destinationBalanceBefore = env.balance(destination); + auto const sponsorBalanceBefore = env.balance(sponsor); + + Account const& recipient = sponsorIsDestination ? sponsor : destination; + auto withdraw = vault.withdraw( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()}); + withdraw[sfDestination] = recipient.human(); + env(withdraw, Fee(fee), sponsor::As(sponsor, spfSponsorFee), Ter(expected)); + env.close(); + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + // Holder is economically unchanged (sequence only); the fee is + // taken from the sponsorship object, not any AccountRoot. + BEAST_EXPECT(env.balance(holder) == holderBalanceBefore); + + if (expected == tesSUCCESS) + { + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore - XRP(100).value()); + if (sponsorIsDestination) + { + // The sponsor receives the payout and is not debited for + // the fee. The sponsor has to BE the destination for + // FeePayerType::SponsorPreFunded to matter. + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore + XRP(100)); + } + else + { + BEAST_EXPECT(env.balance(destination) == destinationBalanceBefore + XRP(100)); + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore); + } + auto const sponsorship = env.le(keylet::sponsorship(sponsor, holder)); + if (!BEAST_EXPECT(sponsorship)) + return; + BEAST_EXPECT(!sponsorship->isFieldPresent(sfFeeAmount)); + return; + } + + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore); + BEAST_EXPECT(env.balance(sponsor) == sponsorBalanceBefore); + if (!sponsorIsDestination) + BEAST_EXPECT(env.balance(destination) == destinationBalanceBefore); + }; + + testcase( + "pre-funded fee XRP withdrawal to a distinct destination succeeds " + "(post-fixCleanup3_4_0)"); + runScenario(all_, tesSUCCESS, false); + + testcase( + "bug: pre-funded sponsor named as withdrawal destination misreads " + "the sender's touched-but-zero delta as a second recipient " + "(pre-fixCleanup3_4_0)"); + runScenario(all_ - fixCleanup3_4_0, tecINVARIANT_FAILED, true); + + testcase( + "bug: pre-funded sponsor named as withdrawal destination receives " + "the full payout (post-fixCleanup3_4_0)"); + runScenario(all_, tesSUCCESS, true); + } + + // Unsponsored third-party XRP withdrawal: the sender's AccountRoot moves + // by exactly -fee. Pre-amendment, the sender-only fee correction then + // collapses that to absence so the dual-recipient guard does not fire. + void + testUnsponsoredWithdrawToDistinctDestinationPreAmendment() + { + using namespace test::jtx; + + testcase( + "unsponsored XRP withdrawal to a distinct destination succeeds " + "(pre-fixCleanup3_4_0)"); + + Env env{*this, all_ - fixCleanup3_4_0}; + Account const owner{"owner"}; + Account const holder{"holder"}; + Account const destination{"destination"}; + env.fund(XRP(10'000), owner, holder, destination); + env.close(); + + Vault const vault{env}; + auto [vaultTx, vaultKeylet] = vault.create({.owner = owner, .asset = xrpIssue()}); + env(vaultTx); + env.close(); + + env(vault.deposit( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()})); + env.close(); + + auto const vaultBefore = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultBefore)) + return; + auto const assetsTotalBefore = vaultBefore->at(sfAssetsTotal); + auto const holderBalanceBefore = env.balance(holder); + auto const destinationBalanceBefore = env.balance(destination); + auto const fee = env.current()->fees().base; + + auto withdraw = vault.withdraw( + {.depositor = holder, .id = vaultKeylet.key, .amount = XRP(100).value()}); + withdraw[sfDestination] = destination.human(); + env(withdraw, Fee(fee), Ter(tesSUCCESS)); + env.close(); + + auto const vaultAfter = env.le(vaultKeylet); + if (!BEAST_EXPECT(vaultAfter)) + return; + BEAST_EXPECT(vaultAfter->at(sfAssetsTotal) == assetsTotalBefore - XRP(100).value()); + BEAST_EXPECT(env.balance(holder) == holderBalanceBefore - fee); + BEAST_EXPECT(env.balance(destination) == destinationBalanceBefore + XRP(100)); + } + public: void run() override @@ -1695,6 +2030,10 @@ public: testBugClawbackRoundTripOvershoot(); testBugWithdrawRoundTripOvershoot(); testBugClawbackAfterLoanImpair(); + testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient(); + testBugSponsorAsDestinationFeeMisappliedToPayout(); + testPrefundedFeeWithdraw(); + testUnsponsoredWithdrawToDistinctDestinationPreAmendment(); } };