diff --git a/tap_aggregator/src/aggregator/v1.rs b/tap_aggregator/src/aggregator/v1.rs index d443d973..6234a54f 100644 --- a/tap_aggregator/src/aggregator/v1.rs +++ b/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
, ) -> Result> { - 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]) -> Result<()> { - let mut receipt_signatures: hash_set::HashSet = hash_set::HashSet::new(); +fn check_signatures_unique( + domain_separator: &Eip712Domain, + receipts: &[Eip712SignedMessage], +) -> 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
, + 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()); } diff --git a/tap_aggregator/src/aggregator/v2.rs b/tap_aggregator/src/aggregator/v2.rs index 97fffbc5..0abb5f31 100644 --- a/tap_aggregator/src/aggregator/v2.rs +++ b/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
, ) -> Result> { - 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]) -> Result<()> { - let mut receipt_signatures: hash_set::HashSet = hash_set::HashSet::new(); +fn check_signatures_unique( + domain_separator: &Eip712Domain, + receipts: &[Eip712SignedMessage], +) -> 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, diff --git a/tap_core/src/manager/tap_manager.rs b/tap_core/src/manager/tap_manager.rs index f6605a3f..6320de2a 100644 --- a/tap_core/src/manager/tap_manager.rs +++ b/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() { diff --git a/tap_core/tests/manager_test.rs b/tap_core/tests/manager_test.rs index e44badbd..cae341cc 100644 --- a/tap_core/tests/manager_test.rs +++ b/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 diff --git a/tap_core/tests/received_receipt_test.rs b/tap_core/tests/received_receipt_test.rs index d7972fbc..8403a9c1 100644 --- a/tap_core/tests/received_receipt_test.rs +++ b/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 diff --git a/tap_eip712_message/src/lib.rs b/tap_eip712_message/src/lib.rs index e0899081..a347c382 100644 --- a/tap_eip712_message/src/lib.rs +++ b/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 Eip712SignedMessage { 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 { + // 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)) } } diff --git a/tap_receipt/src/checks.rs b/tap_receipt/src/checks.rs index cb5d2996..44acbd9e 100644 --- a/tap_receipt/src/checks.rs +++ b/tap_receipt/src/checks.rs @@ -36,6 +36,9 @@ use std::{ sync::{Arc, RwLock}, }; +use tap_eip712_message::Eip712Error; +use thegraph_core::alloy::dyn_abi::Eip712Domain; + use super::{ state::{Checking, Failed}, Context, ReceiptError, ReceiptWithState, WithUniqueId, WithValueAndTimestamp, @@ -94,8 +97,9 @@ type CheckBatchResponse = ( pub trait CheckBatch { fn check_batch( &self, + domain_separator: &Eip712Domain, receipts: Vec>, - ) -> CheckBatchResponse; + ) -> Result, Eip712Error>; } /// Provides a built-in check to verify that the timestamp of a receipt @@ -153,11 +157,9 @@ where { fn check_batch( &self, + _domain_separator: &Eip712Domain, receipts: Vec>, - ) -> ( - Vec>, - Vec>, - ) { + ) -> Result, Eip712Error> { let (mut checking, mut failed) = (vec![], vec![]); for receipt in receipts.into_iter() { let receipt_timestamp_ns = receipt.signed_receipt().timestamp_ns(); @@ -171,7 +173,7 @@ where })); } } - (checking, failed) + Ok((checking, failed)) } } @@ -187,23 +189,21 @@ where { fn check_batch( &self, + domain_separator: &Eip712Domain, receipts: Vec>, - ) -> ( - Vec>, - Vec>, - ) { + ) -> Result, Eip712Error> { let mut signatures = HashSet::new(); let (mut checking, mut failed) = (vec![], vec![]); for received_receipt in receipts.into_iter() { - let unique_id = received_receipt.receipt.unique_id(); + let unique_id = received_receipt.receipt.unique_id(domain_separator)?; if signatures.insert(unique_id) { checking.push(received_receipt); } else { failed.push(received_receipt.perform_state_error(ReceiptError::NonUniqueReceipt)); } } - (checking, failed) + Ok((checking, failed)) } } @@ -236,16 +236,20 @@ mod tests { } } - fn create_signed_receipt_with_custom_value( - value: u128, - ) -> ReceiptWithState> { - let wallet: PrivateKeySigner = PrivateKeySigner::random(); - let eip712_domain_separator: Eip712Domain = eip712_domain! { + fn create_test_domain_separator() -> Eip712Domain { + eip712_domain! { name: "TAP", version: "1", chain_id: 1, verifying_contract: Address:: from([0x11u8; 20]), - }; + } + } + + fn create_signed_receipt_with_custom_value( + value: u128, + ) -> ReceiptWithState> { + let wallet: PrivateKeySigner = PrivateKeySigner::random(); + let eip712_domain_separator = create_test_domain_separator(); let timestamp = SystemTime::now() .duration_since(SystemTime::UNIX_EPOCH) @@ -273,7 +277,10 @@ mod tests { let signed_receipt_2 = create_signed_receipt_with_custom_value(15); let signed_receipt_copy = signed_receipt.clone(); let receipts_batch = vec![signed_receipt, signed_receipt_2, signed_receipt_copy]; - let (valid_receipts, invalid_receipts) = UniqueCheck.check_batch(receipts_batch); + let domain_separator = create_test_domain_separator(); + let (valid_receipts, invalid_receipts) = UniqueCheck + .check_batch(&domain_separator, receipts_batch) + .unwrap(); assert_eq!(valid_receipts.len(), 2); assert_eq!(invalid_receipts.len(), 1); } @@ -282,10 +289,14 @@ mod tests { async fn test_receipt_timestamp_check() { let signed_receipt = create_signed_receipt_with_custom_value(10); let signed_receipt_2 = create_signed_receipt_with_custom_value(15); + + let domain_separator = create_test_domain_separator(); + let receipts_batch = vec![signed_receipt.clone(), signed_receipt_2]; let min_time_stamp = signed_receipt.receipt.message.timestamp_ns + 1; - let (valid_receipts, invalid_receipts) = - TimestampCheck(min_time_stamp).check_batch(receipts_batch); + let (valid_receipts, invalid_receipts) = TimestampCheck(min_time_stamp) + .check_batch(&domain_separator, receipts_batch) + .unwrap(); assert_eq!(valid_receipts.len(), 1); assert_eq!(invalid_receipts.len(), 1); } diff --git a/tap_receipt/src/lib.rs b/tap_receipt/src/lib.rs index 4ba2e432..1188a2ef 100644 --- a/tap_receipt/src/lib.rs +++ b/tap_receipt/src/lib.rs @@ -27,8 +27,8 @@ pub mod state; pub use error::ReceiptError; pub use received_receipt::ReceiptWithState; -use tap_eip712_message::{Eip712SignedMessage, SignatureBytes, SignatureBytesExt}; -use thegraph_core::alloy::sol_types::SolStruct; +use tap_eip712_message::{Eip712Error, Eip712SignedMessage, MessageId}; +use thegraph_core::alloy::{dyn_abi::Eip712Domain, sol_types::SolStruct}; /// Result type for receipt pub type ReceiptResult = Result; @@ -45,7 +45,7 @@ pub trait WithValueAndTimestamp { /// Extension that allows UniqueCheck for any SolStruct receipt pub trait WithUniqueId { type Output: Eq + std::hash::Hash; - fn unique_id(&self) -> Self::Output; + fn unique_id(&self, domain_separator: &Eip712Domain) -> Result; } impl WithValueAndTimestamp for Eip712SignedMessage @@ -65,9 +65,9 @@ impl WithUniqueId for Eip712SignedMessage where T: SolStruct, { - type Output = SignatureBytes; + type Output = MessageId; - fn unique_id(&self) -> Self::Output { - self.signature.get_signature_bytes() + fn unique_id(&self, domain_separator: &Eip712Domain) -> Result { + self.unique_hash(domain_separator) } }