From 73c116404c82948c76dd728b118aa0eab024990b Mon Sep 17 00:00:00 2001 From: ahmed Date: Mon, 6 Mar 2023 12:44:05 +0100 Subject: [PATCH 1/8] fix: return error if signature len does not eq `65` --- engine-precompiles/src/secp256k1.rs | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/engine-precompiles/src/secp256k1.rs b/engine-precompiles/src/secp256k1.rs index f6af4a724..550fe0d7c 100644 --- a/engine-precompiles/src/secp256k1.rs +++ b/engine-precompiles/src/secp256k1.rs @@ -19,7 +19,9 @@ mod consts { // Quite a few library methods rely on this and that should be changed. This // should only be for precompiles. pub fn ecrecover(hash: H256, signature: &[u8]) -> Result { - assert_eq!(signature.len(), 65); + if signature.len() != 65 { + return Err(ExitError::Other(Borrowed("INVALID_SIGNATURE_LENGTH"))); + } #[cfg(feature = "contract")] return sdk::ecrecover(hash, signature).map_err(|e| ExitError::Other(Borrowed(e.as_str()))); @@ -247,4 +249,15 @@ mod tests { .output; assert_eq!(res, expected); } + + #[test] + pub fn test_invalid_signature_length() { + let hash = H256::from_slice( + &hex::decode("1111111111111111111111111111111111111111111111111111111111111111") + .unwrap(), + ); + let signature = hex::decode("123").unwrap(); + let res = ecrecover(hash, &signature); + assert!(res.is_err()); + } } From 66dd9599d3f5a8c2295e621a6175074a8b7fed9b Mon Sep 17 00:00:00 2001 From: ahmed Date: Mon, 6 Mar 2023 13:04:01 +0100 Subject: [PATCH 2/8] fix: avoid magic numbers --- engine-precompiles/src/secp256k1.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/engine-precompiles/src/secp256k1.rs b/engine-precompiles/src/secp256k1.rs index 550fe0d7c..56843c4f0 100644 --- a/engine-precompiles/src/secp256k1.rs +++ b/engine-precompiles/src/secp256k1.rs @@ -11,6 +11,7 @@ mod costs { mod consts { pub(super) const INPUT_LEN: usize = 128; + pub(super) const SIGNATURE_LENGTH: usize = 65; } /// See: `https://ethereum.github.io/yellowpaper/paper.pdf` @@ -19,7 +20,7 @@ mod consts { // Quite a few library methods rely on this and that should be changed. This // should only be for precompiles. pub fn ecrecover(hash: H256, signature: &[u8]) -> Result { - if signature.len() != 65 { + if signature.len() != consts::SIGNATURE_LENGTH { return Err(ExitError::Other(Borrowed("INVALID_SIGNATURE_LENGTH"))); } @@ -89,7 +90,7 @@ impl Precompile for ECRecover { let mut v = [0; 32]; v.copy_from_slice(&input[32..64]); - let mut signature = [0; 65]; // signature is (r, s, v), typed (uint256, uint256, uint8) + let mut signature = [0; consts::SIGNATURE_LENGTH]; // signature is (r, s, v), typed (uint256, uint256, uint8) signature[0..32].copy_from_slice(&input[64..96]); // r signature[32..64].copy_from_slice(&input[96..128]); // s From 4c4aca7be413e2824447c5b14f66126b62bfc1cb Mon Sep 17 00:00:00 2001 From: ahmed Date: Mon, 6 Mar 2023 13:23:40 +0100 Subject: [PATCH 3/8] fix: tests --- engine-precompiles/src/secp256k1.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine-precompiles/src/secp256k1.rs b/engine-precompiles/src/secp256k1.rs index 56843c4f0..bbf6ec2c0 100644 --- a/engine-precompiles/src/secp256k1.rs +++ b/engine-precompiles/src/secp256k1.rs @@ -257,7 +257,7 @@ mod tests { &hex::decode("1111111111111111111111111111111111111111111111111111111111111111") .unwrap(), ); - let signature = hex::decode("123").unwrap(); + let signature = hex::decode("1111").unwrap(); let res = ecrecover(hash, &signature); assert!(res.is_err()); } From e03783df1d0a54f5098021ea88381afb1bd42f22 Mon Sep 17 00:00:00 2001 From: ahmed Date: Wed, 8 Mar 2023 13:59:35 +0100 Subject: [PATCH 4/8] fix: return error on size check in compile time --- engine-precompiles/src/secp256k1.rs | 24 ++++++++---------------- 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/engine-precompiles/src/secp256k1.rs b/engine-precompiles/src/secp256k1.rs index bbf6ec2c0..4794ee9ad 100644 --- a/engine-precompiles/src/secp256k1.rs +++ b/engine-precompiles/src/secp256k1.rs @@ -19,10 +19,13 @@ mod consts { /// See: `https://etherscan.io/address/0000000000000000000000000000000000000001` // Quite a few library methods rely on this and that should be changed. This // should only be for precompiles. -pub fn ecrecover(hash: H256, signature: &[u8]) -> Result { - if signature.len() != consts::SIGNATURE_LENGTH { - return Err(ExitError::Other(Borrowed("INVALID_SIGNATURE_LENGTH"))); - } +pub fn ecrecover( + hash: H256, + signature: &[u8; consts::SIGNATURE_LENGTH], +) -> Result { + // if signature.len() != consts::SIGNATURE_LENGTH { + // return Err(ExitError::Other(Borrowed("INVALID_SIGNATURE_LENGTH"))); + // } #[cfg(feature = "contract")] return sdk::ecrecover(hash, signature).map_err(|e| ExitError::Other(Borrowed(e.as_str()))); @@ -121,7 +124,7 @@ mod tests { use crate::utils::new_context; fn ecverify(hash: H256, signature: &[u8], signer: Address) -> bool { - matches!(ecrecover(hash, signature), Ok(s) if s == signer) + matches!(ecrecover(hash, signature[0..65].try_into().unwrap()), Ok(s) if s == signer) } #[test] @@ -250,15 +253,4 @@ mod tests { .output; assert_eq!(res, expected); } - - #[test] - pub fn test_invalid_signature_length() { - let hash = H256::from_slice( - &hex::decode("1111111111111111111111111111111111111111111111111111111111111111") - .unwrap(), - ); - let signature = hex::decode("1111").unwrap(); - let res = ecrecover(hash, &signature); - assert!(res.is_err()); - } } From 59a7b60c8ce374244abbc30d6f024ed582f47905 Mon Sep 17 00:00:00 2001 From: ahmed Date: Wed, 8 Mar 2023 14:10:52 +0100 Subject: [PATCH 5/8] chore: remove outdated code --- engine-precompiles/src/secp256k1.rs | 4 ---- 1 file changed, 4 deletions(-) diff --git a/engine-precompiles/src/secp256k1.rs b/engine-precompiles/src/secp256k1.rs index 4794ee9ad..76dc5d7e5 100644 --- a/engine-precompiles/src/secp256k1.rs +++ b/engine-precompiles/src/secp256k1.rs @@ -23,10 +23,6 @@ pub fn ecrecover( hash: H256, signature: &[u8; consts::SIGNATURE_LENGTH], ) -> Result { - // if signature.len() != consts::SIGNATURE_LENGTH { - // return Err(ExitError::Other(Borrowed("INVALID_SIGNATURE_LENGTH"))); - // } - #[cfg(feature = "contract")] return sdk::ecrecover(hash, signature).map_err(|e| ExitError::Other(Borrowed(e.as_str()))); From 801ef739167c2476d04799b77db24ed0815f83bf Mon Sep 17 00:00:00 2001 From: ahmed Date: Thu, 16 Mar 2023 12:36:00 +0100 Subject: [PATCH 6/8] chore: update tests --- engine-precompiles/src/secp256k1.rs | 13 +++++++++++++ engine-tests/src/tests/ecrecover.rs | 1 + 2 files changed, 14 insertions(+) diff --git a/engine-precompiles/src/secp256k1.rs b/engine-precompiles/src/secp256k1.rs index 76dc5d7e5..9478e2cb5 100644 --- a/engine-precompiles/src/secp256k1.rs +++ b/engine-precompiles/src/secp256k1.rs @@ -249,4 +249,17 @@ mod tests { .output; assert_eq!(res, expected); } + + #[test] + fn test_extra_input_length() { + let input = hex::decode("18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c000000000000000000000000000000000000000000000000000000000000001c73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75feeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549aabbccddeeff").unwrap(); + let expected = + hex::decode("000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b") + .unwrap(); + let res = ECRecover + .run(&input, Some(EthGas::new(3_000)), &new_context(), false) + .unwrap() + .output; + assert_eq!(res, expected); + } } diff --git a/engine-tests/src/tests/ecrecover.rs b/engine-tests/src/tests/ecrecover.rs index e1362f400..75db7edc1 100644 --- a/engine-tests/src/tests/ecrecover.rs +++ b/engine-tests/src/tests/ecrecover.rs @@ -17,6 +17,7 @@ fn test_ecrecover_geth() { "18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c100000000000000000000000000000000000000000000000000000000000001c73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75feeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549", "18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c000000000000000000000000000000000000001000000000000000000000001c73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75feeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549", "18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c000000000000000000000000000000000000001000000000000000000000011c73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75feeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549", + "18c547e4f7b0f325ad1e56f57e26c745b09a3e503d86e00e5255ff7f715d3d1c000000000000000000000000000000000000000000000000000000000000001c73b1693892219d736caba55bdb67216e485557ea6b6af75f37096c9aa6a5a75feeb940b1d03b21e36b0e47e79769f095fe2ab855bd91e3a38756b7d75a9c4549aabbccddeeff" ]; let outputs = [ Vec::new(), From 94e6f899bf4b122c2b0e14265e9e463f30770a57 Mon Sep 17 00:00:00 2001 From: ahmed Date: Thu, 16 Mar 2023 12:39:47 +0100 Subject: [PATCH 7/8] fix: update test outputs --- engine-tests/src/tests/ecrecover.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/engine-tests/src/tests/ecrecover.rs b/engine-tests/src/tests/ecrecover.rs index 75db7edc1..ec15865c6 100644 --- a/engine-tests/src/tests/ecrecover.rs +++ b/engine-tests/src/tests/ecrecover.rs @@ -25,6 +25,7 @@ fn test_ecrecover_geth() { Vec::new(), Vec::new(), Vec::new(), + hex::decode("000000000000000000000000a94f5374fce5edbc8e2a8697c15331677e6ebf0b").unwrap(), ]; for (input, output) in inputs.iter().zip(outputs.iter()) { From 471e21dae6287974d5640b5bc4420254a74954ac Mon Sep 17 00:00:00 2001 From: Ahmed Ali Date: Thu, 16 Mar 2023 13:17:07 +0100 Subject: [PATCH 8/8] Update engine-precompiles/src/secp256k1.rs Co-authored-by: Oleksandr Anyshchenko --- engine-precompiles/src/secp256k1.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/engine-precompiles/src/secp256k1.rs b/engine-precompiles/src/secp256k1.rs index 9478e2cb5..718d414b3 100644 --- a/engine-precompiles/src/secp256k1.rs +++ b/engine-precompiles/src/secp256k1.rs @@ -120,7 +120,7 @@ mod tests { use crate::utils::new_context; fn ecverify(hash: H256, signature: &[u8], signer: Address) -> bool { - matches!(ecrecover(hash, signature[0..65].try_into().unwrap()), Ok(s) if s == signer) + matches!(ecrecover(hash, signature[0..super::consts::SIGNATURE_LENGTH].try_into().unwrap()), Ok(s) if s == signer) } #[test]