fix: note hash collision (#11869)

barretenberg/cpp/src/barretenberg/crypto/pedersen_hash/pedersen.cpp

@@ -49,6 +49,21 @@ std::vector<typename Curve::BaseField> pedersen_hash_base<Curve>::convert_buffer
  *          `inputs[0] * [generators[hash_index]] + `inputs[1] * [generators[hash_index + 1]]` + ... etc
  *          Potentially useful to ensure multiple hashes with the same domain separator cannot collide.
  *
+ * @note Length inclusion for collision resistance:
+ *       For a given commitment C = a*G1 + b*G2 + c*G3 we take an x-coordinate of C.x and use it as the hash.
+ *       However, due to elliptic curve symmetry about the x-axis, for any x-coordinate,
+ *       there are two points with that x-coordinate. This means -C has the same hash (x-coord) as C,
+ *       and the tuple [-a, -b, -c] produces the same hash as [a, b, c].
+ *
+ *       This property makes the hash trivially not collision resistant without including the length.
+ *       By including the length l, the commitment becomes:
+ *       C = a*G1 + b*G2 + c*G3 + l*G_len
+ *
+ *       Since -l would be -3 (an extraordinarily large number that cannot be a valid preimage length),
+ *       including the length protects against these collisions.
+ *
+ *       For more background, see: https://hackmd.io/@aztec-network/ryzn3JIu3#PedersenHash
+ *
  * @param inputs what are we hashing?
  * @param context Stores generator metadata + context pointer to the generators we are using for this hash
  * @return Fq (i.e. SNARK circuit scalar field, when hashing using a curve defined over the SNARK circuit scalar field)

noir-projects/aztec-nr/aztec/src/encrypted_logs/log_assembly_strategies/default_aes128/note.nr

@@ -534,12 +534,12 @@ mod test {
         let private_log_payload_from_typescript = [
             0x0e9cffc3ddd746affb02410d8f0a823e89939785bcc8e88ee4f3cae05e737c36,
             0x0d460c0e434d846ec1ea286e4090eb56376ff27bddc1aacae1d856549f701fa7,
-            0x00010577790aeabcc2d81ec8d0c99e7f5d2bf2f1452025dc777a178404f851d9,
-            0x003de81cde78411f27a921e16ebbfba71a5570d3f62f1134c90daced33663ba0,
-            0x00856cb19c7d563da183a40a6f8bd4988d1696ad6bf0c717c8fb8f6294bd0366,
-            0x001ed04e4f77a111c7090fcd34c61cfae744e8589a42defba4d0d927dd4679fe,
-            0x00ec09b49d8d4cf548ea62d44c8839b2fd14664e9d1439b199a8d5166e362348,
-            0x004a69de2d410e01010101010101010101010101010101010101010101010101,
+            0x000194e6d7872db8f61e8e59f23580f4db45d13677f873ec473a409cf61fd04d,
+            0x00334e5fb6083721f3eb4eef500876af3c9acfab0a1cb1804b930606fdb0b283,
+            0x00af91db798fa320746831a59b74362dfd0cf9e7c239f6aad11a4b47d0d870ee,
+            0x00d25a054613a83be7be8512f2c09664bc4f7ab60a127b06584f476918581b8a,
+            0x003840d100d8c1d78d4b68b787ed353ebfb8cd2987503d3b472f614f25799a18,
+            0x003f38322629d401010101010101010101010101010101010101010101010101,
             0x0101010101010101010101010101010101010101010101010101010101010101,
             0x0101010101010101010101010101010101010101010101010101010101010101,
             0x0101010101010101010101010101010101010101010101010101010101010101,

noir-projects/aztec-nr/aztec/src/lib.nr

@@ -1,6 +1,5 @@
 mod context;
 mod deploy;
-mod generators;
 mod hash;
 mod history;
 mod keys;

noir-projects/aztec-nr/aztec/src/macros/notes/mod.nr

@@ -58,6 +58,19 @@ comptime fn get_next_note_type_id() -> Field {
 ///         ...
 ///     }
 /// }
+///
+/// # On including length in note hash preimage
+/// For a given commitment C = a*G1 + b*G2 + c*G3 we take an x-coordinate of C.x and use it as the hash.
+/// However, due to elliptic curve symmetry about the x-axis, for any x-coordinate,
+/// there are two points with that x-coordinate. This means -C has the same hash (x-coord) as C,
+/// and the tuple [-a, -b, -c] produces the same hash as [a, b, c].
+///
+/// This property makes the hash trivially not collision resistant without including the length.
+/// By including the length l, the commitment becomes:
+/// C = a*G1 + b*G2 + c*G3 + l*G_len
+///
+/// Since -l would be -3 (an extraordinarily large number that cannot be a valid preimage length),
+/// including the length protects against these collisions.
 comptime fn generate_note_interface(
     s: StructDefinition,
     note_type_id: Field,
@@ -105,10 +118,13 @@ comptime fn generate_note_interface(
     let (new_generators_list, new_scalars_list, _, new_aux_vars) =
         generate_multi_scalar_mul(prefixed_merged_fields);
 
-    let new_generators =
-        new_generators_list.push_back(quote { aztec::generators::G_slot }).join(quote {,});
+    let (g_slot, g_len) = generate_fixed_generators();
+    let new_generators = new_generators_list.push_back(g_slot).push_back(g_len).join(quote {,});
+
+    let merged_fields_len = merged_fields.len() + 1; // +1 for the storage slot appended below
     let new_scalars = new_scalars_list
         .push_back(quote { std::hash::from_field_unsafe(self.header.storage_slot) })
+        .push_back(quote { std::hash::from_field_unsafe($merged_fields_len) })
         .join(quote {,});
 
     (
@@ -269,6 +285,32 @@ pub(crate) comptime fn generate_note_export(
     }
 }
 
+/// Number of fixed generators used to ensure that we don't have a collision of indices in derive_generators(...) in
+/// the generate_multi_scalar_mul(...) function. If the indices collided this could result in a critical vulnerability
+/// (e.g. in case of G_slot collision with other another note field an attacker could move a note to an arbitrary
+/// slot).
+global NUM_FIXED_GENERATORS: u32 = 2;
+
+/// Generates G_slot and G_len generator point quotes.
+comptime fn generate_fixed_generators() -> (Quoted, Quoted) {
+    let generators: [Point; NUM_FIXED_GENERATORS] =
+        derive_generators("aztec_nr_generators".as_bytes(), 0);
+
+    let g_slot_x = generators[0].x;
+    let g_slot_y = generators[0].y;
+    let g_len_x = generators[1].x;
+    let g_len_y = generators[1].y;
+
+    let g_slot = quote {
+        aztec::protocol_types::point::Point { x: $g_slot_x, y: $g_slot_y, is_infinite: false }
+    };
+    let g_len = quote {
+        aztec::protocol_types::point::Point { x: $g_len_x, y: $g_len_y, is_infinite: false }
+    };
+
+    (g_slot, g_len)
+}
+
 /// Generates quotes necessary for multi-scalar multiplication of `indexed_fields` (indexed struct fields). Returns
 /// a tuple containing quotes for generators, scalars, arguments and auxiliary variables. For more info on what are
 /// auxiliary variables and how they are used, see `generate_serialize_to_fields` function.
@@ -291,8 +333,13 @@ comptime fn generate_multi_scalar_mul(
     let mut args_list = &[];
     let mut aux_vars_list = &[];
     for i in 0..indexed_fields.len() {
-        let (field_name, typ, index) = indexed_fields[i];
-        let start_generator_index = index + 1;
+        // Destructure tuple containing:
+        // - field_name: the name of the struct field/member (as a Quoted type)
+        // - typ: the type of the struct field/member (as a Type)
+        // - field_start_index: index where this field starts in the serialized note array (as u32)
+        let (field_name, typ, field_start_index) = indexed_fields[i];
+        // We add NUM_FIXED_GENERATORS to the start index to avoid collision with fixed generators.
+        let start_generator_index = NUM_FIXED_GENERATORS + field_start_index;
         let (serialization_fields, aux_vars) =
             generate_serialize_to_fields(field_name, typ, &[], true);
         for j in 0..serialization_fields.len() {
@@ -331,7 +378,24 @@ comptime fn generate_multi_scalar_mul(
 //
 /// Generates setup payload for a given note struct `s`. The setup payload contains log plaintext and hiding point.
 ///
-/// Example:
+/// # On including length in note hash preimage
+/// The hiding point is computed as a multi-scalar multiplication that includes the length of the preimage
+/// to protect against collisions due to elliptic curve symmetry.
+///
+/// When computing a note hash in the partial notes flow, we take the hiding point, add the nullable fields to it
+/// in public and then we take the x-coordinate of the point and use it as the note hash. E.g. for a given commitment
+/// C = a*G1 + b*G2 + c*G3 we take an x-coordinate of C.x. However, due to elliptic curve symmetry about the x-axis,
+/// for any x-coordinate, there are two points with that x-coordinate. This means -C has the same hash (x-coord) as C,
+/// and the tuple [-a, -b, -c] produces the same hash as [a, b, c].
+///
+/// This property makes the hash trivially not collision resistant without including the length.
+/// By including the length l, the commitment becomes:
+/// C = a*G1 + b*G2 + c*G3 + l*G_len
+///
+/// Since -l would be -3 (an extraordinarily large number that cannot be a valid preimage length),
+/// including the length protects against these collisions.
+///
+/// # Example function output
 /// ```
 /// struct TokenNoteSetupPayload {
 ///     log_plaintext: [u8; 160],
@@ -341,11 +405,17 @@ comptime fn generate_multi_scalar_mul(
 /// impl TokenNoteSetupPayload {
 ///     fn new(mut self, npk_m_hash: Field, randomness: Field, storage_slot: Field) -> TokenNoteSetupPayload {
 ///         let hiding_point = std::embedded_curve_ops::multi_scalar_mul(
-///             [aztec::generators::Ga1, aztec::generators::Ga2, aztec::generators::G_slot],
+///             [
+///                 Point { x: 0x..., y: 0x... },
+///                 Point { x: 0x..., y: 0x... },
+///                 Point { x: 0x..., y: 0x... },
+///                 Point { x: 0x..., y: 0x... }
+///             ],
 ///             [
 ///                 std::hash::from_field_unsafe(npk_m_hash),
 ///                 std::hash::from_field_unsafe(randomness),
-///                 std::hash::from_field_unsafe(storage_slot)
+///                 std::hash::from_field_unsafe(storage_slot),
+///                 std::hash::from_field_unsafe(3)
 ///             ]
 ///         );
 ///
@@ -405,10 +475,13 @@ comptime fn generate_setup_payload(
         .append(new_args_list)
         .push_back(quote { storage_slot: Field })
         .join(quote {,});
-    let new_generators =
-        new_generators_list.push_back(quote { aztec::generators::G_slot }).join(quote {,});
+
+    let (g_slot, g_len) = generate_fixed_generators();
+    let new_generators = new_generators_list.push_back(g_slot).push_back(g_len).join(quote {,});
+    let merged_fields_len = indexed_fixed_fields.len() + indexed_nullable_fields.len() + 1; // +1 for storage_slot
     let new_scalars = new_scalars_list
         .push_back(quote { std::hash::from_field_unsafe(storage_slot) })
+        .push_back(quote { std::hash::from_field_unsafe($merged_fields_len) })
         .join(quote {,});
 
     // Then the log plaintext ones

noir-projects/aztec-nr/aztec/src/test/mocks/mock_note.nr

@@ -1,6 +1,5 @@
 use crate::{
     context::PrivateContext,
-    generators::Ga1 as G_val,
     note::{
         note_header::NoteHeader, note_interface::NoteInterface,
         utils::compute_note_hash_for_nullify,
@@ -9,10 +8,10 @@ use crate::{
 
 use crate::note::note_interface::NullifiableNote;
 use dep::protocol_types::{
-    address::AztecAddress, constants::GENERATOR_INDEX__NOTE_NULLIFIER,
+    address::AztecAddress,
+    constants::{GENERATOR_INDEX__NOTE_HASH, GENERATOR_INDEX__NOTE_NULLIFIER},
     hash::poseidon2_hash_with_separator,
 };
-use dep::std::{embedded_curve_ops::multi_scalar_mul, hash::from_field_unsafe};
 
 global MOCK_NOTE_LENGTH: u32 = 1;
 
@@ -73,9 +72,10 @@ impl NoteInterface<MOCK_NOTE_LENGTH> for MockNote {
             self.header.storage_slot != 0,
             "Storage slot must be set before computing note hash",
         );
-        // We use the unsafe version because the multi_scalar_mul will constrain the scalars.
-        let value_scalar = from_field_unsafe(self.value);
-        multi_scalar_mul([G_val], [value_scalar]).x
+        // We use Poseidon2 instead of multi-scalar multiplication (MSM) here since this is not a partial note
+        // and therefore does not require MSM's additive homomorphism property. Additionally, Poseidon2 uses fewer
+        // constraints.
+        poseidon2_hash_with_separator(self.pack_content(), GENERATOR_INDEX__NOTE_HASH)
     }
 }
 

noir-projects/noir-contracts/contracts/ecdsa_public_key_note/src/lib.nr

@@ -2,13 +2,10 @@ use dep::aztec::prelude::{NoteHeader, NoteInterface, NullifiableNote, PrivateCon
 
 use dep::aztec::{
     note::utils::compute_note_hash_for_nullify, keys::getters::{get_nsk_app, get_public_keys},
-    protocol_types::{address::AztecAddress, constants::GENERATOR_INDEX__NOTE_NULLIFIER, hash::poseidon2_hash_with_separator},
-    macros::notes::note_custom_interface, generators::Ga1 as Gx_1, generators::Ga2 as Gx_2,
-    generators::Ga3 as Gy_1, generators::Ga4 as Gy_2, generators::Ga5 as G_owner, generators::G_slot
+    protocol_types::{address::AztecAddress, constants::{GENERATOR_INDEX__NOTE_NULLIFIER, GENERATOR_INDEX__NOTE_HASH}, hash::poseidon2_hash_with_separator},
+    macros::notes::note_custom_interface
 };
 
-use std::hash::from_field_unsafe;
-
 global ECDSA_PUBLIC_KEY_NOTE_LEN: u32 = 5;
 
 // Stores an ECDSA public key composed of two 32-byte elements
@@ -80,18 +77,10 @@ impl NoteInterface<ECDSA_PUBLIC_KEY_NOTE_LEN> for EcdsaPublicKeyNote {
     }
 
     fn compute_note_hash(self) -> Field {
-        let packed_content = self.pack_content();
-        std::embedded_curve_ops::multi_scalar_mul(
-            [Gx_1, Gx_2, Gy_1, Gy_2, G_owner, G_slot],
-            [
-            from_field_unsafe(packed_content[0]),
-            from_field_unsafe(packed_content[1]),
-            from_field_unsafe(packed_content[2]),
-            from_field_unsafe(packed_content[3]),
-            from_field_unsafe(packed_content[4]),
-            from_field_unsafe(self.get_header().storage_slot)
-        ]
-        ).x
+        // We use Poseidon2 instead of multi-scalar multiplication (MSM) here since this is not a partial note
+        // and therefore does not require MSM's additive homomorphism property. Additionally, Poseidon2 uses fewer
+        // constraints.
+        poseidon2_hash_with_separator(self.pack_content(), GENERATOR_INDEX__NOTE_HASH)
     }
 }
 

noir-projects/noir-protocol-circuits/crates/types/src/constants.nr

@@ -499,6 +499,7 @@ pub global AVM_PUBLIC_COLUMN_MAX_SIZE: u32 = 1024;
 pub global AVM_PUBLIC_INPUTS_FLATTENED_SIZE: u32 =
     2 * AVM_PUBLIC_COLUMN_MAX_SIZE + PUBLIC_CIRCUIT_PUBLIC_INPUTS_LENGTH;
 
+// Note hash generator index which can be used by custom implementations of NoteInterface::compute_note_hash
 pub global GENERATOR_INDEX__NOTE_HASH: u32 = 1;
 pub global GENERATOR_INDEX__NOTE_HASH_NONCE: u32 = 2;
 pub global GENERATOR_INDEX__UNIQUE_NOTE_HASH: u32 = 3;
@@ -547,12 +548,10 @@ pub global GENERATOR_INDEX__OVSK_M: u32 = 50;
 pub global GENERATOR_INDEX__TSK_M: u32 = 51;
 pub global GENERATOR_INDEX__PUBLIC_KEYS_HASH: u32 = 52;
 pub global GENERATOR_INDEX__NOTE_NULLIFIER: u32 = 53;
-pub global GENERATOR_INDEX__NOTE_HIDING_POINT: u32 = 54;
-pub global GENERATOR_INDEX__SYMMETRIC_KEY: u8 = 55;
-pub global GENERATOR_INDEX__SYMMETRIC_KEY_2: u8 = 56;
-
-pub global GENERATOR_INDEX__PUBLIC_TX_HASH: u32 = 57;
-pub global GENERATOR_INDEX__PRIVATE_TX_HASH: u32 = 58;
+pub global GENERATOR_INDEX__SYMMETRIC_KEY: u8 = 54;
+pub global GENERATOR_INDEX__SYMMETRIC_KEY_2: u8 = 55;
+pub global GENERATOR_INDEX__PUBLIC_TX_HASH: u32 = 56;
+pub global GENERATOR_INDEX__PRIVATE_TX_HASH: u32 = 57;
 
 // AVM memory tags
 pub global MEM_TAG_FF: Field = 0;

yarn-project/circuit-types/src/logs/l1_payload/encrypted_log_payload.test.ts

@@ -84,7 +84,7 @@ describe('EncryptedLogPayload', () => {
     const payload = await log.generatePayload(ephSk, recipientCompleteAddress.address, fixedRand);
 
     expect(payload.toBuffer().toString('hex')).toMatchInlineSnapshot(
-      `"0e9cffc3ddd746affb02410d8f0a823e89939785bcc8e88ee4f3cae05e737c360d460c0e434d846ec1ea286e4090eb56376ff27bddc1aacae1d856549f701fa700010577790aeabcc2d81ec8d0c99e7f5d2bf2f1452025dc777a178404f851d9003de81cde78411f27a921e16ebbfba71a5570d3f62f1134c90daced33663ba000856cb19c7d563da183a40a6f8bd4988d1696ad6bf0c717c8fb8f6294bd0366001ed04e4f77a111c7090fcd34c61cfae744e8589a42defba4d0d927dd4679fe00ec09b49d8d4cf548ea62d44c8839b2fd14664e9d1439b199a8d5166e362348004a69de2d410e010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101"`,
+      `"0e9cffc3ddd746affb02410d8f0a823e89939785bcc8e88ee4f3cae05e737c360d460c0e434d846ec1ea286e4090eb56376ff27bddc1aacae1d856549f701fa7000194e6d7872db8f61e8e59f23580f4db45d13677f873ec473a409cf61fd04d00334e5fb6083721f3eb4eef500876af3c9acfab0a1cb1804b930606fdb0b28300af91db798fa320746831a59b74362dfd0cf9e7c239f6aad11a4b47d0d870ee00d25a054613a83be7be8512f2c09664bc4f7ab60a127b06584f476918581b8a003840d100d8c1d78d4b68b787ed353ebfb8cd2987503d3b472f614f25799a18003f38322629d4010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101010101"`,
     );
 
     // Run with AZTEC_GENERATE_TEST_DATA=1 to update noir test data

yarn-project/circuits.js/src/constants.gen.ts

@@ -398,9 +398,8 @@ export enum GeneratorIndex {
   TSK_M = 51,
   PUBLIC_KEYS_HASH = 52,
   NOTE_NULLIFIER = 53,
-  NOTE_HIDING_POINT = 54,
-  SYMMETRIC_KEY = 55,
-  SYMMETRIC_KEY_2 = 56,
-  PUBLIC_TX_HASH = 57,
-  PRIVATE_TX_HASH = 58,
+  SYMMETRIC_KEY = 54,
+  SYMMETRIC_KEY_2 = 55,
+  PUBLIC_TX_HASH = 56,
+  PRIVATE_TX_HASH = 57,
 }