Fix unguarded worker_index read and harden POS commit-reveal

src/cryptonote_core/master_node_voting.cpp

@@ -520,20 +520,34 @@ namespace master_nodes
 
       result = crypto::check_signature(hash, key, vote.signature);
       if (result){
-          /*MDEBUG("Signature accepted for " << vote.type << " voter " << vote.index_in_group << "/" << key
+          MTRACE("Signature accepted for " << vote.type << " voter " << vote.index_in_group << "/" << key
                                            << (vote.type == quorum_type::obligations ? " voting for worker " +
                                                                                        std::to_string(
                                                                                                vote.state_change.worker_index)
                                                                                      : "")
                                  << " at height " << vote.block_height);
-          */
+
           if(vote.type == quorum_type::obligations){
-              //MDEBUG("Signature accepted for " << quorum.workers[vote.state_change.worker_index]);
+              MTRACE("Signature accepted for " << quorum.workers[vote.state_change.worker_index]);
           }
     }
     else {
         vvc.m_signature_not_valid = true;
-        MDEBUG("Signature not accepted for MN " << quorum.workers[vote.state_change.worker_index]);
+
+        // NOTE: `worker_index` only exists on obligation votes -- it is the inactive member of the
+        // vote's union for every other type, so reading it here for e.g. a checkpoint vote both is
+        // UB and yields attacker-controlled bytes taken from the vote's block hash. Checkpoint
+        // quorums also leave `workers` empty, and this path never bounds-checks the index (only the
+        // obligations branch above calls bounds_check_worker_index), so an unguarded read here is an
+        // out-of-bounds access on an empty vector reachable from any peer that sends us a vote.
+        //
+        // Reaching this point on an obligations vote does imply the index was bounds-checked, so
+        // that branch is safe; every other vote type must log via its validator instead.
+        if (vote.type == quorum_type::obligations)
+            MDEBUG("Signature not accepted for MN " << quorum.workers[vote.state_change.worker_index]);
+        else
+            MDEBUG("Signature not accepted for " << vote.type << " voter " << vote.index_in_group
+                                                 << "/" << key << " at height " << vote.block_height);
     }
 
     return result;

src/cryptonote_core/pos.cpp

@@ -672,16 +672,21 @@ void POS::handle_message(void *quorumnet_state, POS::message const &msg)
       auto &value  = quorum[msg.quorum_position];
       if (value) return;
 
-      if (auto const &hash = context.transient.random_value_hashes.wait.data[msg.quorum_position]; hash)
+      auto const &hash = context.transient.random_value_hashes.wait.data[msg.quorum_position];
+      if (!hash)
       {
-        auto derived = blake2b_hash(msg.random_value.value.data, sizeof(msg.random_value.value.data));
-        if (derived != *hash)
-        {
-          MTRACE(log_prefix(context) << "Dropping " << msg_source_string(context, msg)
-                                    << ". Rederived random value hash " << derived << " does not match original hash "
-                                    << *hash);
-          return;
-        }
+        MTRACE(log_prefix(context) << "Dropping " << msg_source_string(context, msg)
+                                  << ". No random value hash was committed for this quorum position");
+        return;
+      }
+
+      auto derived = blake2b_hash(msg.random_value.value.data, sizeof(msg.random_value.value.data));
+      if (derived != *hash)
+      {
+        MTRACE(log_prefix(context) << "Dropping " << msg_source_string(context, msg)
+                                  << ". Rederived random value hash " << derived << " does not match original hash "
+                                  << *hash);
+        return;
       }
 
       value = msg.random_value.value;
@@ -1653,9 +1658,16 @@ round_state send_and_wait_for_signed_blocks(round_context &context, master_nodes
     if (!enforce_validator_participation_and_timeouts(context, stage, node_list, timed_out, all_received))
       return goto_preparing_for_next_round(context);
 
-    // Select signatures randomly so we don't always just take the first N required signatures.
-    // Then sort just the first N required signatures, so signatures are added
-    // to the block in sorted order, but were chosen randomly.
+    // NOTE: Collect the quorum positions that signed, in ascending order, and take the first
+    // POS_BLOCK_REQUIRED_SIGNATURES of them.
+    //
+    // Any subset of the participating validators is equally valid here: verification only
+    // requires exactly N signatures, given in ascending order, from validators set in the
+    // block's participation bitset (see verify_quorum_signatures). Which subset we pick has no
+    // further effect either -- a validator's participation is scored from POS.validator_bitset,
+    // not from the signatures that made it into the block, so leaving a validator out does not
+    // count against it. Quorum positions are themselves re-randomised every block, so taking
+    // the lowest N does not favour any particular master node over time.
     std::array<size_t, master_nodes::POS_QUORUM_NUM_VALIDATORS> indices = {};
     size_t indices_count = 0;
 
@@ -1664,10 +1676,7 @@ round_state send_and_wait_for_signed_blocks(round_context &context, master_nodes
       if (quorum[index])
         indices[indices_count++] = index;
 
-    // Random select from first 'N' POS_BLOCK_REQUIRED_SIGNATURES from indices_count entries.
     assert(indices_count >= master_nodes::POS_BLOCK_REQUIRED_SIGNATURES);
-    std::array<size_t, master_nodes::POS_BLOCK_REQUIRED_SIGNATURES> selected = {};
-    std::sample(indices.begin(), indices.begin() + indices_count, selected.begin(), selected.size(), tools::rng);
 
     // Add Signatures
     cryptonote::block &final_block = context.transient.signed_block.final_block;

src/wallet/wallet2.cpp

@@ -9802,7 +9802,7 @@ void wallet2::light_wallet_get_outs(std::vector<std::vector<tools::wallet2::get_
           break;
         }
       }
-      THROW_WALLET_EXCEPTION_IF(!found_amount , error::wallet_internal_error, "Outputs for amount " + boost::lexical_cast<std::string>(ores.amount_outs[amount_key].amount) + " not found" );
+      THROW_WALLET_EXCEPTION_IF(!found_amount , error::wallet_internal_error, "Outputs for amount " + boost::lexical_cast<std::string>(amount) + " not found" );
 
       LOG_PRINT_L2("Index " << i << "/" << light_wallet_requested_outputs_count << ": idx " << ores.amount_outs[amount_key].outputs[i].global_index << " (real " << td.m_global_output_index << "), unlocked " << "(always in light)" << ", key " << ores.amount_outs[0].outputs[i].public_key);