diff --git a/include/xrpl/ledger/helpers/CredentialHelpers.h b/include/xrpl/ledger/helpers/CredentialHelpers.h index 8e78a009235..bcb4e5d3e20 100644 --- a/include/xrpl/ledger/helpers/CredentialHelpers.h +++ b/include/xrpl/ledger/helpers/CredentialHelpers.h @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -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); diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index e83e1c97b62..b0d6bd052f4 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/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 */ diff --git a/src/libxrpl/ledger/helpers/AMMHelpers.cpp b/src/libxrpl/ledger/helpers/AMMHelpers.cpp index df6d335085a..b73b2ded45c 100644 --- a/src/libxrpl/ledger/helpers/AMMHelpers.cpp +++ b/src/libxrpl/ledger/helpers/AMMHelpers.cpp @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -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(tecINTERNAL); // LCOV_EXCL_LINE auto const lowLimit = sle->getFieldAmount(sfLowLimit); diff --git a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp index 226ea100e9c..3a9621bb880 100644 --- a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp +++ b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp @@ -5,8 +5,10 @@ #include #include #include +#include #include #include +#include #include #include #include @@ -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 { + if (nodeType == ltCREDENTIAL) + return {deleteSLE(view, sleItem, j), SkipEntry::No}; + + return {tesSUCCESS, SkipEntry::Yes}; + }, + j, + maxNodesToDelete); +} + NotTEC checkFields(STTx const& tx, beast::Journal j) { diff --git a/src/libxrpl/tx/Transactor.cpp b/src/libxrpl/tx/Transactor.cpp index 4b562692d75..1ea6e9c41b1 100644 --- a/src/libxrpl/tx/Transactor.cpp +++ b/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 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) { diff --git a/src/libxrpl/tx/invariants/MPTInvariant.cpp b/src/libxrpl/tx/invariants/MPTInvariant.cpp index 77c5ad781e3..b27a1f28af3 100644 --- a/src/libxrpl/tx/invariants/MPTInvariant.cpp +++ b/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 " diff --git a/src/libxrpl/tx/transactors/credentials/CredentialCreate.cpp b/src/libxrpl/tx/transactors/credentials/CredentialCreate.cpp index e902ee73a6c..a514119a5d4 100644 --- a/src/libxrpl/tx/transactors/credentials/CredentialCreate.cpp +++ b/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."; diff --git a/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp b/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp index b36977d2256..9b862d6b1aa 100644 --- a/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp +++ b/src/libxrpl/tx/transactors/lending/LoanBrokerDelete.cpp @@ -4,11 +4,13 @@ #include #include #include +#include #include #include #include #include #include +#include #include #include #include @@ -141,6 +143,19 @@ LoanBrokerDelete::doApply() 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)) { diff --git a/src/libxrpl/tx/transactors/vault/VaultDelete.cpp b/src/libxrpl/tx/transactors/vault/VaultDelete.cpp index 497a2f24654..f579829046e 100644 --- a/src/libxrpl/tx/transactors/vault/VaultDelete.cpp +++ b/src/libxrpl/tx/transactors/vault/VaultDelete.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -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); diff --git a/src/test/app/AMM_test.cpp b/src/test/app/AMM_test.cpp index f19743026c4..c21facff5f1 100644 --- a/src/test/app/AMM_test.cpp +++ b/src/test/app/AMM_test.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include #include @@ -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(); diff --git a/src/test/app/LoanBroker_test.cpp b/src/test/app/LoanBroker_test.cpp index f6f85a0ccab..2a1547137f7 100644 --- a/src/test/app/LoanBroker_test.cpp +++ b/src/test/app/LoanBroker_test.cpp @@ -6,6 +6,7 @@ #include #include #include +#include #include #include #include @@ -2723,6 +2724,72 @@ class LoanBroker_test : public beast::unit_test::Suite runTestCases(all_ - fixCleanup3_2_0); } + void + testCredentialPinsPseudoAccount() + { + using namespace test::jtx; + using namespace loanBroker; + + // FN-36: a credential issued to a LoanBroker pseudo-account can't be + // accepted or deleted by it, so it stays pinned in the pseudo-account's + // owner directory and blocks LoanBrokerDelete with tecHAS_OBLIGATIONS. + // fixCleanup3_4_0 rejects such credentials up front and, for + // pins created before the amendment, removes them on LoanBrokerDelete. + Account const alice{"alice"}; // vault & broker owner + Account const attacker{"attacker"}; + char const credType[] = "FN36"; + + Env env{*this, all_ - fixCleanup3_4_0}; + env.fund(XRP(1'000'000), alice, attacker); + env.close(); + + Vault const vault{env}; + auto [vtx, vkeylet] = vault.create({.owner = alice, .asset = xrpIssue()}); + env(vtx); + env.close(); + BEAST_EXPECT(env.le(vkeylet)); + + auto const brokerKeylet = keylet::loanBroker(alice.id(), env.seq(alice)); + env(set(alice.id(), vkeylet.key)); + env.close(); + + auto const broker = env.le(brokerKeylet); + BEAST_EXPECT(broker); + Account const pseudo{"broker pseudo-account", broker->at(sfAccount)}; + env.memoize(pseudo); + + // Before the amendment: the pin succeeds and blocks broker deletion. + testcase("Credential pins broker pseudo-account (amendment disabled)"); + env(credentials::create(pseudo, attacker, credType)); + env.close(); + + auto const credKey = credentials::keylet(pseudo, attacker, credType); + BEAST_EXPECT(env.le(credKey)); + BEAST_EXPECT(ownerCount(env, attacker) == 1); + + env(del(alice.id(), brokerKeylet.key), Ter(tecHAS_OBLIGATIONS)); + env.close(); + + env.enableFeature(fixCleanup3_4_0); + env.close(); + + // Part 1: a new pin is rejected outright. + testcase("Credential on broker pseudo-account rejected (amendment enabled)"); + env(credentials::create(pseudo, attacker, "FN36B"), Ter(tecPSEUDO_ACCOUNT)); + env.close(); + + // Part 2: the pre-existing pin no longer blocks deletion; the credential + // is cleaned up and the issuer's owner count is restored. + testcase("LoanBrokerDelete removes pinned credential (amendment enabled)"); + env(del(alice.id(), brokerKeylet.key)); + env.close(); + + BEAST_EXPECT(!env.le(credKey)); + BEAST_EXPECT(!env.le(brokerKeylet)); + BEAST_EXPECT(!env.le(keylet::account(pseudo.id()))); + BEAST_EXPECT(ownerCount(env, attacker) == 0); + } + public: void run() override @@ -2741,6 +2808,7 @@ class LoanBroker_test : public beast::unit_test::Suite testDisabled(); testLifecycle(); + testCredentialPinsPseudoAccount(); testInvalidLoanBrokerDelete(); testInvalidLoanBrokerSet(); testRequireAuth(); diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 12ad7e6782d..570777dd24a 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -7645,6 +7645,124 @@ class Vault_test : public beast::unit_test::Suite } } + void + testCredentialPinsPseudoAccount() + { + using namespace test::jtx; + + // FN-36: a credential issued to a vault pseudo-account can't be + // accepted or deleted by it (pseudo-accounts can't sign), so it stays + // pinned in the pseudo-account's owner directory and blocks VaultDelete + // with tecHAS_OBLIGATIONS. fixCleanup3_4_0 rejects such + // credentials up front and, for pins created before the amendment, + // removes them on VaultDelete. + Account const owner{"owner"}; + Account const attacker{"attacker"}; + char const credType[] = "FN36"; + + Env env{*this, all_ - fixCleanup3_4_0}; + env.fund(XRP(1'000'000), owner, attacker); + env.close(); + + Vault const vault{env}; + PrettyAsset const asset = xrpIssue(); + auto [tx, keylet] = vault.create({.owner = owner, .asset = asset}); + env(tx); + env.close(); + + auto const vaultSle = env.le(keylet); + BEAST_EXPECT(vaultSle); + Account const pseudo{"vault pseudo-account", vaultSle->at(sfAccount)}; + env.memoize(pseudo); + + // The pseudo-account owns the share issuance; the pin must not change + // its owner count (an unaccepted credential is owned by the issuer). + auto const pseudoOwnerCount = ownerCount(env, pseudo); + + // Before the amendment: the pin succeeds. + testcase("Credential pins vault pseudo-account (amendment disabled)"); + env(credentials::create(pseudo, attacker, credType)); + env.close(); + + auto const credKey = credentials::keylet(pseudo, attacker, credType); + BEAST_EXPECT(env.le(credKey)); + BEAST_EXPECT(ownerCount(env, attacker) == 1); + BEAST_EXPECT(ownerCount(env, pseudo) == pseudoOwnerCount); + + // The pin blocks deletion of an otherwise-empty vault. + env(vault.del({.owner = owner, .id = keylet.key}), Ter(tecHAS_OBLIGATIONS)); + env.close(); + + env.enableFeature(fixCleanup3_4_0); + env.close(); + + // Part 1: a new pin is rejected outright. + testcase("Credential on vault pseudo-account rejected (amendment enabled)"); + env(credentials::create(pseudo, attacker, "FN36B"), Ter(tecPSEUDO_ACCOUNT)); + env.close(); + + // Part 2: the pre-existing pin no longer blocks deletion; the credential + // is cleaned up and the issuer's owner count is restored. + testcase("VaultDelete removes pinned credential (amendment enabled)"); + env(vault.del({.owner = owner, .id = keylet.key})); + env.close(); + + BEAST_EXPECT(!env.le(credKey)); + BEAST_EXPECT(!env.le(keylet)); + BEAST_EXPECT(!env.le(::xrpl::keylet::account(pseudo.id()))); + BEAST_EXPECT(ownerCount(env, attacker) == 0); + } + + void + testCredentialPinOverflow() + { + using namespace test::jtx; + testcase("Credential pin cleanup is bounded (tecINCOMPLETE)"); + + // A pseudo-account can be pinned with more credentials than one + // transaction is allowed to clean up. VaultDelete then removes them a + // bounded batch at a time, returning tecINCOMPLETE until the last batch. + Account const owner{"owner"}; + Account const attacker{"attacker"}; + + Env env{*this, all_ - fixCleanup3_4_0}; + env.fund(XRP(10'000'000), owner, attacker); + env.close(); + + Vault const vault{env}; + auto [tx, keylet] = vault.create({.owner = owner, .asset = xrpIssue()}); + env(tx); + env.close(); + auto const vaultSle = env.le(keylet); + BEAST_EXPECT(vaultSle); + Account const pseudo{"vault pseudo-account", vaultSle->at(sfAccount)}; + env.memoize(pseudo); + + // Pin more than one cleanup batch's worth of credentials. + std::uint16_t const count = kMaxDeletablePseudoAccountCredentials + 3; + for (std::uint16_t i = 0; i < count; ++i) + env(credentials::create(pseudo, attacker, std::to_string(i))); + env.close(); + BEAST_EXPECT(ownerCount(env, attacker) == count); + + env.enableFeature(fixCleanup3_4_0); + env.close(); + + // First delete removes one bounded batch and reports it isn't finished. + env(vault.del({.owner = owner, .id = keylet.key}), Ter(tecINCOMPLETE)); + env.close(); + BEAST_EXPECT(env.le(keylet)); // vault still exists + auto const remaining = ownerCount(env, attacker); + BEAST_EXPECT(remaining > 0 && remaining < count); + + // Second delete finishes the cleanup and removes the vault. + env(vault.del({.owner = owner, .id = keylet.key})); + env.close(); + BEAST_EXPECT(!env.le(keylet)); + BEAST_EXPECT(!env.le(::xrpl::keylet::account(pseudo.id()))); + BEAST_EXPECT(ownerCount(env, attacker) == 0); + } + void testVaultDepositFreezeIOU() { @@ -8317,6 +8435,8 @@ class Vault_test : public beast::unit_test::Suite testVaultEscrowedMPT(); testAssetsMaximum(); testVaultDeleteMemoData(); + testCredentialPinsPseudoAccount(); + testCredentialPinOverflow(); testBug6LimitBypassWithShares(); testRemoveEmptyHoldingLockedAmount(); testRemoveEmptyHoldingConfidentialBalances();