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