From b2ab2f606c6a21f623484cc535149db6c3efbe9a Mon Sep 17 00:00:00 2001 From: akildemir <34187742+akildemir@users.noreply.github.com> Date: Wed, 20 Aug 2025 17:26:36 +0300 Subject: [PATCH] Security fixes (#48) * unify tx versions; add missing protocol tx checks * fixed errors with protocol_tx handling pre-Carrot * fixed error caused by setting coinbase_tx version to 4 * fix eph pubkey check for protocol tx verification * Update tx_pool.cpp --------- Co-authored-by: Some Random Crypto Guy Co-authored-by: somerandomcryptoguy <139346562+somerandomcryptoguy@users.noreply.github.com> --- src/blockchain_db/blockchain_db.cpp | 2 +- src/carrot_impl/format_utils.cpp | 2 +- .../cryptonote_format_utils.cpp | 2 +- src/cryptonote_core/blockchain.cpp | 44 ++++++++++++++++--- src/cryptonote_core/cryptonote_tx_utils.cpp | 4 +- src/wallet/tx_builder.cpp | 3 -- 6 files changed, 41 insertions(+), 16 deletions(-) diff --git a/src/blockchain_db/blockchain_db.cpp b/src/blockchain_db/blockchain_db.cpp index 81ac0134b..e35b2a62c 100644 --- a/src/blockchain_db/blockchain_db.cpp +++ b/src/blockchain_db/blockchain_db.cpp @@ -238,7 +238,7 @@ void BlockchainDB::add_transaction(const crypto::hash& blk_hash, const std::pair LOG_PRINT_L1("Failed to get output unlock time, aborting transaction addition"); throw std::runtime_error("Unexpected error getting output unlock_time, aborting"); } - if (miner_tx && tx.version == 2) + if (miner_tx && tx.version >= 2) { cryptonote::tx_out vout = tx.vout[i]; // TODO: avoid multiple expensive zeroCommitVartime call here + get_outs_by_last_locked_block + ver_non_input_consensus diff --git a/src/carrot_impl/format_utils.cpp b/src/carrot_impl/format_utils.cpp index b97ed6cc0..3ca3b185e 100644 --- a/src/carrot_impl/format_utils.cpp +++ b/src/carrot_impl/format_utils.cpp @@ -420,7 +420,7 @@ cryptonote::transaction store_carrot_to_coinbase_transaction_v1( cryptonote::transaction tx; tx.type = tx_type; tx.pruned = false; - tx.version = 2; + tx.version = TRANSACTION_VERSION_CARROT; tx.unlock_time = CRYPTONOTE_MINED_MONEY_UNLOCK_WINDOW; tx.vin.reserve(1); tx.vout.reserve(nouts); diff --git a/src/cryptonote_basic/cryptonote_format_utils.cpp b/src/cryptonote_basic/cryptonote_format_utils.cpp index 5779f378f..1a5e9ef4e 100644 --- a/src/cryptonote_basic/cryptonote_format_utils.cpp +++ b/src/cryptonote_basic/cryptonote_format_utils.cpp @@ -1383,7 +1383,7 @@ namespace cryptonote { if (hf_version >= HF_VERSION_CARROT) { - // from v11, require outputs be carrot outputs + // from v10, require outputs be carrot outputs CHECK_AND_ASSERT_MES(o.target.type() == typeid(txout_to_carrot_v1), false, "wrong variant type: " << o.target.type().name() << ", expected txout_to_carrot_v1 in transaction id=" << get_transaction_hash(tx)); } else { diff --git a/src/cryptonote_core/blockchain.cpp b/src/cryptonote_core/blockchain.cpp index 11e5f3780..7da27dc6e 100644 --- a/src/cryptonote_core/blockchain.cpp +++ b/src/cryptonote_core/blockchain.cpp @@ -1348,6 +1348,11 @@ bool Blockchain::prevalidate_miner_transaction(const block& b, uint64_t height, CHECK_AND_ASSERT_MES(b.miner_tx.vin[0].type() == typeid(txin_gen), false, "coinbase transaction in the block has the wrong type"); CHECK_AND_ASSERT_MES(b.miner_tx.version > 1, false, "Invalid coinbase transaction version"); + if (hf_version >= HF_VERSION_CARROT) { + CHECK_AND_ASSERT_MES(b.miner_tx.version == TRANSACTION_VERSION_CARROT, false, "miner transaction has wrong version"); + CHECK_AND_ASSERT_MES(b.miner_tx.type == cryptonote::transaction_type::MINER, false, "miner transaction has wrong type"); + } + // for v2 txes (ringct), we only accept empty rct signatures for miner transactions, if (hf_version >= HF_VERSION_REJECT_SIGS_IN_COINBASE && b.miner_tx.version >= 2) { @@ -1398,6 +1403,11 @@ bool Blockchain::prevalidate_protocol_transaction(const block& b, uint64_t heigh CHECK_AND_ASSERT_MES(b.protocol_tx.vin[0].type() == typeid(txin_gen), false, "coinbase protocol transaction in the block has the wrong type"); CHECK_AND_ASSERT_MES(b.protocol_tx.version > 1, false, "Invalid coinbase protocol transaction version"); + if (hf_version >= HF_VERSION_CARROT) { + CHECK_AND_ASSERT_MES(b.protocol_tx.version == TRANSACTION_VERSION_CARROT, false, "protocol transaction has wrong version"); + CHECK_AND_ASSERT_MES(b.protocol_tx.type == cryptonote::transaction_type::PROTOCOL, false, "protocol transaction has wrong type"); + } + // for v2 txes (ringct), we only accept empty rct signatures for protocol transactions, if (hf_version >= HF_VERSION_REJECT_SIGS_IN_COINBASE && b.protocol_tx.version >= 2) { @@ -1621,7 +1631,7 @@ bool Blockchain::validate_protocol_transaction(const block& b, uint64_t height, ); if (hf_version >= HF_VERSION_CARROT) { - // TODO: add other verifications for carrot yield payouts + size_t output_idx = 0; for (auto it = carrot_yield_payouts.begin(); it != carrot_yield_payouts.end(); it++, output_idx++) { // Verify the output key @@ -1629,6 +1639,16 @@ bool Blockchain::validate_protocol_transaction(const block& b, uint64_t height, cryptonote::get_output_public_key(b.protocol_tx.vout[output_idx], out_key); CHECK_AND_ASSERT_MES(out_key == it->first.return_address, false, "Incorrect output key detected in protocol_tx"); + // Verify the return pubkey + if (b.protocol_tx.vout.size() > 1) { + const auto additional_pubkeys = cryptonote::get_additional_tx_pub_keys_from_extra(b.protocol_tx.extra); + CHECK_AND_ASSERT_MES(additional_pubkeys.size() > output_idx, false, "Missing return pubkey detected in protocol_tx"); + CHECK_AND_ASSERT_MES(additional_pubkeys[output_idx] == it->first.return_pubkey, false, "Incorrect return pubkey detected in protocol_tx"); + } else { + const auto main_pubkey = cryptonote::get_tx_pub_key_from_extra(b.protocol_tx.extra); + CHECK_AND_ASSERT_MES(main_pubkey == it->first.return_pubkey, false, "Incorrect return pubkey detected in protocol_tx"); + } + // Verify the output amount uint64_t expected_amount = it->second; CHECK_AND_ASSERT_MES(b.protocol_tx.vout[output_idx].amount == expected_amount, false, "Incorrect output amount detected in protocol_tx. expected_amount: " << expected_amount); @@ -1636,11 +1656,21 @@ bool Blockchain::validate_protocol_transaction(const block& b, uint64_t height, // Verify the output asset type std::string out_asset_type; cryptonote::get_output_asset_type(b.protocol_tx.vout[output_idx], out_asset_type); - uint8_t hf_yield = m_hardfork->get_ideal_version(it->first.block_height); - if (hf_yield >= HF_VERSION_SALVIUM_ONE_PROOFS) - CHECK_AND_ASSERT_MES(out_asset_type == "SAL1", false, "Incorrect output asset_type (!= SAL1) detected in protocol_tx"); - else - CHECK_AND_ASSERT_MES(out_asset_type == "SAL", false, "Incorrect output asset_type (!= SAL) detected in protocol_tx"); + CHECK_AND_ASSERT_MES(out_asset_type == "SAL1", false, "Incorrect output asset_type (!= SAL1) detected in protocol_tx"); + + // Verify the view tag + CHECK_AND_ASSERT_MES( + boost::get( + b.protocol_tx.vout[output_idx].target + ).view_tag == it->first.return_view_tag, false, "Incorrect view tag detected in protocol_tx" + ); + + // Verify the anchor encrypted + CHECK_AND_ASSERT_MES( + boost::get( + b.protocol_tx.vout[output_idx].target + ).encrypted_janus_anchor == it->first.return_anchor_enc, false, "Incorrect anchor detected in protocol_tx" + ); } return true; @@ -3685,7 +3715,7 @@ bool Blockchain::check_tx_outputs(const transaction& tx, tx_verification_context */ if (hf_version >= HF_VERSION_CARROT) { - // from v11, force the new SalviumOne RCT data + // from v10, force the new SalviumOne RCT data if (tx.type == cryptonote::transaction_type::TRANSFER || tx.type == cryptonote::transaction_type::STAKE || tx.type == cryptonote::transaction_type::BURN || tx.type == cryptonote::transaction_type::CONVERT || tx.type == cryptonote::transaction_type::AUDIT) { if (tx.rct_signatures.type != rct::RCTTypeSalviumOne) { MERROR_VER("SalviumOne data required after v" + std::to_string(HF_VERSION_CARROT)); diff --git a/src/cryptonote_core/cryptonote_tx_utils.cpp b/src/cryptonote_core/cryptonote_tx_utils.cpp index ed580d2db..0925ed42f 100644 --- a/src/cryptonote_core/cryptonote_tx_utils.cpp +++ b/src/cryptonote_core/cryptonote_tx_utils.cpp @@ -346,10 +346,8 @@ namespace cryptonote // Clear the TX contents tx.set_null(); - tx.type = cryptonote::transaction_type::PROTOCOL; - - // Force the TX type to 2 tx.version = 2; + tx.type = cryptonote::transaction_type::PROTOCOL; const bool do_carrot = hard_fork_version >= HF_VERSION_CARROT; if (do_carrot) diff --git a/src/wallet/tx_builder.cpp b/src/wallet/tx_builder.cpp index 8e42b5949..c0a162cb5 100644 --- a/src/wallet/tx_builder.cpp +++ b/src/wallet/tx_builder.cpp @@ -541,9 +541,6 @@ std::vector make_carrot_transaction_proposa const std::set &subaddr_indices, const wallet2::unique_index_container &subtract_fee_from_outputs) { - wallet2::transfer_container transfers; - w.get_transfers(transfers); - const bool use_per_byte_fee = w.use_fork_rules(HF_VERSION_PER_BYTE_FEE, 0); CHECK_AND_ASSERT_THROW_MES(use_per_byte_fee, "make_carrot_transaction_proposals_wallet2_transfer: not using per-byte base fee");