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

ERC-681 implementation bug: Wrong value on ERC-20 token transactions #1549

Closed
dwjorgeb opened this issue May 5, 2020 · 8 comments · Fixed by #1708
Closed

ERC-681 implementation bug: Wrong value on ERC-20 token transactions #1549

dwjorgeb opened this issue May 5, 2020 · 8 comments · Fixed by #1708
Labels
type-bug Something isn't working

Comments

@dwjorgeb
Copy link

dwjorgeb commented May 5, 2020

The bug

When parsing ERC-681 encoded URIs or QR Codes for ERC-20 token transactions, the uint256 parameter is parsed wrong.

MetaMask seems to assume the uint256 value is on main token unit, instead of atomic unit.

Example:
ethereum:0xdAC17F958D2ee523a2206206994597C13D831ec7/transfer?address=0x8e23ee67d1332ad560396262c48ffbb01f93d052&uint256=1500000

should be parsed to 1.5 USDT, but instead is parsed as 1.5 million USDT

In order to parse 1.5 USDT the URI needs to be constructed like:

ethereum:0xdAC17F958D2ee523a2206206994597C13D831ec7/transfer?address=0x8e23ee67d1332ad560396262c48ffbb01f93d052&uint256=1.5

Which not only doesn't match what's on the spec (https://github.com/ethereum/EIPs/blob/master/EIPS/eip-681.md)

Requesting payments in ERC #20 tokens involves a request to call the transfer function of the token contract with an address and a uint256 typed parameter, containing the beneficiary address and the amount in atomic units, respectively.

but also makes no sense, since it's supposed to be a unsigned integer, not a decimal value.

To Reproduce

  1. Generate a ERC-20 token QR Code or payment URI (https://brunobar79.github.io/eip681-link-generator/)
  2. Parse it with MetaMask

Expected behavior
Token value parsed in atomic units

Smartphone (please complete the following information):

  • Device: Galaxy S9
  • OS: Android 10
  • Version: 28
@dwjorgeb dwjorgeb added the type-bug Something isn't working label May 5, 2020
@dwjorgeb
Copy link
Author

dwjorgeb commented May 5, 2020

As reported here: ethereum/EIPs#681

@dwjorgeb
Copy link
Author

Any update on this?

@omnat
Copy link
Contributor

omnat commented Jun 18, 2020

We can't reproduce this @dwjorgeb
The link you shared didn't work for me (MetaMask just opens, but doesn't open the link), but if I generate a deeplink with the generator, it worked as expected.

Can you try it with the latest version 0.2.18 and let us know if this is still an issue?

@dwjorgeb
Copy link
Author

hello @omnat

Can confirm this still happens with 0.2.18.

What values did you put on the payment URI?

@omnat
Copy link
Contributor

omnat commented Jun 23, 2020

@dwjorgeb I put 1.5 USDT. The link that was generated also showed it correctly.

@dwjorgeb
Copy link
Author

@dwjorgeb I put 1.5 USDT. The link that was generated also showed it correctly.

exactly, but that's not what it's in the ERC-681 spec.

as per the spec text:

Requesting payments in ERC #20 tokens involves a request to call the transfer function of the token contract with an address and a uint256 typed parameter, containing the beneficiary address and the amount in atomic units, respectively.

atomic units means it can't be divisible, so it needs to take into account the number of decimals of each token. In the case of USDT it has 6 decimals, so 1.5 USDT should be 1500000.

Doesn't even make sense to send a float (1.5), as the variable you're setting is literally called uint256 (unsigned integer 256 bits)

@estebanmino
Copy link
Contributor

hey @dwjorgeb thank you very much for reporting this, pushed a PR to fix it

@dwjorgeb
Copy link
Author

dwjorgeb commented Jul 22, 2020

Great :) I'm glad to help. @estebanmino

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants