search

immutability-io / vault-ethereum

A plugin that turns Vault into an Ethereum wallet.
246 stars 66 forks source link

unusable message signing (ethereum/accounts/:name/sign and ethereum/accounts/:name/verify) #74

Open mortimr opened 4 years ago

mortimr commented 4 years ago

Detailed Description

I'm currently trying to sign an EIP712 payload with vault-ethereum. I'm using a lib that is properly signing and verifying payloads that can also be verified by the EVM (https://github.com/ticket721/e712).

Right now I am unable to properly verify the signatures generated by vault-ethereum. When using e712 or eth-sig-util, I end up with a failed verification.

Also the verify route does not seem to work on vault-ethereum.

Why is there no encoding argument like on the sign-tx route ?

Context

Signing messages (and more precisely encoded EIP712 payloads) is primordial as this EIP is becoming the standard for signing. I still cannot identify where the issue is coming from (maybe the format of the data is not good, adding an 'hex' encoding option might be the solution as I'm currently send some binary unreadable strings hoping it gets properly evaluated on vault-ethereum's side).

Possible Implementation

Maybe it can work by reusing what you done in the sign-tx encoding section. Maybe it's coming from something else (differing keccak256 implems, differing signing algorithms). But I'm pretty confident that the tools cited above are properly working as the signatures can get properly verified on-chain.

Your Environment

cypherhat commented 4 years ago

I will try to take a look at this later this evening.

mortimr commented 4 years ago

Hi @cypherhat, any news on the issue ? Let me know if I can help you with anything

cypherhat commented 4 years ago

I apologize - I have 2 full time commitments... I will definitely try this weekend to evaluate your issue. If you have an implementation suggestion, please suggest. Or submit a PR.

cypherhat commented 4 years ago

I think I should provide a native EIP712 signer method. As I mentioned, I have a heavy workload - I am gonna add this as a feature request. I will make every effort to address this ASAP. Contributions are welcome.

mortimr commented 4 years ago

Hi @cypherhat, I currently have a big task to finish. I’ll look into the issue aswell in ~2-3 days. I think the issue can be solved quite easily by debugging the current “sign” and “verify” method and make sur of their interoperability with other tools. First thing I’ll try is to accept hex encoded arguments for the “sign” method in order to have an encoding format to properly relay binary as hex. I’ll try to debug and compare the results of the keccak256 and ecdsa with other tools and will come back to you then :)

mortimr commented 4 years ago

Hi @cypherhat, the issue came from the keccak256 implementation I was using. The sign route seems to work. But the verify route on the accounts is not working at all. I'll let you look into it.

mortimr commented 4 years ago

Sorry, my bad, the issue came from the hash you are doing without giving the ability to encode. I fixed it locally and will raise a PR with an extra argument encoding on the sign route.

cypherhat commented 4 years ago

Thanks man!

Remember to wash your hands before you submit the PR.

MonarthS commented 4 years ago

@mortimr & @cypherhat any updates on this, as I'm facing same issue.