Skip to content

Commit

Permalink
feat: Remove methods to sign an arbitraty message (#1294)
Browse files Browse the repository at this point in the history 
## What ❔

Removes public APIs that allow signing arbitrary messages.
Replaces #482

## Why ❔

The functionality to sign arbitrary messages was used in zkSync Lite
(where it was inherited from), but is not needed for zkSync Era.
Moreover, the implementation is controversial, e.g. we can either add or
not add the Ethereum standard prefix, and without a use case we don't
really have a justification for either option.
Overall, it reduces the surface of public API we commit to support.

## Checklist

<!-- Check your PR fulfills the following items. -->
<!-- For draft PRs check the boxes as you complete them. -->

- [x] PR title corresponds to the body of PR (we generate changelog
entries from PRs).
- [x] Tests for the changes have been added / updated.
- [x] Documentation comments have been added / updated.
- [x] Code has been formatted via `zk fmt` and `zk lint`.
- [x] Spellcheck has been run via `zk spellcheck`.
- [x] Linkcheck has been run via `zk linkcheck`.
  • Loading branch information
@popzxc
popzxc committed Feb 29, 2024
1 parent e279456 commit 8904123
Show file tree
Hide file tree
Showing 5 changed files with 5 additions and 152 deletions.
131 changes: 3 additions & 128 deletions core/lib/eth_signer/src/json_rpc_signer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,64 +29,15 @@ pub enum AddressOrIndex {
Index(usize),
}

/// Describes whether to add a prefix `\x19Ethereum Signed Message:\n`
/// when requesting a message signature.
#[derive(Debug, Clone)]
pub enum SignerType {
NotNeedPrefix,
NeedPrefix,
}

#[derive(Debug, Clone)]
pub struct JsonRpcSigner {
rpc_addr: String,
client: reqwest::Client,
address: Option<Address>,
signer_type: Option<SignerType>,
}

#[async_trait::async_trait]
impl EthereumSigner for JsonRpcSigner {
/// The sign method calculates an Ethereum specific signature with:
/// checks if the server adds a prefix if not then adds
/// return sign(keccak256("\x19Ethereum Signed Message:\n" + len(message) + message))).
async fn sign_message(&self, msg: &[u8]) -> Result<PackedEthSignature, SignerError> {
let signature: PackedEthSignature = {
let msg = match &self.signer_type {
Some(SignerType::NotNeedPrefix) => msg.to_vec(),
Some(SignerType::NeedPrefix) => {
let prefix = format!("\x19Ethereum Signed Message:\n{}", msg.len());
let mut bytes = Vec::with_capacity(prefix.len() + msg.len());
bytes.extend_from_slice(prefix.as_bytes());
bytes.extend_from_slice(msg);

bytes
}
None => {
return Err(SignerError::MissingEthSigner);
}
};

let message = JsonRpcRequest::sign_message(self.address()?, &msg);
let ret = self
.post(&message)
.await
.map_err(|err| SignerError::SigningFailed(err.to_string()))?;
serde_json::from_value(ret)
.map_err(|err| SignerError::SigningFailed(err.to_string()))?
};

let signed_bytes = PackedEthSignature::message_to_signed_bytes(msg);
// Checks the correctness of the message signature without a prefix
if is_signature_from_address(&signature, &signed_bytes, self.address()?)? {
Ok(signature)
} else {
Err(SignerError::SigningFailed(
"Invalid signature from JsonRpcSigner".to_string(),
))
}
}

/// Signs typed struct using Ethereum private key by EIP-712 signature standard.
/// Result of this function is the equivalent of RPC calling `eth_signTypedData`.
async fn sign_typed_data<S: EIP712TypedStructure + Sync>(
Expand Down Expand Up @@ -154,14 +105,12 @@ impl JsonRpcSigner {
pub async fn new(
rpc_addr: impl Into<String>,
address_or_index: Option<AddressOrIndex>,
signer_type: Option<SignerType>,
password_to_unlock: Option<String>,
) -> Result<Self, SignerError> {
let mut signer = Self {
rpc_addr: rpc_addr.into(),
client: reqwest::Client::new(),
address: None,
signer_type,
};

// If the user has not specified either the index or the address,
Expand All @@ -180,12 +129,6 @@ impl JsonRpcSigner {
signer.unlock(&password).await?;
}

// If it is not known whether it is necessary
// to add a prefix to messages, then we define this.
if signer.signer_type.is_none() {
signer.detect_signer_type().await?;
};

Ok(signer)
}

Expand Down Expand Up @@ -217,47 +160,6 @@ impl JsonRpcSigner {
self.address.ok_or(SignerError::DefineAddress)
}

/// Server can either add the prefix `\x19Ethereum Signed Message:\n` to the message and not add.
/// Checks if a prefix should be added to the message.
pub async fn detect_signer_type(&mut self) -> Result<(), SignerError> {
// If the `sig_type` is set, then we do not need to detect it from the server.
if self.signer_type.is_some() {
return Ok(());
}

let msg = "JsonRpcSigner type was not specified. Sign this message to detect the signer type. It only has to be done once per session";
let msg_with_prefix = format!("\x19Ethereum Signed Message:\n{}{}", msg.len(), msg);

let signature: PackedEthSignature = {
let message = JsonRpcRequest::sign_message(self.address()?, msg.as_bytes());

let ret = self
.post(&message)
.await
.map_err(|err| SignerError::SigningFailed(err.to_string()))?;
serde_json::from_value(ret)
.map_err(|err| SignerError::SigningFailed(err.to_string()))?
};

let msg_signed_bytes = PackedEthSignature::message_to_signed_bytes(msg.as_bytes());
if is_signature_from_address(&signature, &msg_signed_bytes, self.address()?)? {
self.signer_type = Some(SignerType::NotNeedPrefix);
}

let msg_with_prefix_signed_bytes =
PackedEthSignature::message_to_signed_bytes(msg_with_prefix.as_bytes());
if is_signature_from_address(&signature, &msg_with_prefix_signed_bytes, self.address()?)? {
self.signer_type = Some(SignerType::NeedPrefix);
}

match self.signer_type.is_some() {
true => Ok(()),
false => Err(SignerError::SigningFailed(
"Failed to get the correct signature".to_string(),
)),
}
}

/// Unlocks the current account, after that the server can sign messages and transactions.
pub async fn unlock(&self, password: &str) -> Result<(), SignerError> {
let message = JsonRpcRequest::unlock_account(self.address()?, password);
Expand Down Expand Up @@ -368,17 +270,6 @@ mod messages {
Self::create("personal_unlockAccount", params)
}

/// The sign method calculates an Ethereum specific signature with:
/// sign(keccak256("\x19Ethereum Signed Message:\n" + len(message) + message))).
/// The address to sign with must be unlocked.
pub fn sign_message(address: Address, message: &[u8]) -> Self {
let params = vec![
serde_json::to_value(address).expect("serialization fail"),
serde_json::to_value(format!("0x{}", encode(message))).expect("serialization fail"),
];
Self::create("eth_sign", params)
}

/// Signs typed struct using Ethereum private key by EIP-712 signature standard.
/// The address to sign with must be unlocked.
pub fn sign_typed_data<S: EIP712TypedStructure + Sync>(
Expand Down Expand Up @@ -440,9 +331,9 @@ mod tests {
use futures::future::{AbortHandle, Abortable};
use jsonrpc_core::{Failure, Id, Output, Success, Version};
use serde_json::json;
use zksync_types::{tx::primitives::PackedEthSignature, Address, H256};
use zksync_types::{tx::primitives::PackedEthSignature, H256};

use super::{is_signature_from_address, messages::JsonRpcRequest};
use super::messages::JsonRpcRequest;
use crate::{raw_ethereum_tx::TransactionParameters, EthereumSigner, JsonRpcSigner};

#[post("/")]
Expand All @@ -458,14 +349,6 @@ mod tests {
create_success(json!(addresses))
}
"personal_unlockAccount" => create_success(json!(true)),
"eth_sign" => {
let _address: Address = serde_json::from_value(req.params[0].clone()).unwrap();
let data: String = serde_json::from_value(req.params[1].clone()).unwrap();
let data_bytes = hex::decode(&data[2..]).unwrap();
let signature =
PackedEthSignature::sign(&state.private_keys[0], &data_bytes).unwrap();
create_success(json!(signature))
}
"eth_signTransaction" => {
let tx_value = json!(req.params[0].clone()).to_string();
let tx = tx_value.as_bytes();
Expand Down Expand Up @@ -535,15 +418,7 @@ mod tests {
private_keys: vec![H256::repeat_byte(0x17)],
});
// Get address is ok, unlock address is ok, recover address from signature is also ok
let client = JsonRpcSigner::new(address, None, None, None).await.unwrap();
let msg = b"some_text_message";

let signature = client.sign_message(msg).await.unwrap();
let signed_bytes = PackedEthSignature::message_to_signed_bytes(msg);
assert!(
is_signature_from_address(&signature, &signed_bytes, client.address().unwrap())
.unwrap()
);
let client = JsonRpcSigner::new(address, None, None).await.unwrap();

let transaction_signature = client
.sign_transaction(TransactionParameters::default())
Expand Down
1 change: 0 additions & 1 deletion core/lib/eth_signer/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ pub mod raw_ethereum_tx;

#[async_trait]
pub trait EthereumSigner: 'static + Send + Sync + Clone {
async fn sign_message(&self, message: &[u8]) -> Result<PackedEthSignature, SignerError>;
async fn sign_typed_data<S: EIP712TypedStructure + Sync>(
&self,
domain: &Eip712Domain,
Expand Down
8 changes: 0 additions & 8 deletions core/lib/eth_signer/src/pk_signer.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,6 @@ impl EthereumSigner for PrivateKeySigner {
.map_err(|_| SignerError::DefineAddress)
}

/// The sign method calculates an Ethereum specific signature with:
/// sign(keccak256("\x19Ethereum Signed Message:\n" + len(message) + message))).
async fn sign_message(&self, message: &[u8]) -> Result<PackedEthSignature, SignerError> {
let signature = PackedEthSignature::sign(&self.private_key, message)
.map_err(|err| SignerError::SigningFailed(err.to_string()))?;
Ok(signature)
}

/// Signs typed struct using Ethereum private key by EIP-712 signature standard.
/// Result of this function is the equivalent of RPC calling `eth_signTypedData`.
async fn sign_typed_data<S: EIP712TypedStructure + Sync>(
Expand Down
7 changes: 0 additions & 7 deletions core/lib/types/src/tx/primitives/packed_eth_signature.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,13 +66,6 @@ impl PackedEthSignature {
Ok(PackedEthSignature(ETHSignature::from(signature)))
}

/// Signs message using Ethereum private key, results are identical to signature created
/// using `geth`, `ethers.js`, etc. No hashing and prefixes required.
pub fn sign(private_key: &H256, msg: &[u8]) -> Result<PackedEthSignature, ParityCryptoError> {
let signed_bytes = Self::message_to_signed_bytes(msg);
Self::sign_raw(private_key, &signed_bytes)
}

pub fn sign_raw(
private_key: &H256,
signed_bytes: &H256,
Expand Down
10 changes: 2 additions & 8 deletions core/tests/loadnext/src/corrupted_tx.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ use zksync_eth_signer::{
error::SignerError, raw_ethereum_tx::TransactionParameters, EthereumSigner,
};
use zksync_types::{
fee::Fee, l2::L2Tx, Address, EIP712TypedStructure, Eip712Domain, PackedEthSignature, H256,
fee::Fee, l2::L2Tx, Address, EIP712TypedStructure, Eip712Domain, PackedEthSignature,
};

use crate::command::IncorrectnessModifier;
Expand Down Expand Up @@ -56,9 +56,7 @@ pub struct CorruptedSigner {

impl CorruptedSigner {
fn bad_signature() -> PackedEthSignature {
let private_key = H256::random();
let message = b"bad message";
PackedEthSignature::sign(&private_key, message).unwrap()
PackedEthSignature::default()
}

pub fn new(address: Address) -> Self {
Expand All @@ -68,10 +66,6 @@ impl CorruptedSigner {

#[async_trait]
impl EthereumSigner for CorruptedSigner {
async fn sign_message(&self, _message: &[u8]) -> Result<PackedEthSignature, SignerError> {
Ok(Self::bad_signature())
}

async fn sign_typed_data<S: EIP712TypedStructure + Sync>(
&self,
_domain: &Eip712Domain,
Expand Down

0 comments on commit 8904123

Please sign in to comment.