Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Signature verify when create wallet (not using RELAY) #178

Closed

Conversation

patogallaiovlabs
Copy link

Tests (BIG Disclaimer!!!)

  • This has only been tested manually, no formal test developed nor modified in this PR so far, this is a draft.

Context

When using the SmartWalletFactory contract without using the relay, directly from an EOA, the function "createUserSmartWallet" is called. This function checks if the signature belongs to the owner of the wallet and that the message signed is: hash(<owner> + <recoverer> + <index>).

Issue

Using Metamask/Nifty wallets at the moment of signing the message, a prefix is added so the final message would be:
"\x19Ethereum Signed Message:\n" + msg.length + hash(<owner> + <recoverer> + <index>).
This is due to eth_sign is deprecated by those wallets:

Solution

So, in order to be compatible with this behaviour is that in this PR the verification of the signature (only in the case of direct deploy, without use of the RELAY mechanism) has been changed to the following:

  • Message to sign in the client side is: hash(<verifier_address> + <owner> + <recoverer> + <index>)
  • Client should use presonal_sign function (which prepends "\x19Ethereum Signed Message:\n" + msg.length).
  • The recovery process in the Contract is:
    • msg = "\x19Ethereum Signed Message:\n" + msg.length + hash(<verifier_address>+<owner> + <recoverer> + <index>)
    • hash(msg).recover(signature)

@raullaprida
Copy link
Collaborator

@mortelli You can add me as reviewer once it is ready-for-review!

@mortelli
Copy link
Contributor

relevant PRs:

  1. contracts
  2. tests

@antomor
Copy link
Collaborator

antomor commented Dec 2, 2022

These changes have been included in the relevant PRs.

@antomor antomor closed this Dec 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants