Wallet Connect login not working

sachben91 commented 1 year ago

Environment

Chrome on Desktop
Safari on Ios

Steps to reproduce

Click login with Wallet Connect on desktop
Finish the signing process on your phone on metamask
Exit out of the sign in modal on the desktop and check if you are logged in
Refresh the page to see if login happens

Loom

Expected Results

Logged into Common using Wallet Connect after clicking out of the sign in box or after refresh

Actual Results

Not logged into Common using Wallet Connect after clicking out of sign in after completing signing process on the phone and not logged in on refresh either

Priority for launch

blocking the launch since this login flow is not working at all

masvelio commented 1 year ago

Reproduced locally and on frack. It works correctly on beta or prod. First thing is that maybe we need to whitelist frack or localhost to make walletconnect working?

Other thing is that our backend throws error when I try to use metamask wallet on from my phone.

2023-09-12 16:05:33,890 INFO [verifySessionSignature.ts] Eth verification failed for 0x067a7910789f214A13E195a025F881E9B59C4D76: Error: Invalid signature: signature did not match /^(.+)\/([A-Za-z0-9]+)\/(0x[A-Fa-f0-9]+)$/
    at verifySessionSignature (/Users/marcin/Desktop/Projects/commonwealth/packages/commonwealth/server/util/verifySessionSignature.ts:255:15)
    at processAddress (/Users/marcin/Desktop/Projects/commonwealth/packages/commonwealth/server/routes/verifyAddress.ts:77:47)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at verifyAddress (/Users/marcin/Desktop/Projects/commonwealth/packages/commonwealth/server/routes/verifyAddress.ts:217:3)
2023-09-12 16:05:33,895 WARN [verifyAddress.ts] Failed to verify signature for 0x067a7910789f214A13E195a025F881E9B59C4D76: AppError: Invalid signature, please re-register
    at processAddress (/Users/marcin/Desktop/Projects/commonwealth/packages/commonwealth/server/routes/verifyAddress.ts:89:13)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at verifyAddress (/Users/marcin/Desktop/Projects/commonwealth/packages/commonwealth/server/routes/verifyAddress.ts:217:3)
e AppError: Invalid signature, please re-register
    at processAddress (/Users/marcin/Desktop/Projects/commonwealth/packages/commonwealth/server/routes/verifyAddress.ts:89:13)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)
    at verifyAddress (/Users/marcin/Desktop/Projects/commonwealth/packages/commonwealth/server/routes/verifyAddress.ts:217:3) {
  status: 400
}

Would love @raykyri to take a look on this. cc @jnaviask @MuonShot

kurtisassad commented 1 year ago

Part of the problem is in verifySessionSignature.ts here:

      const signaturePattern = /^(.+)\/([A-Za-z0-9]+)\/(0x[A-Fa-f0-9]+)$/;
      const signaturePatternMatch = signaturePattern.exec(signatureString);
      ....
      const [_, domain, nonce, signatureData] = signaturePatternMatch;

This pattern does not match a valid ethereum signature. For example, after logging in with walletconnect the signature passed is: 0xc7ddf3a5c5b57eeaf36ad331c33a40154b1e8d6c5331c4164442d8b1979f65ad46dda2723cff4b38b5a46648804aff7ecd835b433cf7404d4372dcded95053261b

But this pattern expects (url) (nonce) (signatureData) all non-optionally, so the logic needs to change.

Also later on when we createSiweMessage, in keys.ts we use this url: domain: new URL(domain).host So it will also throw an error if the URL is not defined (ERR_INVALID_URL)

raykyri commented 1 year ago

Fixed the WalletConnect issue, it was using the wrong signature format as you said. Tested and working both on desktop and mobile including on Frack.

Feel free to close if you think this resolves everything!

masvelio commented 1 year ago

fixed with this commit https://github.com/hicommonwealth/commonwealth/pull/3426/commits/a9936eaaa2fcb6f6c0e3a440f1f05c2a6a5efc2a

hicommonwealth / commonwealth