chainapsis / keplr-wallet

The most powerful wallet for the Cosmos ecosystem and the Interchain
https://www.keplr.app
Other
764 stars 456 forks source link

Unexpected result when using offline signer with cosmjs #128

Closed samepant closed 3 years ago

samepant commented 3 years ago

I'm working on a multisignature application for the Interchain Foundation and have been running into some issues with how Keplr handles offline signing when compared to Cosmjs. There might be a simple explanation/fix for this, but I'm a little out of my depth when it comes to encoding/decoding byte arrays, and would appreciate any insight that you can offer.

In order to zero in on the issue, I made a little repo that demonstrates it more directly than my main project repo. In this example I:

Cosmjs outputs { bodyBytes: Uint8Array(168), signatures: [Uint8Array(64)]} Keplr outputs { bodyBytes: String, signatures: [Uint8Array(64)]} the string value: Co4BChwvY29zbW9zLmJhbmsudjFiZXRhMS5Nc2dTZW5kEm4KLWNvc21vczEwbm1kZjZudDJxenZnbjlxMm51d21tZmMzNTl5ZmVzbXUzZ3cyMhItY29zbW9zMXZxcGpsandzeW5zbjU4ZHVnejB3OHV0N2t1bjd0OGxzMnFrbXNxGg4KBXVhdG9tEgU1MDAwMBIVVG9rZW5zIGZyb20gdXMgdG8geW91

I'm not sure why there is a discrepancy between these two, but I'm not having much luck decoding the string that Keplr is returning for bodyBytes.

My questions:

Other helpful links:

Here's the repo for my larger multisig project, and here's where I'm specifically doing the signing.

thank you 🙏

Thunnini commented 3 years ago

Hi @samepant, really appreciate putting together this issue.

First of all, I can confirm that this is a bug. I wasn’t aware that there was a toJSON() method already built into protobuf message, and this seems to be the issue. When the background and the webpage communicate, Keplr internally has logic to serialize/deserialize types such as Uint8Array, but because of things like Message#toJSON() doesn’t follow the usual Keplr logic, but protobuf’s json serializing logic, the results are different.

Seems like Protobuf messages convert Uint8Array to base64 string, and Long types as string. Therefore, in the case above, the signed values resulted in a base64. Until now, we have only tested methods such methods for sending txs such as sendTokens in CosmJS, and methods like this serialize the tx to proto before sending the transaction so there weren’t issues. However, methods that only request the signature don’t serialize again so the values resulted in a string.

I’ve went ahead and fixed this issue, and should be included in the next release.

By the way, even if you use your implementation Keplr’s resulting signature and your code’s CosmJS result will be different. This is intended behavior. Keplr’s offline signer is compatible with both Amino and Direct signer. However, in CosmJS’s Stargate client if the signer is compatible with direct signer, direct signer is used so Keplr’s offline signer will only work with direct signer for CosmJS Stargate client. Because you are using Amino signer for your signer, the transaction will be sent as LEGACY_AMINO so the sign doc will be different and the resulting signature is also different.

I will think about adding methods getOfflineSignerOnlyAmino() and getOfflineSignerOnlyDirect() for cases like these.

samepant commented 3 years ago

Appreciate the quick response and thanks for the bugfix :)

It's unclear to me whether multisignature transactions signed with a direct signer will be accepted as valid, so I'll give it a try when the new release comes out. If not I'll wait until something like getOfflineSignerOnlyAmino() is implemented.

thanks again for the explanation.

Thunnini commented 3 years ago

Hi @samepant the new extenstion release has been published. Please check if the issue is resolved in Keplr extension v0.8.12.

dogemos commented 3 years ago

Hi @samepant just checking in to see if the update resolved your issue.

samepant commented 3 years ago

Hey all, keplr is now returning expected data after signing, so that's good!

Unfortunately my local node is rejecting the multisignature transaction as unauthorized, I believe because Keplr's offline signer is using the Direct Signer. I'm working on an example to confirm this and will post results later today. If that is indeed the case I will need the potential getOfflineSignerOnlyAmino() method @Thunnini mentioned above.

samepant commented 3 years ago

Ok just confirmed my theory.

Signing a multisignature transaction using cosmjs's SigningStargateClient.offline() allows the verification to go through. So I need a way force Keplr to allow me to use an offline signer with amino, whether that's something like getOfflineSignerOnlyAmino() or some other way.

Let me know what you think, and thanks for working on this.

samepant commented 3 years ago

Just realized that @Thunnini added getOfflineSignerOnlyAmino, sorry for not reading that more clearly 😅

Thunnini commented 3 years ago

Hi @samepant, realized that the documentation was not updated alongside the latest release. I've went ahead and added the relevant documentation for the API and usage here.

samepant commented 3 years ago

I had a chance to implement getOfflineSignerOnlyAmino in my app today and am still seeing transactions being rejected as unauthorized.

I compared the output from getOfflineSignerOnlyAmino and cosmjs's offline signer for the same mnemonic and the signatures are not equal. Is this expected? It's very possible there is something simple I am missing.

Thunnini commented 3 years ago

Hey, sorry for the late response. This issue was closed so I couldn't recognize your question. I looked around your repo and I found that the one reason that the signature from the Keplr is different from the CosmJS's offline signer. Because the Keplr overrides the fee as one of the options ["low", "average", "high"] by default, the actual transaction which the Keplr signs can be different from the requested tx on the frontend. Even though CosmJS handles this automatically, it could be a problem in such a case. For this problem, Keplr supports the option that prevents the Keplr overriding the fee. You can set the options like below

window.keplr.defaultOptions = {
  sign: {
    preferNoSetMemo: true,
    preferNoSetFee: true
  }
}

With this options, Keplr will override the fee and memo only if the user explicitly decides to set the fee manually. Even though I use the cosmoshub mainnet, my test with the above options returns the same signature with the CosmJS's offline signer. Can you try with the above options?

Thunnini commented 3 years ago

@samepant Additionally, the Keplr libraries of v0.9.0 are in the process of being developed and are not matched with the actual Keplr production. I recommend you use the Keplr libraries with v0.8.13. @keplr-wallet/types@0.8.13 should include the typings of above options.

samepant commented 3 years ago

@Thunnini Thank you so much!! That did it, signatures are matching and transactions are going through.