From b34ecc476a09fe79395363efda7ed7977751537c Mon Sep 17 00:00:00 2001 From: Shawn Xie <35279399+shawnxie999@users.noreply.github.com> Date: Thu, 26 Mar 2026 13:16:51 -0400 Subject: [PATCH] Add ProofReader and auth/lock and overflow edge case checks (#6651) --- include/xrpl/protocol/ConfidentialTransfer.h | 55 ++++++ include/xrpl/protocol/Protocol.h | 9 +- .../token/ConfidentialMPTConvert.cpp | 27 ++- .../token/ConfidentialMPTConvertBack.cpp | 30 +--- .../transactors/token/ConfidentialMPTSend.cpp | 51 ++---- src/test/app/ConfidentialTransfer_test.cpp | 157 +++++++++++++++++- 6 files changed, 256 insertions(+), 73 deletions(-) diff --git a/include/xrpl/protocol/ConfidentialTransfer.h b/include/xrpl/protocol/ConfidentialTransfer.h index 5b13aaa6b0..34378ec418 100644 --- a/include/xrpl/protocol/ConfidentialTransfer.h +++ b/include/xrpl/protocol/ConfidentialTransfer.h @@ -490,4 +490,59 @@ computeSendRemainder(Slice const& balanceCommitment, Slice const& amountCommitme */ std::optional computeConvertBackRemainder(Slice const& commitment, uint64_t amount); + +/** + * @brief Sequential reader for extracting proof components from a ZKProof blob. + * + * Encapsulates the offset-based arithmetic for slicing a concatenated proof + * blob into its individual components (equality proofs, Pedersen linkage + * proofs, bulletproofs, etc.). Performs bounds checking on every read and + * tracks whether the entire blob has been consumed. + * + * Usage: + * @code + * ProofReader reader(tx[sfZKProof]); + * auto equalityProof = reader.read(sizeEquality); + * auto pedersenProof = reader.read(ecPedersenProofLength); + * if (!equalityProof || !pedersenProof || !reader.done()) + * return tecINTERNAL; + * @endcode + */ +class ProofReader +{ + Slice data_; + std::size_t offset_ = 0; + +public: + explicit ProofReader(Slice data) : data_(data) + { + } + + /** + * @brief Read the next @p length bytes from the proof blob. + * + * @param length Number of bytes to read. + * @return A Slice of the requested bytes, or std::nullopt if there are + * not enough remaining bytes. + */ + [[nodiscard]] std::optional + read(std::size_t length) + { + if (offset_ + length > data_.size()) + return std::nullopt; + auto result = data_.substr(offset_, length); + offset_ += length; + return result; + } + + /** + * @brief Returns true when every byte has been consumed. + */ + [[nodiscard]] bool + done() const + { + return offset_ == data_.size(); + } +}; + } // namespace xrpl diff --git a/include/xrpl/protocol/Protocol.h b/include/xrpl/protocol/Protocol.h index 9562e612dd..b0ebe30d76 100644 --- a/include/xrpl/protocol/Protocol.h +++ b/include/xrpl/protocol/Protocol.h @@ -307,7 +307,7 @@ std::size_t constexpr permissionMaxSize = 10; /** The maximum number of transactions that can be in a batch. */ std::size_t constexpr maxBatchTxCount = 8; -/** EC ElGamal ciphertext length 33-byte */ +/** Length of one component of EC ElGamal ciphertext */ std::size_t constexpr ecGamalEncryptedLength = 33; /** EC ElGamal ciphertext length: two 33-byte components concatenated */ @@ -331,9 +331,6 @@ std::size_t constexpr ecBlindingFactorLength = 32; /** Length of Schnorr ZKProof for public key registration in bytes */ std::size_t constexpr ecSchnorrProofLength = 65; -/** Length of ElGamal ciphertext equality proof in bytes */ -std::size_t constexpr ecCiphertextEqualityProofLength = 261; - /** Length of ElGamal Pedersen linkage proof in bytes */ std::size_t constexpr ecPedersenProofLength = 195; @@ -347,9 +344,9 @@ std::size_t constexpr ecSingleBulletproofLength = 688; std::size_t constexpr ecDoubleBulletproofLength = 754; /** Compressed EC point prefix for even y-coordinate */ -static constexpr std::uint8_t ecCompressedPrefixEvenY = 0x02; +std::uint8_t constexpr ecCompressedPrefixEvenY = 0x02; /** Compressed EC point prefix for odd y-coordinate */ -static constexpr std::uint8_t ecCompressedPrefixOddY = 0x03; +std::uint8_t constexpr ecCompressedPrefixOddY = 0x03; } // namespace xrpl diff --git a/src/libxrpl/tx/transactors/token/ConfidentialMPTConvert.cpp b/src/libxrpl/tx/transactors/token/ConfidentialMPTConvert.cpp index 6ba8cffafe..5cf5a84607 100644 --- a/src/libxrpl/tx/transactors/token/ConfidentialMPTConvert.cpp +++ b/src/libxrpl/tx/transactors/token/ConfidentialMPTConvert.cpp @@ -86,6 +86,21 @@ ConfidentialMPTConvert::preclaim(PreclaimContext const& ctx) return tecOBJECT_NOT_FOUND; auto const mptIssue = MPTIssue{issuanceID}; + + // Explicit freeze and auth checks are required because accountHolds + // with fhZERO_IF_FROZEN/ahZERO_IF_UNAUTHORIZED only implicitly rejects + // non-zero amounts. A zero-amount convert would bypass those implicit + // checks, allowing frozen or unauthorized accounts to register ElGamal + // keys and initialize confidential balance fields. + + // Check lock + if (auto const ter = checkFrozen(ctx.view, account, mptIssue); !isTesSuccess(ter)) + return ter; + + // Check auth + if (auto const ter = requireAuth(ctx.view, mptIssue, account); !isTesSuccess(ter)) + return ter; + STAmount const mptAmount = STAmount(MPTAmount{static_cast(amount)}, mptIssue); if (accountHolds( @@ -119,9 +134,10 @@ ConfidentialMPTConvert::preclaim(PreclaimContext const& ctx) getConvertContextHash(account, issuanceID, ctx.tx.getSeqProxy().value()); // when register new pk, verify through schnorr proof - if (!isTesSuccess(verifySchnorrProof(holderPubKey, ctx.tx[sfZKProof], contextHash))) + if (auto const ter = verifySchnorrProof(holderPubKey, ctx.tx[sfZKProof], contextHash); + !isTesSuccess(ter)) { - return tecBAD_PROOF; + return ter; } } else @@ -167,9 +183,12 @@ ConfidentialMPTConvert::doApply() // Converting decreases regular balance and increases confidential outstanding. // The confidential outstanding tracks total tokens in confidential form globally. + auto const currentCOA = (*sleIssuance)[~sfConfidentialOutstandingAmount].value_or(0); + if (amtToConvert > maxMPTokenAmount - currentCOA) + return tecINTERNAL; // LCOV_EXCL_LINE + (*sleMptoken)[sfMPTAmount] = amt - amtToConvert; - (*sleIssuance)[sfConfidentialOutstandingAmount] = - (*sleIssuance)[~sfConfidentialOutstandingAmount].value_or(0) + amtToConvert; + (*sleIssuance)[sfConfidentialOutstandingAmount] = currentCOA + amtToConvert; Slice const holderEc = ctx_.tx[sfHolderEncryptedAmount]; Slice const issuerEc = ctx_.tx[sfIssuerEncryptedAmount]; diff --git a/src/libxrpl/tx/transactors/token/ConfidentialMPTConvertBack.cpp b/src/libxrpl/tx/transactors/token/ConfidentialMPTConvertBack.cpp index 13653852ee..825bcc1db1 100644 --- a/src/libxrpl/tx/transactors/token/ConfidentialMPTConvertBack.cpp +++ b/src/libxrpl/tx/transactors/token/ConfidentialMPTConvertBack.cpp @@ -99,34 +99,19 @@ verifyProofs( valid = false; } - // Parse proof components using offset - auto const proof = tx[sfZKProof]; - size_t remainingLength = proof.size(); - size_t currentOffset = 0; + // Extract proof components + ProofReader reader(tx[sfZKProof]); - // Extract Pedersen linkage proof - if (remainingLength < ecPedersenProofLength) - return tecINTERNAL; // LCOV_EXCL_LINE + auto const pedersenProof = reader.read(ecPedersenProofLength); + auto const bulletproof = reader.read(ecSingleBulletproofLength); - auto const pedersenProof = proof.substr(currentOffset, ecPedersenProofLength); - currentOffset += ecPedersenProofLength; - remainingLength -= ecPedersenProofLength; - - // Extract bulletproof - if (remainingLength < ecSingleBulletproofLength) - return tecINTERNAL; // LCOV_EXCL_LINE - - auto const bulletproof = proof.substr(currentOffset, ecSingleBulletproofLength); - currentOffset += ecSingleBulletproofLength; - remainingLength -= ecSingleBulletproofLength; - - if (remainingLength != 0) + if (!pedersenProof || !bulletproof || !reader.done()) return tecINTERNAL; // LCOV_EXCL_LINE // verify el gamal pedersen linkage if (auto const ter = verifyPcmLinkage( PcmLinkageType::balance, - pedersenProof, + *pedersenProof, (*mptoken)[sfConfidentialBalanceSpending], holderPubKey, tx[sfBalanceCommitment], @@ -144,7 +129,8 @@ verifyProofs( // The bulletproof verifies that the remaining balance is non-negative std::vector commitments{Slice(pcRem->data(), pcRem->size())}; - if (auto const ter = verifyAggregatedBulletproof(bulletproof, commitments, contextHash); + if (auto const ter = + verifyAggregatedBulletproof(*bulletproof, commitments, contextHash); !isTesSuccess(ter)) { valid = false; diff --git a/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp b/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp index 826cf53201..4fe47947b0 100644 --- a/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp +++ b/src/libxrpl/tx/transactors/token/ConfidentialMPTSend.cpp @@ -80,44 +80,17 @@ verifySendProofs( auto const hasAuditor = ctx.tx.isFieldPresent(sfAuditorEncryptedAmount); auto const recipientCount = getConfidentialRecipientCount(hasAuditor); - auto const proof = ctx.tx[sfZKProof]; - size_t remainingLength = proof.size(); - size_t currentOffset = 0; - // Extract equality proof - auto const sizeEquality = getEqualityProofSize(recipientCount); - if (remainingLength < sizeEquality) - return tecINTERNAL; // LCOV_EXCL_LINE + // Extract proof components + ProofReader reader(ctx.tx[sfZKProof]); - auto const equalityProof = proof.substr(currentOffset, sizeEquality); - currentOffset += sizeEquality; - remainingLength -= sizeEquality; + auto const equalityProof = reader.read(getEqualityProofSize(recipientCount)); + auto const amountLinkageProof = reader.read(ecPedersenProofLength); + auto const balanceLinkageProof = reader.read(ecPedersenProofLength); + auto const rangeProof = reader.read(ecDoubleBulletproofLength); - // Extract Pedersen linkage proof for amount commitment - if (remainingLength < ecPedersenProofLength) - return tecINTERNAL; // LCOV_EXCL_LINE - - auto const amountLinkageProof = proof.substr(currentOffset, ecPedersenProofLength); - currentOffset += ecPedersenProofLength; - remainingLength -= ecPedersenProofLength; - - // Extract Pedersen linkage proof for balance commitment - if (remainingLength < ecPedersenProofLength) - return tecINTERNAL; // LCOV_EXCL_LINE - - auto const balanceLinkageProof = proof.substr(currentOffset, ecPedersenProofLength); - currentOffset += ecPedersenProofLength; - remainingLength -= ecPedersenProofLength; - - // Extract range proof - if (remainingLength < ecDoubleBulletproofLength) - return tecINTERNAL; - - auto const rangeProof = proof.substr(currentOffset, ecDoubleBulletproofLength); - currentOffset += ecDoubleBulletproofLength; - remainingLength -= ecDoubleBulletproofLength; - - if (remainingLength != 0) + if (!equalityProof || !amountLinkageProof || !balanceLinkageProof || !rangeProof || + !reader.done()) return tecINTERNAL; // LCOV_EXCL_LINE // Prepare recipient list @@ -150,7 +123,7 @@ verifySendProofs( // Verify the multi-ciphertext equality proof if (auto const ter = verifyMultiCiphertextEqualityProof( - equalityProof, recipients, recipientCount, contextHash); + *equalityProof, recipients, recipientCount, contextHash); !isTesSuccess(ter)) { valid = false; @@ -159,7 +132,7 @@ verifySendProofs( // Verify amount linkage if (auto const ter = verifyPcmLinkage( PcmLinkageType::amount, - amountLinkageProof, + *amountLinkageProof, ctx.tx[sfSenderEncryptedAmount], (*sleSenderMPToken)[sfHolderEncryptionKey], ctx.tx[sfAmountCommitment], @@ -172,7 +145,7 @@ verifySendProofs( // Verify balance linkage if (auto const ter = verifyPcmLinkage( PcmLinkageType::balance, - balanceLinkageProof, + *balanceLinkageProof, (*sleSenderMPToken)[sfConfidentialBalanceSpending], (*sleSenderMPToken)[sfHolderEncryptionKey], ctx.tx[sfBalanceCommitment], @@ -194,7 +167,7 @@ verifySendProofs( commitments.push_back(ctx.tx[sfAmountCommitment]); commitments.push_back(Slice{pcRem->data(), pcRem->size()}); - if (auto const ter = verifyAggregatedBulletproof(rangeProof, commitments, contextHash); + if (auto const ter = verifyAggregatedBulletproof(*rangeProof, commitments, contextHash); !isTesSuccess(ter)) { valid = false; diff --git a/src/test/app/ConfidentialTransfer_test.cpp b/src/test/app/ConfidentialTransfer_test.cpp index 53b436e839..d2ebda7202 100644 --- a/src/test/app/ConfidentialTransfer_test.cpp +++ b/src/test/app/ConfidentialTransfer_test.cpp @@ -1156,7 +1156,7 @@ class ConfidentialTransfer_test : public beast::unit_test::suite .account = bob, .amt = 10, .holderPubKey = mptAlice.getPubKey(bob), - .err = tecINSUFFICIENT_FUNDS, + .err = tecLOCKED, }); mptAlice.set({ @@ -1211,7 +1211,7 @@ class ConfidentialTransfer_test : public beast::unit_test::suite .account = bob, .amt = 10, .holderPubKey = mptAlice.getPubKey(bob), - .err = tecINSUFFICIENT_FUNDS, + .err = tecNO_AUTH, }); // auth bob @@ -1227,6 +1227,89 @@ class ConfidentialTransfer_test : public beast::unit_test::suite }); } + // frozen account cannot bypass freeze check with amount=0 + { + Env env{*this, features}; + Account const alice("alice"); + Account const bob("bob"); + MPTTester mptAlice(env, alice, {.holders = {bob}}); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanLock | tfMPTCanConfidentialAmount, + }); + + mptAlice.authorize({ + .account = bob, + }); + mptAlice.pay(alice, bob, 100); + + mptAlice.generateKeyPair(alice); + + mptAlice.set({.account = alice, .issuerPubKey = mptAlice.getPubKey(alice)}); + + // lock bob + mptAlice.set({ + .account = alice, + .holder = bob, + .flags = tfMPTLock, + }); + + mptAlice.generateKeyPair(bob); + + // amount=0 should still be rejected when locked + mptAlice.convert({ + .account = bob, + .amt = 0, + .holderPubKey = mptAlice.getPubKey(bob), + .err = tecLOCKED, + }); + } + + // unauthorized account cannot bypass auth check with amount=0 + { + Env env{*this, features}; + Account const alice("alice"); + Account const bob("bob"); + MPTTester mptAlice(env, alice, {.holders = {bob}}); + + mptAlice.create({ + .ownerCount = 1, + .flags = + tfMPTCanTransfer | tfMPTCanLock | tfMPTRequireAuth | tfMPTCanConfidentialAmount, + }); + + mptAlice.authorize({ + .account = bob, + }); + mptAlice.authorize({ + .account = alice, + .holder = bob, + }); + mptAlice.pay(alice, bob, 100); + + mptAlice.generateKeyPair(alice); + + mptAlice.set({.account = alice, .issuerPubKey = mptAlice.getPubKey(alice)}); + + mptAlice.generateKeyPair(bob); + + // Unauthorize bob + mptAlice.authorize({ + .account = alice, + .holder = bob, + .flags = tfMPTUnauthorize, + }); + + // amount=0 should still be rejected when unauthorized + mptAlice.convert({ + .account = bob, + .amt = 0, + .holderPubKey = mptAlice.getPubKey(bob), + .err = tecNO_AUTH, + }); + } + // cannot convert if auditor key is set, but auditor amount is not // provided { @@ -1376,6 +1459,76 @@ class ConfidentialTransfer_test : public beast::unit_test::suite .err = tecBAD_PROOF, }); } + + // no holder key on ledger and no key in tx + { + Env env{*this, features}; + Account const alice("alice"); + Account const bob("bob"); + MPTTester mptAlice(env, alice, {.holders = {bob}}); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanLock | tfMPTCanConfidentialAmount, + }); + + mptAlice.authorize({ + .account = bob, + }); + mptAlice.pay(alice, bob, 100); + + mptAlice.generateKeyPair(alice); + mptAlice.generateKeyPair(bob); + + mptAlice.set({.account = alice, .issuerPubKey = mptAlice.getPubKey(alice)}); + + // bob has not registered a holder key, and doesn't provide one + mptAlice.convert({ + .account = bob, + .amt = 10, + .err = tecNO_PERMISSION, + }); + } + + // all public balance already converted, try to convert more + { + Env env{*this, features}; + Account const alice("alice"); + Account const bob("bob"); + MPTTester mptAlice(env, alice, {.holders = {bob}}); + + mptAlice.create({ + .ownerCount = 1, + .flags = tfMPTCanTransfer | tfMPTCanLock | tfMPTCanConfidentialAmount, + }); + + mptAlice.authorize({ + .account = bob, + }); + mptAlice.pay(alice, bob, 100); + + mptAlice.generateKeyPair(alice); + + mptAlice.set({.account = alice, .issuerPubKey = mptAlice.getPubKey(alice)}); + + mptAlice.generateKeyPair(bob); + + // convert entire public balance + mptAlice.convert({ + .account = bob, + .amt = 100, + .holderPubKey = mptAlice.getPubKey(bob), + }); + + env.require(mptbalance(mptAlice, bob, 0)); + + // try to convert 1 more — no public balance left + mptAlice.convert({ + .account = bob, + .amt = 1, + .err = tecINSUFFICIENT_FUNDS, + }); + } } void