From f5fd243601d53df645a687fb2c453f2b9ed6de01 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:09:11 +0200 Subject: [PATCH 1/5] 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. --- .../xrpl/ledger/helpers/RippleStateHelpers.h | 8 +- include/xrpl/ledger/helpers/TokenHelpers.h | 6 + src/libxrpl/ledger/helpers/MPTokenHelpers.cpp | 2 + .../ledger/helpers/RippleStateHelpers.cpp | 16 +- src/libxrpl/ledger/helpers/TokenHelpers.cpp | 26 ++ .../lending/LoanBrokerCoverWithdraw.cpp | 7 + .../tx/transactors/vault/VaultWithdraw.cpp | 9 + src/test/app/vault/VaultBugs_test.cpp | 251 ++++++++++++++++++ 8 files changed, 318 insertions(+), 7 deletions(-) diff --git a/include/xrpl/ledger/helpers/RippleStateHelpers.h b/include/xrpl/ledger/helpers/RippleStateHelpers.h index a0508d074f..5350182cff 100644 --- a/include/xrpl/ledger/helpers/RippleStateHelpers.h +++ b/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( diff --git a/include/xrpl/ledger/helpers/TokenHelpers.h b/include/xrpl/ledger/helpers/TokenHelpers.h index 5153b43cb2..2a2f1b568e 100644 --- a/include/xrpl/ledger/helpers/TokenHelpers.h +++ b/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, diff --git a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp b/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp index 1b9bb19ad4..27dcd84675 100644 --- a/src/libxrpl/ledger/helpers/MPTokenHelpers.cpp +++ b/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))) diff --git a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp index 706564db6f..6fb72fd7f4 100644 --- a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp +++ b/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. diff --git a/src/libxrpl/ledger/helpers/TokenHelpers.cpp b/src/libxrpl/ledger/helpers/TokenHelpers.cpp index aaf99a3c0a..2c2c943a9f 100644 --- a/src/libxrpl/ledger/helpers/TokenHelpers.cpp +++ b/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( + [&](TIss const& issue) -> bool { + return holdingExists(view, account, issue); + }, + asset.value()); +} + TER addEmptyHolding( ApplyViewContext ctx, diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp index e914596599..88b6f8c38b 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerCoverWithdraw.cpp +++ b/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 = diff --git a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp b/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp index 697612af3e..4dc5b95c89 100644 --- a/src/libxrpl/tx/transactors/vault/VaultWithdraw.cpp +++ b/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 diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index 04f31c9526..9a2ea5124d 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -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(); + 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(); + 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(); + 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(); } }; From 249e6133e2c12c3ded72ea43b3d9d8a0c4fb1ebc Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Tue, 1 Sep 2026 16:10:02 +0200 Subject: [PATCH 2/5] chore: Include base_uint.h for uint256 to_string clang-tidy include-cleaner requires a direct include for to_string(domainId) in the private-vault DefaultRipple test. --- src/test/app/vault/VaultBugs_test.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index 9a2ea5124d..ee13c1035e 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -15,6 +15,7 @@ #include #include +#include #include #include #include From 8720fa025a7bd2974b6dd69194666a29a8d882bd Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:04:56 +0200 Subject: [PATCH 3/5] test: Assert freeze and payout deltas for addEmptyHolding self-withdraw --- src/test/app/vault/VaultBugs_test.cpp | 75 +++++++++++++++++++++++---- 1 file changed, 65 insertions(+), 10 deletions(-) diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index d5d3b924ef..c0983511ad 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -1683,13 +1683,17 @@ private: // 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. + // create-path DefaultRipple miss to terNO_RIPPLE. Global freeze on an + // existing line is still rejected later by checkWithdrawFreeze. void testBugSelfWithdrawAfterIssuerClearsDefaultRipple() { using namespace test::jtx; - auto runExistingLine = [this](FeatureBitset features, TER selfExpected) { + auto runExistingLine = [this]( + FeatureBitset features, + TER selfExpected, + bool issuerGlobalFreeze = false) { Env env(*this, features); Account const issuer{"issuer"}; Account const alice{"alice"}; @@ -1721,22 +1725,51 @@ private: env(fclear(issuer, asfDefaultRipple)); env.close(); + if (issuerGlobalFreeze) + { + env(fset(issuer, asfGlobalFreeze)); + 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(); + // Alice's USD line is unchanged; a later deposit still succeeds + // unless the issuer is globally frozen. + if (!issuerGlobalFreeze) + { + env(vault.deposit({.depositor = alice, .id = vaultKeylet.key, .amount = usd(10)})); + env.close(); + } + + Number const destBefore = env.balance(alice, usd.raw()).number(); + Number const vaultBefore = env.le(vaultKeylet)->at(sfAssetsTotal); + Number const withdrawAmt{50}; 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(); + Number const destAfter = env.balance(alice, usd.raw()).number(); + Number const vaultAfter = env.le(vaultKeylet)->at(sfAssetsTotal); + if (isTesSuccess(selfExpected)) + { + BEAST_EXPECT(destAfter == destBefore + withdrawAmt); + BEAST_EXPECT(vaultAfter == vaultBefore - withdrawAmt); + } + else + { + BEAST_EXPECT(destAfter == destBefore); + BEAST_EXPECT(vaultAfter == vaultBefore); + } + + if (!issuerGlobalFreeze) + { + 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) { @@ -1815,8 +1848,25 @@ private: env.close(); BEAST_EXPECT(env.le(keylet::trustLine(alice.id(), usdIssue))); + Number const destBefore = env.balance(alice, usd.raw()).number(); + Number const coverBefore = env.le(brokerKeylet)->at(sfCoverAvailable); + Number const withdrawAmt{50}; + env(coverWithdraw(alice, brokerKeylet.key, usd(50).value()), Ter(selfExpected)); env.close(); + + Number const destAfter = env.balance(alice, usd.raw()).number(); + Number const coverAfter = env.le(brokerKeylet)->at(sfCoverAvailable); + if (isTesSuccess(selfExpected)) + { + BEAST_EXPECT(destAfter == destBefore + withdrawAmt); + BEAST_EXPECT(coverAfter == coverBefore - withdrawAmt); + } + else + { + BEAST_EXPECT(destAfter == destBefore); + BEAST_EXPECT(coverAfter == coverBefore); + } }; auto runPrivateVault = [this](FeatureBitset features, TER selfExpected, TER destExpected) { @@ -1890,6 +1940,11 @@ private: "asfDefaultRipple when the trust line exists (post-fixCleanup3_4_0)"); runExistingLine(all_, tesSUCCESS); + testcase( + "bug: VaultWithdraw to self with an existing line still gets " + "tecFROZEN under asfGlobalFreeze (post-fixCleanup3_4_0)"); + runExistingLine(all_, tecFROZEN, true); + testcase( "bug: VaultWithdraw to self fails with tecINTERNAL after issuer " "clears asfDefaultRipple and the trust line was deleted " From 02b6e2b6533b7de8ebdf783f83082cc012974ee4 Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:32:53 +0200 Subject: [PATCH 4/5] chore: Use exists() for addEmptyHolding trust-line presence --- src/libxrpl/ledger/helpers/RippleStateHelpers.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp index 6fb72fd7f4..b1e6a47f19 100644 --- a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp +++ b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp @@ -659,7 +659,7 @@ addEmptyHolding( // 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)) + if (fix340Enabled && ctx.view.exists(index)) return tecDUPLICATE; if (isGlobalFrozen(ctx.view, issuerId)) @@ -672,7 +672,7 @@ addEmptyHolding( if (!sleSrc->isFlag(lsfDefaultRipple)) return fix340Enabled ? TER{terNO_RIPPLE} : tecINTERNAL; // If the line already exists, don't create it again. - if (!fix340Enabled && ctx.view.read(index)) + if (!fix340Enabled && ctx.view.exists(index)) return tecDUPLICATE; // A reserve sponsor only covers tx.Account's own objects. From 64284358828313af910fedbf19c0e7a355d0a85e Mon Sep 17 00:00:00 2001 From: Vito <5780819+Tapanito@users.noreply.github.com> Date: Wed, 2 Sep 2026 10:55:57 +0200 Subject: [PATCH 5/5] fix: Skip canAddHolding in LoanSet when the holding already exists --- .../xrpl/ledger/helpers/RippleStateHelpers.h | 14 ++-- .../ledger/helpers/RippleStateHelpers.cpp | 5 ++ .../tx/transactors/lending/LoanSet.cpp | 14 +++- src/test/app/lending/LoanPay_test.cpp | 65 +++++++++++++++ src/test/app/lending/LoanSet_test.cpp | 61 ++++++++++++++ src/test/app/vault/VaultBugs_test.cpp | 83 +++++++++++++++---- 6 files changed, 216 insertions(+), 26 deletions(-) diff --git a/include/xrpl/ledger/helpers/RippleStateHelpers.h b/include/xrpl/ledger/helpers/RippleStateHelpers.h index 5350182cff..9ee96b0060 100644 --- a/include/xrpl/ledger/helpers/RippleStateHelpers.h +++ b/include/xrpl/ledger/helpers/RippleStateHelpers.h @@ -239,12 +239,14 @@ canTransfer(ReadView const& view, Issue const& issue, AccountID const& from, Acc //------------------------------------------------------------------------------ /** - * 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. + * After fixCleanup3_4_0, if the destination already holds this IOU, returns + * tecDUPLICATE and does not consult issuer freeze or DefaultRipple. Freeze + * and DefaultRipple still apply on the create path (DefaultRipple off is + * terNO_RIPPLE). Transactors that may create a new holding in doApply must + * call canAddHolding() in preclaim only when that destination does not + * already hold the asset. Do not call canAddHolding() merely because + * addEmptyHolding() is invoked: that helper does not check whether a + * holding already exists. */ [[nodiscard]] TER addEmptyHolding( diff --git a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp index b1e6a47f19..cc02b56305 100644 --- a/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp +++ b/src/libxrpl/ledger/helpers/RippleStateHelpers.cpp @@ -669,6 +669,11 @@ addEmptyHolding( auto const sleDst = ctx.view.peek(keylet::account(dstId)); if (!sleDst || !sleSrc) return tefINTERNAL; // LCOV_EXCL_LINE + // Create path: DefaultRipple is still required. terNO_RIPPLE is + // intentional so VaultWithdraw / CoverWithdraw fail in preclaim via + // canAddHolding (retryable, no fee) rather than claiming a tec* fee + // in doApply. Transactor::operator() will not apply and will not + // convert it to tefINTERNAL. if (!sleSrc->isFlag(lsfDefaultRipple)) return fix340Enabled ? TER{terNO_RIPPLE} : tecINTERNAL; // If the line already exists, don't create it again. diff --git a/src/libxrpl/tx/transactors/lending/LoanSet.cpp b/src/libxrpl/tx/transactors/lending/LoanSet.cpp index b67c244bac..9ad76212f6 100644 --- a/src/libxrpl/tx/transactors/lending/LoanSet.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanSet.cpp @@ -372,8 +372,18 @@ LoanSet::preclaim(PreclaimContext const& ctx) } } - if (auto const ter = canAddHolding(ctx.view, asset)) - return ter; + // canAddHolding does not look at existing lines. After + // fixCleanup3_4_0, addEmptyHolding is a no-op when the destination + // already holds the asset, so skip this gate unless a holding would + // actually be created (borrower always; broker owner if there is an + // origination fee). + auto const originationFee = tx[~sfLoanOriginationFee].value_or(Number{}); + if (!ctx.view.rules().enabled(fixCleanup3_4_0) || !holdingExists(ctx.view, borrower, asset) || + (originationFee != beast::kZero && !holdingExists(ctx.view, brokerOwner, asset))) + { + if (auto const ter = canAddHolding(ctx.view, asset)) + return ter; + } // vaultPseudo is going to send funds, so it can't be frozen. if (auto const ret = checkFrozen(ctx.view, vaultPseudo, asset)) diff --git a/src/test/app/lending/LoanPay_test.cpp b/src/test/app/lending/LoanPay_test.cpp index 038ef4067b..3360e68272 100644 --- a/src/test/app/lending/LoanPay_test.cpp +++ b/src/test/app/lending/LoanPay_test.cpp @@ -37,6 +37,7 @@ #include #include #include +#include #include namespace xrpl::test { @@ -1419,6 +1420,69 @@ private: BEAST_EXPECT(stateAfter.nextPaymentDate == exactDueDate); } + // LoanPay does not call canAddHolding. addEmptyHolding recreates the + // broker-owner holding when the borrower is also the broker owner. After + // fixCleanup3_4_0 an existing line is a no-op even if DefaultRipple is + // off; pre-fix that path dies with tecINTERNAL. + void + testLoanPaySelfBrokerExistingLineDefaultRipple() + { + using namespace jtx; + using namespace loan; + + auto run = [this](FeatureBitset features, TER expected) { + testcase( + std::string( + "LoanPay broker-owner borrower existing line after " + "issuer clears asfDefaultRipple (") + + (features[fixCleanup3_4_0] ? "post" : "pre") + "-fixCleanup3_4_0)"); + + 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"]}; + env(trust(alice, usd(100'000))); + env.close(); + env(pay(issuer, alice, usd(50'000))); + env.close(); + + auto const broker = createVaultAndBroker(env, usd, alice); + auto const brokerSle = env.le(keylet::loanBroker(broker.brokerID)); + if (!BEAST_EXPECT(brokerSle)) + return; + auto const loanKeylet = + keylet::loan(broker.brokerID, SeqProxy::rawSequence(brokerSle->at(sfLoanSequence))); + + Number const serviceFee = usd(2).value(); + env(set(alice, broker.brokerID, usd(1'000).value()), + Sig(sfCounterpartySignature, alice), + kLoanServiceFee(serviceFee), + Fee(env.current()->fees().base * 2)); + env.close(); + + env(fclear(issuer, asfDefaultRipple)); + env.close(); + BEAST_EXPECT(env.le(keylet::trustLine(alice.id(), usd.raw().get()))); + + auto const state = getCurrentState(env, broker, loanKeylet); + STAmount const payment{ + usd, + roundPeriodicPayment(usd, state.periodicPayment + serviceFee, state.loanScale)}; + + env(pay(alice, loanKeylet.key, payment), Ter(expected)); + env.close(); + }; + + run(all_ - fixCleanup3_4_0, tecINTERNAL); + run(all_, tesSUCCESS); + } + void runAmendmentIndependent() { @@ -1429,6 +1493,7 @@ private: testLoanPayCatchUpFeeAtExactDueDatePostAmendment(); testLoanPayCatchUpFeeAtExactDueDatePreAmendment(); testRepayIntoUnauthorizedVault(); + testLoanPaySelfBrokerExistingLineDefaultRipple(); } // Tests run under each entry in amendmentCombinations(). diff --git a/src/test/app/lending/LoanSet_test.cpp b/src/test/app/lending/LoanSet_test.cpp index 9829f23138..9e2202a2bb 100644 --- a/src/test/app/lending/LoanSet_test.cpp +++ b/src/test/app/lending/LoanSet_test.cpp @@ -31,6 +31,7 @@ #include #include #include +#include #include #include @@ -764,6 +765,65 @@ private: }); } + // LoanSet used to call canAddHolding unconditionally, so an existing + // borrower line still failed with terNO_RIPPLE after the issuer cleared + // DefaultRipple. After fixCleanup3_4_0, skip that gate when the holding + // already exists. + void + testLoanSetExistingLineAfterIssuerClearsDefaultRipple() + { + using namespace jtx; + using namespace loan; + + auto run = [this](FeatureBitset features, TER expected) { + testcase( + std::string( + "LoanSet existing borrower line after issuer " + "clears asfDefaultRipple (") + + (features[fixCleanup3_4_0] ? "post" : "pre") + "-fixCleanup3_4_0)"); + + Env env(*this, features); + Account const issuer{"issuer"}; + Account const lender{"lender"}; + Account const borrower{"borrower"}; + + env.fund(XRP(10'000), issuer, lender, borrower); + env.close(); + env(fset(issuer, asfDefaultRipple)); + env.close(); + + PrettyAsset const usd{issuer["USD"]}; + env(trust(lender, usd(100'000))); + env(trust(borrower, usd(100'000))); + env.close(); + env(pay(issuer, lender, usd(50'000))); + env(pay(issuer, borrower, usd(1'000))); + env.close(); + BEAST_EXPECT(env.le(keylet::trustLine(borrower.id(), usd.raw().get()))); + + auto const broker = createVaultAndBroker(env, usd, lender); + + env(fclear(issuer, asfDefaultRipple)); + env.close(); + + Number const destBefore = env.balance(borrower, usd.raw()).number(); + env(set(borrower, broker.brokerID, usd(100).value()), + Sig(sfCounterpartySignature, lender), + Fee(env.current()->fees().base * 2), + Ter(expected)); + env.close(); + + Number const destAfter = env.balance(borrower, usd.raw()).number(); + if (isTesSuccess(expected)) + BEAST_EXPECT(destAfter == destBefore + Number{100}); + else + BEAST_EXPECT(destAfter == destBefore); + }; + + run(all_ - fixCleanup3_4_0, terNO_RIPPLE); + run(all_, tesSUCCESS); + } + public: void run() override @@ -773,6 +833,7 @@ public: testLoanSet(features); testLoanSetClosedEnded(); + testLoanSetExistingLineAfterIssuerClearsDefaultRipple(); } }; diff --git a/src/test/app/vault/VaultBugs_test.cpp b/src/test/app/vault/VaultBugs_test.cpp index c0983511ad..5b6e756e59 100644 --- a/src/test/app/vault/VaultBugs_test.cpp +++ b/src/test/app/vault/VaultBugs_test.cpp @@ -1869,23 +1869,67 @@ private: } }; - auto runPrivateVault = [this](FeatureBitset features, TER selfExpected, TER destExpected) { + auto runDeletedCoverWithdraw = [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(); + env(trust(alice, usd(10'000))); + env.close(); + env(pay(issuer, alice, usd(600))); + 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(trust(alice, usd(0))); + env.close(); + BEAST_EXPECT(!env.le(keylet::trustLine(alice.id(), usdIssue))); + env(fclear(issuer, asfDefaultRipple)); + env.close(); + + env(coverWithdraw(alice, brokerKeylet.key, usd(50).value()), Ter(selfExpected)); + env.close(); + }; + + auto runPrivateVault = [this](FeatureBitset features, TER selfExpected) { 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.fund(XRP(10'000), issuer, alice, 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(); @@ -1919,14 +1963,6 @@ private: 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( @@ -1969,16 +2005,27 @@ private: runCoverWithdraw(all_, tesSUCCESS); testcase( - "bug: private VaultWithdraw to self fails with tecINTERNAL after " - "issuer clears asfDefaultRipple; third-party dest still works " + "bug: LoanBrokerCoverWithdraw to self fails with tecINTERNAL after " + "issuer clears asfDefaultRipple and the trust line was deleted " "(pre-fixCleanup3_4_0)"); - runPrivateVault(all_ - fixCleanup3_4_0, tecINTERNAL, tesSUCCESS); + runDeletedCoverWithdraw(all_ - fixCleanup3_4_0, tecINTERNAL); + + testcase( + "bug: LoanBrokerCoverWithdraw to self fails with terNO_RIPPLE after " + "issuer clears asfDefaultRipple and the trust line was deleted " + "(post-fixCleanup3_4_0)"); + runDeletedCoverWithdraw(all_, terNO_RIPPLE); + + testcase( + "bug: private VaultWithdraw to self fails with tecINTERNAL after " + "issuer clears asfDefaultRipple even though the trust line exists " + "(pre-fixCleanup3_4_0)"); + runPrivateVault(all_ - fixCleanup3_4_0, tecINTERNAL); 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); + "asfDefaultRipple when the trust line exists (post-fixCleanup3_4_0)"); + runPrivateVault(all_, tesSUCCESS); } // Bug 1: a sponsored XRP VaultWithdraw to a distinct destination is