From 9e1fe9a85e800b11bd82e594eff5198f64c57c3c Mon Sep 17 00:00:00 2001 From: Bronek Kozicki Date: Wed, 28 May 2025 15:28:18 +0100 Subject: [PATCH] Fix: Improve handling of expired credentials in `VaultDeposit` (#5452) This change returns `tecEXPIRED` from VaultDeposit to allow the Transactor to remove the expired credentials. --- src/test/app/Vault_test.cpp | 96 ++++++++++++++++++++---- src/xrpld/app/misc/CredentialHelpers.cpp | 6 +- src/xrpld/ledger/detail/View.cpp | 19 ++++- 3 files changed, 100 insertions(+), 21 deletions(-) diff --git a/src/test/app/Vault_test.cpp b/src/test/app/Vault_test.cpp index 67cc3812df..5aab737669 100644 --- a/src/test/app/Vault_test.cpp +++ b/src/test/app/Vault_test.cpp @@ -2273,6 +2273,8 @@ class Vault_test : public beast::unit_test::suite env(pay(issuer, owner, asset(500))); env.trust(asset(1000), depositor); env(pay(issuer, depositor, asset(500))); + env.trust(asset(1000), charlie); + env(pay(issuer, charlie, asset(5))); env.close(); auto [tx, keylet] = vault.create( @@ -2362,7 +2364,7 @@ class Vault_test : public beast::unit_test::suite env(credentials::create(depositor, credIssuer1, credType)); env(credentials::accept(depositor, credIssuer1, credType)); env(credentials::create(charlie, credIssuer1, credType)); - env(credentials::accept(charlie, credIssuer1, credType)); + // charlie's credential not accepted env.close(); auto credSle = env.le(credKeylet); BEAST_EXPECT(credSle != nullptr); @@ -2376,7 +2378,7 @@ class Vault_test : public beast::unit_test::suite tx = vault.deposit( {.depositor = charlie, .id = keylet.key, .amount = asset(50)}); - env(tx, ter{tecINSUFFICIENT_FUNDS}); + env(tx, ter{tecNO_AUTH}); env.close(); } @@ -2384,6 +2386,8 @@ class Vault_test : public beast::unit_test::suite testcase("private vault depositor lost authorization"); env(credentials::deleteCred( credIssuer1, depositor, credIssuer1, credType)); + env(credentials::deleteCred( + credIssuer1, charlie, credIssuer1, credType)); env.close(); auto credSle = env.le(credKeylet); BEAST_EXPECT(credSle == nullptr); @@ -2396,18 +2400,84 @@ class Vault_test : public beast::unit_test::suite env.close(); } - { - testcase("private vault depositor new authorization"); - env(credentials::create(depositor, credIssuer2, credType)); - env(credentials::accept(depositor, credIssuer2, credType)); - env.close(); + auto const shares = [&env, keylet = keylet, this]() -> Asset { + auto const vault = env.le(keylet); + BEAST_EXPECT(vault != nullptr); + return MPTIssue(vault->at(sfShareMPTID)); + }(); - auto tx = vault.deposit( - {.depositor = depositor, - .id = keylet.key, - .amount = asset(50)}); - env(tx); - env.close(); + { + testcase("private vault expired authorization"); + uint32_t const closeTime = env.current() + ->info() + .parentCloseTime.time_since_epoch() + .count(); + { + auto tx0 = + credentials::create(depositor, credIssuer2, credType); + tx0[sfExpiration] = closeTime + 20; + env(tx0); + tx0 = credentials::create(charlie, credIssuer2, credType); + tx0[sfExpiration] = closeTime + 20; + env(tx0); + env.close(); + + env(credentials::accept(depositor, credIssuer2, credType)); + env(credentials::accept(charlie, credIssuer2, credType)); + env.close(); + } + + { + auto tx1 = vault.deposit( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(50)}); + env(tx1); + env.close(); + + auto const tokenKeylet = keylet::mptoken( + shares.get().getMptID(), depositor.id()); + BEAST_EXPECT(env.le(tokenKeylet) != nullptr); + } + + { + // time advance + env.close(); + env.close(); + env.close(); + + auto const credsKeylet = + credentials::keylet(depositor, credIssuer2, credType); + BEAST_EXPECT(env.le(credsKeylet) != nullptr); + + auto tx2 = vault.deposit( + {.depositor = depositor, + .id = keylet.key, + .amount = asset(1)}); + env(tx2, ter{tecEXPIRED}); + env.close(); + + BEAST_EXPECT(env.le(credsKeylet) == nullptr); + } + + { + auto const credsKeylet = + credentials::keylet(charlie, credIssuer2, credType); + BEAST_EXPECT(env.le(credsKeylet) != nullptr); + auto const tokenKeylet = keylet::mptoken( + shares.get().getMptID(), charlie.id()); + BEAST_EXPECT(env.le(tokenKeylet) == nullptr); + + auto tx3 = vault.deposit( + {.depositor = charlie, + .id = keylet.key, + .amount = asset(2)}); + env(tx3, ter{tecEXPIRED}); + + env.close(); + BEAST_EXPECT(env.le(credsKeylet) == nullptr); + BEAST_EXPECT(env.le(tokenKeylet) == nullptr); + } } { diff --git a/src/xrpld/app/misc/CredentialHelpers.cpp b/src/xrpld/app/misc/CredentialHelpers.cpp index 03ad1f9c80..81355f1792 100644 --- a/src/xrpld/app/misc/CredentialHelpers.cpp +++ b/src/xrpld/app/misc/CredentialHelpers.cpp @@ -336,9 +336,7 @@ verifyValidDomain( credentials.push_back(keyletCredential.key); } - // Result intentionally ignored. - [[maybe_unused]] bool _ = credentials::removeExpired(view, credentials, j); - + bool const foundExpired = credentials::removeExpired(view, credentials, j); for (auto const& h : credentials) { auto sleCredential = view.read(keylet::credential(h)); @@ -349,7 +347,7 @@ verifyValidDomain( return tesSUCCESS; } - return tecNO_PERMISSION; + return foundExpired ? tecEXPIRED : tecNO_PERMISSION; } TER diff --git a/src/xrpld/ledger/detail/View.cpp b/src/xrpld/ledger/detail/View.cpp index aa6e2dda8f..e9499a287a 100644 --- a/src/xrpld/ledger/detail/View.cpp +++ b/src/xrpld/ledger/detail/View.cpp @@ -2391,8 +2391,19 @@ enforceMPTokenAuthorization( auto const keylet = keylet::mptoken(mptIssuanceID, account); auto const sleToken = view.read(keylet); // NOTE: might be null auto const maybeDomainID = sleIssuance->at(~sfDomainID); - bool const authorizedByDomain = maybeDomainID.has_value() && - verifyValidDomain(view, account, *maybeDomainID, j) == tesSUCCESS; + bool expired = false; + bool const authorizedByDomain = [&]() -> bool { + // NOTE: defensive here, shuld be checked in preclaim + if (!maybeDomainID.has_value()) + return false; // LCOV_EXCL_LINE + + auto const ter = verifyValidDomain(view, account, *maybeDomainID, j); + if (isTesSuccess(ter)) + return true; + if (ter == tecEXPIRED) + expired = true; + return false; + }(); if (!authorizedByDomain && sleToken == nullptr) { @@ -2403,14 +2414,14 @@ enforceMPTokenAuthorization( // 3. Account has all expired credentials (deleted in verifyValidDomain) // // Either way, return tecNO_AUTH and there is nothing else to do - return tecNO_AUTH; + return expired ? tecEXPIRED : tecNO_AUTH; } else if (!authorizedByDomain && maybeDomainID.has_value()) { // Found an MPToken but the account is not authorized and we expect // it to have been authorized by the domain. This could be because the // credentials used to create the MPToken have expired or been deleted. - return tecNO_AUTH; + return expired ? tecEXPIRED : tecNO_AUTH; } else if (!authorizedByDomain) {