Skip to content

Commit

Permalink
fix(tariscript): multisig ordered signatures and pubkeys (#5961)
Browse files Browse the repository at this point in the history 
Description
---
Update the multisig op code checks to care about the order of keys and
signatures. This is to assist with reducing the amount of iterations and
processing a node is required to perform to validate a collection.

Motivation and Context
---
Low/Minor audit issue to reduce processing for nodes.

Closes: #5806 

How Has This Been Tested?
---
Test suite / CI

What process can a PR reviewer use to test or verify this change?
---
Look at the test additions. They make it more clear what the effects of
the changes are.

Breaking Changes
---

- [x] None
- [ ] Requires data directory on base node to be deleted
- [ ] Requires hard fork
- [ ] Other - Please specify

---------

Co-authored-by: SW van Heerden <[email protected]>
  • Loading branch information
@brianp @SWvheerden
brianp and SWvheerden authored Nov 17, 2023
1 parent 9b90c3f commit 14e334a
Showing 1 changed file with 48 additions and 11 deletions.
59 changes: 48 additions & 11 deletions infrastructure/tari_script/src/script.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -591,7 +591,8 @@ impl TariScript {
///
/// Notes:
/// * The _m_ signatures are expected to be the top _m_ items on the stack.
/// * Every public key can be used AT MOST once.
/// * The ordering of signatures on the stack MUST match the relative ordering of the corresponding public keys.
/// * The list may contain duplicate keys, but each occurrence of a public key may be used AT MOST once.
/// * Every signature MUST be a valid signature using one of the public keys
/// * _m_ and _n_ must be positive AND m <= n AND n <= MAX_MULTISIG_LIMIT (32).
fn check_multisig(
Expand All @@ -616,18 +617,28 @@ impl TariScript {
})
.collect::<Result<Vec<RistrettoSchnorr>, ScriptError>>()?;

let mut key_signed = vec![false; public_keys.len()];
// keep a hashset of unique signatures used to prevent someone putting the same signature in more than once.
#[allow(clippy::mutable_key_type)]
let mut sig_set = HashSet::new();

let mut agg_pub_key = RistrettoPublicKey::default();
// Check every signature against each public key looking for a valid signature

// Create an iterator that allows each pubkey to only be checked a single time as they are
// removed from the collection when referenced
let mut pub_keys = public_keys.iter();

// Signatures and public keys must be ordered
for s in &signatures {
for (i, pk) in public_keys.iter().enumerate() {
if !sig_set.contains(s) && !key_signed[i] && s.verify_raw_canonical(pk, &message) {
// This prevents Alice creating 2 different sigs against her public key
key_signed[i] = true;
if pub_keys.len() == 0 {
return Ok(None);
}

if sig_set.contains(s) {
continue;
}

for pk in pub_keys.by_ref() {
if s.verify_raw_canonical(pk, &message) {
sig_set.insert(s);
agg_pub_key = agg_pub_key + pk;
break;
Expand Down Expand Up @@ -1182,6 +1193,9 @@ mod test {
let inputs = inputs!(s_alice.clone(), s_bob.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(1));
let inputs = inputs!(s_bob.clone(), s_alice.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(0));
let inputs = inputs!(s_eve.clone(), s_bob.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(0));
Expand Down Expand Up @@ -1220,9 +1234,12 @@ mod test {
let inputs = inputs!(s_alice.clone(), s_carol.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(1));
let inputs = inputs!(s_carol.clone(), s_bob.clone());
let inputs = inputs!(s_bob.clone(), s_carol.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(1));
let inputs = inputs!(s_carol.clone(), s_bob.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(0));
let inputs = inputs!(s_carol.clone(), s_eve.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(0));
Expand All @@ -1238,7 +1255,11 @@ mod test {
let inputs = inputs!(s_alice.clone(), s_alice.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(0));
let inputs = inputs!(s_alice.clone(), s_alice2);
let inputs = inputs!(s_alice.clone(), s_alice2.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(1));
// Interesting case where either sig could match either pubkey
let inputs = inputs!(s_alice2, s_alice.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(1));

Expand All @@ -1250,6 +1271,9 @@ mod test {
let inputs = inputs!(s_alice.clone(), s_bob.clone(), s_carol.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(1));
let inputs = inputs!(s_carol.clone(), s_alice.clone(), s_bob.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(0));
let inputs = inputs!(s_eve.clone(), s_bob.clone(), s_carol);
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(0));
Expand Down Expand Up @@ -1370,6 +1394,9 @@ mod test {
let inputs = inputs!(Number(1), s_alice.clone(), s_bob.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(1));
let inputs = inputs!(Number(1), s_bob.clone(), s_alice.clone());
let err = script.execute(&inputs).unwrap_err();
assert_eq!(err, ScriptError::VerifyFailed);
let inputs = inputs!(Number(1), s_eve.clone(), s_bob.clone());
let err = script.execute(&inputs).unwrap_err();
assert_eq!(err, ScriptError::VerifyFailed);
Expand Down Expand Up @@ -1403,9 +1430,12 @@ mod test {
let inputs = inputs!(Number(1), s_alice.clone(), s_carol.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(1));
let inputs = inputs!(Number(1), s_carol.clone(), s_bob.clone());
let inputs = inputs!(Number(1), s_bob.clone(), s_carol.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(1));
let inputs = inputs!(Number(1), s_carol.clone(), s_bob.clone());
let err = script.execute(&inputs).unwrap_err();
assert_eq!(err, ScriptError::VerifyFailed);
let inputs = inputs!(Number(1), s_carol.clone(), s_eve.clone());
let err = script.execute(&inputs).unwrap_err();
assert_eq!(err, ScriptError::VerifyFailed);
Expand All @@ -1427,7 +1457,11 @@ mod test {
let agg_pub_key = script.execute(&inputs).unwrap();
assert_eq!(agg_pub_key, StackItem::PublicKey(p_bob.clone() + p_carol.clone()));

let inputs = inputs!(s_alice.clone(), s_carol.clone(), s_bob.clone());
let inputs = inputs!(s_carol.clone(), s_bob.clone());
let err = script.execute(&inputs).unwrap_err();
assert_eq!(err, ScriptError::VerifyFailed);

let inputs = inputs!(s_alice.clone(), s_bob.clone(), s_carol.clone());
let err = script.execute(&inputs).unwrap_err();
assert_eq!(err, ScriptError::NonUnitLengthStack);

Expand All @@ -1443,6 +1477,9 @@ mod test {
let inputs = inputs!(Number(1), s_alice.clone(), s_bob.clone(), s_carol.clone());
let result = script.execute(&inputs).unwrap();
assert_eq!(result, Number(1));
let inputs = inputs!(Number(1), s_bob.clone(), s_alice.clone(), s_carol.clone());
let err = script.execute(&inputs).unwrap_err();
assert_eq!(err, ScriptError::VerifyFailed);
let inputs = inputs!(Number(1), s_eve.clone(), s_bob.clone(), s_carol);
let err = script.execute(&inputs).unwrap_err();
assert_eq!(err, ScriptError::VerifyFailed);
Expand Down

0 comments on commit 14e334a

Please sign in to comment.