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

Valid wallet address not recognized #2339

Closed
noameppel opened this issue Dec 27, 2018 · 7 comments
Closed

Valid wallet address not recognized #2339

noameppel opened this issue Dec 27, 2018 · 7 comments
Labels
type: discussion Items that are primarily a discussion around a feature or issue. May evolve to be actionable.

Comments

@noameppel
Copy link

  1. Visit https://mycrypto.com/account

  2. Click on "View Address". Paste in:
    0x7BdDB2deD86B8e635a179F997BB964285CaD129A

The address is not recognized as valid.

screenshot 2018-12-27 09 51 13

  1. Paste in:
    0x7BdDB2deD86B8e635a179F997BB964285CaD129a
    (Ending with a lower case "a").

The address is recognized as valid.

screenshot 2018-12-27 09 51 18

@blurpesec blurpesec added the type: issue Items that document a problem or bug with an existing feature. label Dec 27, 2018
@blurpesec
Copy link
Collaborator

I think this is probably an issue with the isValidChecksum check that is conducted. Not sure though. Will give it a look. Thanks for the heads up :-)

@tayvano
Copy link
Contributor

tayvano commented Dec 27, 2018

EIP 55 is the one that describes how the backwards-compatible checksums work:

I assume we are using the ethereumjs-utils implementation?

https://github.com/ethereumjs/ethereumjs-util/blob/75f529458bc7dc84f85fd0446d0fac92d991c262/index.js#L473-L475

I think the issue may be that simply making the last letter lowercase doesn't necessarily void the checksum detection. The check verifies that both:

  1. the address is a valid address (it's 40 hex characters)

  2. the address matches what is output by toChecksumAddress().

In this case it doesn't fulfill both requirements so returns false.

@blurpesec
Copy link
Collaborator

blurpesec commented Jan 6, 2019

After looking into this: This definitely has to do with isValidChecksum. While the potential issues associated with invalid checksummed addresses on this view-only section of the app is small, it's probably better left alone as there are only a few small edge-cases where a user wouldn't use an address that is either lowercase or valid checksummed addresses.

The options are now:

  1. Should we leave this as-is so that people are used to using checksummed addresses in their current form?
  2. Or should we introduce the change to allow this, by casting all user-input addresses toLowerCase() (only on the view-only pages).

@blurpesec blurpesec added type: discussion Items that are primarily a discussion around a feature or issue. May evolve to be actionable. and removed type: issue Items that document a problem or bug with an existing feature. labels Jan 6, 2019
@ButteryGuitars
Copy link

Was seeing this pretty frequently with MyCrypto for Win 1.5.3. Haven't seen it in 1.5.6 yet, but also haven't been using it for more than a few days.

@tayvano
Copy link
Contributor

tayvano commented Feb 8, 2019

@ButteryGuitars Where were you getting the addresses from? e.g. an exchange, block explorer, etc?

@ButteryGuitars
Copy link

@ButteryGuitars Where were you getting the addresses from? e.g. an exchange, block explorer, etc?

Behavior doesn't appear to be tied to specific source of address. Exchanges, other ETH wallets under my control.

Deleting the 0 at the beginning of the address and re-adding always fixes.

@wtzb
Copy link
Collaborator

wtzb commented Jan 21, 2020

Thank you for opening this issue. Since this issue applies to an older, no longer supported version of MyCrypto, I'm closing this to keep a better overview of issues that are currently applicable.

If you believe this was in error, or if this issue still applies, please don't hesitate to comment or reach out to us directly. Thanks for helping us and letting us know of any issues you might run into!

@wtzb wtzb closed this as completed Jan 21, 2020
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: discussion Items that are primarily a discussion around a feature or issue. May evolve to be actionable.
Projects
None yet
Development

No branches or pull requests

5 participants