MyEtherWallet / etherwallet

https://vintage.myetherwallet.com
MIT License
2.34k stars 1.72k forks source link

Enable Parity Recovery #551

Open 5chdn opened 7 years ago

5chdn commented 7 years ago

I see you disabled the parity recovery options. I welcome this step and agree this caused a lot of issues. The parity recovery option is not really to be considered a brain wallet but a recovery option for the initial displayed seedphrase. To circumvent all the recent issues with empty recovery strings and too short brain wallets, you could check if the recovery phrase contains at least 12 words.

Sample phrase generated by parity:

joylessly collide oversleep crummy surgery unworthy mud unfasten outrank coil tropics profound

Please let me know what you think.

ref https://github.com/paritytech/parity/issues/4518

and https://github.com/kvhnuke/etherwallet/issues/289

tayvano commented 7 years ago

I'm not opposed to this. We will see. The main priority was getting it offline ASAP as we had 3 confirmed reports of stolen funds in 12 hours with hundreds of unread emails -- I had no idea how widespread this was.

What I failed to realized is that new users, instead of generating a wallet on the homepage like I stupidly assumed they would do, were simply typing in whatever words they felt like into that box and getting the address and calling it a day.

Or, even when they did generate a new wallet, any time that wallet didn't validate for whatever reason (.docx keystore, a pdf of their paper wallet, extra characters in the private key field) they would type it into Parity and either call it a day, but then not realize that they needed to repeat these steps in order to unlock in in in the future.

And then https://etherscan.io/address/0xd6a755bce1844ad6b7681118b18f54d63e5c13f9 popped up on our radar and it almost certainly is a guy mining low entropy phrases.

Removing was easier than figuring out what UX updates we needed to make and what the underlying issues were.

What you should put on your radar is high n encrypted keys salted with email: https://medium.com/@homakov/why-brainwallet-are-great-for-cryptocurrency-ff73dd65ecd9

5chdn commented 7 years ago

What I failed to realized is that new users, instead of generating a wallet on the homepage like I stupidly assumed they would do, were simply typing in whatever words they felt like into that box and getting the address and calling it a day.

Yeah, we are already preparing the 2nd hotfix in a row for this issue.

Removing was easier than figuring out what UX updates we needed to make and what the underlying issues were.

Just let me know how we can be of any help. Basically only allowing only 12 word phrases from dictionary should be sufficient. Everything else is only used on testnet or development chains and can be ignored for services like MEW.

Thanks.

tayvano commented 7 years ago

We will discuss internally and see thoughts. Is there a word that you are using to describe these besides "Parity Phrases"

One thing I've noticed is that new users don't know what Parity is and therefore that label is meaningless to them. I'd like to match your UI in terms of validation & semantics so we can be consistent, but am struggling (really struggling) to see the world in the eyes of a new user who prefers clicking things over thinking about clicking things. 😩

I'll keep you posted. Thank you.

Dingy123 commented 7 years ago

Will the dist-v3.8.8.zip issue be corrected soon? The page is still unresponsive when trying to recover via parity phrase.

3esmit commented 7 years ago

Please, don't disable insecure from being created/used, it might be useful sometimes. For example I used MEW to faucet from empty parity recovery phrase to a new user in kovan. Also, I am one of the people that use crafted brainwallets and is not happy with "computer generated".

What I do, is an computer assisted phrase generation, I manually try random phrases at live.ether.camp generator until I find something nice, than I change some words to make the phrase make some meaning for "brain anchoring" the phrase, so I never forget it. Also, I might want to do some random changes, such as using symbols, not using spaces (alternating word separators or no separation at-all).

We should have is the option to recover any entropy account, but when a low entropy account is about to be loaded, a big and scary warning saying:

WARNING, INSECURE ACCOUNT! 
THE ENTROPY USED IS TOO LOW!
DO NOT USE THIS ACCOUNT OR YOUR FUNDS WILL BE AT RISK!

A message tip like this sites functionality would be really appreciated: https://howsecureismypassword.net/ https://lastpass.com/howsecure.php

tayvano commented 7 years ago

@3esmit This is how we used to approached it.

Then ~100 ETH were stolen from low entropy accounts and I had to break the new to all those users.

If you want to generate low entropy wallets while understanding the risks, you are free to. You are also free to fork it.

I am depressed and tired with telling people that they lost ETH and simply do not have the time nor energy nor optimism to deal with this at the moment.

If you shoot me an email, I'll give you the snippet to show Parity on the current version of the site.

3esmit commented 7 years ago

@tayvano Thanks, I actually know how to do it in node console. I'm sorry all this happen, but the issue was the miss of warnings. New users actually have no idea what are this options, and will try just the first that works. They assume wrong because there was no warning. You did right removing the option as fast as possible, blocking further errors. Your work is always been amazing and you really care about community <3. I'm just proposing to don't block low entropy, some people might need to use it and know what they are doing. BTW, see this https://etherscan.io/address/0x7E5F4552091A69125d5DfCb7b8C2659029395Bdf -- the private key of this address is 0000000000000000000000000000000000000000000000000000000000000001 which is a low entropy privatekey, this also could be tested by the warning: too repeating bytes. This address recieved some ETH, and I guess probably was someone that did knew what was doing? Yeah, the user must be very dumb to use a low entropy private key, but users are users.

tayvano commented 7 years ago

Yeah there is another one who had PK 00000...

What was interesting about that one is it isn't actually a valid key on the curve so the ETH are inaccessible / burned :(

Users will be users. I really really am trying to figure out how to make this foolproof and secure. It seems that anything that requires a user to save or remember anything isn't going to work out though. >.<

5chdn commented 7 years ago

Please, don't disable insecure from being created/used, it might be useful sometimes. For example I used MEW to faucet from empty parity recovery phrase to a new user in kovan.

Parity wallet still allows you to generate insecure brain wallets for all testnets and development chains. But since MEW mainly operates on main network, it should not!

tayvano commented 7 years ago

We can revisit in a bit and figure out a way to make this work for everyone. The barrier to entry for generating low entropy wallets needs to be higher than it was before, but lower than it is now. I'm already frustrated because we use weak parity phrases all the time for testing stuff. 😬

5chdn commented 7 years ago

We should have is the option to recover any entropy account, but when a low entropy account is about to be loaded, a big and scary warning saying:

I disagree, warnings get largly ignored. I'm still not understanding why you insist to keep this feature in MEW. You could also use parity or any other lib to use your low-entropy accounts for testing.

What I do, is an computer assisted phrase generation

But why do you need MEW to assist you with this? Also, you still got use older versions of MEW to maintain this feature for your private needs.

johnelgithub commented 7 years ago

Hi Tayvano,

I just checked my wallet today and I was surprised that the Parity Phrase was no longer supported since I cannot access the account. I tried to follow the knowledge base but when successfully downloaded the file at https://github.com/kvhnuke/etherwallet/releases/tag/v3.8.7 and clicked the index.html icon, the window that comes up does not have an option to enter my Parity Phrase to proceed with the token transfer.

Please advise on how to proceed with this concern.

Regards,

Johnel

5chdn commented 7 years ago

@johnelgithub works for me, did you download dist-v3.8.7.zip ?

You could also use Parity 0;-)

johnelgithub commented 7 years ago

Hi,

Yes, I downloaded it but the page appears to be unresponsive with the unclickable buttons and no option to input my Parity Phrase.

How to do it with Parity 0?

Thank you for responding.

johnelgithub commented 7 years ago

@5chdn

3 8 7

5chdn commented 7 years ago

@johnelgithub did you extract all files from the zip? seems the javascript is not loading. Does it work with any other browser?

Parity for all platforms is available here: https://github.com/paritytech/parity/releases

tayvano commented 7 years ago

@5chdn Its something @kvhnuke is fixing right now with local storage. No idea why its now just a problem but we should have a fix in a big.

johnelgithub commented 7 years ago

@5chdn

Thanks for the recommendations, I will check on it later. Tried IE and no option to input my Parity Phrase but to generate a new wallet address instead. I hope I can do the workaround here with your assistance. Thanks bigtime.

@kvhnuke and @tayvano thanks for looking into this.

tayvano commented 7 years ago

@johnelgithub Let me know if this works. crosses fingers

https://github.com/kvhnuke/etherwallet/releases/download/v3.9.0/dist-v3.9.0-parityenabled.zip

5chdn commented 7 years ago

Interesting, but this is not yet checking the dictionary, does it?

johnelgithub commented 7 years ago

@tayvano

Tried to download v3.9.0 from the link you provided but the same window will appear. Chrome has the latest version Version 59.0.3071.86 in a Window 32bit PC still got the same page with unresponsive buttons.

On the other hand, the Parity Phrase I chose before came with a Private Key that I kept in my files and when I used it to access my wallet, it worked.

I think I can take it from here. Thank you very much for your assistance.

-Johnel

maciejhirsz commented 7 years ago

Hi @tayvano, is this still up for being restored in MEW?

We've released a wordlist package to handle seed phrases coming from Parity.

git: https://github.com/paritytech/wordlist npm: https://www.npmjs.com/package/@parity/wordlist

In JS you can call verifyPhrase(phrase, 12) as a means to make sure that the provided phrase is indeed created by Parity (is correct length and all words are coming from the dictionary used to create the seed phrases).

5chdn commented 7 years ago

Hey MEW team, I know this is low priority, but could you comment if this will make it into v4? And if not, will you still retain that parity-disclaimer?

gagarin55 commented 6 years ago

Parity should implement BIP39 + BIP32 instead, in this case all users will be happy and can transfer their accs between wallets