xmtp / xmtp-js

XMTP client SDKs, content types, and other packages written in TypeScript
https://xmtp.org/docs
211 stars 40 forks source link

Bug: Invalid signature in Client.canMessage #371

Closed elisealix22 closed 1 year ago

elisealix22 commented 1 year ago

Describe the bug

await Client.canMessage("0xC322E8Ec33e9b0a34c7cD185C616087D9842ad50", {env: "production"})

Fails with Uncaught Error: invalid signature. I was able to reproduce locally and here is the stacktrace:

Screenshot 2023-04-25 at 11 40 49 AM

Seems like it is hitting this line in Signature#getPublicKey:

https://github.com/xmtp/xmtp-js/blob/fb4883cf81ce340b4caf9f1ccb60340808e303e9/src/crypto/Signature.ts#L93-L95

Expected behavior

It returns false if the user is not on the network and true if they are (no errors).

Steps to reproduce the bug

Run the following line of code using our SDK (on main branch):

await Client.canMessage("0xC322E8Ec33e9b0a34c7cD185C616087D9842ad50", {env: "production"})
neekolas commented 1 year ago

Would be great to figure out why it is failing for that user. Maybe there are malformed contact bundles on the network.

Either way, we should return false in cases where the contact is junk

neekolas commented 1 year ago

Nothing seems that funky when running xmtp-debug

CleanShot 2023-04-25 at 09 51 44@2x
elisealix22 commented 1 year ago

Ok it looks like we're getting into a state where a signature's ecdsaCompact bytes are undefined and walletEcdsaCompact bytes are present, and Signature#getPublicKey expects escdaCompact bytes only.

I can naively wrap keyBundle?.walletSignatureAddress() in a try/catch -- but I'm curious if there's a better fix here.

Image

elisealix22 commented 1 year ago

Ok the issue seems to be that this address had a malfomred v1 bundle and a correctly formatted v2 bundle. We were throwing an exception in the v1 before we got to the v2 in the loop of contact bundles. I verified this exception is NOT happening in the iOS and Android SDKs so it does appear to be specific to how the JS SDK is handling this case. A fix is to wrap the get wallet address method in a try/catch in the loop of contact bundles so it will eventually try the v2 bundle and see the contact IS on the network -- which will return true for canMessage.

mkobetic commented 1 year ago

OK, with https://github.com/xmtp/xmtp-debug/pull/3 I can definitely confirm that the V1 bundle is malformed. When I dump the most recent bundle pair for my address, the V1 bundle seems OK. I'm wondering if one of our other SDKs may be to blame, i.e. not doing the V2->V1 conversion correctly. @elisealix22 can your team verify that?

Specifically this is the bit that didn't seem to happen for that bad bundle https://github.com/xmtp/xmtp-js/blob/main/src/crypto/PublicKey.ts#L192-L196

mkobetic commented 1 year ago

Not sure if the date on the bad bundle can help narrow down the suspects, but it's timestamped 2023-03-04T03:35:14.928Z

mkobetic commented 1 year ago

Hm, plot thickens, turns out the process that I thought could create this situation happens in reverse. We load/create a V1 private key bundle and then convert it to V2 https://github.com/xmtp/xmtp-js/blob/main/src/keystore/InMemoryKeystore.ts#L43 it seems. So I have no idea how we would end up with a V1 public key using the wallet signature type. @neekolas any ideas?

elisealix22 commented 1 year ago

I believe we might have found a root cause in the Flutter SDK: https://github.com/xmtp/xmtp-flutter/pull/67

mkobetic commented 1 year ago

The flutter PR does seem to show a bug, but is it clear how the bad bundle was actually generated? As I mentioned in https://github.com/xmtp/xmtp-js/issues/371#issuecomment-1523899103, I had it backwards, I thought we generated V2 bundle on account initialization and then converted it to V1, but it seems to be the other way around, at least in the xmtp-js land. So I'm again not quite sure how that bad bundle came to be.

elisealix22 commented 1 year ago

@mkobetic It looks like in flutter land, when we call saveContact() we generate both v1 and v2 bundles at the same time (which would indicate why the timestamp was the same). So I do think if people are saving contacts in a flutter app, they would create the bad bundle.

When creating a client we call ensureSavedContact():

https://github.com/xmtp/xmtp-flutter/blob/5c69224c7c73350b9c3acc0af131f381cdb0c46d/lib/src/client.dart#LL91C28-L91C46

which calls into saveContact() without any params:

https://github.com/xmtp/xmtp-flutter/blob/5c69224c7c73350b9c3acc0af131f381cdb0c46d/lib/src/contact.dart#L97

which will create v1 and v2 contact by default:

https://github.com/xmtp/xmtp-flutter/blob/5c69224c7c73350b9c3acc0af131f381cdb0c46d/lib/src/contact.dart#L62