fix: Keep LoanBrokerDelete valid for auth-required MPT cover (#8144)

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:
     /**

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