matter-labs / zksync

zkSync: trustless scaling and privacy engine for Ethereum
https://zksync.io
Apache License 2.0
4.88k stars 2.69k forks source link

EIP1271 signature recovery support for Gnosis safe #270

Open junaidev opened 3 years ago

junaidev commented 3 years ago

Currently zk sync server is unable to recover EIP 1271 signature via gnosis safe implementation of isValidSignature(...) .

isValidSignature() of Gnosis safe supports following:

in Zk sync the message is passed along with prefix and its length, so it doesn't work.

Purposed Change: For supporting EIP 1271 for gnosis safe, need to remove prefix before message.

popzxc commented 3 years ago

Actually, this is done intentionally, since some signers always add this prefix. In case of the wallet not adding the prefix, it is always possible to add the prefix, but not vise versa.

For example, in zksync.js you can provide an EthSignerType argument in which you can describe whether the signer is adding prefixes to messages or not. It is described in docs.

If this argument is omitted, zksync.js will try to deduce whether the signer adds prefixes automatically, but for EIP1271 it's not as easy as for ECDSA signatures.

Anyway, no matter which kind of signer and/or SDK is used, it is always possible to sign the expected messages.

junaidev commented 3 years ago

Some signers but not all might, depends on requirement.

In case of EIP1271 recovery is_eip1271_signature_correct(...) https://github.com/matter-labs/zksync/blob/47bb16fe233d2695c50ccf291dc77dd9f0036dd0/core/bin/zksync_api/src/eth_checker.rs#L55 you are "always" adding prefix via get_sign_message(...) https://github.com/matter-labs/zksync/blob/47bb16fe233d2695c50ccf291dc77dd9f0036dd0/core/bin/zksync_api/src/eth_checker.rs#L46 before passing to isValidSignature which doent work with gnosis safe https://github.com/gnosis/safe-contracts/blob/development/contracts/GnosisSafe.sol#L325 . Reason explained in above issue opened. so for supporting zksync wallet with L1 n of k style multisig ( Gnosis safe multisig ) this needs to be fixed.

e00dan commented 1 year ago

Is this issue a blocker for deploying Safe on current zksync testnet or should it work despite this?

YoleYu commented 1 year ago

any update on Era ?

bxpana commented 1 year ago

Is this issue a blocker for deploying Safe on current zksync testnet or should it work despite this?

To my understanding Safe won't be available on zkSync Lite, but will be deployed on zkSync Era.

bxpana commented 1 year ago

any update on Era ?

https://blog.matter-labs.io/gm-zkevm-171b12a26b36