Skip to content

Commit

Permalink
refactor: simplify complexity in verify() signature implementation (#287
Browse files Browse the repository at this point in the history 
)

* update signature verification to throw error when false

* add unit test

* inverted checks

* update unit test assert
  • Loading branch information
@eguajardo
eguajardo authored Mar 4, 2024
1 parent aad015b commit e120fa8
Show file tree
Hide file tree
Showing 3 changed files with 99 additions and 38 deletions.
6 changes: 3 additions & 3 deletions contracts/multisig/src/contract/execute.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ pub fn register_pub_key(

// to prevent anyone from registering a public key that belongs to someone else,
// we require the sender to sign their own address using the private key
if !signed_sender_address.verify(address_hash.as_slice(), &public_key)? {
return Err(ContractError::InvalidPublicKeyRegistrationSignature);
}
signed_sender_address
.verify(address_hash.as_slice(), &public_key)
.map_err(|_| ContractError::InvalidPublicKeyRegistrationSignature)?;

save_pub_key(deps.storage, info.sender.clone(), public_key.clone())?;

Expand Down
81 changes: 62 additions & 19 deletions contracts/multisig/src/key.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,18 +174,22 @@ impl KeyTyped for Signature {
}

impl Signature {
pub fn verify<T: AsRef<[u8]>>(
&self,
msg: T,
pub_key: &PublicKey,
) -> Result<bool, ContractError> {
pub fn verify<T: AsRef<[u8]>>(&self, msg: T, pub_key: &PublicKey) -> Result<(), ContractError> {
if !self.matches_type(pub_key) {
return Err(ContractError::KeyTypeMismatch);
}

match self.key_type() {
let res = match self.key_type() {
KeyType::Ecdsa => ecdsa_verify(msg.as_ref(), self, pub_key.as_ref()),
KeyType::Ed25519 => ed25519_verify(msg.as_ref(), self.as_ref(), pub_key.as_ref()),
}?;

if res {
Ok(())
} else {
Err(ContractError::SignatureVerificationFailed {
reason: "unable to verify signature".into(),
})
}
}
}
Expand Down Expand Up @@ -391,12 +395,12 @@ mod ecdsa_tests {
.unwrap();
let message = MsgToSign::try_from(ecdsa_test_data::message()).unwrap();
let public_key = PublicKey::try_from((KeyType::Ecdsa, ecdsa_test_data::pub_key())).unwrap();
let result = signature.verify(&message, &public_key).unwrap();
assert_eq!(result, true);
let result = signature.verify(&message, &public_key);
assert!(result.is_ok(), "{:?}", result)
}

#[test]
fn test_verify_signature_invalid_signature() {
fn should_fail_sig_verification_when_using_different_valid_sig() {
let invalid_signature = HexBinary::from_hex(
"a112231719403227b297139cc6beef82a4e034663bfe48cf732687860b16227a51e4bd6be96fceeecf8e77fe7cdd4f5567d71aed5388484d1f2ba355298c954e",
)
Expand All @@ -405,8 +409,32 @@ mod ecdsa_tests {
let signature: Signature = (KeyType::Ecdsa, invalid_signature).try_into().unwrap();
let message = MsgToSign::try_from(ecdsa_test_data::message()).unwrap();
let public_key = PublicKey::try_from((KeyType::Ecdsa, ecdsa_test_data::pub_key())).unwrap();
let result = signature.verify(&message, &public_key).unwrap();
assert_eq!(result, false);
let result = signature.verify(&message, &public_key);
assert_eq!(
result.unwrap_err(),
ContractError::SignatureVerificationFailed {
reason: "unable to verify signature".into(),
}
);
}

#[test]
fn should_fail_sig_verification_when_invalid_sig() {
let invalid_signature = HexBinary::from_hex(
"00000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000",
)
.unwrap();

let signature: Signature = (KeyType::Ecdsa, invalid_signature).try_into().unwrap();
let message = MsgToSign::try_from(ecdsa_test_data::message()).unwrap();
let public_key = PublicKey::try_from((KeyType::Ecdsa, ecdsa_test_data::pub_key())).unwrap();
let result = signature.verify(&message, &public_key);
assert_eq!(
result.unwrap_err(),
ContractError::SignatureVerificationFailed {
reason: "Crypto error: signature error".into(),
}
);
}

#[test]
Expand All @@ -421,8 +449,13 @@ mod ecdsa_tests {
.unwrap();
let message = MsgToSign::try_from(ecdsa_test_data::message()).unwrap();
let public_key = PublicKey::try_from((KeyType::Ecdsa, invalid_pub_key)).unwrap();
let result = signature.verify(&message, &public_key).unwrap();
assert_eq!(result, false);
let result = signature.verify(&message, &public_key);
assert_eq!(
result.unwrap_err(),
ContractError::SignatureVerificationFailed {
reason: "unable to verify signature".into(),
}
);
}
}

Expand Down Expand Up @@ -502,8 +535,8 @@ mod ed25519_tests {
let message = MsgToSign::try_from(ed25519_test_data::message()).unwrap();
let public_key =
PublicKey::try_from((KeyType::Ed25519, ed25519_test_data::pub_key())).unwrap();
let result = signature.verify(&message, &public_key).unwrap();
assert_eq!(result, true);
let result = signature.verify(&message, &public_key);
assert!(result.is_ok(), "{:?}", result)
}

#[test]
Expand All @@ -517,8 +550,13 @@ mod ed25519_tests {
let message = MsgToSign::try_from(ed25519_test_data::message()).unwrap();
let public_key =
PublicKey::try_from((KeyType::Ed25519, ed25519_test_data::pub_key())).unwrap();
let result = signature.verify(&message, &public_key).unwrap();
assert_eq!(result, false);
let result = signature.verify(&message, &public_key);
assert_eq!(
result.unwrap_err(),
ContractError::SignatureVerificationFailed {
reason: "unable to verify signature".into(),
}
);
}

#[test]
Expand All @@ -531,7 +569,12 @@ mod ed25519_tests {
Signature::try_from((KeyType::Ed25519, ed25519_test_data::signature())).unwrap();
let message = MsgToSign::try_from(ed25519_test_data::message()).unwrap();
let public_key = PublicKey::Ed25519(invalid_pub_key);
let result = signature.verify(&message, &public_key).unwrap();
assert_eq!(result, false);
let result = signature.verify(&message, &public_key);
assert_eq!(
result.unwrap_err(),
ContractError::SignatureVerificationFailed {
reason: "unable to verify signature".into(),
}
);
}
}
50 changes: 34 additions & 16 deletions contracts/multisig/src/signing.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use std::collections::HashMap;

use connection_router_api::ChainName;
use cosmwasm_schema::cw_serde;
use cosmwasm_std::{Addr, Uint256, Uint64};
use cosmwasm_std::{Addr, HexBinary, Uint256, Uint64};
use signature_verifier_api::client::SignatureVerifier;

use crate::{
Expand Down Expand Up @@ -73,33 +73,51 @@ pub fn validate_session_signature(
});
}

let valid = sig_verifier.map_or_else(
|| signature.verify(&session.msg, pub_key),
|verifier| {
verifier
.verify_signature(
sig_verifier
.map_or_else(
|| signature.verify(&session.msg, pub_key),
|sig_verifier| {
call_sig_verifier(
sig_verifier,
signature.as_ref().into(),
session.msg.as_ref().into(),
pub_key.as_ref().into(),
signer.to_string(),
session.id,
)
.map_err(|err| ContractError::SignatureVerificationFailed {
reason: err.to_string(),
})
},
)?;

if !valid {
return Err(ContractError::InvalidSignature {
},
)
.map_err(|_| ContractError::InvalidSignature {
session_id: session.id,
signer: signer.into(),
});
}
})?;

Ok(())
}

fn call_sig_verifier(
sig_verifier: SignatureVerifier,
signature: HexBinary,
message: HexBinary,
pub_key: HexBinary,
signer: String,
session_id: Uint64,
) -> Result<(), ContractError> {
let res = sig_verifier
.verify_signature(signature, message, pub_key, signer, session_id)
.map_err(|err| ContractError::SignatureVerificationFailed {
reason: err.to_string(),
})?;

if !res {
Err(ContractError::SignatureVerificationFailed {
reason: "unable to verify signature".into(),
})
} else {
Ok(())
}
}

fn signers_weight(signatures: &HashMap<String, Signature>, worker_set: &WorkerSet) -> Uint256 {
signatures
.keys()
Expand Down

0 comments on commit e120fa8

Please sign in to comment.