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

Co-authored-by: Ed Hennis <ed@ripple.com>

include/xrpl/ledger/CredentialHelpers.h

@@ -39,16 +39,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,

src/libxrpl/ledger/CredentialHelpers.cpp

@@ -28,17 +28,16 @@ namespace ripple {
 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::uint32_t const exp = 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.info().parentCloseTime;
@@ -50,12 +49,14 @@ 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;
         }
     }
@@ -217,7 +218,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;
@@ -343,7 +344,10 @@ verifyValidDomain(
             credentials.push_back(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));
@@ -354,7 +358,7 @@ verifyValidDomain(
             return tesSUCCESS;
     }
 
-    return foundExpired ? tecEXPIRED : tecNO_PERMISSION;
+    return *foundExpired ? tecEXPIRED : tecNO_PERMISSION;
 }
 
 TER
@@ -374,9 +378,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))
     {

src/test/app/Credentials_test.cpp

@@ -1109,6 +1109,125 @@ 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()->info().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 domain = pdomain::getObjects(becky, env).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 &&
+                ripple::credentials::checkExpired(
+                    *sleCred, env.current()->info().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.push_back(credKeylet.key);
+        beast::Journal const j{beast::Journal::getNullSink()};
+
+        auto const dpTer =
+            ripple::verifyDepositPreauth(*stx, av, subject, becky, {}, j);
+        auto sleCredAfter = av.read(credKeylet);
+        BEAST_EXPECT(sleCredAfter && (sleCredAfter->getFlags() & lsfAccepted));
+
+        auto const domTer =
+            ripple::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
     {
@@ -1124,6 +1243,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/app/misc/PermissionedDEXHelpers.cpp

@@ -48,7 +48,7 @@ accountInDomain(
                 return false;
 
             return !credentials::checkExpired(
-                sleCred, view.info().parentCloseTime);
+                *sleCred, view.info().parentCloseTime);
         });
 
     return inDomain;

src/xrpld/app/tx/detail/Credentials.cpp

@@ -261,7 +261,7 @@ CredentialDelete::doApply()
         return tefINTERNAL;  // LCOV_EXCL_LINE
 
     if ((subject != account_) && (issuer != account_) &&
-        !checkExpired(sleCred, ctx_.view().info().parentCloseTime))
+        !checkExpired(*sleCred, ctx_.view().info().parentCloseTime))
     {
         JLOG(j_.trace()) << "Can't delete non-expired credential.";
         return tecNO_PERMISSION;
@@ -354,8 +354,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().info().parentCloseTime))
+    if (checkExpired(*sleCred, view().info().parentCloseTime))
     {
         JLOG(j_.trace()) << "Credential is expired: " << sleCred->getText();
         // delete expired credentials even if the transaction failed

src/xrpld/app/tx/detail/Transactor.cpp

@@ -1091,7 +1091,15 @@ removeExpiredCredentials(
     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/xrpld/rpc/handlers/DepositAuthorized.cpp

@@ -154,7 +154,7 @@ doDepositAuthorized(RPC::JsonContext& context)
             }
 
             if (credentials::checkExpired(
-                    sleCred, ledger->info().parentCloseTime))
+                    *sleCred, ledger->info().parentCloseTime))
             {
                 RPC::inject_error(
                     rpcBAD_CREDENTIALS, "credentials are expired", result);