Address potential security bug and improve testing for the logic constraint function

cpp/src/barretenberg/plonk/composer/splitting_tmp/ultra_plonk_composer.test.cpp

@@ -914,4 +914,4 @@ TEST(ultra_plonk_composer_splitting_tmp, ram)
     EXPECT_EQ(result, true);
 }
 
-} // namespace proof_system::plonk
+} // namespace proof_system::plonk

cpp/src/barretenberg/plonk/composer/ultra_composer.cpp

@@ -1256,13 +1256,25 @@ void UltraComposer::create_new_range_constraint(const uint32_t variable_index,
     }
 }
 
-void UltraComposer::process_range_list(const RangeList& list)
+void UltraComposer::process_range_list(RangeList& list)
 {
     assert_valid_variables(list.variable_indices);
 
     ASSERT(list.variable_indices.size() > 0);
+
+    // replace witness index in variable_indices with the real variable index i.e. if a copy constraint has been
+    // applied on a variable after it was range constrained, this makes sure the indices in list point to the updated
+    // index in the range list so the set equivalence does not fail
+    for (uint32_t& x : list.variable_indices) {
+        x = real_variable_index[x];
+    }
+    // remove duplicate witness indices to prevent the sorted list set size being wrong!
+    std::sort(list.variable_indices.begin(), list.variable_indices.end());
+    auto back_iterator = std::unique(list.variable_indices.begin(), list.variable_indices.end());
+    list.variable_indices.erase(back_iterator, list.variable_indices.end());
+
     // go over variables
-    // for each variable, create mirror variable with same value - with tau tag
+    // iterate over each variable and create mirror variable with same value - with tau tag
     // need to make sure that, in original list, increments of at most 3
     std::vector<uint64_t> sorted_list;
     sorted_list.reserve(list.variable_indices.size());
@@ -1297,7 +1309,7 @@ void UltraComposer::process_range_list(const RangeList& list)
 
 void UltraComposer::process_range_lists()
 {
-    for (const auto& i : range_lists)
+    for (auto& i : range_lists)
         process_range_list(i.second);
 }
 

cpp/src/barretenberg/plonk/composer/ultra_composer.hpp

@@ -58,6 +58,8 @@ class UltraComposer : public ComposerBase {
         uint64_t target_range;
         uint32_t range_tag;
         uint32_t tau_tag;
+        // contains the list of variable indices that are range constrained to target_range
+        // as ordered in the vector of variable indoces from the composer
         std::vector<uint32_t> variable_indices;
     };
 
@@ -447,7 +449,7 @@ class UltraComposer : public ComposerBase {
     }
 
     RangeList create_range_list(const uint64_t target_range);
-    void process_range_list(const RangeList& list);
+    void process_range_list(RangeList& list);
     void process_range_lists();
 
     /**

cpp/src/barretenberg/plonk/composer/ultra_composer.test.cpp

@@ -793,4 +793,30 @@ TYPED_TEST(ultra_composer, range_checks_on_duplicates)
     TestFixture::prove_and_verify(composer, /*expected_result=*/true);
 }
 
+// Ensure copy constraints added on variables smaller than 2^14, which have been previously
+// range constrained, do not break the set equivalence checks because of indices mismatch.
+// 2^14 is DEFAULT_PLOOKUP_RANGE_BITNUM i.e. the maximum size before a variable gets sliced
+// before range constraints are applied to it.
+TEST(ultra_composer, range_constraint_small_variable)
+{
+    auto composer = UltraComposer();
+    uint16_t mask = (1 << 8) - 1;
+    int a = engine.get_random_uint16() & mask;
+    uint32_t a_idx = composer.add_variable(fr(a));
+    uint32_t b_idx = composer.add_variable(fr(a));
+    ASSERT_NE(a_idx, b_idx);
+    uint32_t c_idx = composer.add_variable(fr(a));
+    ASSERT_NE(c_idx, b_idx);
+    composer.create_range_constraint(b_idx, 8, "bad range");
+    composer.assert_equal(a_idx, b_idx);
+    composer.create_range_constraint(c_idx, 8, "bad range");
+    composer.assert_equal(a_idx, c_idx);
+
+    auto prover = composer.create_prover();
+    auto proof = prover.construct_proof();
+    auto verifier = composer.create_verifier();
+    bool result = verifier.verify_proof(proof);
+    EXPECT_EQ(result, true);
+}
+
 } // namespace proof_system::plonk::test_ultra_composer

cpp/src/barretenberg/stdlib/primitives/logic/logic.cpp

@@ -2,13 +2,19 @@
 
 #include "../composers/composers.hpp"
 #include "../plookup/plookup.hpp"
+#include "barretenberg/common/assert.hpp"
+#include "barretenberg/numeric/uint256/uint256.hpp"
+#include "barretenberg/stdlib/primitives/field/field.hpp"
+#include <cstddef>
 
 namespace proof_system::plonk::stdlib {
 
 /**
  * @brief A logical AND or XOR over a variable number of bits.
  *
- * @details Defaults to basic Composer method if not using plookup-compatible composer
+ * @details Defaults to basic Composer method if not using plookup-compatible composer. If the left and right operands
+ * are larger than num_bit, the result will be truncated to num_bits. However, the two operands could be
+ * range-constrained to num_bits before the call which would remove the need to range constrain inside this function.
  *
  * @tparam Composer
  * @param a
@@ -18,10 +24,17 @@ namespace proof_system::plonk::stdlib {
  * @return field_t<Composer>
  */
 template <typename Composer>
-field_t<Composer> logic<Composer>::create_logic_constraint(field_pt& a, field_pt& b, size_t num_bits, bool is_xor_gate)
+field_t<Composer> logic<Composer>::create_logic_constraint(
+    field_pt& a,
+    field_pt& b,
+    size_t num_bits,
+    bool is_xor_gate,
+    const std::function<std::pair<uint256_t, uint256_t>(uint256_t, uint256_t, size_t)>& get_chunk)
 {
-    // can't extend past field size!
+    // ensure the number of bits doesn't exceed field size and is not negatove
     ASSERT(num_bits < 254);
+    ASSERT(num_bits > 0);
+
     if (a.is_constant() && b.is_constant()) {
         uint256_t a_native(a.get_value());
         uint256_t b_native(b.get_value());
@@ -31,59 +44,72 @@ field_t<Composer> logic<Composer>::create_logic_constraint(field_pt& a, field_pt
     if (a.is_constant() && !b.is_constant()) {
         Composer* ctx = b.get_context();
         uint256_t a_native(a.get_value());
-        field_t<Composer> a_witness = field_pt::from_witness_index(ctx, ctx->put_constant_variable(a_native));
-        return create_logic_constraint(a_witness, b, num_bits, is_xor_gate);
+        field_pt a_witness = field_pt::from_witness_index(ctx, ctx->put_constant_variable(a_native));
+        return create_logic_constraint(a_witness, b, num_bits, is_xor_gate, get_chunk);
     }
     if (!a.is_constant() && b.is_constant()) {
         Composer* ctx = a.get_context();
         uint256_t b_native(b.get_value());
         field_pt b_witness = field_pt::from_witness_index(ctx, ctx->put_constant_variable(b_native));
-        return create_logic_constraint(a, b_witness, num_bits, is_xor_gate);
+        return create_logic_constraint(a, b_witness, num_bits, is_xor_gate, get_chunk);
     }
     if constexpr (Composer::type == ComposerType::PLOOKUP) {
         Composer* ctx = a.get_context();
 
         const size_t num_chunks = (num_bits / 32) + ((num_bits % 32 == 0) ? 0 : 1);
-        uint256_t left(a.get_value());
-        uint256_t right(b.get_value());
+        auto left((uint256_t)a.get_value());
+        auto right((uint256_t)b.get_value());
+
+        field_pt a_accumulator(barretenberg::fr::zero());
+        field_pt b_accumulator(barretenberg::fr::zero());
 
         field_pt res(ctx, 0);
         for (size_t i = 0; i < num_chunks; ++i) {
-            uint256_t left_chunk = left & ((uint256_t(1) << 32) - 1);
-            uint256_t right_chunk = right & ((uint256_t(1) << 32) - 1);
-
-            const field_pt a_chunk = witness_pt(ctx, left_chunk);
-            const field_pt b_chunk = witness_pt(ctx, right_chunk);
+            size_t chunk_size = (i != num_chunks - 1) ? 32 : num_bits - i * 32;
+            auto [left_chunk, right_chunk] = get_chunk(left, right, chunk_size);
 
+            field_pt a_chunk = witness_pt(ctx, left_chunk);
+            field_pt b_chunk = witness_pt(ctx, right_chunk);
             field_pt result_chunk = 0;
             if (is_xor_gate) {
                 result_chunk =
                     stdlib::plookup_read::read_from_2_to_1_table(plookup::MultiTableId::UINT32_XOR, a_chunk, b_chunk);
+
             } else {
                 result_chunk =
                     stdlib::plookup_read::read_from_2_to_1_table(plookup::MultiTableId::UINT32_AND, a_chunk, b_chunk);
             }
 
-            uint256_t scaling_factor = uint256_t(1) << (32 * i);
-            res += result_chunk * scaling_factor;
+            auto scaling_factor = uint256_t(1) << (32 * i);
+            a_accumulator += a_chunk * scaling_factor;
+            b_accumulator += b_chunk * scaling_factor;
 
-            if (i == num_chunks - 1) {
-                const size_t final_num_bits = num_bits - (i * 32);
-                if (final_num_bits != 32) {
-                    ctx->create_range_constraint(a_chunk.witness_index, final_num_bits, "bad range on a");
-                    ctx->create_range_constraint(b_chunk.witness_index, final_num_bits, "bad range on b");
-                }
+            if (chunk_size != 32) {
+                ctx->create_range_constraint(
+                    a_chunk.witness_index, chunk_size, "stdlib logic: bad range on final chunk of left operand");
+                ctx->create_range_constraint(
+                    b_chunk.witness_index, chunk_size, "stdlib logic: bad range on final chunk of right operand");
             }
 
+            res += result_chunk * scaling_factor;
+
             left = left >> 32;
             right = right >> 32;
         }
+        field_pt a_slice = a.slice(static_cast<uint8_t>(num_bits - 1), 0)[1];
+        field_pt b_slice = b.slice(static_cast<uint8_t>(num_bits - 1), 0)[1];
+        a_slice.assert_equal(a_accumulator, "stdlib logic: failed to reconstruct left operand");
+        b_slice.assert_equal(b_accumulator, "stdlib logic: failed to reconstruct right operand");
 
         return res;
     } else {
+        // If the composer doesn't have lookups we call the expensive logic constraint gate
+        // which creates constraints for each bit. We only create constraints up to num_bits.
         Composer* ctx = a.get_context();
+        field_pt a_slice = a.slice(static_cast<uint8_t>(num_bits - 1), 0)[1];
+        field_pt b_slice = b.slice(static_cast<uint8_t>(num_bits - 1), 0)[1];
         auto accumulator_triple = ctx->create_logic_constraint(
-            a.normalize().get_witness_index(), b.normalize().get_witness_index(), num_bits, is_xor_gate);
+            a_slice.normalize().get_witness_index(), b_slice.normalize().get_witness_index(), num_bits, is_xor_gate);
         auto out_idx = accumulator_triple.out[accumulator_triple.out.size() - 1];
         return field_t<Composer>::from_witness_index(ctx, out_idx);
     }

cpp/src/barretenberg/stdlib/primitives/logic/logic.hpp

@@ -1,17 +1,31 @@
 #pragma once
+#include "barretenberg/numeric/uint256/uint256.hpp"
 #include "barretenberg/stdlib/primitives/composers/composers_fwd.hpp"
 #include "barretenberg/stdlib/primitives/field/field.hpp"
 #include "barretenberg/stdlib/primitives/witness/witness.hpp"
+#include <cstdint>
+#include <functional>
+#include <utility>
 
 namespace proof_system::plonk::stdlib {
 
 template <typename Composer> class logic {
-  private:
+  public:
     using field_pt = field_t<Composer>;
     using witness_pt = witness_t<Composer>;
 
   public:
-    static field_pt create_logic_constraint(field_pt& a, field_pt& b, size_t num_bits, bool is_xor_gate);
+    static field_pt create_logic_constraint(
+        field_pt& a,
+        field_pt& b,
+        size_t num_bits,
+        bool is_xor_gate,
+        const std::function<std::pair<uint256_t, uint256_t>(uint256_t, uint256_t, size_t)>& get_chunk =
+            [](uint256_t left, uint256_t right, size_t chunk_size) {
+                uint256_t left_chunk = left & ((uint256_t(1) << chunk_size) - 1);
+                uint256_t right_chunk = right & ((uint256_t(1) << chunk_size) - 1);
+                return std::make_pair(left_chunk, right_chunk);
+            });
 };
 
 EXTERN_STDLIB_TYPE(logic);