Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NeoLogin to /wallets #843

Merged
merged 3 commits into from
Nov 20, 2019
Merged

Add NeoLogin to /wallets #843

merged 3 commits into from
Nov 20, 2019

Conversation

corollari
Copy link
Contributor

This PR adds the NeoLogin wallet to the list of wallets present in https://neo.org/wallets.

I've tried to remain impartial in my description of the wallet, especially when exposing it's drawbacks (like the fact it's a light node, has worse security compared to wallets that only store keys locally, has a central server that stores encrypted keys and requires users to pay close attention to having strong enough passwords) but I'm the creator of this wallet, so I'm quite biased.

Any corrections are welcome!

@longfeiWan9
Copy link
Member

We have tried the wallets. It brakes when we send NEP-5 asset to the wallet.

@chenquanyu
Copy link

My wallet address is ANXHpL2TDJE6dhXd6RvyfTbTfwcWkMUesC
The NEO and GAS balance shows correctly before, but the page became blank after I transferred 2 NEOFISH to the address. And after I transfer the NEOFISH back, the page is still blank.

image

@corollari
Copy link
Contributor Author

That is definitely a bug that needs to be fixed, we'll look into it and get back

@corollari
Copy link
Contributor Author

I just fixed it, the bug was caused by some code that tried to render a row for every asset that the account had, getting the asset's icons from a dictionary that had the following shape:

ASSETS = {
    'NEO': {},
    'GAS': {}
}

Then when it tried to render a row for neofish it broke because the dictionary query failed.

The reason it did not work when you removed the neofish from the wallet is because neoscan still takes them into account when reporting them via the API and the explorer, see https://neoscan.io/address/ANXHpL2TDJE6dhXd6RvyfTbTfwcWkMUesC

@chenquanyu
Copy link

I have found my private key stored in Local Storage without encryption. It seems not safe.
image

@chenzhitong
Copy link
Member

image

@corollari
Copy link
Contributor Author

We do store plaintext private keys on localStorage when the user requests to store the keys locally on registration or ticks "remember me", so it's not the primary mode of operation (we do not do this by default, only after the user performs specific actions requesting it).

We could encrypt the keys stored on localStorage, but from our point of view this doesn't seem to cause a significant difference in the security model:

  • If we get hacked or turn malicious and modify the webpage in order to take the keys and send them to our servers then this code getting triggered would require the user to use our wallet. Now, due to the fact that the keys are not encrypted any use of the wallet would trigger the key stealing code when if we were to encrypt the keys then the user would need to perform an action with their wallet in order to decrypt the keys. But if we consider the possibility that when people open their wallet they usually have a specific action they wish to perform, then for all this people the encryption would make no difference. Furthermore there's the possibility that the attackers uses some social engineering tactics and requests every user to input their password ("posting some message about upgrading to a new version for example"), at this point for all the users that fall for this (which i believe would be a sizable majority given that the they trust our wallet, otherwise they would not be using it) would have made no difference.
  • If an attacker gains access to the host system then it would be possible for him to install keyloggers that captured the user's password and decrypted the stored keys. So the only difference will be that the attacker would get the keys at a later date instead of immediately. There's also the possibility that the user never uses the wallet again, keeping the keys safe, but this should be extremely uncommon.
  • We've also taken steps to ensure that attacks cannot be performed on individuals but they have to be performed on every single user and are thus easily detectable. We do this by using github for our code hosting and a DNS providers that are well known and don't offer the possibility of serving specific DNS records to specific users. To get around this an attacker would need to hack either github or our DNS providers, and if they manage to do either of these there are much much juicier targets for them to attack.

Also please note that we are not posing this wallet as a replacement for other wallets, we tell everyone that this wallet is less secure compared to other wallets such as Neon or O3 and that it should only be used for small quantities of money. We've been very clear that this wallet trades security for convenience as a design decision and therefore should not be used if you plan to just store huge amounts of neo, but should be used for small amounts that you just plan to use on dApps.

We've also discussed the possibility of forcing users to move to more secure wallets if they ever end up with a significant amount of tokens in our wallet, and might implement this in the future.

@chenquanyu
Copy link

Hi Albert
We have some basic requirements for the wallet:

  1. The wallet should have issues feedback link;
  2. The wallet should support English and Chinese at least.

@corollari
Copy link
Contributor Author

@chenquanyu That's fair, by an issues feedback link do you mean displaying a link to our support tickets? In such case, is a link to the issues tab of our github repo fine?

@chenquanyu
Copy link

Yes, it's fine👍

@corollari
Copy link
Contributor Author

Some updates:

  • We have already added the issues button on both our widget and wallet page, it's in the PR that adds fiat gateways (waiting for that to be finished to push it live)
  • We'll be removing plaintext private keys on localStorage, we'll allow people to store encrypted private keys in case they don't want to send those to our servers and we will maintain the rememberMe functionality by using the BroadcastChannel standard and sharing information between tabs (it will not persist between browser sessions tho).
  • Working on the translation.

@corollari
Copy link
Contributor Author

Translated everything and added "report issue" buttons, it's all live now.

@chenzhitong chenzhitong merged commit dd74456 into neo-project:master Nov 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants