Skip to content

Commit 2f06fc7

Browse files
suchapalaversuchapalaver
authored
Merge pull request #291 from semiotic-ai/suchapalaver/improve-receipt-deduplication-with-signature-normalization
2 parents 2badfe0 + cc8e00f commit 2f06fc7

File tree

8 files changed

+156
-63
lines changed

8 files changed

+156
-63
lines changed

‎tap_aggregator/src/aggregator/v1.rs‎

Lines changed: 75 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
// Copyright 2023-, Semiotic AI, Inc.
22
// SPDX-License-Identifier: Apache-2.0
33

4-
use std::collections::{hash_set, HashSet};
4+
use std::collections::HashSet;
55

66
use anyhow::{bail, Ok, Result};
77
use rayon::prelude::*;
8-
use tap_core::signed_message::{Eip712SignedMessage, SignatureBytes, SignatureBytesExt};
8+
use tap_core::{receipt::WithUniqueId, signed_message::Eip712SignedMessage};
99
use tap_graph::{Receipt, ReceiptAggregateVoucher};
1010
use thegraph_core::alloy::{
1111
dyn_abi::Eip712Domain, primitives::Address, signers::local::PrivateKeySigner,
@@ -19,7 +19,7 @@ pub fn check_and_aggregate_receipts(
1919
wallet: &PrivateKeySigner,
2020
accepted_addresses: &HashSet<Address>,
2121
) -> Result<Eip712SignedMessage<ReceiptAggregateVoucher>> {
22-
check_signatures_unique(receipts)?;
22+
check_signatures_unique(domain_separator, receipts)?;
2323

2424
// Check that the receipts are signed by an accepted signer address
2525
receipts.par_iter().try_for_each(|receipt| {
@@ -93,14 +93,17 @@ fn check_allocation_id(
9393
Ok(())
9494
}
9595

96-
fn check_signatures_unique(receipts: &[Eip712SignedMessage<Receipt>]) -> Result<()> {
97-
let mut receipt_signatures: hash_set::HashSet<SignatureBytes> = hash_set::HashSet::new();
96+
fn check_signatures_unique(
97+
domain_separator: &Eip712Domain,
98+
receipts: &[Eip712SignedMessage<Receipt>],
99+
) -> Result<()> {
100+
let mut receipt_signatures = HashSet::new();
98101
for receipt in receipts.iter() {
99-
let signature = receipt.signature.get_signature_bytes();
102+
let signature = receipt.unique_id(domain_separator)?;
100103
if !receipt_signatures.insert(signature) {
101104
return Err(tap_core::Error::DuplicateReceiptSignature(format!(
102105
"{:?}",
103-
receipt.signature
106+
receipt.unique_id(domain_separator)?
104107
))
105108
.into());
106109
}
@@ -136,7 +139,9 @@ mod tests {
136139
use tap_core::{signed_message::Eip712SignedMessage, tap_eip712_domain};
137140
use tap_graph::{Receipt, ReceiptAggregateVoucher};
138141
use thegraph_core::alloy::{
139-
dyn_abi::Eip712Domain, primitives::Address, signers::local::PrivateKeySigner,
142+
dyn_abi::Eip712Domain,
143+
primitives::{Address, U256},
144+
signers::{local::PrivateKeySigner, Signature},
140145
};
141146

142147
use super::*;
@@ -163,6 +168,66 @@ mod tests {
163168
tap_eip712_domain(1, Address::from([0x11u8; 20]))
164169
}
165170

171+
#[rstest]
172+
#[test]
173+
fn test_signature_malleability_vulnerability(
174+
keys: (PrivateKeySigner, Address),
175+
allocation_ids: Vec<Address>,
176+
domain_separator: Eip712Domain,
177+
) {
178+
// Create a test receipt
179+
let receipt = Eip712SignedMessage::new(
180+
&domain_separator,
181+
Receipt::new(allocation_ids[0], 42).unwrap(),
182+
&keys.0,
183+
)
184+
.unwrap();
185+
186+
// Get the original signature components
187+
let r = receipt.signature.r();
188+
let s = receipt.signature.s();
189+
let v = receipt.signature.v();
190+
191+
// Create a malleated signature by changing the s value and flipping v
192+
// Get the Secp256k1 curve order
193+
let n = U256::from_str_radix(
194+
"FFFFFFFFFFFFFFFFFFFFFFFFFFFFFFFEBAAEDCE6AF48A03BBFD25E8CD0364141",
195+
16,
196+
)
197+
.unwrap();
198+
let s_malleated = n - s;
199+
let v_malleated = !v; // Flip the parity bit
200+
201+
// Create a new signature with the malleated components
202+
let signature_malleated = Signature::new(r, s_malleated, v_malleated);
203+
204+
// Create a new signed message with the malleated signature but same message
205+
let receipt_malleated = Eip712SignedMessage {
206+
message: receipt.message.clone(),
207+
signature: signature_malleated,
208+
};
209+
210+
// Verify that both signatures recover to the same signer
211+
let original_signer = receipt.recover_signer(&domain_separator).unwrap();
212+
let malleated_signer = receipt_malleated.recover_signer(&domain_separator).unwrap();
213+
214+
assert_eq!(
215+
original_signer, malleated_signer,
216+
"Both signatures should recover to the same signer"
217+
);
218+
219+
// Try to check if signatures are unique using the current implementation
220+
let receipts = vec![receipt, receipt_malleated];
221+
222+
// This should return an error because the signatures are different
223+
// but the messages are the same, which if allowed would present a security vulnerability
224+
let result = check_signatures_unique(&domain_separator, &receipts);
225+
226+
// The result should be an error because the malleated signature is not treated as unique
227+
// and is detected as a duplicate
228+
assert!(result.is_err());
229+
}
230+
166231
#[rstest]
167232
#[test]
168233
fn check_signatures_unique_fail(
@@ -181,7 +246,7 @@ mod tests {
181246
receipts.push(receipt.clone());
182247
receipts.push(receipt);
183248

184-
let res = check_signatures_unique(&receipts);
249+
let res = check_signatures_unique(&domain_separator, &receipts);
185250
assert!(res.is_err());
186251
}
187252

@@ -208,7 +273,7 @@ mod tests {
208273
.unwrap(),
209274
];
210275

211-
let res = check_signatures_unique(&receipts);
276+
let res = check_signatures_unique(&domain_separator, &receipts);
212277
assert!(res.is_ok());
213278
}
214279

‎tap_aggregator/src/aggregator/v2.rs‎

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
// Copyright 2023-, Semiotic AI, Inc.
22
// SPDX-License-Identifier: Apache-2.0
33

4-
use std::collections::{hash_set, HashSet};
4+
use std::collections::HashSet;
55

66
use anyhow::{bail, Ok, Result};
77
use rayon::prelude::*;
8-
use tap_core::signed_message::{Eip712SignedMessage, SignatureBytes, SignatureBytesExt};
8+
use tap_core::{receipt::WithUniqueId, signed_message::Eip712SignedMessage};
99
use tap_graph::v2::{Receipt, ReceiptAggregateVoucher};
1010
use thegraph_core::alloy::{
1111
dyn_abi::Eip712Domain, primitives::Address, signers::local::PrivateKeySigner,
@@ -19,7 +19,7 @@ pub fn check_and_aggregate_receipts(
1919
wallet: &PrivateKeySigner,
2020
accepted_addresses: &HashSet<Address>,
2121
) -> Result<Eip712SignedMessage<ReceiptAggregateVoucher>> {
22-
check_signatures_unique(receipts)?;
22+
check_signatures_unique(domain_separator, receipts)?;
2323

2424
// Check that the receipts are signed by an accepted signer address
2525
receipts.par_iter().try_for_each(|receipt| {
@@ -148,14 +148,17 @@ fn check_allocation_id(
148148
Ok(())
149149
}
150150

151-
fn check_signatures_unique(receipts: &[Eip712SignedMessage<Receipt>]) -> Result<()> {
152-
let mut receipt_signatures: hash_set::HashSet<SignatureBytes> = hash_set::HashSet::new();
151+
fn check_signatures_unique(
152+
domain_separator: &Eip712Domain,
153+
receipts: &[Eip712SignedMessage<Receipt>],
154+
) -> Result<()> {
155+
let mut receipt_signatures = HashSet::new();
153156
for receipt in receipts.iter() {
154-
let signature = receipt.signature.get_signature_bytes();
157+
let signature = receipt.unique_id(domain_separator)?;
155158
if !receipt_signatures.insert(signature) {
156159
return Err(tap_core::Error::DuplicateReceiptSignature(format!(
157160
"{:?}",
158-
receipt.signature
161+
receipt.unique_id(domain_separator)?
159162
))
160163
.into());
161164
}
@@ -251,7 +254,7 @@ mod tests {
251254
receipts.push(receipt.clone());
252255
receipts.push(receipt);
253256

254-
let res = super::check_signatures_unique(&receipts);
257+
let res = super::check_signatures_unique(&domain_separator, &receipts);
255258
assert!(res.is_err());
256259
}
257260

@@ -281,13 +284,13 @@ mod tests {
281284
.unwrap(),
282285
];
283286

284-
let res = super::check_signatures_unique(&receipts);
287+
let res = super::check_signatures_unique(&domain_separator, &receipts);
285288
assert!(res.is_ok());
286289
}
287290

288291
#[rstest]
289292
#[test]
290-
/// Test that a receipt with a timestamp greater then the rav timestamp passes
293+
/// Test that a receipt with a timestamp greater than the rav timestamp passes
291294
fn check_receipt_timestamps(
292295
keys: (PrivateKeySigner, Address),
293296
allocation_id: Address,

‎tap_core/src/manager/tap_manager.rs‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -138,12 +138,13 @@ where
138138
let mut failed_receipts = vec![];
139139

140140
// check for timestamp
141-
let (checking_receipts, already_failed) =
142-
TimestampCheck(min_timestamp_ns).check_batch(checking_receipts);
141+
let (checking_receipts, already_failed) = TimestampCheck(min_timestamp_ns)
142+
.check_batch(&self.domain_separator, checking_receipts)?;
143143
failed_receipts.extend(already_failed);
144144

145145
// check for uniqueness
146-
let (checking_receipts, already_failed) = UniqueCheck.check_batch(checking_receipts);
146+
let (checking_receipts, already_failed) =
147+
UniqueCheck.check_batch(&self.domain_separator, checking_receipts)?;
147148
failed_receipts.extend(already_failed);
148149

149150
for receipt in checking_receipts.into_iter() {

‎tap_core/tests/manager_test.rs‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -139,7 +139,7 @@ async fn manager_verify_and_store_varying_initial_checks(
139139
&signer,
140140
)
141141
.unwrap();
142-
let query_id = signed_receipt.unique_hash();
142+
let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
143143
query_appraisals.write().unwrap().insert(query_id, value);
144144
escrow_storage
145145
.write()
@@ -182,7 +182,7 @@ async fn manager_create_rav_request_all_valid_receipts(
182182
&signer,
183183
)
184184
.unwrap();
185-
let query_id = signed_receipt.unique_hash();
185+
let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
186186
stored_signed_receipts.push(signed_receipt.clone());
187187
query_appraisals.write().unwrap().insert(query_id, value);
188188
assert!(manager
@@ -277,7 +277,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts(
277277
&signer,
278278
)
279279
.unwrap();
280-
let query_id = signed_receipt.unique_hash();
280+
let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
281281
stored_signed_receipts.push(signed_receipt.clone());
282282
query_appraisals.write().unwrap().insert(query_id, value);
283283
assert!(manager
@@ -321,7 +321,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts(
321321
)
322322
.unwrap();
323323

324-
let query_id = signed_receipt.unique_hash();
324+
let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
325325
stored_signed_receipts.push(signed_receipt.clone());
326326
query_appraisals.write().unwrap().insert(query_id, value);
327327
assert!(manager
@@ -389,7 +389,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts_consecutive_tim
389389
receipt.timestamp_ns = starting_min_timestamp + query_id + 1;
390390
let signed_receipt = Eip712SignedMessage::new(&domain_separator, receipt, &signer).unwrap();
391391

392-
let query_id = signed_receipt.unique_hash();
392+
let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
393393
stored_signed_receipts.push(signed_receipt.clone());
394394
query_appraisals.write().unwrap().insert(query_id, value);
395395
assert!(manager
@@ -436,7 +436,7 @@ async fn manager_create_multiple_rav_requests_all_valid_receipts_consecutive_tim
436436
let mut receipt = Receipt::new(allocation_ids[0], value).unwrap();
437437
receipt.timestamp_ns = starting_min_timestamp + query_id + 1;
438438
let signed_receipt = Eip712SignedMessage::new(&domain_separator, receipt, &signer).unwrap();
439-
let query_id = signed_receipt.unique_hash();
439+
let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
440440
stored_signed_receipts.push(signed_receipt.clone());
441441
query_appraisals.write().unwrap().insert(query_id, value);
442442
assert!(manager

‎tap_core/tests/received_receipt_test.rs‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ async fn partial_then_full_check_valid_receipt(
113113
)
114114
.unwrap();
115115

116-
let query_id = signed_receipt.unique_hash();
116+
let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
117117

118118
// add escrow for sender
119119
escrow_storage
@@ -156,7 +156,7 @@ async fn partial_then_finalize_valid_receipt(
156156
&signer,
157157
)
158158
.unwrap();
159-
let query_id = signed_receipt.unique_hash();
159+
let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
160160

161161
// add escrow for sender
162162
escrow_storage
@@ -203,7 +203,7 @@ async fn standard_lifetime_valid_receipt(
203203
)
204204
.unwrap();
205205

206-
let query_id = signed_receipt.unique_hash();
206+
let query_id = signed_receipt.unique_hash(&domain_separator).unwrap();
207207

208208
// add escrow for sender
209209
escrow_storage

‎tap_eip712_message/src/lib.rs‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@
2525
use serde::{Deserialize, Serialize};
2626
use thegraph_core::alloy::{
2727
dyn_abi::Eip712Domain,
28-
primitives::{Address, Signature},
28+
primitives::{keccak256, Address, Signature},
2929
signers::{local::PrivateKeySigner, SignerSync},
3030
sol_types::SolStruct,
3131
};
@@ -105,8 +105,21 @@ impl<M: SolStruct> Eip712SignedMessage<M> {
105105
Ok(recovered_address)
106106
}
107107

108-
/// Use this as a simple key for testing
109-
pub fn unique_hash(&self) -> MessageId {
110-
MessageId(self.message.eip712_hash_struct().into())
108+
/// Generate a uniqueness identifier using both the message content and the recovered signer
109+
pub fn unique_hash(&self, domain_separator: &Eip712Domain) -> Result<MessageId, Eip712Error> {
110+
// Get the hash of just the message content
111+
let message_hash = self.message.eip712_hash_struct();
112+
113+
// Recover the signer address
114+
let signer = self.recover_signer(domain_separator)?;
115+
116+
// Create a new hash combining both the message hash and the signer address
117+
let mut input = Vec::with_capacity(32 + 20); // 32 bytes for hash, 20 bytes for address
118+
input.extend_from_slice(&message_hash.0);
119+
input.extend_from_slice(signer.as_ref());
120+
121+
let combined_hash = keccak256(&input);
122+
123+
Ok(MessageId(*combined_hash))
111124
}
112125
}

0 commit comments

Comments
 (0)