fix: Treat an existing IOU line as a no-op in addEmptyHolding

Self-destination VaultWithdraw and LoanBrokerCoverWithdraw called addEmptyHolding and only tolerated tecDUPLICATE, so an issuer clearing asfDefaultRipple made those payouts fail with tecINTERNAL even when the destination already held the asset.

include/xrpl/ledger/helpers/RippleStateHelpers.h

@@ -239,8 +239,12 @@ canTransfer(ReadView const& view, Issue const& issue, AccountID const& from, Acc
 //------------------------------------------------------------------------------
 
 /**
- * Any transactors that call addEmptyHolding() in doApply must call
- * canAddHolding() in preflight with the same View and Asset
+ * If the destination already holds this IOU, returns tecDUPLICATE and does
+ * not consult issuer freeze or DefaultRipple (post-fixCleanup3_4_0).
+ * Transactors that may create a new holding in doApply must call
+ * canAddHolding() in preclaim with the same View and Asset. Do not call
+ * canAddHolding() merely because addEmptyHolding() is invoked: that helper
+ * does not check whether a holding already exists.
  */
 [[nodiscard]] TER
 addEmptyHolding(

include/xrpl/ledger/helpers/TokenHelpers.h

@@ -319,6 +319,12 @@ transferRate(ReadView const& view, STAmount const& amount);
 [[nodiscard]] TER
 canAddHolding(ReadView const& view, Asset const& asset);
 
+/**
+ * True if the account already holds this asset (or is the issuer / XRP).
+ */
+[[nodiscard]] bool
+holdingExists(ReadView const& view, AccountID const& account, Asset const& asset);
+
 [[nodiscard]] TER
 addEmptyHolding(
     ApplyViewContext ctx,

src/libxrpl/ledger/helpers/MPTokenHelpers.cpp

@@ -184,6 +184,8 @@ addEmptyHolding(
     auto const mpt = ctx.view.peek(keylet::mptokenIssuance(mptID));
     if (!mpt)
         return tefINTERNAL;  // LCOV_EXCL_LINE
+    // Unlike IOU addEmptyHolding (post-fixCleanup3_4_0), a locked issuance is
+    // still rejected before the "MPToken already exists" short circuit.
     if (mpt->isFlag(lsfMPTLocked))
         return tefINTERNAL;  // LCOV_EXCL_LINE
     if (ctx.view.peek(keylet::mptoken(mptID, accountID)))

src/libxrpl/ledger/helpers/RippleStateHelpers.cpp

@@ -652,21 +652,27 @@ addEmptyHolding(
 
     auto const& issuerId = issue.getIssuer();
     auto const& currency = issue.currency;
-    if (isGlobalFrozen(ctx.view, issuerId))
-        return tecFROZEN;  // LCOV_EXCL_LINE
-
     auto const& srcId = issuerId;
     auto const& dstId = accountID;
     auto const high = srcId > dstId;
     auto const index = keylet::trustLine(srcId, dstId, currency);
+    // Post-fixCleanup3_4_0: an existing line is a no-op. Issuer freeze and
+    // DefaultRipple only matter when this function has to create a line.
+    bool const fix340Enabled = ctx.view.rules().enabled(fixCleanup3_4_0);
+    if (fix340Enabled && ctx.view.read(index))
+        return tecDUPLICATE;
+
+    if (isGlobalFrozen(ctx.view, issuerId))
+        return tecFROZEN;  // LCOV_EXCL_LINE
+
     auto const sleSrc = ctx.view.peek(keylet::account(srcId));
     auto const sleDst = ctx.view.peek(keylet::account(dstId));
     if (!sleDst || !sleSrc)
         return tefINTERNAL;  // LCOV_EXCL_LINE
     if (!sleSrc->isFlag(lsfDefaultRipple))
-        return tecINTERNAL;  // LCOV_EXCL_LINE
+        return fix340Enabled ? TER{terNO_RIPPLE} : tecINTERNAL;
     // If the line already exists, don't create it again.
-    if (ctx.view.read(index))
+    if (!fix340Enabled && ctx.view.read(index))
         return tecDUPLICATE;
 
     // A reserve sponsor only covers tx.Account's own objects.

src/libxrpl/ledger/helpers/TokenHelpers.cpp

@@ -583,6 +583,32 @@ canAddHolding(ReadView const& view, Asset const& asset)
         asset.value());
 }
 
+[[nodiscard]] bool
+holdingExists(ReadView const& view, AccountID const& account, Issue const& issue)
+{
+    if (issue.native() || account == issue.getIssuer())
+        return true;
+    return view.exists(keylet::trustLine(account, issue));
+}
+
+[[nodiscard]] bool
+holdingExists(ReadView const& view, AccountID const& account, MPTIssue const& mptIssue)
+{
+    if (account == mptIssue.getIssuer())
+        return true;
+    return view.exists(keylet::mptoken(mptIssue.getMptID(), account));
+}
+
+[[nodiscard]] bool
+holdingExists(ReadView const& view, AccountID const& account, Asset const& asset)
+{
+    return std::visit(
+        [&]<ValidIssueType TIss>(TIss const& issue) -> bool {
+            return holdingExists(view, account, issue);
+        },
+        asset.value());
+}
+
 TER
 addEmptyHolding(
     ApplyViewContext ctx,

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

@@ -65,6 +65,7 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
 {
     auto const fix320Enabled = ctx.view.rules().enabled(fixCleanup3_2_0);
     auto const fix330Enabled = ctx.view.rules().enabled(fixCleanup3_3_0);
+    auto const fix340Enabled = ctx.view.rules().enabled(fixCleanup3_4_0);
     auto const& tx = ctx.tx;
 
     auto const account = tx[sfAccount];
@@ -140,6 +141,12 @@ LoanBrokerCoverWithdraw::preclaim(PreclaimContext const& ctx)
     if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType))
         return ter;
 
+    if (fix340Enabled && account == dstAcct && !holdingExists(ctx.view, dstAcct, vaultAsset))
+    {
+        if (auto const ter = canAddHolding(ctx.view, vaultAsset); !isTesSuccess(ter))
+            return ter;
+    }
+
     if (fix330Enabled)
     {
         if (auto const ret =

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

@@ -205,6 +205,15 @@ VaultWithdraw::preclaim(PreclaimContext const& ctx)
     if (auto const ter = requireAuth(ctx.view, vaultAsset, dstAcct, authType); !isTesSuccess(ter))
         return ter;
 
+    // Fail early when self-destination would have to create a holding.
+    // Skip when a holding already exists: canAddHolding does not look at that,
+    // and would block a no-op create (the DefaultRipple-cleared self-withdraw).
+    if (fix340Enabled && account == dstAcct && !holdingExists(ctx.view, dstAcct, vaultAsset))
+    {
+        if (auto const ter = canAddHolding(ctx.view, vaultAsset); !isTesSuccess(ter))
+            return ter;
+    }
+
     // The checks above only establish that an account may hold the asset. A
     // private vault additionally restricts who may take part in it, so paying
     // its asset out to a third party requires both ends of that payout to be

src/test/app/vault/VaultBugs_test.cpp

@@ -8,6 +8,7 @@
 #include <test/jtx/fee.h>
 #include <test/jtx/flags.h>
 #include <test/jtx/pay.h>
+#include <test/jtx/permissioned_domains.h>
 #include <test/jtx/sig.h>
 #include <test/jtx/ter.h>
 #include <test/jtx/trust.h>
@@ -1674,6 +1675,255 @@ private:
         }
     }
 
+    // addEmptyHolding() used to check isGlobalFrozen(issuer) and
+    // !lsfDefaultRipple before the "line already exists" tecDUPLICATE
+    // short circuit. doWithdraw() calls addEmptyHolding() for a
+    // self-destination payout and only tolerates tecDUPLICATE, so
+    // tecINTERNAL from a missing DefaultRipple flag aborted the
+    // withdrawal. fixCleanup3_4_0 checks existence first and maps the
+    // create-path DefaultRipple miss to terNO_RIPPLE.
+    void
+    testBugSelfWithdrawAfterIssuerClearsDefaultRipple()
+    {
+        using namespace test::jtx;
+
+        auto runExistingLine = [this](FeatureBitset features, TER selfExpected) {
+            Env env(*this, features);
+            Account const issuer{"issuer"};
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+
+            env.fund(XRP(10'000), issuer, alice, bob);
+            env.close();
+            env(fset(issuer, asfDefaultRipple));
+            env.close();
+
+            PrettyAsset const usd{issuer["USD"]};
+            Issue const usdIssue = usd.raw().get<Issue>();
+            env(trust(alice, usd(10'000)));
+            env(trust(bob, usd(10'000)));
+            env.close();
+            env(pay(issuer, alice, usd(1'000)));
+            env.close();
+
+            Vault const vault{env};
+            auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = usd});
+            env(vaultTx);
+            env.close();
+
+            env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(500)}));
+            env.close();
+
+            env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)}));
+            env.close();
+
+            env(fclear(issuer, asfDefaultRipple));
+            env.close();
+
+            BEAST_EXPECT(env.le(keylet::trustLine(alice.id(), usdIssue)));
+
+            // Alice's USD line is unchanged; a later deposit still succeeds.
+            env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(10)}));
+            env.close();
+
+            env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)}),
+                Ter(selfExpected));
+            env.close();
+
+            auto destTx =
+                vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)});
+            destTx[sfDestination] = bob.human();
+            env(destTx);
+            env.close();
+        };
+
+        auto runDeletedLine = [this](FeatureBitset features, TER selfExpected) {
+            Env env(*this, features);
+            Account const issuer{"issuer"};
+            Account const alice{"alice"};
+
+            env.fund(XRP(10'000), issuer, alice);
+            env.close();
+            env(fset(issuer, asfDefaultRipple));
+            env.close();
+
+            PrettyAsset const usd{issuer["USD"]};
+            Issue const usdIssue = usd.raw().get<Issue>();
+            env(trust(alice, usd(10'000)));
+            env.close();
+            env(pay(issuer, alice, usd(500)));
+            env.close();
+
+            Vault const vault{env};
+            auto [vaultTx, vaultKeylet] = vault.create({.owner = alice, .asset = usd});
+            env(vaultTx);
+            env.close();
+
+            env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(500)}));
+            env.close();
+
+            env(trust(alice, usd(0)));
+            env.close();
+            BEAST_EXPECT(!env.le(keylet::trustLine(alice.id(), usdIssue)));
+            env(fclear(issuer, asfDefaultRipple));
+            env.close();
+
+            env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)}),
+                Ter(selfExpected));
+            env.close();
+        };
+
+        auto runCoverWithdraw = [this](FeatureBitset features, TER selfExpected) {
+            using namespace loan_broker;
+
+            Env env(*this, features);
+            Account const issuer{"issuer"};
+            Account const alice{"alice"};
+
+            env.fund(XRP(10'000), issuer, alice);
+            env.close();
+            env(fset(issuer, asfDefaultRipple));
+            env.close();
+
+            PrettyAsset const usd{issuer["USD"]};
+            Issue const usdIssue = usd.raw().get<Issue>();
+            env(trust(alice, usd(10'000)));
+            env.close();
+            env(pay(issuer, alice, usd(1'000)));
+            env.close();
+
+            Vault const vault{env};
+            auto const [createTx, vaultKeylet, subscriptionDate] = vault.createClosedEnded(
+                {.owner = alice, .asset = usd, .subscriptionOffset = std::chrono::seconds{60}});
+            (void)subscriptionDate;
+            env(createTx);
+            env.close();
+
+            env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(500)}));
+            env.close();
+
+            auto const brokerKeylet =
+                keylet::loanBroker(alice.id(), SeqProxy::rawSequence(env.seq(alice)));
+            env(set(alice, vaultKeylet.key));
+            env.close();
+            env(coverDeposit(alice, brokerKeylet.key, usd(100).value()));
+            env.close();
+
+            env(fclear(issuer, asfDefaultRipple));
+            env.close();
+            BEAST_EXPECT(env.le(keylet::trustLine(alice.id(), usdIssue)));
+
+            env(coverWithdraw(alice, brokerKeylet.key, usd(50).value()), Ter(selfExpected));
+            env.close();
+        };
+
+        auto runPrivateVault = [this](FeatureBitset features, TER selfExpected, TER destExpected) {
+            Env env(*this, features);
+            Account const issuer{"issuer"};
+            Account const alice{"alice"};
+            Account const bob{"bob"};
+            Account const pdOwner{"pdOwner"};
+            Account const credIssuer{"credIssuer"};
+            std::string const credType = "credential";
+
+            env.fund(XRP(10'000), issuer, alice, bob, pdOwner, credIssuer);
+            env.close();
+            env(fset(issuer, asfDefaultRipple));
+            env.close();
+
+            PrettyAsset const usd{issuer["USD"]};
+            env(trust(alice, usd(10'000)));
+            env(trust(bob, usd(10'000)));
+            env.close();
+            env(pay(issuer, alice, usd(1'000)));
+            env.close();
+
+            Vault const vault{env};
+            auto [vaultTx, vaultKeylet] =
+                vault.create({.owner = alice, .asset = usd, .flags = tfVaultPrivate});
+            env(vaultTx);
+            env.close();
+
+            pdomain::Credentials const credentials{{.issuer = credIssuer, .credType = credType}};
+            env(pdomain::setTx(pdOwner, credentials));
+            auto const domainId = pdomain::getNewDomain(env.meta());
+            {
+                auto domainTx = vault.set({.owner = alice, .id = vaultKeylet.key});
+                domainTx[sfDomainID] = to_string(domainId);
+                env(domainTx);
+                env.close();
+            }
+
+            env(credentials::create(alice, credIssuer, credType));
+            env(credentials::accept(alice, credIssuer, credType));
+            env.close();
+
+            env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(500)}));
+            env.close();
+
+            env(fclear(issuer, asfDefaultRipple));
+            env.close();
+
+            env(vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)}),
+                Ter(selfExpected));
+            env.close();
+
+            // Bob has a USD line but is not in the vault's domain; post-fixCleanup3_4_0
+            // that is not a usable destination.
+            auto destTx =
+                vault.withdraw({.depositor = alice, .id = vaultKeylet.key, .amount = usd(50)});
+            destTx[sfDestination] = bob.human();
+            env(destTx, Ter(destExpected));
+            env.close();
+        };
+
+        testcase(
+            "bug: VaultWithdraw to self fails with tecINTERNAL after issuer "
+            "clears asfDefaultRipple even though the trust line exists "
+            "(pre-fixCleanup3_4_0)");
+        runExistingLine(all_ - fixCleanup3_4_0, tecINTERNAL);
+
+        testcase(
+            "bug: VaultWithdraw to self succeeds after issuer clears "
+            "asfDefaultRipple when the trust line exists (post-fixCleanup3_4_0)");
+        runExistingLine(all_, tesSUCCESS);
+
+        testcase(
+            "bug: VaultWithdraw to self fails with tecINTERNAL after issuer "
+            "clears asfDefaultRipple and the trust line was deleted "
+            "(pre-fixCleanup3_4_0)");
+        runDeletedLine(all_ - fixCleanup3_4_0, tecINTERNAL);
+
+        testcase(
+            "bug: VaultWithdraw to self fails with terNO_RIPPLE after issuer "
+            "clears asfDefaultRipple and the trust line was deleted "
+            "(post-fixCleanup3_4_0)");
+        runDeletedLine(all_, terNO_RIPPLE);
+
+        testcase(
+            "bug: LoanBrokerCoverWithdraw to self fails with tecINTERNAL after "
+            "issuer clears asfDefaultRipple even though the trust line exists "
+            "(pre-fixCleanup3_4_0)");
+        runCoverWithdraw(all_ - fixCleanup3_4_0, tecINTERNAL);
+
+        testcase(
+            "bug: LoanBrokerCoverWithdraw to self succeeds after issuer clears "
+            "asfDefaultRipple when the trust line exists (post-fixCleanup3_4_0)");
+        runCoverWithdraw(all_, tesSUCCESS);
+
+        testcase(
+            "bug: private VaultWithdraw to self fails with tecINTERNAL after "
+            "issuer clears asfDefaultRipple; third-party dest still works "
+            "(pre-fixCleanup3_4_0)");
+        runPrivateVault(all_ - fixCleanup3_4_0, tecINTERNAL, tesSUCCESS);
+
+        testcase(
+            "bug: private VaultWithdraw to self succeeds after issuer clears "
+            "asfDefaultRipple; third-party dest is tecNO_AUTH "
+            "(post-fixCleanup3_4_0)");
+        runPrivateVault(all_, tesSUCCESS, tecNO_AUTH);
+    }
+
 public:
     void
     run() override
@@ -1695,6 +1945,7 @@ public:
         testBugClawbackRoundTripOvershoot();
         testBugWithdrawRoundTripOvershoot();
         testBugClawbackAfterLoanImpair();
+        testBugSelfWithdrawAfterIssuerClearsDefaultRipple();
     }
 };