diff --git a/include/xrpl/ledger/helpers/CredentialHelpers.h b/include/xrpl/ledger/helpers/CredentialHelpers.h index c60bd6c7fa..43d7f47518 100644 --- a/include/xrpl/ledger/helpers/CredentialHelpers.h +++ b/include/xrpl/ledger/helpers/CredentialHelpers.h @@ -67,6 +67,20 @@ checkArray(STArray const& credentials, unsigned maxSize, beast::Journal j); TER verifyValidDomain(ApplyView& view, AccountID const& account, uint256 domainID, beast::Journal j); +// Check DepositPreauth authorization +TER +checkDepositPreauth( + STTx const& tx, + ReadView const& view, + AccountID const& src, + AccountID const& dst, + std::shared_ptr const& sleDst, + beast::Journal j); + +// Remove expired credentials +TER +cleanupExpiredCredentials(STTx const& tx, ApplyView& view, beast::Journal j); + // Check expired credentials and for existing DepositPreauth ledger object TER verifyDepositPreauth( diff --git a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp index 22122216bc..9a9e3d47d3 100644 --- a/src/libxrpl/ledger/helpers/CredentialHelpers.cpp +++ b/src/libxrpl/ledger/helpers/CredentialHelpers.cpp @@ -339,9 +339,9 @@ verifyValidDomain(ApplyView& view, AccountID const& account, uint256 domainID, b } TER -verifyDepositPreauth( +checkDepositPreauth( STTx const& tx, - ApplyView& view, + ReadView const& view, AccountID const& src, AccountID const& dst, std::shared_ptr const& sleDst, @@ -353,20 +353,16 @@ verifyDepositPreauth( // 2. If src is deposit preauthorized by dst (either by account or by // credentials). - bool const credentialsPresent = tx.isFieldPresent(sfCredentialIDs); - - if (credentialsPresent && credentials::removeExpired(view, tx.getFieldV256(sfCredentialIDs), j)) - return tecEXPIRED; - if (sleDst && ((sleDst->getFlags() & lsfDepositAuth) != 0u)) { if (src != dst) { if (!view.exists(keylet::depositPreauth(dst, src))) { - return !credentialsPresent ? tecNO_PERMISSION - : credentials::authorizedDepositPreauth( - view, tx.getFieldV256(sfCredentialIDs), dst); + return !tx.isFieldPresent(sfCredentialIDs) + ? tecNO_PERMISSION + : credentials::authorizedDepositPreauth( + view, tx.getFieldV256(sfCredentialIDs), dst); } } } @@ -374,4 +370,29 @@ verifyDepositPreauth( return tesSUCCESS; } +TER +cleanupExpiredCredentials(STTx const& tx, ApplyView& view, beast::Journal j) +{ + if (tx.isFieldPresent(sfCredentialIDs) && + credentials::removeExpired(view, tx.getFieldV256(sfCredentialIDs), j)) + return tecEXPIRED; + + return tesSUCCESS; +} + +TER +verifyDepositPreauth( + STTx const& tx, + ApplyView& view, + AccountID const& src, + AccountID const& dst, + std::shared_ptr const& sleDst, + beast::Journal j) +{ + if (auto const err = cleanupExpiredCredentials(tx, view, j); !isTesSuccess(err)) + return err; + + return checkDepositPreauth(tx, view, src, dst, sleDst, j); +} + } // namespace xrpl diff --git a/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp b/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp index d97439fd9a..72b358790d 100644 --- a/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp +++ b/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp @@ -247,6 +247,12 @@ ConfidentialMPTSend::preclaim(PreclaimContext const& ctx) !isTesSuccess(err)) return err; + // Check deposit preauth before the expensive ZK proof verification. + // Uses read-only view. + if (auto const err = checkDepositPreauth(ctx.tx, ctx.view, account, destination, sleDst, ctx.j); + !isTesSuccess(err)) + return err; + return verifySendProofs(ctx, sleSenderMPToken, sleDestinationMPToken, sleIssuance); } @@ -264,8 +270,9 @@ ConfidentialMPTSend::doApply() if (!sleSenderMPToken || !sleDestinationMPToken || !sleDestAcct) return tecINTERNAL; // LCOV_EXCL_LINE - if (auto err = verifyDepositPreauth( - ctx_.tx, ctx_.view(), account_, destination, sleDestAcct, ctx_.journal); + // Deposit preauth authorization was already verified in preclaim. + // Remove any expired credentials. + if (auto err = cleanupExpiredCredentials(ctx_.tx, ctx_.view(), ctx_.journal); !isTesSuccess(err)) return err; diff --git a/src/test/app/ConfidentialTransfer_test.cpp b/src/test/app/ConfidentialTransfer_test.cpp index f292d34cec..688b4931ba 100644 --- a/src/test/app/ConfidentialTransfer_test.cpp +++ b/src/test/app/ConfidentialTransfer_test.cpp @@ -42,6 +42,7 @@ #include #include +#include #include #include #include @@ -4167,6 +4168,137 @@ class ConfidentialTransfer_test : public beast::unit_test::Suite // Now Carol can send with credentials mpt.send({.account = carol, .dest = bob, .amt = 10, .credentials = {{credIdx}}}); } + + auto const expireTime = 30; + + // Lambda function that returns the credential index after creating a + // credential that expires shortly after the current ledger time. + auto createExpiringCredential = [&](Env& env, Account const& subject) -> std::string { + auto jv = credentials::create(subject, dpIssuer, credType); + auto const expiry = + env.current()->header().parentCloseTime.time_since_epoch().count() + expireTime; + jv[sfExpiration.jsonName] = expiry; + env(jv); + env.close(); + env(credentials::accept(subject, dpIssuer, credType)); + env.close(); + auto const credentials = credentials::ledgerEntry(env, subject, dpIssuer, credType); + return credentials[jss::result][jss::index].asString(); + }; + + auto credentialDeleted = [&](Env& env, Account const& subject) -> bool { + auto const credentials = credentials::ledgerEntry(env, subject, dpIssuer, credType); + return credentials[jss::result].isMember(jss::error) && + credentials[jss::result][jss::error] == "entryNotFound"; + }; + + // TEST 4: Expired credential with matching depositPreauth entry. + // checkDepositPreauth in preclaim returns tesSUCCESS (the expired + // credential still exists and matches the depositPreauth key), so ZK + // proofs run. cleanupExpiredCredentials in doApply then removes the + // expired credential and returns tecEXPIRED. + { + Env env(*this, features); + env.fund(XRP(50000), dpIssuer); + env.close(); + + ConfidentialEnv confEnv{env, alice, {{bob, 100, 50}, {carol, 100, 50}}}; + auto& mpt = confEnv.mpt; + env(fset(bob, asfDepositAuth)); + env.close(); + + auto const credIdx = createExpiringCredential(env, carol); + + // Bob authorizes carol's credential type + env(deposit::authCredentials(bob, {{.issuer = dpIssuer, .credType = credType}})); + env.close(); + + // Advance ledger past credential expiration + env.close(std::chrono::seconds(expireTime)); + + // Send fails with tecEXPIRED; the expired credential is cleaned up + mpt.send({ + .account = carol, + .dest = bob, + .amt = 10, + .credentials = {{credIdx}}, + .err = tecEXPIRED, + }); + env.close(); + + BEAST_EXPECT(credentialDeleted(env, carol)); + } + + // TEST 5: Expired credential, destination has no depositAuth. + // checkDepositPreauth in preclaim returns tesSUCCESS even with expired credentials, + // because we want to keep the checkDepositPreauth part before the expensive proof + // verification. cleanupExpiredCredentials in doApply removes the expired credential and + // returns tecEXPIRED. + { + Env env(*this, features); + env.fund(XRP(50000), dpIssuer); + env.close(); + + ConfidentialEnv confEnv{env, alice, {{bob, 100, 50}, {carol, 100, 50}}}; + auto& mpt = confEnv.mpt; + + auto const credIdx = createExpiringCredential(env, carol); + + // Advance ledger past credential expiration + env.close(std::chrono::seconds(expireTime)); + + // Send fails with tecEXPIRED; the expired credential is cleaned up + mpt.send({ + .account = carol, + .dest = bob, + .amt = 10, + .credentials = {{credIdx}}, + .err = tecEXPIRED, + }); + env.close(); + + BEAST_EXPECT(credentialDeleted(env, carol)); + } + + // TEST 6: Expired credential, depositAuth enabled but credential + // not authorized by bob. + // checkDepositPreauth in preclaim calls checkDepositPreauth which + // finds no match and returns tecNO_PERMISSION. doApply never runs, so + // the expired credential is not cleaned up by this transaction. This is + // a deliberate tradeoff: allowing doApply to run solely for cleanup + // would require bypassing the preclaim short-circuit, forcing every + // validator to run the expensive ZK proof verification before + // discovering the authorization failure. Expired credentials here will + // be cleaned up opportunistically by a future transaction that + // references them. + { + Env env(*this, features); + env.fund(XRP(50000), dpIssuer); + env.close(); + + ConfidentialEnv confEnv{env, alice, {{bob, 100, 50}, {carol, 100, 50}}}; + auto& mpt = confEnv.mpt; + env(fset(bob, asfDepositAuth)); + env.close(); + + auto const credIdx = createExpiringCredential(env, carol); + + // Advance ledger past credential expiration + env.close(std::chrono::seconds(expireTime)); + + // Fails with tecNO_PERMISSION. + mpt.send({ + .account = carol, + .dest = bob, + .amt = 10, + .credentials = {{credIdx}}, + .err = tecNO_PERMISSION, + }); + env.close(); + + // Expired credential is not deleted + BEAST_EXPECT(!credentialDeleted(env, carol)); + } } void @@ -9881,7 +10013,7 @@ class ConfidentialTransfer_test : public beast::unit_test::Suite testHomomorphicCiphertextModification(features); testConvertBackHomomorphicUnderflow(features); - // invalid curve points + // Invalid curve points testSendInvalidCurvePoints(features); testSendWrongGroupPointInjection(features); testIdentityElementRejection(features);