hicommonwealth / commonwealth

A platform for decentralized communities
https://commonwealth.im
GNU General Public License v3.0
68 stars 44 forks source link

SAFE wallet support via WalletConnect #3880

Closed jnaviask closed 1 year ago

jnaviask commented 1 year ago

Description

Gnosis Safe, now SAFE, is a WalletConnect wallet. However, users of SAFE are having issues logging in. We need to diagnose the reason why SAFE wallet does not work + push a fix.

Ideally we should support both SAFE mobile and desktop wallets (= 3 cases: desktop app x desktop wallet / desktop app x mobile wallet / mobile app x mobile wallet -- these may need to be treated separately).

Take this opportunity to get familiar with WalletConnect and the Wallet login flow in general, as we want to gather information about how to dramatically improve our login flow in the near future.

Acceptance Criteria

ianrowan commented 1 year ago

Looking into this now, do we have any test gnosis safes/wallets set up to test this. There are gas fees associated with creating a Safe wallet so wanted to check how to proceed there. Will attempt to use goerli testnet in the meantime but not sure if the devex/reproducability will be different

CC: @jnaviask @CowMuon

ianrowan commented 1 year ago

There is a general bug in the way we're injecting evm providers into wallet connect here which will be easily fixed.

Some slightly bad news, signing messages(canvas login) is a pretty complex transaction for SAFE: https://docs.safe.global/learn/safe-core/safe-core-protocol/signatures/eip-1271

The tl;dr is that to sign messages

  1. Implement a custom function to send a signature request to SAFE
  2. Multi-sig threshold on signers must be met(ie 3 of 5 must sign)
  3. This becomes a transaction on chain(meaning users pay gas to log in)
  4. We verify the signature by making requests to the contract to check if the signature was approved, expecting a magic value.

So if my understanding here is right safe users would all need to sign every login and pay gas on each login. verification on our backend would need to poll until this whole process completes

CC: @jnaviask @CowMuon @zakhap

jnaviask commented 1 year ago

There is a general bug in the way we're injecting evm providers into wallet connect here which will be easily fixed.

Feel free to ticket this + deploy a fix!

As for the other aspects -- are there alternative mechanisms exposed by SAFE that we can use to verify ownership of an address other than a signature-response flow?

ianrowan commented 1 year ago

@jnaviask

We can potentially create our own logic for login at least(it would get tricky with on-chain txs).

Could be something like

  1. Get signers from a safe contract(the UX would be sort of odd here as we would need them to manually provide the safe address)
  2. Request a signature from the local user
  3. Verify signature and verify signature is from a signer
  4. Login on behalf of SAFE(Account would represent safe address but operated by signer address)

Would be cool to allow multiple signers to each login under 1 account in this case thinking beyond

jnaviask commented 1 year ago

This is a super interesting idea. How does the WalletConnect flow actually work? I'm curious how they can even do WalletConnect stuff without being able to provide signatures / etc.

ianrowan commented 1 year ago

This is a super interesting idea. How does the WalletConnect flow actually work? I'm curious how they can even do WalletConnect stuff without being able to provide signatures / etc.

@jnaviask so safe exclusively uses WC as its dapp connector.

When we go through our login flow for example, on the safe wallet app, any owner of the SAFE can paste the WC code/scan QR from the site. From here it works like 2FA: approve app, WC connection complete and creates a web3 provider that is injected with an RPC for the network and the public key for the SAFE.

Where we run into issue is that the sign messages + send transaction functions work totally different although theyre on the same WC interface even though we still get a provider. On the backend when signMessage is called WC:

  1. sends message through a WSS connection to safe(safe users need to keep the DAPP page open on their app or connection closes)
  2. SAFE owner(s) 'sign' the message like usual on the safe app, but this doesnt produce a signature rather a transaction on the safe SC which is just a proof of signature from all users.
  3. WC receives back data that a single user has signed/voted on the transaction, but no actual signature b/c smart contracts cannot produce signature as they dont have pkeys

So long story short WC signMessage and sendTransaction functions in a SAFE context only prompt users to build the transactions and approve on their smart contract but do not directly produce a signature or transaction

This being said, we can allow login via WC b/c we do get confirmation the wss connection is created + the address of the SAFE when connecting via the QR code and SAFE app, but we cannot use session keys

jnaviask commented 1 year ago

I see. This is kind of difficult, because we have no way of validating on the backend that the WC message received on the client is valid, i.e. it could be spoofed. Need to figure out a workaround for these security considerations.

Beyond the main question of security, we also face the question of how should multisig addresses work as part of Commonwealth, at the domain level? This question is out of scope for this ticket, but blocks this ticket's implementation, so for the time being we can backlog this ticket.

jnaviask commented 1 year ago

cc @raykyri who may find this interesting.

raykyri commented 1 year ago

Interesting re: signatures. We can look into implementing EIP-1271 verifiers on the Canvas side to make this easier.

From a product standpoint, it might be desirable to steer towards a model where individual users can tag themselves with a SAFE that they're linked to, within a larger theme of enriching Common accounts with their associations.

I think this is something that has come up in product conversations, where delegates want to associate their onchain identities with Twitter accounts (e.g. I run into people like https://twitter.com/VelvetMilkman) and it would make sense for their username to be tagged w/ their Twitter account if they desire. Similarly, if "Sam" is the member of a SAFE, it seems useful for them to be able to connect that SAFE to their profile.

We can potentially create our own logic for login at least(it would get tricky with on-chain txs).

Could be something like

  1. Get signers from a safe contract(the UX would be sort of odd here as we would need them to manually provide the safe address)
  2. Request a signature from the local user
  3. Verify signature and verify signature is from a signer
  4. Login on behalf of SAFE(Account would represent safe address but operated by signer address)

Would be cool to allow multiple signers to each login under 1 account in this case thinking beyond

I also like this.

ianrowan commented 1 year ago

@raykyri Implementing SAFE compliant EIP-1271 on the canvas side would be a great approach I think. I could also pave the way for future smart contract accounts/wallets(ERC4337, etc) which will be working with a similar approach for signature verification

jnaviask commented 1 year ago

Great research & discussion here. Closing for now, will revisit the insights from this ticket down the road.