search

spruceid / siwe-py

A Python implementation of Sign-In with Ethereum
https://login.xyz
Apache License 2.0
66 stars 29 forks source link

Multisigs/Gnosis safe wallets do not work out of the box #63

Open UTkzhang opened 3 months ago

UTkzhang commented 3 months ago

In verify function,

   try:
      address = w3.eth.account.recover_message(message, signature=signature)
  except ValueError:
      address = None
  except eth_utils.exceptions.ValidationError as e:
      raise InvalidSignature from None

This recover_message runs into the ValidationError: "Unexpected recoverable signature length: Expected 65, but got 130 bytes" for a 2 signer signature, and raises Exception with cause None instead of going on to check_contract_wallet_signature.

This should not be the behaviour and causes all multisig signers to not work with SIWE lib out of the box.

Reproduce: try signing a SIWE message with 2 signers multisig like safe wallet and see if verify is able to authenticate it.

Proposed fix: if sig is not 65, instead of raising, continue to check check_contract_wallet_signature eip1271 directly, because it is either a multisig or the user passed in the wrong string somehow. Only raise if check_contract_wallet_signature fails.

sbihel commented 3 months ago

In your call to the verify function, do you supply a provider? Here is an example with a test for contract wallets https://github.com/spruceid/siwe-py/blob/53f71d805c39d6118d0f47620940bbe2c5699779/tests/test_siwe.py#L82

UTkzhang commented 3 months ago

Yes the provider isn’t the issue - it’s the fact that multi sig contract wallets have non standard signature lengths which aren’t handled corrected at the moment

UTkzhang commented 3 months ago

In the sample test json, I see the sig is a single signer from a smart contract wallet. Try a multi signer variant (2 signers for example).

The multi sig signature is a concat of all the individual signatures so in the test you can reproduce by doubling the length of the signature.

sbihel commented 3 months ago

I'm fairly certain that the error about the signature length is a red herring as it is the reason why the EIP-191 failed, but it doesn't mean EIP-1271 wasn't tried (it's unfortunate, but there's no way of knowing in advance what "kind" of signature it is).

Would you be able to provide an example? You mentioned Safe and it does support EIP-1271, so I'm curious what is the root cause.

UTkzhang commented 3 months ago

Yea my point is exactly that - the standard recover fails, the eip1271 is implemented correctly, no issues there.

The bug is that due to multi sig wallets returning a signature that is of non standard length, it errors on the recover method and never makes it to the subsequent eip1271 verification.

As highlighted in original issue: This recover_message runs into the ValidationError: "Unexpected recoverable signature length: Expected 65, but got 130 bytes" for a 2 signer signature, and raises Exception with cause None instead of going on to check_contract_wallet_signature.

So it is the eip191 failing, but it is also confirmed that the eip1271 isn’t tried because it raises on this recover_message line instead of continuing like all other cases.

UTkzhang commented 3 months ago 
try:
      address = w3.eth.account.recover_message(message, signature=signature) # fails due to wrong sig length
  except ValueError:
      address = None
  except eth_utils.exceptions.ValidationError as e:
      raise InvalidSignature from None # raised to upper level and never makes it to the next line for eip1271
sbihel commented 3 months ago

I looked a bit more into it and we already have a test for EIP-1271 with a signature that's 66 bytes long https://github.com/spruceid/siwe/blob/4290e1fb3702c97ffee16112492216ad4c4654c1/test/eip1271.json#L8. Would you be able to provide an example because I don't know how to reproduce it right now?

UTkzhang commented 3 months ago

Make the test signature a 130 byte one and it can be reproduced. Add a test with a eip1271 signature that is 130 bytes long.

like literally double the current signature by copy pasting itself and append to end of the signature - the signature would be expected to fail at the eip1271 level since it’s now a made up one, but it won’t. It will fail before it reaches the check_contract_wallet_signature line. That’s the bug.

UTkzhang commented 3 months ago

You can get the signature to a real transaction by signing it with more than one signer, any multisig wallet will concat the signatures together, so 2 signers would be 130 bytes, and adding 65 bytes for each signer added