cryptonote_core: add change address sanity check

Prevents silly mistakes where wrong change address is passed

src/cryptonote_core/cryptonote_tx_utils.cpp

@@ -28,9 +28,11 @@
 // 
 // Parts of this file are originally copyright (c) 2012-2013 The Cryptonote developers
 
+#include <optional>
 #include <unordered_set>
 #include <random>
 #include "include_base_utils.h"
+#include "misc_log_ex.h"
 #include "string_tools.h"
 using namespace epee;
 
@@ -46,6 +48,40 @@ using namespace epee;
 
 using namespace crypto;
 
+
+namespace
+{
+//---------------------------------------------------------------
+/**
+ * @brief check if can re-derive change address from device / keys
+ * @param change_addr address to attempt to re-derive
+ * @param subaddresses subaddress map
+ * @param keys account keys of sender
+ * @return subaddress index of `change_addr` if in the subaddress map and re-derives from device, otherwise nullopt
+ */
+std::optional<cryptonote::subaddress_index> sanity_check_change_address(
+  const cryptonote::account_public_address& change_addr,
+  const std::unordered_map<crypto::public_key, cryptonote::subaddress_index>& subaddresses,
+  const cryptonote::account_keys &keys
+)
+{
+  // guess/find subaddress index of `change_addr`, works for main addresses if `subaddresses` is empty
+  cryptonote::subaddress_index subaddr_index{}; // (0, 0) by default
+  const auto subaddr_it = subaddresses.find(change_addr.m_spend_public_key);
+  if (subaddr_it != subaddresses.cend())
+    subaddr_index = subaddr_it->second;
+
+  // if device does not return same address given index, then fail
+  hw::device &hwdev = keys.get_device();
+  const auto recomputed_addr = hwdev.get_subaddress(keys, subaddr_index);
+  if (change_addr != recomputed_addr)
+    return std::nullopt;
+
+  return {subaddr_index};
+}
+//---------------------------------------------------------------
+} //anonymous namespace
+
 namespace cryptonote
 {
   //---------------------------------------------------------------
@@ -213,6 +249,10 @@ namespace cryptonote
       return false;
     }
 
+    std::optional<cryptonote::subaddress_index> recognized_change_index;
+    if (change_addr)
+      recognized_change_index = sanity_check_change_address(*change_addr, subaddresses, sender_account_keys);
+
     std::vector<rct::key> amount_keys;
     tx.set_null();
     amount_keys.clear();
@@ -406,6 +446,11 @@ namespace cryptonote
     for(const tx_destination_entry& dst_entr: destinations)
     {
       CHECK_AND_ASSERT_MES(dst_entr.amount > 0 || tx.version > 1, false, "Destination with wrong amount: " << dst_entr.amount);
+      const bool matches_change_addr = change_addr && dst_entr.addr == *change_addr;
+      const bool is_bad_change_dst = matches_change_addr && dst_entr.amount > 0 && !recognized_change_index;
+      CHECK_AND_ASSERT_MES(!is_bad_change_dst, false,
+        "Non-zero amount change address is not recognized as belonging to the sender account");
+
       crypto::public_key out_eph_public_key;
       crypto::view_tag view_tag;
 

tests/core_tests/bulletproof_plus.cpp

@@ -136,7 +136,7 @@ bool gen_bpp_tx_validation_base::generate_with(std::vector<test_event_entry>& ev
     std::unordered_map<crypto::public_key, cryptonote::subaddress_index> subaddresses;
     subaddresses[miner_accounts[n].get_keys().m_account_address.m_spend_public_key] = {0,0};
     rct_txes.resize(rct_txes.size() + 1);
-    bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector<uint8_t>(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]);
+    bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, boost::none, std::vector<uint8_t>(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]);
     CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction");
 
     if (post_tx && !post_tx(rct_txes.back(), n))

tests/core_tests/bulletproofs.cpp

@@ -136,7 +136,7 @@ bool gen_bp_tx_validation_base::generate_with(std::vector<test_event_entry>& eve
     std::unordered_map<crypto::public_key, cryptonote::subaddress_index> subaddresses;
     subaddresses[miner_accounts[n].get_keys().m_account_address.m_spend_public_key] = {0,0};
     rct_txes.resize(rct_txes.size() + 1);
-    bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector<uint8_t>(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]);
+    bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, boost::none, std::vector<uint8_t>(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]);
     CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction");
 
     if (post_tx && !post_tx(rct_txes.back(), n))

tests/core_tests/rct.cpp

@@ -122,7 +122,7 @@ bool gen_rct_tx_validation_base::generate_with_full(std::vector<test_event_entry
     std::vector<crypto::secret_key> additional_tx_keys;
     std::unordered_map<crypto::public_key, cryptonote::subaddress_index> subaddresses;
     subaddresses[miner_accounts[n].get_keys().m_account_address.m_spend_public_key] = {0,0};
-    bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector<uint8_t>(), rct_txes[n], tx_key, additional_tx_keys, true);
+    bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, boost::none, std::vector<uint8_t>(), rct_txes[n], tx_key, additional_tx_keys, true);
     CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction");
     events.push_back(rct_txes[n]);
     starting_rct_tx_hashes.push_back(get_transaction_hash(rct_txes[n]));
@@ -229,7 +229,7 @@ bool gen_rct_tx_validation_base::generate_with_full(std::vector<test_event_entry
   std::vector<crypto::secret_key> additional_tx_keys;
   std::unordered_map<crypto::public_key, cryptonote::subaddress_index> subaddresses;
   subaddresses[miner_accounts[0].get_keys().m_account_address.m_spend_public_key] = {0,0};
-  bool r = construct_tx_and_get_tx_key(miner_accounts[0].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector<uint8_t>(), tx, tx_key, additional_tx_keys, true, rct_config, use_view_tags);
+  bool r = construct_tx_and_get_tx_key(miner_accounts[0].get_keys(), subaddresses, sources, destinations, boost::none, std::vector<uint8_t>(), tx, tx_key, additional_tx_keys, true, rct_config, use_view_tags);
   CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction");
 
   if (post_tx)

tests/core_tests/rct2.cpp

@@ -136,7 +136,7 @@ bool gen_rct2_tx_validation_base::generate_with(std::vector<test_event_entry>& e
     std::unordered_map<crypto::public_key, cryptonote::subaddress_index> subaddresses;
     subaddresses[miner_accounts[n].get_keys().m_account_address.m_spend_public_key] = {0,0};
     rct_txes.resize(rct_txes.size() + 1);
-    bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, cryptonote::account_public_address{}, std::vector<uint8_t>(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]);
+    bool r = construct_tx_and_get_tx_key(miner_accounts[n].get_keys(), subaddresses, sources, destinations, boost::none, std::vector<uint8_t>(), rct_txes.back(), tx_key, additional_tx_keys, true, rct_config[n]);
     CHECK_AND_ASSERT_MES(r, false, "failed to construct transaction");
 
     if (post_tx && !post_tx(rct_txes.back(), n))

tests/performance_tests/check_tx_signature.h

@@ -72,7 +72,7 @@ public:
     std::unordered_map<crypto::public_key, cryptonote::subaddress_index> subaddresses;
     subaddresses[this->m_miners[this->real_source_idx].get_keys().m_account_address.m_spend_public_key] = {0,0};
     rct::RCTConfig rct_config{range_proof_type, bp_version};
-    if (!construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, destinations, cryptonote::account_public_address{}, std::vector<uint8_t>(), m_tx, tx_key, additional_tx_keys, rct, rct_config))
+    if (!construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, destinations, boost::none, std::vector<uint8_t>(), m_tx, tx_key, additional_tx_keys, rct, rct_config))
       return false;
 
     get_transaction_prefix_hash(m_tx, m_tx_prefix_hash);
@@ -136,7 +136,7 @@ public:
     m_txes.resize(a_num_txes);
     for (size_t n = 0; n < a_num_txes; ++n)
     {
-      if (!construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, destinations, cryptonote::account_public_address{}, std::vector<uint8_t>(), m_txes[n], tx_key, additional_tx_keys, true, {rct::RangeProofPaddedBulletproof, 2}))
+      if (!construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, destinations, boost::none, std::vector<uint8_t>(), m_txes[n], tx_key, additional_tx_keys, true, {rct::RangeProofPaddedBulletproof, 2}))
         return false;
     }
 

tests/performance_tests/construct_tx.h

@@ -74,7 +74,7 @@ public:
     std::unordered_map<crypto::public_key, cryptonote::subaddress_index> subaddresses;
     subaddresses[this->m_miners[this->real_source_idx].get_keys().m_account_address.m_spend_public_key] = {0,0};
     rct::RCTConfig rct_config{range_proof_type, bp_version};
-    return cryptonote::construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, m_destinations, cryptonote::account_public_address{}, std::vector<uint8_t>(), m_tx, tx_key, additional_tx_keys, rct, rct_config);
+    return cryptonote::construct_tx_and_get_tx_key(this->m_miners[this->real_source_idx].get_keys(), subaddresses, this->m_sources, m_destinations, boost::none, std::vector<uint8_t>(), m_tx, tx_key, additional_tx_keys, rct, rct_config);
   }
 
 private: