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 <somerandomcryptoguy@protonmail.com>
Co-authored-by: somerandomcryptoguy <139346562+somerandomcryptoguy@users.noreply.github.com>

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

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);

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 {

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<cryptonote::txout_to_carrot_v1>(
+          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<cryptonote::txout_to_carrot_v1>(
+          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));

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)

src/wallet/tx_builder.cpp

@@ -541,9 +541,6 @@ std::vector<carrot::CarrotTransactionProposalV1> make_carrot_transaction_proposa
     const std::set<uint32_t> &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");