fix!: multiple traces had ghost row injection vulnerabilities

barretenberg/cpp/pil/vm2/opcodes/emit_unencrypted_log.pil

@@ -221,7 +221,9 @@ namespace emit_unencrypted_log;
     // So what we landed with is to always read memory except we can't read memory due to the first case.
     // This should be fine since the user already paid for the memory reads
     pol commit sel_should_read_memory;
-    sel_should_read_memory = is_write_memory_value * (1 - error_out_of_bounds);
+    // Gate by sel to avoid ghost rows triggering memory reads.
+    #[SEL_SHOULD_READ_MEMORY_IS_SEL_AND_WRITE_MEM_AND_NO_ERR]
+    sel_should_read_memory = sel * is_write_memory_value * (1 - error_out_of_bounds);
 
     #[LOG_ADDRESS_INCREMENT]
     NOT_END * ((log_address + is_write_memory_value) - log_address') = 0;
@@ -291,4 +293,3 @@ namespace emit_unencrypted_log;
         precomputed.clk,
         public_inputs.cols[0]
     };
-

barretenberg/cpp/pil/vm2/opcodes/get_contract_instance.pil

@@ -140,6 +140,10 @@ namespace get_contract_instance;
     // Do not allow is_valid_member_enum to be 1 if the precomputed lookup is disabled.
     #[IS_VALID_MEMBER_ENUM_ONLY_SET_BY_PRECOMPUTED_LOOKUP]
     WRITES_OUT_OF_BOUNDS * is_valid_member_enum = 0;
+    // Force is_valid_writes_in_bounds to 0 when sel is 0, and therefore also force is_valid_member_enum to 0
+    // when combined with the constraint above. This prevents ghost row injection attacks.
+    #[IS_VALID_WRITES_IN_BOUNDS_REQUIRES_SEL]
+    is_valid_writes_in_bounds * (1 - sel) = 0;
 
     // Error aggregation
     // "Some/any error happened"

barretenberg/cpp/pil/vm2/to_radix_mem.pil

@@ -267,6 +267,9 @@ namespace to_radix_mem;
     // On following rows, we propagate sel_should_write_mem.
     #[SEL_SHOULD_WRITE_MEM_CONTINUITY]
     NOT_LAST * (sel_should_write_mem' - sel_should_write_mem) = 0;
+    // Force sel_should_write_mem to 0 when sel is 0 (prevents ghost row injection attacks)
+    #[SEL_SHOULD_WRITE_MEM_REQUIRES_SEL]
+    sel_should_write_mem * (1 - sel) = 0;
 
     pol commit output_tag;
     // Conditional Assignment: is_output_bits ? U1 : U8

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/emit_unencrypted_log.test.cpp

@@ -10,6 +10,7 @@
 #include "barretenberg/vm2/constraining/testing/check_relation.hpp"
 #include "barretenberg/vm2/generated/relations/emit_unencrypted_log.hpp"
 #include "barretenberg/vm2/generated/relations/lookups_emit_unencrypted_log.hpp"
+#include "barretenberg/vm2/generated/relations/perms_emit_unencrypted_log.hpp"
 #include "barretenberg/vm2/simulation/events/emit_unencrypted_log_event.hpp"
 #include "barretenberg/vm2/simulation/events/event_emitter.hpp"
 #include "barretenberg/vm2/simulation/events/gt_event.hpp"
@@ -18,6 +19,7 @@
 #include "barretenberg/vm2/testing/macros.hpp"
 #include "barretenberg/vm2/testing/public_inputs_builder.hpp"
 #include "barretenberg/vm2/tracegen/gt_trace.hpp"
+#include "barretenberg/vm2/tracegen/memory_trace.hpp"
 #include "barretenberg/vm2/tracegen/opcodes/emit_unencrypted_log_trace.hpp"
 #include "barretenberg/vm2/tracegen/precomputed_trace.hpp"
 #include "barretenberg/vm2/tracegen/public_inputs_trace.hpp"
@@ -31,6 +33,8 @@ using simulation::EmitUnencryptedLogWriteEvent;
 using simulation::TrackedSideEffects;
 using testing::PublicInputsBuilder;
 using tracegen::EmitUnencryptedLogTraceBuilder;
+using tracegen::MemoryTraceBuilder;
+using tracegen::PrecomputedTraceBuilder;
 using tracegen::PublicInputsTraceBuilder;
 using tracegen::TestTraceContainer;
 using FF = AvmFlavorSettings::FF;
@@ -639,6 +643,93 @@ TEST(EmitUnencryptedLogConstrainingTest, NegativeContractAddressConsistency)
         "CONTRACT_ADDRESS_CONSISTENCY");
 }
 
+// =====================================================================
+// Ghost Row Injection Vulnerability Tests
+// =====================================================================
+// These tests verify that ghost rows (sel=0) cannot fire permutations.
+// This is a defensive/sanity check: even though the situation is hard to exploit,
+// we still enforce the selector gating to prevent accidental ghost reads.
+// The vulnerability: is_write_memory_value is only boolean-constrained,
+// not constrained to be 0 when sel=0. This allows ghost rows to fire
+// the #[READ_MEM] permutation via sel_should_read_memory.
+//
+// VULNERABILITY SUMMARY:
+// - is_write_memory_value is only boolean-constrained
+// - When sel=0, is_write_memory_value can still be set to 1
+// - This makes sel_should_read_memory = 1 (via derived constraint)
+// - This fires the #[READ_MEM] permutation from a ghost row
+//
+// REQUIRED FIX:
+// Gate by sel to avoid ghost rows triggering memory reads.
+// sel_should_read_memory = sel * is_write_memory_value * (1 - error_out_of_bounds);
+
+// This test verifies that the fix for the ghost row injection vulnerability works.
+// The constraint `is_write_memory_value * (1 - sel) = 0` should prevent ghost rows
+// from setting is_write_memory_value=1 when sel=0.
+TEST(EmitUnencryptedLogConstrainingTest, NegativeGhostRowInjectionBlocked)
+{
+    TestTraceContainer trace;
+    MemoryTraceBuilder memory_trace_builder;
+    PrecomputedTraceBuilder precomputed_trace_builder;
+
+    uint32_t malicious_clk = 42;
+    uint16_t malicious_space_id = 1;
+    MemoryAddress malicious_log_addr = 0xDEAD;
+    FF malicious_value = 0x1337;
+    MemoryTag malicious_tag = MemoryTag::FF;
+
+    std::vector<simulation::MemoryEvent> mem_events = {
+        {
+            .execution_clk = malicious_clk,
+            .mode = simulation::MemoryMode::READ,
+            .addr = malicious_log_addr,
+            .value = MemoryValue::from<FF>(malicious_value),
+            .space_id = malicious_space_id,
+        },
+    };
+
+    precomputed_trace_builder.process_sel_range_8(trace);
+    precomputed_trace_builder.process_sel_range_16(trace);
+    precomputed_trace_builder.process_misc(trace, 1 << 16);
+    precomputed_trace_builder.process_tag_parameters(trace);
+    memory_trace_builder.process(mem_events, trace);
+
+    uint32_t memory_row = 0;
+    for (uint32_t row = 0; row < trace.get_num_rows(); row++) {
+        if (trace.get(C::memory_sel, row) == 1) {
+            memory_row = row;
+            break;
+        }
+    }
+
+    // Attempt ghost row injection: sel=0 but is_write_memory_value=1
+    uint32_t ghost_row = 0;
+    trace.set(ghost_row,
+              std::vector<std::pair<Column, FF>>{
+                  { C::precomputed_first_row, 1 },
+                  { C::precomputed_clk, ghost_row },
+                  { C::precomputed_zero, 0 },
+                  { C::emit_unencrypted_log_sel, 0 },
+                  { C::emit_unencrypted_log_is_write_memory_value, 1 },
+                  { C::emit_unencrypted_log_error_out_of_bounds, 0 },
+                  { C::emit_unencrypted_log_sel_should_read_memory, 1 },
+                  { C::emit_unencrypted_log_execution_clk, malicious_clk },
+                  { C::emit_unencrypted_log_space_id, malicious_space_id },
+                  { C::emit_unencrypted_log_log_address, malicious_log_addr },
+                  { C::emit_unencrypted_log_value, malicious_value },
+                  { C::emit_unencrypted_log_tag, static_cast<uint8_t>(malicious_tag) },
+                  { C::emit_unencrypted_log_public_inputs_value, malicious_value },
+              });
+
+    trace.set(C::memory_sel_unencrypted_log_read, memory_row, 1);
+
+    // The fix: sel_should_read_memory = sel * is_write_memory_value * (1 - error_out_of_bounds)
+    // Gating by sel should cause the relation check to fail
+    // because sel_should_read_memory=1 and sel=0 violates this constraint
+    EXPECT_THROW_WITH_MESSAGE(check_relation<emit_unencrypted_log>(trace),
+                              "SEL_SHOULD_READ_MEMORY_IS_SEL_AND_WRITE_MEM_AND_NO_ERR");
+}
+
 } // namespace
 
 } // namespace bb::avm2::constraining

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/get_contract_instance.test.cpp

@@ -10,10 +10,12 @@
 #include "barretenberg/vm2/constraining/testing/check_relation.hpp"
 #include "barretenberg/vm2/generated/relations/get_contract_instance.hpp"
 #include "barretenberg/vm2/generated/relations/lookups_get_contract_instance.hpp"
+#include "barretenberg/vm2/generated/relations/perms_get_contract_instance.hpp"
 #include "barretenberg/vm2/simulation/events/event_emitter.hpp"
 #include "barretenberg/vm2/simulation/events/get_contract_instance_event.hpp"
 #include "barretenberg/vm2/testing/fixtures.hpp"
 #include "barretenberg/vm2/testing/macros.hpp"
+#include "barretenberg/vm2/tracegen/memory_trace.hpp"
 #include "barretenberg/vm2/tracegen/opcodes/get_contract_instance_trace.hpp"
 #include "barretenberg/vm2/tracegen/precomputed_trace.hpp"
 #include "barretenberg/vm2/tracegen/test_trace_container.hpp"
@@ -24,6 +26,7 @@ namespace {
 using simulation::EventEmitter;
 using simulation::GetContractInstanceEvent;
 using tracegen::GetContractInstanceTraceBuilder;
+using tracegen::MemoryTraceBuilder;
 using tracegen::PrecomputedTraceBuilder;
 using tracegen::TestTraceContainer;
 using FF = AvmFlavorSettings::FF;
@@ -398,5 +401,89 @@ TEST(GetContractInstanceConstrainingTest, IntegrationTracegenOutOfBounds)
     check_relation<get_contract_instance>(trace);
 }
 
+// =====================================================================
+// Ghost Row Injection Vulnerability Tests
+// =====================================================================
+// These tests verify that ghost rows (sel=0) cannot fire permutations.
+// The fix: is_valid_member_enum * (1 - sel) = 0 ensures is_valid_member_enum
+// is forced to 0 when sel=0, preventing ghost rows from firing permutations.
+//
+// Test that the fix blocks ghost row injection attacks.
+// Attack pattern:
+// 1. Create legitimate memory WRITE events (destination side)
+// 2. Build memory trace from those events
+// 3. Inject ghost get_contract_instance row with sel=0 but is_valid_member_enum=1
+// 4. The fix should cause the relation check to fail
+TEST(GetContractInstanceConstrainingTest, NegativeGhostRowInjectionBlocked)
+{
+    TestTraceContainer trace;
+    MemoryTraceBuilder memory_trace_builder;
+    PrecomputedTraceBuilder precomputed_trace_builder;
+
+    // Attacker-controlled values
+    uint32_t malicious_clk = 42;
+    uint16_t malicious_space_id = 1;
+    MemoryAddress malicious_dst_offset = 0x100;
+    uint1_t malicious_instance_exists = 1;
+    MemoryTag exists_tag = MemoryTag::U1;
+
+    // Create legitimate memory events
+    std::vector<simulation::MemoryEvent> mem_events = {
+        {
+            .execution_clk = malicious_clk,
+            .mode = simulation::MemoryMode::WRITE,
+            .addr = malicious_dst_offset,
+            .value = MemoryValue::from<uint1_t>(malicious_instance_exists),
+            .space_id = malicious_space_id,
+        },
+    };
+
+    // Build memory trace (destination side)
+    precomputed_trace_builder.process_sel_range_8(trace);
+    precomputed_trace_builder.process_sel_range_16(trace);
+    precomputed_trace_builder.process_misc(trace, 1 << 16);
+    precomputed_trace_builder.process_tag_parameters(trace);
+    memory_trace_builder.process(mem_events, trace);
+
+    // Find where the memory row was placed
+    uint32_t memory_row = 0;
+    for (uint32_t row = 0; row < trace.get_num_rows(); row++) {
+        if (trace.get(C::memory_sel, row) == 1) {
+            memory_row = row;
+            break;
+        }
+    }
+
+    // Inject ghost get_contract_instance row
+    // Ghost row: sel = 0, but is_valid_member_enum = 1 (attack attempt)
+    uint32_t ghost_row = 0;
+    trace.set(ghost_row,
+              std::vector<std::pair<Column, FF>>{
+                  { C::precomputed_first_row, 1 },
+                  { C::precomputed_clk, ghost_row },
+                  { C::get_contract_instance_sel, 0 },
+                  { C::get_contract_instance_is_valid_member_enum, 1 },
+                  { C::get_contract_instance_is_valid_writes_in_bounds, 1 },
+                  { C::get_contract_instance_exists_tag, static_cast<uint8_t>(exists_tag) },
+                  { C::get_contract_instance_clk, malicious_clk },
+                  { C::get_contract_instance_space_id, malicious_space_id },
+                  { C::get_contract_instance_dst_offset, malicious_dst_offset },
+                  { C::get_contract_instance_instance_exists, static_cast<uint64_t>(malicious_instance_exists) },
+                  { C::get_contract_instance_sel_error, 0 },
+                  { C::get_contract_instance_is_deployer, 0 },
+                  { C::get_contract_instance_is_class_id, 0 },
+                  { C::get_contract_instance_is_init_hash, 0 },
+                  { C::get_contract_instance_selected_member, 0 },
+                  { C::get_contract_instance_member_write_offset, malicious_dst_offset + 1 },
+                  { C::get_contract_instance_member_tag, static_cast<uint8_t>(MemoryTag::FF) },
+              });
+
+    trace.set(C::memory_sel_get_contract_instance_exists_write, memory_row, 1);
+
+    // The fix: is_valid_writes_in_bounds * (1 - sel) = 0 should cause the relation check to fail
+    // (in conjunction with WRITES_OUT_OF_BOUNDS * is_valid_member_enum = 0)
+    EXPECT_THROW_WITH_MESSAGE(check_relation<get_contract_instance>(trace), "IS_VALID_WRITES_IN_BOUNDS_REQUIRES_SEL");
+}
+
 } // namespace
 } // namespace bb::avm2::constraining

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp

@@ -8,6 +8,7 @@
 #include "barretenberg/vm2/constraining/testing/check_relation.hpp"
 #include "barretenberg/vm2/generated/relations/lookups_to_radix.hpp"
 #include "barretenberg/vm2/generated/relations/lookups_to_radix_mem.hpp"
+#include "barretenberg/vm2/generated/relations/perms_to_radix_mem.hpp"
 #include "barretenberg/vm2/simulation/events/gt_event.hpp"
 #include "barretenberg/vm2/simulation/events/range_check_event.hpp"
 #include "barretenberg/vm2/simulation/gadgets/range_check.hpp"
@@ -20,6 +21,7 @@
 #include "barretenberg/vm2/testing/macros.hpp"
 #include "barretenberg/vm2/tracegen/execution_trace.hpp"
 #include "barretenberg/vm2/tracegen/gt_trace.hpp"
+#include "barretenberg/vm2/tracegen/memory_trace.hpp"
 #include "barretenberg/vm2/tracegen/precomputed_trace.hpp"
 #include "barretenberg/vm2/tracegen/test_trace_container.hpp"
 #include "barretenberg/vm2/tracegen/to_radix_trace.hpp"
@@ -32,6 +34,7 @@ using ::testing::StrictMock;
 
 using tracegen::ExecutionTraceBuilder;
 using tracegen::GreaterThanTraceBuilder;
+using tracegen::MemoryTraceBuilder;
 using tracegen::PrecomputedTraceBuilder;
 using tracegen::TestTraceContainer;
 using tracegen::ToRadixTraceBuilder;
@@ -1006,6 +1009,106 @@ TEST(ToRadixMemoryConstrainingTest, ComplexTest)
                       lookup_to_radix_mem_input_output_to_radix_settings>(trace);
 }
 
+// =====================================================================
+// Ghost Row Injection Vulnerability Tests
+// =====================================================================
+// These tests verify that ghost rows (sel=0) cannot fire permutations.
+// The fix: sel_should_write_mem * (1 - sel) = 0 ensures sel_should_write_mem
+// is forced to 0 when sel=0, preventing ghost rows from firing permutations.
+
+// Test that ghost rows (sel=0) cannot set sel_should_write_mem=1
+TEST(ToRadixMemoryConstrainingTest, NegativeGhostRowMemoryWrite_RelationsOnly)
+{
+    // Try to create a ghost row (sel=0) with sel_should_write_mem=1
+    // which would fire the #[WRITE_MEM] permutation
+    TestTraceContainer trace({
+        {
+            { C::precomputed_first_row, 1 },
+        },
+        {
+            { C::to_radix_mem_sel, 0 },                  // Ghost row: gadget not active
+            { C::to_radix_mem_sel_should_write_mem, 1 }, // Try to fire memory write anyway
+            { C::to_radix_mem_execution_clk, 1 },
+            { C::to_radix_mem_space_id, 1 },
+            { C::to_radix_mem_dst_addr, 100 },
+            { C::to_radix_mem_limb_value, 999 }, // Arbitrary limb value
+            { C::to_radix_mem_output_tag, 2 },   // U8 tag
+        },
+    });
+
+    // The fix: sel_should_write_mem * (1 - sel) = 0
+    // When sel=0 and sel_should_write_mem=1: 1 * (1-0) = 1 != 0 -> FAILS
+    EXPECT_THROW_WITH_MESSAGE(check_relation<to_radix_mem>(trace), "SEL_SHOULD_WRITE_MEM_REQUIRES_SEL");
+}
+
+// Test that the fix blocks ghost row injection attacks with full traces.
+// Attack pattern:
+// 1. Create legitimate memory WRITE events (destination side)
+// 2. Build memory trace from those events
+// 3. Inject ghost to_radix_mem row with sel=0 but sel_should_write_mem=1
+// 4. The fix should cause the relation check to fail
+TEST(ToRadixMemoryConstrainingTest, NegativeGhostRowInjectionBlocked)
+{
+    TestTraceContainer trace;
+    MemoryTraceBuilder memory_trace_builder;
+    PrecomputedTraceBuilder precomputed_trace_builder;
+
+    // Attacker-controlled values
+    uint32_t malicious_clk = 42;
+    uint16_t malicious_space_id = 1;
+    MemoryAddress malicious_addr = 0xDEAD;
+    uint8_t malicious_limb_value = 0x99;
+    MemoryTag malicious_tag = MemoryTag::U8;
+
+    // Create legitimate memory events
+    std::vector<simulation::MemoryEvent> mem_events = {
+        {
+            .execution_clk = malicious_clk,
+            .mode = simulation::MemoryMode::WRITE,
+            .addr = malicious_addr,
+            .value = MemoryValue::from_tag(malicious_tag, malicious_limb_value),
+            .space_id = malicious_space_id,
+        },
+    };
+
+    // Build memory trace (destination side)
+    precomputed_trace_builder.process_sel_range_8(trace);
+    precomputed_trace_builder.process_sel_range_16(trace);
+    precomputed_trace_builder.process_misc(trace, 1 << 16);
+    precomputed_trace_builder.process_tag_parameters(trace);
+    memory_trace_builder.process(mem_events, trace);
+
+    // Find where the memory row was placed
+    uint32_t memory_row = 0;
+    for (uint32_t row = 0; row < trace.get_num_rows(); row++) {
+        if (trace.get(C::memory_sel, row) == 1) {
+            memory_row = row;
+            break;
+        }
+    }
+
+    // Inject ghost to_radix_mem row
+    // Ghost row: sel = 0, but sel_should_write_mem = 1 (attack attempt)
+    uint32_t ghost_row = 0;
+    trace.set(ghost_row,
+              std::vector<std::pair<Column, FF>>{
+                  { C::precomputed_first_row, 1 },
+                  { C::precomputed_clk, ghost_row },
+                  { C::to_radix_mem_sel, 0 },
+                  { C::to_radix_mem_sel_should_write_mem, 1 },
+                  { C::to_radix_mem_execution_clk, malicious_clk },
+                  { C::to_radix_mem_space_id, malicious_space_id },
+                  { C::to_radix_mem_dst_addr, malicious_addr },
+                  { C::to_radix_mem_limb_value, malicious_limb_value },
+                  { C::to_radix_mem_output_tag, static_cast<uint8_t>(malicious_tag) },
+              });
+
+    trace.set(C::memory_sel_to_radix_write, memory_row, 1);
+
+    // The fix: sel_should_write_mem * (1 - sel) = 0 should cause the relation check to fail
+    EXPECT_THROW_WITH_MESSAGE(check_relation<to_radix_mem>(trace), "SEL_SHOULD_WRITE_MEM_REQUIRES_SEL");
+}
+
 } // namespace
 
 } // namespace bb::avm2::constraining