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 tests for ERC20 as per pdaian's suggestions #616

Closed
spalladino opened this issue Dec 19, 2017 · 1 comment
Closed

Add tests for ERC20 as per pdaian's suggestions #616

spalladino opened this issue Dec 19, 2017 · 1 comment
Labels
good first issue Low hanging fruit for new contributors to get involved!

Comments

@spalladino
Copy link
Contributor

On his audit for Tether, @pdaian identified some code paths in the ERC20 token contract which did not have explicit unit tests to exercise them.

Add a test case for each of the following:

  • Testing the code path in approve that does not allow non-zero approval with an
    existing non-zero allowance. Furthermore, the documentation does not sufficiently
    warn contract implementers to use the "set to 0, check, then reset" pattern for more
    than one approval operation.
  • transferFrom should also check for the ability to transfer more than balance when
    allowance is correctly set.
  • Tests with zero values are generally missing, though manual inspection shows that
    the class’s guarantees will not be violated with such values. Interestingly, the transferFrom
    and transfer methods impose a payload size restriction representing such
    a check, for which a test is missing. The allowance method does not feature this
    modifier, allowing for the creation of transfer approvals that can never be executed.
    This does not seem to be a substantial security risk in the contract.
  • Tests with zero-length (null) addresses are generally missing, though manual inspection
    shows that the class’s guarantees will not be violated with such addresses.
@spalladino spalladino added the good first issue Low hanging fruit for new contributors to get involved! label Dec 19, 2017
@facuspagnuolo
Copy link
Contributor

Fixed in #686

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Low hanging fruit for new contributors to get involved!
Projects
None yet
Development

No branches or pull requests

3 participants