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

Fix FP in erc20-interface as a result of ERC721 similarities #215

Merged
merged 13 commits into from May 7, 2019

Conversation

Projects
None yet
3 participants
@Xenomega
Copy link
Member

commented Apr 26, 2019

This PR aims to resolve #186 . It checks if any ERC721 functions exist before assuming a contract is implementing ERC20. This is due to the fact that ERC721 has similar definitions to ERC20.

More information about the implementation of this PR can be found in the issue thread.

@Xenomega Xenomega changed the title Fix FP in erc20-interface as a result of ERC721 similarities [WIP] Fix FP in erc20-interface as a result of ERC721 similarities Apr 26, 2019

-Relaxed erc20-interface detector to report incorrect function signat…
…ures even if the function was not declared in that contract immediately

-Created erc721-interface detector, similar to erc20-interface.
@Xenomega

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

As of commit 7238e9b :

  • Added an erc721-interface detector, which is similar to erc20-interface, but checks the properties according to an ERC721 token.
  • erc20-interface now checks return types of more functions than it did before.
  • erc20-interface used to only report poorly formed functions declared in the immediate contract (not inherited). This was relaxed, as parts of the interface might be provided in other base classes which would not pass checks identifying it as an ERC20 token on its own, and these functions would not be reported about. ERC20 function checks are only performed if any of the core ERC20 functions exist (transfer, approve, etc) in order to minimize FP.
    • A good example of this is seen with ERC721: If the sole ERC165 function is provided through a seperate contract which ERC721 depends on, the ERC721 detector would note the ERC721 contract as a contract to check the functions of, but not ERC165. This would cause the ERC165 function checks to be skipped in the detector. The same can happen in ERC20 if parts are provided through different contracts that wouldn't individually meet the criteria to be scanned.

Tests were changed to test every incorrect function signature would be reported. All reported the correct information, and the referenced contracts which affected the issue linked above do not report any false positives anymore.

Other token standards which have been checked:

ERC 223:
- Does not conflict. Backwards compatible with ERC20, returns same values. Will not be identified at ERC721

ERC 777:
- Does not conflict. Partial backwards compatibility with ERC20, returns same values. Will not be identified at ERC721

ERC 1155:
- Will be identified as ERC721, with no difference in return values -> non problematic.

ERC 1400:
- Does not conflict with ERC20 or ERC721. Backwards compatible with ERC20.

ERC 1410: 
- Shares `balanceOf` with ERC20, non problematic.

ERC 1594:
- Does not conflict with ERC20 or ERC721. Backwards compatible with ERC20.

ERC 1643:
- Not related, non problematic.

ERC 1644:
- Does not conflict with ERC20 or ERC721. Backwards compatible with ERC20.

@Xenomega Xenomega changed the title [WIP] Fix FP in erc20-interface as a result of ERC721 similarities Fix FP in erc20-interface as a result of ERC721 similarities Apr 29, 2019

@Xenomega Xenomega requested a review from montyly May 2, 2019

montyly added some commits May 6, 2019

- Improve literals parsing and Constant conversion:
        - add bool type to constant
        - convert integer using 'e' (1e10)
        - convert constant uint -> int when possible
- Api added:
        - Constant.original_value: str version of the constant expression
        - Structure.elems_ordered: keep original structure declaration order
Merge pull request #227 from crytic/dev-fix-types
Improve conversion of Literals

@montyly montyly merged commit f0cb66d into master May 7, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details

@montyly montyly deleted the dev-fp-erc20-interface branch May 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.