fix: Reject credentials on pseudo-accounts and unpin Vault/LoanBroker (FN-36)

include/xrpl/ledger/helpers/CredentialHelpers.h

@@ -13,6 +13,7 @@
 #include <xrpl/protocol/STVector256.h>
 #include <xrpl/protocol/TER.h>
 
+#include <cstdint>
 #include <memory>
 #include <set>
 #include <utility>
@@ -32,6 +33,29 @@ checkExpired(SLE const& sleCredential, NetClock::time_point const& closed);
 [[nodiscard]] TER
 deleteSLE(ApplyView& view, SLE::ref sleCredential, beast::Journal j);
 
+/**
+ * @brief Remove credentials pinned to a pseudo-account's owner directory.
+ *
+ * Cleans up credentials that were linked to a pseudo-account (Vault, LoanBroker,
+ * AMM) before fixCleanup3_4_0, which such an account can neither accept nor
+ * delete. Only credentials are removed, at most @p maxNodesToDelete of them; on
+ * reaching that bound the result is `tecINCOMPLETE` and the caller must
+ * propagate it so a later transaction resumes.
+ *
+ * @param view Mutable ledger view.
+ * @param pseudoAcct The pseudo-account whose directory is cleaned.
+ * @param maxNodesToDelete Upper bound on directory entries processed in one call.
+ * @param j Journal for diagnostics.
+ * @return tesSUCCESS once no credentials remain, tecINCOMPLETE if the bound was
+ *         reached, or a deletion error.
+ */
+[[nodiscard]] TER
+deletePseudoAccountCredentials(
+    ApplyView& view,
+    AccountID const& pseudoAcct,
+    std::uint16_t maxNodesToDelete,
+    beast::Journal j);
+
 // Amendment and parameters checks for sfCredentialIDs field
 NotTEC
 checkFields(STTx const& tx, beast::Journal j);

include/xrpl/protocol/Protocol.h

@@ -354,6 +354,12 @@ using TxID = uint256;
  */
 constexpr std::uint16_t kMaxDeletableAmmTrustLines = 512;
 
+/**
+ * The maximum number of credentials to delete from a pseudo-account's owner
+ * directory in a single transaction.
+ */
+constexpr std::uint16_t kMaxDeletablePseudoAccountCredentials = 512;
+
 /**
  * The maximum length of a URI inside an Oracle
  */

src/libxrpl/ledger/helpers/AMMHelpers.cpp

@@ -11,6 +11,7 @@
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/ledger/Sandbox.h>
 #include <xrpl/ledger/View.h>
+#include <xrpl/ledger/helpers/CredentialHelpers.h>
 #include <xrpl/ledger/helpers/RippleStateHelpers.h>
 #include <xrpl/ledger/helpers/TokenHelpers.h>
 #include <xrpl/protocol/AMMCore.h>
@@ -689,6 +690,11 @@ deleteAMMTrustLines(
 
                 return {deleteAMMTrustLine(sb, sleItem, ammAccountID, j), SkipEntry::No};
             }
+            // A credential can be pinned to the AMM pseudo-account before
+            // fixCleanup3_4_0. Clean it up here, inside the same bounded walk, so
+            // the pinned AMM can be deleted. See fixCleanup3_4_0.
+            if (sb.rules().enabled(fixCleanup3_4_0) && nodeType == ltCREDENTIAL)
+                return {credentials::deleteSLE(sb, sleItem, j), SkipEntry::No};
             // LCOV_EXCL_START
             JLOG(j.error()) << "deleteAMMObjects: deleting non-trustline or non-MPT " << nodeType;
             return {tecINTERNAL, SkipEntry::No};
@@ -766,6 +772,8 @@ deleteAMMAccount(Sandbox& sb, Asset const& asset, Asset const& asset2, beast::Jo
         // LCOV_EXCL_STOP
     }
 
+    // deleteAMMTrustLines also removes any credentials pinned to the AMM
+    // pseudo-account before fixCleanup3_4_0, within its bounded walk.
     if (auto const ter = deleteAMMTrustLines(sb, ammAccountID, kMaxDeletableAmmTrustLines, j);
         !isTesSuccess(ter))
         return ter;
@@ -907,6 +915,11 @@ isOnlyLiquidityProvider(ReadView const& view, Issue const& ammIssue, AccountID c
                 ++nMPT;
                 continue;
             }
+            // A credential can be pinned to the AMM pseudo-account before
+            // fixCleanup3_4_0. Ignore it here; VaultDelete-style
+            // cleanup in deleteAMMAccount removes it when the AMM is deleted.
+            if (view.rules().enabled(fixCleanup3_4_0) && entryType == ltCREDENTIAL)
+                continue;
             if (entryType != ltRIPPLE_STATE)
                 return std::unexpected<TER>(tecINTERNAL);  // LCOV_EXCL_LINE
             auto const lowLimit = sle->getFieldAmount(sfLowLimit);

src/libxrpl/ledger/helpers/CredentialHelpers.cpp

@@ -5,8 +5,10 @@
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/basics/chrono.h>
 #include <xrpl/beast/utility/Journal.h>
+#include <xrpl/beast/utility/instrumentation.h>
 #include <xrpl/ledger/ApplyView.h>
 #include <xrpl/ledger/ReadView.h>
+#include <xrpl/ledger/View.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/protocol/AccountID.h>
 #include <xrpl/protocol/Feature.h>
@@ -123,6 +125,36 @@ deleteSLE(ApplyView& view, SLE::ref sleCredential, beast::Journal j)
     return tesSUCCESS;
 }
 
+TER
+deletePseudoAccountCredentials(
+    ApplyView& view,
+    AccountID const& pseudoAcct,
+    std::uint16_t maxNodesToDelete,
+    beast::Journal j)
+{
+    XRPL_ASSERT(
+        isPseudoAccount(view.read(keylet::account(pseudoAcct))),
+        "xrpl::credentials::deletePseudoAccountCredentials : is a pseudo-account");
+
+    // Delete the credentials linked into the pseudo-account's owner directory,
+    // visiting at most maxNodesToDelete entries. Any other object is left in
+    // place; the caller's own checks decide whether the remaining directory
+    // blocks deletion. If the bound is reached, cleanupOnAccountDelete returns
+    // tecINCOMPLETE and the caller propagates it so a later transaction resumes.
+    return cleanupOnAccountDelete(
+        view,
+        keylet::ownerDir(pseudoAcct),
+        [&view, &j](LedgerEntryType nodeType, uint256 const&, SLE::pointer& sleItem)
+            -> std::pair<TER, SkipEntry> {
+            if (nodeType == ltCREDENTIAL)
+                return {deleteSLE(view, sleItem, j), SkipEntry::No};
+
+            return {tesSUCCESS, SkipEntry::Yes};
+        },
+        j,
+        maxNodesToDelete);
+}
+
 NotTEC
 checkFields(STTx const& tx, beast::Journal j)
 {

src/libxrpl/tx/Transactor.cpp

@@ -1435,7 +1435,8 @@ Transactor::processPersistentChanges(TER result, XRPAmount fee)
     //        should be used, making it possible to do more useful work
     //        when transactions fail with a `tec` code.
 
-    auto typesForResult = [](TER const ter) {
+    auto typesForResult = [credentialCleanup =
+                               view().rules().enabled(fixCleanup3_4_0)](TER const ter) {
         std::unordered_set<LedgerEntryType> types;
         if ((ter == tecOVERSIZE) || (ter == tecKILLED))
         {
@@ -1444,6 +1445,11 @@ Transactor::processPersistentChanges(TER result, XRPAmount fee)
         else if (ter == tecINCOMPLETE)
         {
             types.insert(ltRIPPLE_STATE);
+            // A bounded pseudo-account credential cleanup (VaultDelete /
+            // LoanBrokerDelete) persists its partial credential deletions so a
+            // later transaction can resume. See fixCleanup3_4_0.
+            if (credentialCleanup)
+                types.insert(ltCREDENTIAL);
         }
         else if (ter == tecEXPIRED)
         {

src/libxrpl/tx/invariants/MPTInvariant.cpp

@@ -231,6 +231,13 @@ ValidMPTIssuance::finalize(
 
         if (hasPrivilege(tx, DestroyMptIssuance))
         {
+            // A VaultDelete that is still cleaning up credentials pinned to its
+            // pseudo-account before fixCleanup3_4_0 returns tecINCOMPLETE and has
+            // not yet reached the share issuance. Don't require the issuance to
+            // be removed until the deletion completes (a later transaction).
+            if (rules.enabled(fixCleanup3_4_0) && result == tecINCOMPLETE)
+                return mptIssuancesDeleted_ == 0 && mptIssuancesCreated_ == 0;
+
             if (mptIssuancesDeleted_ == 0)
             {
                 JLOG(j.fatal()) << "Invariant failed: MPT issuance deletion "

src/libxrpl/tx/transactors/credentials/CredentialCreate.cpp

@@ -90,6 +90,16 @@ CredentialCreate::preclaim(PreclaimContext const& ctx)
         return tecNO_TARGET;
     }
 
+    // A pseudo-account (Vault, LoanBroker, AMM) can't sign, so it can never
+    // accept or delete a credential issued to it. Such a credential would stay
+    // pinned in the pseudo-account's owner directory forever and block deletion
+    // of the owning object (tecHAS_OBLIGATIONS). Reject it up front.
+    if (ctx.view.rules().enabled(fixCleanup3_4_0) && isPseudoAccount(ctx.view, subject))
+    {
+        JLOG(ctx.j.trace()) << "Subject is a pseudo-account.";
+        return tecPSEUDO_ACCOUNT;
+    }
+
     if (ctx.view.exists(keylet::credential(subject, ctx.tx[sfAccount], credType)))
     {
         JLOG(ctx.j.trace()) << "Credential already exists.";

src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp

@@ -4,11 +4,13 @@
 #include <xrpl/basics/Number.h>
 #include <xrpl/beast/utility/Zero.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
+#include <xrpl/ledger/helpers/CredentialHelpers.h>
 #include <xrpl/ledger/helpers/LendingHelpers.h>
 #include <xrpl/ledger/helpers/TokenHelpers.h>
 #include <xrpl/protocol/Asset.h>
 #include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
+#include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
 #include <xrpl/protocol/STAmount.h>
 #include <xrpl/protocol/STLedgerEntry.h>
@@ -141,6 +143,19 @@
 
     auto const brokerPseudoID = broker->at(sfAccount);
 
+    // Remove any credentials pinned to the broker pseudo-account before anything
+    // else. They would otherwise keep its owner directory alive and block
+    // deletion with tecHAS_OBLIGATIONS. Doing it first means a bounded,
+    // tecINCOMPLETE cleanup can be resumed by a later transaction without having
+    // already torn down the broker. See fixCleanup3_4_0.
+    if (view().rules().enabled(fixCleanup3_4_0))
+    {
+        if (auto const ter = credentials::deletePseudoAccountCredentials(
+                view(), brokerPseudoID, kMaxDeletablePseudoAccountCredentials, j_);
+            !isTesSuccess(ter))
+            return ter;
+    }
+
     if (!view().dirRemove(
             keylet::ownerDir(accountID_), broker->at(sfOwnerNode), broker->key(), false))
     {

src/libxrpl/tx/transactors/vault/VaultDelete.cpp

@@ -4,6 +4,7 @@
 #include <xrpl/basics/base_uint.h>
 #include <xrpl/beast/utility/Zero.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
+#include <xrpl/ledger/helpers/CredentialHelpers.h>
 #include <xrpl/ledger/helpers/MPTokenHelpers.h>
 #include <xrpl/ledger/helpers/TokenHelpers.h>
 #include <xrpl/protocol/AccountID.h>
@@ -100,6 +101,19 @@ VaultDelete::doApply()
     if (!vault)
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
+    // Remove any credentials pinned to the vault pseudo-account before anything
+    // else. They would otherwise keep its owner directory alive and block
+    // deletion with tecHAS_OBLIGATIONS. Doing it first means a bounded,
+    // tecINCOMPLETE cleanup can be resumed by a later transaction without having
+    // already torn down the vault. See fixCleanup3_4_0.
+    if (view().rules().enabled(fixCleanup3_4_0))
+    {
+        if (auto const ter = credentials::deletePseudoAccountCredentials(
+                view(), vault->at(sfAccount), kMaxDeletablePseudoAccountCredentials, j_);
+            !isTesSuccess(ter))
+            return ter;
+    }
+
     // Destroy the asset holding.
     auto asset = vault->at(sfAsset);
 

src/test/app/AMM_test.cpp

@@ -4,6 +4,7 @@
 #include <test/jtx/Env.h>
 #include <test/jtx/TestHelpers.h>
 #include <test/jtx/amount.h>
+#include <test/jtx/credentials.h>
 #include <test/jtx/envconfig.h>
 #include <test/jtx/escrow.h>
 #include <test/jtx/fee.h>
@@ -5143,6 +5144,55 @@ struct AMM_test : public jtx::AMMTest
             {features});
     }
 
+    void
+    testCredentialPinsPseudoAccount()
+    {
+        testcase("Credential pins AMM pseudo-account");
+
+        using namespace jtx;
+        FeatureBitset const all{testableAmendments()};
+
+        // FN-36: a credential issued to an AMM pseudo-account can't be accepted
+        // or deleted by it. Before fixCleanup3_4_0 it stays pinned in
+        // the pseudo-account's owner directory and makes AMM deletion fail with
+        // tecINTERNAL (deleteAMMTrustLines rejects the unexpected directory
+        // entry).
+        Account const attacker{"attacker"};
+        char const credType[] = "FN36";
+
+        Env env(*this, all - fixCleanup3_4_0);
+        fund(env, gw_, {alice_}, XRP(20'000), {USD(10'000)});
+        env.fund(XRP(1'000), attacker);
+        env.close();
+
+        AMM amm(env, alice_, XRP(10'000), USD(10'000));
+        Account const ammAcct{"amm pseudo-account", amm.ammAccount()};
+        env.memoize(ammAcct);
+
+        env(credentials::create(ammAcct, attacker, credType));
+        env.close();
+        auto const credKey = credentials::keylet(ammAcct, attacker, credType);
+        BEAST_EXPECT(env.le(credKey));
+
+        // Emptying the AMM would auto-delete it, but the pinned credential makes
+        // deleteAMMAccount fail; the withdraw is rolled back and the AMM stays.
+        amm.withdrawAll(alice_, std::nullopt, Ter(tecINTERNAL));
+        BEAST_EXPECT(amm.ammExists());
+
+        env.enableFeature(fixCleanup3_4_0);
+        env.close();
+
+        // Part 1: a new pin is rejected outright.
+        env(credentials::create(ammAcct, attacker, "FN36B"), Ter(tecPSEUDO_ACCOUNT));
+        env.close();
+
+        // Part 2: the pre-existing pin is cleaned up and the AMM deletes.
+        amm.withdrawAll(alice_);
+        BEAST_EXPECT(!amm.ammExists());
+        BEAST_EXPECT(!env.le(credKey));
+        BEAST_EXPECT(!env.le(keylet::ownerDir(amm.ammAccount())));
+    }
+
     void
     testAutoDelete()
     {
@@ -7216,6 +7266,7 @@ struct AMM_test : public jtx::AMMTest
         FeatureBitset const all{testableAmendments()};
         testInvalidInstance();
         testInstanceCreate();
+        testCredentialPinsPseudoAccount();
         for (auto const& f : amendmentCombinations({fixCleanup3_3_0, featureAMMClawback}))
             testInvalidDeposit(f);
         testDeposit();