fix: improve receipt deduplication with signature normalization

tap_aggregator/src/aggregator/v1.rs

@@ -1,11 +1,11 @@
 // Copyright 2023-, Semiotic AI, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
-use std::collections::{hash_set, HashSet};
+use std::collections::HashSet;
 
 use anyhow::{bail, Ok, Result};
 use rayon::prelude::*;
-use tap_core::signed_message::{Eip712SignedMessage, SignatureBytes, SignatureBytesExt};
+use tap_core::{receipt::WithUniqueId, signed_message::Eip712SignedMessage};
 use tap_graph::{Receipt, ReceiptAggregateVoucher};
 use thegraph_core::alloy::{
     dyn_abi::Eip712Domain, primitives::Address, signers::local::PrivateKeySigner,
@@ -19,7 +19,7 @@ pub fn check_and_aggregate_receipts(
     wallet: &PrivateKeySigner,
     accepted_addresses: &HashSet<Address>,
 ) -> Result<Eip712SignedMessage<ReceiptAggregateVoucher>> {
-    check_signatures_unique(receipts)?;
+    check_signatures_unique(domain_separator, receipts)?;
 
     // Check that the receipts are signed by an accepted signer address
     receipts.par_iter().try_for_each(|receipt| {
@@ -93,14 +93,17 @@ fn check_allocation_id(
     Ok(())
 }
 
-fn check_signatures_unique(receipts: &[Eip712SignedMessage<Receipt>]) -> Result<()> {
-    let mut receipt_signatures: hash_set::HashSet<SignatureBytes> = hash_set::HashSet::new();
+fn check_signatures_unique(
+    domain_separator: &Eip712Domain,
+    receipts: &[Eip712SignedMessage<Receipt>],
+) -> Result<()> {
+    let mut receipt_signatures = HashSet::new();
     for receipt in receipts.iter() {
-        let signature = receipt.signature.get_signature_bytes();
+        let signature = receipt.unique_id(domain_separator)?;
         if !receipt_signatures.insert(signature) {
             return Err(tap_core::Error::DuplicateReceiptSignature(format!(
                 "{:?}",
-                receipt.signature
+                receipt.unique_id(domain_separator)?
             ))
             .into());
         }
@@ -136,7 +139,9 @@ mod tests {
     use tap_core::{signed_message::Eip712SignedMessage, tap_eip712_domain};
     use tap_graph::{Receipt, ReceiptAggregateVoucher};
     use thegraph_core::alloy::{
-        dyn_abi::Eip712Domain, primitives::Address, signers::local::PrivateKeySigner,
+        dyn_abi::Eip712Domain,
+        primitives::{Address, U256},
+        signers::{local::PrivateKeySigner, Signature},
     };
 
     use super::*;
@@ -163,6 +168,66 @@ mod tests {
         tap_eip712_domain(1, Address::from([0x11u8; 20]))
     }
 
+    #[rstest]
+    #[test]
+    fn test_signature_malleability_vulnerability(
+        keys: (PrivateKeySigner, Address),
+        allocation_ids: Vec<Address>,
+        domain_separator: Eip712Domain,
+    ) {
+        // Create a test receipt
+        let receipt = Eip712SignedMessage::new(
+            &domain_separator,
+            Receipt::new(allocation_ids[0], 42).unwrap(),
+            &keys.0,
+        )
+        .unwrap();
+
+        // Get the original signature components
+        let r = receipt.signature.r();
+        let s = receipt.signature.s();
+        let v = receipt.signature.v();
+
+        // Create a malleated signature by changing the s value and flipping v
+        // Get the Secp256k1 curve order
+        let n = U256::from_str_radix(
+            "FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141",
+            16,
+        )
+        .unwrap();
+        let s_malleated = n - s;
+        let v_malleated = !v; // Flip the parity bit
+
+        // Create a new signature with the malleated components
+        let signature_malleated = Signature::new(r, s_malleated, v_malleated);
+
+        // Create a new signed message with the malleated signature but same message
+        let receipt_malleated = Eip712SignedMessage {
+            message: receipt.message.clone(),
+            signature: signature_malleated,
+        };
+
+        // Verify that both signatures recover to the same signer
+        let original_signer = receipt.recover_signer(&domain_separator).unwrap();
+        let malleated_signer = receipt_malleated.recover_signer(&domain_separator).unwrap();
+
+        assert_eq!(
+            original_signer, malleated_signer,
+            "Both signatures should recover to the same signer"
+        );
+
+        // Try to check if signatures are unique using the current implementation
+        let receipts = vec![receipt, receipt_malleated];
+
+        // This should return an error because the signatures are different
+        // but the messages are the same, which if allowed would present a security vulnerability
+        let result = check_signatures_unique(&domain_separator, &receipts);
+
+        // The result should be an error because the malleated signature is not treated as unique
+        // and is detected as a duplicate
+        assert!(result.is_err());
+    }
+
     #[rstest]
     #[test]
     fn check_signatures_unique_fail(
@@ -181,7 +246,7 @@ mod tests {
         receipts.push(receipt.clone());
         receipts.push(receipt);
 
-        let res = check_signatures_unique(&receipts);
+        let res = check_signatures_unique(&domain_separator, &receipts);
         assert!(res.is_err());
     }
 
@@ -208,7 +273,7 @@ mod tests {
             .unwrap(),
         ];
 
-        let res = check_signatures_unique(&receipts);
+        let res = check_signatures_unique(&domain_separator, &receipts);
         assert!(res.is_ok());
     }
 

tap_aggregator/src/aggregator/v2.rs

@@ -1,11 +1,11 @@
 // Copyright 2023-, Semiotic AI, Inc.
 // SPDX-License-Identifier: Apache-2.0
 
-use std::collections::{hash_set, HashSet};
+use std::collections::HashSet;
 
 use anyhow::{bail, Ok, Result};
 use rayon::prelude::*;
-use tap_core::signed_message::{Eip712SignedMessage, SignatureBytes, SignatureBytesExt};
+use tap_core::{receipt::WithUniqueId, signed_message::Eip712SignedMessage};
 use tap_graph::v2::{Receipt, ReceiptAggregateVoucher};
 use thegraph_core::alloy::{
     dyn_abi::Eip712Domain, primitives::Address, signers::local::PrivateKeySigner,
@@ -19,7 +19,7 @@ pub fn check_and_aggregate_receipts(
     wallet: &PrivateKeySigner,
     accepted_addresses: &HashSet<Address>,
 ) -> Result<Eip712SignedMessage<ReceiptAggregateVoucher>> {
-    check_signatures_unique(receipts)?;
+    check_signatures_unique(domain_separator, receipts)?;
 
     // Check that the receipts are signed by an accepted signer address
     receipts.par_iter().try_for_each(|receipt| {
@@ -148,14 +148,17 @@ fn check_allocation_id(
     Ok(())
 }
 
-fn check_signatures_unique(receipts: &[Eip712SignedMessage<Receipt>]) -> Result<()> {
-    let mut receipt_signatures: hash_set::HashSet<SignatureBytes> = hash_set::HashSet::new();
+fn check_signatures_unique(
+    domain_separator: &Eip712Domain,
+    receipts: &[Eip712SignedMessage<Receipt>],
+) -> Result<()> {
+    let mut receipt_signatures = HashSet::new();
     for receipt in receipts.iter() {
-        let signature = receipt.signature.get_signature_bytes();
+        let signature = receipt.unique_id(domain_separator)?;
         if !receipt_signatures.insert(signature) {
             return Err(tap_core::Error::DuplicateReceiptSignature(format!(
                 "{:?}",
-                receipt.signature
+                receipt.unique_id(domain_separator)?
             ))
             .into());
         }
@@ -251,7 +254,7 @@ mod tests {
         receipts.push(receipt.clone());
         receipts.push(receipt);
 
-        let res = super::check_signatures_unique(&receipts);
+        let res = super::check_signatures_unique(&domain_separator, &receipts);
         assert!(res.is_err());
     }
 
@@ -281,13 +284,13 @@ mod tests {
             .unwrap(),
         ];
 
-        let res = super::check_signatures_unique(&receipts);
+        let res = super::check_signatures_unique(&domain_separator, &receipts);
         assert!(res.is_ok());
     }
 
     #[rstest]
     #[test]
-    /// Test that a receipt with a timestamp greater then the rav timestamp passes
+    /// Test that a receipt with a timestamp greater than the rav timestamp passes
     fn check_receipt_timestamps(
         keys: (PrivateKeySigner, Address),
         allocation_id: Address,

tap_core/src/manager/tap_manager.rs

@@ -138,12 +138,13 @@ where
         let mut failed_receipts = vec![];
 
         // check for timestamp
-        let (checking_receipts, already_failed) =
-            TimestampCheck(min_timestamp_ns).check_batch(checking_receipts);
+        let (checking_receipts, already_failed) = TimestampCheck(min_timestamp_ns)
+            .check_batch(&self.domain_separator, checking_receipts)?;
         failed_receipts.extend(already_failed);
 
         // check for uniqueness
-        let (checking_receipts, already_failed) = UniqueCheck.check_batch(checking_receipts);
+        let (checking_receipts, already_failed) =
+            UniqueCheck.check_batch(&self.domain_separator, checking_receipts)?;
         failed_receipts.extend(already_failed);
 
         for receipt in checking_receipts.into_iter() {

tap_core/tests/manager_test.rs

@@ -139,7 +139,7 @@ async fn manager_verify_and_store_varying_initial_checks(
         &signer,
     )
     .unwrap();
-    let query_id = signed_receipt.unique_hash();
+    let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
     query_appraisals.write().unwrap().insert(query_id, value);
     escrow_storage
         .write()
@@ -182,7 +182,7 @@ async fn manager_create_rav_request_all_valid_receipts(
             &signer,
         )
         .unwrap();
-        let query_id = signed_receipt.unique_hash();
+        let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
         stored_signed_receipts.push(signed_receipt.clone());
         query_appraisals.write().unwrap().insert(query_id, value);
         assert!(manager
@@ -277,7 +277,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts(
             &signer,
         )
         .unwrap();
-        let query_id = signed_receipt.unique_hash();
+        let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
         stored_signed_receipts.push(signed_receipt.clone());
         query_appraisals.write().unwrap().insert(query_id, value);
         assert!(manager
@@ -321,7 +321,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts(
         )
         .unwrap();
 
-        let query_id = signed_receipt.unique_hash();
+        let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
         stored_signed_receipts.push(signed_receipt.clone());
         query_appraisals.write().unwrap().insert(query_id, value);
         assert!(manager
@@ -389,7 +389,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts_consecutive_tim
         receipt.timestamp_ns = starting_min_timestamp + query_id + 1;
         let signed_receipt = Eip712SignedMessage::new(&domain_separator, receipt, &signer).unwrap();
 
-        let query_id = signed_receipt.unique_hash();
+        let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
         stored_signed_receipts.push(signed_receipt.clone());
         query_appraisals.write().unwrap().insert(query_id, value);
         assert!(manager
@@ -436,7 +436,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts_consecutive_tim
         let mut receipt = Receipt::new(allocation_ids[0], value).unwrap();
         receipt.timestamp_ns = starting_min_timestamp + query_id + 1;
         let signed_receipt = Eip712SignedMessage::new(&domain_separator, receipt, &signer).unwrap();
-        let query_id = signed_receipt.unique_hash();
+        let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
         stored_signed_receipts.push(signed_receipt.clone());
         query_appraisals.write().unwrap().insert(query_id, value);
         assert!(manager

tap_core/tests/received_receipt_test.rs

@@ -113,7 +113,7 @@ async fn partial_then_full_check_valid_receipt(
     )
     .unwrap();
 
-    let query_id = signed_receipt.unique_hash();
+    let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
 
     // add escrow for sender
     escrow_storage
@@ -156,7 +156,7 @@ async fn partial_then_finalize_valid_receipt(
         &signer,
     )
     .unwrap();
-    let query_id = signed_receipt.unique_hash();
+    let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
 
     // add escrow for sender
     escrow_storage
@@ -203,7 +203,7 @@ async fn standard_lifetime_valid_receipt(
     )
     .unwrap();
 
-    let query_id = signed_receipt.unique_hash();
+    let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
 
     // add escrow for sender
     escrow_storage

tap_eip712_message/src/lib.rs

@@ -25,7 +25,7 @@
 use serde::{Deserialize, Serialize};
 use thegraph_core::alloy::{
     dyn_abi::Eip712Domain,
-    primitives::{Address, Signature},
+    primitives::{keccak256, Address, Signature},
     signers::{local::PrivateKeySigner, SignerSync},
     sol_types::SolStruct,
 };
@@ -105,8 +105,21 @@ impl<M: SolStruct> Eip712SignedMessage<M> {
         Ok(recovered_address)
     }
 
-    /// Use this as a simple key for testing
-    pub fn unique_hash(&self) -> MessageId {
-        MessageId(self.message.eip712_hash_struct().into())
+    /// Generate a uniqueness identifier using both the message content and the recovered signer
+    pub fn unique_hash(&self, domain_separator: &Eip712Domain) -> Result<MessageId, Eip712Error> {
+        // Get the hash of just the message content
+        let message_hash = self.message.eip712_hash_struct();
+
+        // Recover the signer address
+        let signer = self.recover_signer(domain_separator)?;
+
+        // Create a new hash combining both the message hash and the signer address
+        let mut input = Vec::with_capacity(32 + 20); // 32 bytes for hash, 20 bytes for address
+        input.extend_from_slice(&message_hash.0);
+        input.extend_from_slice(signer.as_ref());
+
+        let combined_hash = keccak256(&input);
+
+        Ok(MessageId(*combined_hash))
     }
 }