fix: Stop tx processing if failed to delete expired credentials (#6715) (#6962)

Co-authored-by: Ed Hennis <ed@ripple.com>
Co-authored-by: Ayaz Salikhov <mathbunnyru@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

include/xrpl/ledger/helpers/CredentialHelpers.h

@@ -19,14 +19,10 @@ namespace credentials {
 
 // Check if credential sfExpiration field has passed ledger's parentCloseTime
 bool
-checkExpired(std::shared_ptr<SLE const> const& sleCredential, NetClock::time_point const& closed);
-
-// Return true if any expired credential was found in arr (and deleted)
-bool
-removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j);
+checkExpired(SLE const& sleCredential, NetClock::time_point const& closed);
 
 // Actually remove a credentials object from the ledger
-TER
+[[nodiscard]] TER
 deleteSLE(ApplyView& view, std::shared_ptr<SLE> const& sleCredential, beast::Journal j);
 
 // Amendment and parameters checks for sfCredentialIDs field

src/libxrpl/ledger/helpers/CredentialHelpers.cpp

@@ -1,5 +1,6 @@
 #include <xrpl/ledger/helpers/CredentialHelpers.h>
 
+#include <xrpl/basics/Expected.h>
 #include <xrpl/basics/Log.h>
 #include <xrpl/basics/Slice.h>
 #include <xrpl/basics/base_uint.h>
@@ -9,6 +10,7 @@
 #include <xrpl/ledger/ReadView.h>
 #include <xrpl/ledger/helpers/AccountRootHelpers.h>
 #include <xrpl/protocol/AccountID.h>
+#include <xrpl/protocol/Feature.h>
 #include <xrpl/protocol/Indexes.h>
 #include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/Protocol.h>
@@ -33,15 +35,16 @@ namespace xrpl {
 namespace credentials {
 
 bool
-checkExpired(std::shared_ptr<SLE const> const& sleCredential, NetClock::time_point const& closed)
+checkExpired(SLE const& sleCredential, NetClock::time_point const& closed)
 {
     std::uint32_t const exp =
-        (*sleCredential)[~sfExpiration].value_or(std::numeric_limits<std::uint32_t>::max());
+        sleCredential[~sfExpiration].value_or(std::numeric_limits<std::uint32_t>::max());
     std::uint32_t const now = closed.time_since_epoch().count();
     return now > exp;
 }
 
-bool
+[[nodiscard]]
+static Expected<bool, TER>
 removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j)
 {
     auto const closeTime = view.header().parentCloseTime;
@@ -53,11 +56,13 @@ removeExpired(ApplyView& view, STVector256 const& arr, beast::Journal const j)
         auto const k = keylet::credential(h);
         auto const sleCred = view.peek(k);
 
-        if (sleCred && checkExpired(sleCred, closeTime))
+        if (sleCred && checkExpired(*sleCred, closeTime))
         {
             JLOG(j.trace()) << "Credentials are expired. Cred: " << sleCred->getText();
             // delete expired credentials even if the transaction failed
-            deleteSLE(view, sleCred, j);
+            auto const err = deleteSLE(view, sleCred, j);
+            if (view.rules().enabled(fixSecurity3_1_3) && !isTesSuccess(err))
+                return Unexpected(err);
             foundExpired = true;
         }
     }
@@ -205,7 +210,7 @@ validDomain(ReadView const& view, uint256 domainID, AccountID const& subject)
         // allows expired credentials to be deleted by any transaction.
         if (sleCredential)
         {
-            if (checkExpired(sleCredential, closeTime))
+            if (checkExpired(*sleCredential, closeTime))
             {
                 foundExpired = true;
                 continue;
@@ -324,7 +329,10 @@ verifyValidDomain(ApplyView& view, AccountID const& account, uint256 domainID, b
             credentials.pushBack(keyletCredential.key);
     }
 
-    bool const foundExpired = credentials::removeExpired(view, credentials, j);
+    auto const foundExpired = credentials::removeExpired(view, credentials, j);
+    if (!foundExpired.has_value())
+        return foundExpired.error();
+
     for (auto const& h : credentials)
     {
         auto sleCredential = view.read(keylet::credential(h));
@@ -335,7 +343,7 @@ verifyValidDomain(ApplyView& view, AccountID const& account, uint256 domainID, b
             return tesSUCCESS;
     }
 
-    return foundExpired ? tecEXPIRED : tecNO_PERMISSION;
+    return *foundExpired ? tecEXPIRED : tecNO_PERMISSION;
 }
 
 TER
@@ -355,8 +363,15 @@ verifyDepositPreauth(
 
     bool const credentialsPresent = tx.isFieldPresent(sfCredentialIDs);
 
-    if (credentialsPresent && credentials::removeExpired(view, tx.getFieldV256(sfCredentialIDs), j))
-        return tecEXPIRED;
+    if (credentialsPresent)
+    {
+        auto const foundExpired =
+            credentials::removeExpired(view, tx.getFieldV256(sfCredentialIDs), j);
+        if (!foundExpired.has_value())
+            return foundExpired.error();
+        if (*foundExpired)
+            return tecEXPIRED;
+    }
 
     if (sleDst && ((sleDst->getFlags() & lsfDepositAuth) != 0u))
     {

src/libxrpl/ledger/helpers/PermissionedDEXHelpers.cpp

@@ -35,7 +35,7 @@ accountInDomain(ReadView const& view, AccountID const& account, Domain const& do
         if (!sleCred || !sleCred->isFlag(lsfAccepted))
             return false;
 
-        return !credentials::checkExpired(sleCred, view.header().parentCloseTime);
+        return !credentials::checkExpired(*sleCred, view.header().parentCloseTime);
     });
 
     return inDomain;

src/libxrpl/tx/Transactor.cpp

@@ -1010,7 +1010,14 @@ removeExpiredCredentials(ApplyView& view, std::vector<uint256> const& creds, bea
     for (auto const& index : creds)
     {
         if (auto const sle = view.peek(keylet::credential(index)))
-            credentials::deleteSLE(view, sle, viewJ);
+        {
+            if (auto const ter = credentials::deleteSLE(view, sle, viewJ); !isTesSuccess(ter))
+            {
+                JLOG(viewJ.error())
+                    << "removeExpiredCredentials: failed to delete expired credential. Err: "
+                    << transToken(ter);
+            }
+        }
     }
 }
 

src/libxrpl/tx/transactors/credentials/CredentialAccept.cpp

@@ -105,8 +105,10 @@ CredentialAccept::doApply()
     auto const credType(ctx_.tx[sfCredentialType]);
     Keylet const credentialKey = keylet::credential(account_, issuer, credType);
     auto const sleCred = view().peek(credentialKey);  // Checked in preclaim()
+    if (!sleCred)
+        return tefINTERNAL;  // LCOV_EXCL_LINE
 
-    if (checkExpired(sleCred, view().header().parentCloseTime))
+    if (checkExpired(*sleCred, view().header().parentCloseTime))
     {
         JLOG(j_.trace()) << "Credential is expired: " << sleCred->getText();
         // delete expired credentials even if the transaction failed

src/libxrpl/tx/transactors/credentials/CredentialDelete.cpp

@@ -87,7 +87,7 @@ CredentialDelete::doApply()
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
     if ((subject != account_) && (issuer != account_) &&
-        !checkExpired(sleCred, ctx_.view().header().parentCloseTime))
+        !checkExpired(*sleCred, ctx_.view().header().parentCloseTime))
     {
         JLOG(j_.trace()) << "Can't delete non-expired credential.";
         return tecNO_PERMISSION;

src/test/app/Credentials_test.cpp

@@ -5,9 +5,12 @@
 #include <test/jtx/acctdelete.h>
 #include <test/jtx/amount.h>
 #include <test/jtx/credentials.h>
+#include <test/jtx/deposit.h>
 #include <test/jtx/directory.h>
 #include <test/jtx/fee.h>
 #include <test/jtx/noop.h>
+#include <test/jtx/pay.h>
+#include <test/jtx/permissioned_domains.h>
 #include <test/jtx/ter.h>
 #include <test/jtx/ticket.h>
 #include <test/jtx/txflags.h>
@@ -24,11 +27,13 @@
 #include <xrpl/protocol/LedgerFormats.h>
 #include <xrpl/protocol/Protocol.h>
 #include <xrpl/protocol/SField.h>
+#include <xrpl/protocol/STTx.h>
 #include <xrpl/protocol/TER.h>
 #include <xrpl/protocol/TxFlags.h>
 #include <xrpl/protocol/jss.h>
 
 #include <cstdint>
+#include <cstring>
 #include <memory>
 #include <string_view>
 
@@ -1028,6 +1033,121 @@ struct Credentials_test : public beast::unit_test::Suite
         }
     }
 
+    void
+    testRemoveExpiredCorruption(FeatureBitset features)
+    {
+        bool const fixEnabled = features[fixSecurity3_1_3];
+        testcase(
+            "removeExpired ignores deleteSLE failure " +
+            (fixEnabled ? std::string(" after fix") : std::string(" before fix")));
+
+        using namespace test::jtx;
+
+        char const credType[] = "abcde";
+        Account const issuer{"issuer"};
+        Account const subject{"subject"};
+        Account const becky{"becky"};
+
+        Env env{*this, features};
+        env.fund(XRP(10000), issuer, subject, becky);
+        env.close();
+
+        // Create credential with short expiration
+        auto jv = credentials::create(subject, issuer, credType);
+        uint32_t const expiration =
+            env.current()->header().parentCloseTime.time_since_epoch().count() + 40;
+        jv[sfExpiration.jsonName] = expiration;
+        env(jv);
+        env.close();
+
+        auto const credLE = credentials::ledgerEntry(env, subject, issuer, credType);
+        std::string const credIdx = credLE[jss::result][jss::index].asString();
+
+        // Subject accepts the credential
+        env(credentials::accept(subject, issuer, credType));
+        env.close();
+
+        // Build the credential keylet
+        auto const credKeylet =
+            keylet::credential(subject.id(), issuer.id(), Slice(credType, std::strlen(credType)));
+
+        // Verify credential exists and is accepted
+        {
+            auto const sleCred = env.current()->read(credKeylet);
+            BEAST_EXPECT(sleCred && sleCred->getFlags() & lsfAccepted);
+        }
+
+        // Create DepositPreauth
+        env(deposit::authCredentials(becky, {{subject, credType}}));
+        env.close();
+        // env();
+        auto jtx = env.jt(pay(subject, becky, XRP(100)), credentials::Ids({credIdx}));
+        if (!BEAST_EXPECT(jtx.stx))
+            return;
+        auto const stx = std::make_shared<STTx>(*jtx.stx);
+
+        // Create PermissionedDomain
+        env(pdomain::setTx(becky, {{issuer, credType}}));
+        env.close();
+        auto const objects = pdomain::getObjects(becky, env);
+        if (!BEAST_EXPECT(!objects.empty()))
+            return;
+        auto const domain = objects.begin()->first;
+
+        using namespace std::chrono_literals;
+        env.close(50s);
+
+        // Verify time has advanced past expiration
+        {
+            auto const sleCred = env.current()->read(credKeylet);
+            BEAST_EXPECT(
+                sleCred &&
+                xrpl::credentials::checkExpired(*sleCred, env.current()->header().parentCloseTime));
+        }
+
+        // Create an ApplyViewImpl on top of the current closed ledger
+        // and corrupt it by erasing the issuer's account SLE
+        auto const open = env.current();
+        ApplyViewImpl av(&*open, TapNone);
+
+        // Erase the issuer's account to simulate ledger corruption
+        auto sleIssuer = av.peek(keylet::account(issuer.id()));
+        if (!BEAST_EXPECT(sleIssuer))
+            return;
+        av.erase(sleIssuer);
+        BEAST_EXPECT(!av.exists(keylet::account(issuer.id())));
+
+        // Credential still exists before removeExpired
+        BEAST_EXPECT(av.exists(credKeylet));
+
+        // Call removeExpired on the corrupted view
+        STVector256 credHashes;
+        credHashes.pushBack(credKeylet.key);
+        beast::Journal const j{beast::Journal::getNullSink()};
+
+        auto const dpTer = xrpl::verifyDepositPreauth(*stx, av, subject, becky, {}, j);
+        auto sleCredAfter = av.read(credKeylet);
+        BEAST_EXPECT(sleCredAfter && (sleCredAfter->getFlags() & lsfAccepted));
+
+        auto const domTer = xrpl::verifyValidDomain(av, subject.id(), domain, j);
+        sleCredAfter = av.read(credKeylet);
+        BEAST_EXPECT(sleCredAfter && (sleCredAfter->getFlags() & lsfAccepted));
+
+        if (fixEnabled)
+        {
+            // removeExpired returns error, cred wasn't deleted
+            BEAST_EXPECT(dpTer == tecINTERNAL);
+            BEAST_EXPECT(domTer == tecINTERNAL);
+        }
+        else
+        {
+            // removeExpired returns true (claims it found & deleted expired
+            // creds)
+            BEAST_EXPECT(dpTer == tecEXPIRED);
+            BEAST_EXPECT(isTesSuccess(domTer));
+        }
+    }
+
     void
     run() override
     {
@@ -1043,6 +1163,9 @@ struct Credentials_test : public beast::unit_test::Suite
         testFlags(all - fixInvalidTxFlags);
         testFlags(all);
         testRPC();
+
+        testRemoveExpiredCorruption(all - fixSecurity3_1_3);
+        testRemoveExpiredCorruption(all | fixSecurity3_1_3);
     }
 };
 

src/xrpld/rpc/handlers/orderbook/DepositAuthorized.cpp

@@ -141,7 +141,7 @@ doDepositAuthorized(RPC::JsonContext& context)
                 return result;
             }
 
-            if (credentials::checkExpired(sleCred, ledger->header().parentCloseTime))
+            if (credentials::checkExpired(*sleCred, ledger->header().parentCloseTime))
             {
                 RPC::injectError(RpcBadCredentials, "credentials are expired", result);
                 return result;