Merge remote-tracking branch 'origin/develop' into tapanito/empty-holding

# Conflicts:
#	src/test/app/vault/VaultBugs_test.cpp

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<TER, XRPAmount>
     reset(XRPAmount fee);
 
-    static FeePayer
-    getFeePayer(ReadView const& view, STTx const& tx);
-
     TER
     consumeSeqProxy(SLE::pointer const& sleAccount);
     TER

include/xrpl/tx/invariants/MPTInvariant.h

@@ -215,6 +215,13 @@ class ValidMPTTransfer
     // Deleted MPToken
     // MPToken key: true if MPTAuthorized is set
     hash_map<uint256, bool> 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<AccountID, bool> pseudoAccountsBefore_;
 
 public:
     /**

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<AccountID>
+    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<DeltaInfo>
-    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.

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

src/libxrpl/tx/invariants/VaultInvariant.cpp

@@ -21,6 +21,7 @@
 #include <xrpl/protocol/TER.h>
 #include <xrpl/protocol/TxFormats.h>
 #include <xrpl/protocol/XRPAmount.h>
+#include <xrpl/tx/Transactor.h>
 #include <xrpl/tx/invariants/InvariantCheckPrivilege.h>
 
 #include <algorithm>
@@ -235,21 +236,57 @@ ValidVault::deltaAssets(AccountID const& id) const
         vaultAsset.value());
 }
 
+std::optional<AccountID>
+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::DeltaInfo>
-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<DeltaInfo> {
-                        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

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),

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.
     }

src/test/app/vault/VaultBugs_test.cpp

@@ -10,6 +10,7 @@
 #include <test/jtx/pay.h>
 #include <test/jtx/permissioned_domains.h>
 #include <test/jtx/sig.h>
+#include <test/jtx/sponsor.h>
 #include <test/jtx/ter.h>
 #include <test/jtx/trust.h>
 #include <test/jtx/vault.h>
@@ -1925,6 +1926,340 @@ private:
         runPrivateVault(all_, tesSUCCESS, tecNO_AUTH);
     }
 
+    // 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
@@ -1947,6 +2282,10 @@ public:
         testBugWithdrawRoundTripOvershoot();
         testBugClawbackAfterLoanImpair();
         testBugSelfWithdrawAfterIssuerClearsDefaultRipple();
+        testBugSponsoredWithdrawZeroDeltaMisclassifiedAsSecondRecipient();
+        testBugSponsorAsDestinationFeeMisappliedToPayout();
+        testPrefundedFeeWithdraw();
+        testUnsponsoredWithdrawToDistinctDestinationPreAmendment();
     }
 };