From 8d2a2ce09c33f75b3262184e11ce3c82e6d6fb60 Mon Sep 17 00:00:00 2001 From: David Pokora Date: Fri, 26 Apr 2019 16:43:17 -0400 Subject: [PATCH 1/7] Fix false positives as a result of ERC721 interfaces appearing similar to ERC20. --- slither/core/declarations/contract.py | 2 +- slither/detectors/erc20/incorrect_interface.py | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index 787172843..8a3fcbca9 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -535,7 +535,7 @@ class Contract(ChildSlither, SourceMapping): Returns: bool """ - full_names = [f.full_name for f in self.functions] + full_names = set([f.full_name for f in self.functions]) return 'transfer(address,uint256)' in full_names and\ 'transferFrom(address,address,uint256)' in full_names and\ 'approve(address,uint256)' in full_names diff --git a/slither/detectors/erc20/incorrect_interface.py b/slither/detectors/erc20/incorrect_interface.py index 94dc6f15b..ec7d308e2 100644 --- a/slither/detectors/erc20/incorrect_interface.py +++ b/slither/detectors/erc20/incorrect_interface.py @@ -52,6 +52,19 @@ contract Token{ Returns: list(str) : list of incorrect function signatures """ + # Obtain all function names for this contract + full_names = set([f.full_name for f in contract.functions]) + + # If this contract implements a function from ERC721, we can assume it is an ERC721 token. These tokens + # offer functions which are similar to ERC20, but are not compatible. + if ('ownerOf(uint256)' in full_names or + 'safeTransferFrom(address,address,uint256,bytes)' in full_names or + 'safeTransferFrom(address,address,uint256)' in full_names or + 'setApprovalForAll(address,bool)' in full_names or + 'getApproved(uint256)' in full_names or + 'isApprovedForAll(address,address)' in full_names): + return [] + functions = [f for f in contract.functions if f.contract == contract and \ IncorrectERC20InterfaceDetection.incorrect_erc20_interface(f.signature)] return functions From 7238e9b0337aa002b348d2837a6a2eb93d0be74b Mon Sep 17 00:00:00 2001 From: David Pokora Date: Fri, 26 Apr 2019 20:27:03 -0400 Subject: [PATCH 2/7] -Relaxed erc20-interface detector to report incorrect function signatures even if the function was not declared in that contract immediately -Created erc721-interface detector, similar to erc20-interface. --- slither/detectors/all_detectors.py | 5 +- slither/detectors/{erc20 => erc}/__init__.py | 0 .../incorrect_erc20_interface.py} | 44 +++++-- .../erc/incorrect_erc721_interface.py | 113 ++++++++++++++++++ .../unindexed_event_parameters.py | 0 tests/incorrect_erc20_interface.sol | 7 +- tests/incorrect_erc721_interface.sol | 16 +++ 7 files changed, 168 insertions(+), 17 deletions(-) rename slither/detectors/{erc20 => erc}/__init__.py (100%) rename slither/detectors/{erc20/incorrect_interface.py => erc/incorrect_erc20_interface.py} (62%) create mode 100644 slither/detectors/erc/incorrect_erc721_interface.py rename slither/detectors/{erc20 => erc}/unindexed_event_parameters.py (100%) create mode 100644 tests/incorrect_erc721_interface.sol diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 36c35496c..f0382516d 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -28,8 +28,9 @@ from .shadowing.builtin_symbols import BuiltinSymbolShadowing from .operations.block_timestamp import Timestamp from .statements.calls_in_loop import MultipleCallsInLoop from .statements.incorrect_strict_equality import IncorrectStrictEquality -from .erc20.incorrect_interface import IncorrectERC20InterfaceDetection -from .erc20.unindexed_event_parameters import UnindexedERC20EventParameters +from .erc.incorrect_erc20_interface import IncorrectERC20InterfaceDetection +from .erc.incorrect_erc721_interface import IncorrectERC721InterfaceDetection +from .erc.unindexed_event_parameters import UnindexedERC20EventParameters from .statements.deprecated_calls import DeprecatedStandards from .source.rtlo import RightToLeftOverride # diff --git a/slither/detectors/erc20/__init__.py b/slither/detectors/erc/__init__.py similarity index 100% rename from slither/detectors/erc20/__init__.py rename to slither/detectors/erc/__init__.py diff --git a/slither/detectors/erc20/incorrect_interface.py b/slither/detectors/erc/incorrect_erc20_interface.py similarity index 62% rename from slither/detectors/erc20/incorrect_interface.py rename to slither/detectors/erc/incorrect_erc20_interface.py index ec7d308e2..2dd4ad3e0 100644 --- a/slither/detectors/erc20/incorrect_interface.py +++ b/slither/detectors/erc/incorrect_erc20_interface.py @@ -5,6 +5,18 @@ Some contracts do not return a bool on transfer/transferFrom/approve, which may from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +def is_possible_erc20(contract): + """ + Checks if the provided contract could be attempting to implement ERC20 standards. + :param contract: The contract to check for token compatibility. + :return: Returns a boolean indicating if the provided contract met the token standard. + """ + full_names = set([f.full_name for f in contract.functions]) + return 'transfer(address,uint256)' in full_names or \ + 'transferFrom(address,address,uint256)' in full_names or \ + 'approve(address,uint256)' in full_names + + class IncorrectERC20InterfaceDetection(AbstractDetector): """ Incorrect ERC20 Interface @@ -18,7 +30,7 @@ class IncorrectERC20InterfaceDetection(AbstractDetector): WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-erc20-interface' WIKI_TITLE = 'Incorrect erc20 interface' - WIKI_DESCRIPTION = 'Lack of return value for the ERC20 `approve`/`transfer`/`transferFrom` functions. A contract compiled with solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing.' + WIKI_DESCRIPTION = 'Incorrect return values for ERC20 functions. A contract compiled with solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing.' WIKI_EXPLOIT_SCENARIO = ''' ```solidity contract Token{ @@ -28,7 +40,7 @@ contract Token{ ``` `Token.transfer` does not return a boolean. Bob deploys the token. Alice creates a contract that interacts with it but assumes a correct ERC20 interface implementation. Alice's contract is unable to interact with Bob's contract.''' - WIKI_RECOMMENDATION = 'Return a boolean for the `approve`/`transfer`/`transferFrom` functions.' + WIKI_RECOMMENDATION = 'Set the appropriate return values and value-types for the defined ERC20 functions.' @staticmethod def incorrect_erc20_interface(signature): @@ -43,6 +55,15 @@ contract Token{ if name == 'approve' and parameters == ['address', 'uint256'] and returnVars != ['bool']: return True + if name == 'allowance' and parameters == ['address', 'address'] and returnVars != ['uint256']: + return True + + if name == 'balanceOf' and parameters == ['address'] and returnVars != ['uint256']: + return True + + if name == 'totalSupply' and parameters == [] and returnVars != ['uint256']: + return True + return False @staticmethod @@ -52,28 +73,25 @@ contract Token{ Returns: list(str) : list of incorrect function signatures """ - # Obtain all function names for this contract - full_names = set([f.full_name for f in contract.functions]) + # Verify this is an ERC20 contract. + if not is_possible_erc20(contract): + return [] # If this contract implements a function from ERC721, we can assume it is an ERC721 token. These tokens # offer functions which are similar to ERC20, but are not compatible. - if ('ownerOf(uint256)' in full_names or - 'safeTransferFrom(address,address,uint256,bytes)' in full_names or - 'safeTransferFrom(address,address,uint256)' in full_names or - 'setApprovalForAll(address,bool)' in full_names or - 'getApproved(uint256)' in full_names or - 'isApprovedForAll(address,address)' in full_names): + # NOTE: This detector is dependent on this one, so we import here to avoid errors. + from slither.detectors.erc.incorrect_erc721_interface import is_possible_erc721 + if is_possible_erc721(contract): return [] - functions = [f for f in contract.functions if f.contract == contract and \ - IncorrectERC20InterfaceDetection.incorrect_erc20_interface(f.signature)] + functions = [f for f in contract.functions if IncorrectERC20InterfaceDetection.incorrect_erc20_interface(f.signature)] return functions def _detect(self): """ Detect incorrect erc20 interface Returns: - dict: [contrat name] = set(str) events + dict: [contract name] = set(str) events """ results = [] for c in self.contracts: diff --git a/slither/detectors/erc/incorrect_erc721_interface.py b/slither/detectors/erc/incorrect_erc721_interface.py new file mode 100644 index 000000000..900666674 --- /dev/null +++ b/slither/detectors/erc/incorrect_erc721_interface.py @@ -0,0 +1,113 @@ +""" +Detect incorrect erc721 interface. +""" +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.detectors.erc.incorrect_erc20_interface import is_possible_erc20 + + +def is_possible_erc721(contract): + """ + Checks if the provided contract could be attempting to implement ERC721 standards. + :param contract: The contract to check for token compatibility. + :return: Returns a boolean indicating if the provided contract met the token standard. + """ + full_names = set([f.full_name for f in contract.functions]) + return is_possible_erc20(contract) and \ + ('ownerOf(uint256)' in full_names or + 'safeTransferFrom(address,address,uint256,bytes)' in full_names or + 'safeTransferFrom(address,address,uint256)' in full_names or + 'setApprovalForAll(address,bool)' in full_names or + 'getApproved(uint256)' in full_names or + 'isApprovedForAll(address,address)' in full_names) + + +class IncorrectERC721InterfaceDetection(AbstractDetector): + """ + Incorrect ERC721 Interface + """ + + ARGUMENT = 'erc721-interface' + HELP = 'Incorrect ERC721 interfaces' + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.HIGH + + WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-erc721-interface' + + WIKI_TITLE = 'Incorrect erc721 interface' + WIKI_DESCRIPTION = 'Incorrect return values for ERC721 functions. A contract compiled with solidity > 0.4.22 interacting with these functions will fail to execute them, as the return value is missing.' + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Token{ + function ownerOf(uint256 _tokenId) external view returns (bool); + //... +} +``` +`Token.ownerOf` does not return an address as ERC721 expects. Bob deploys the token. Alice creates a contract that interacts with it but assumes a correct ERC721 interface implementation. Alice's contract is unable to interact with Bob's contract.''' + + WIKI_RECOMMENDATION = 'Set the appropriate return values and value-types for the defined ERC721 functions.' + + @staticmethod + def incorrect_erc721_interface(signature): + (name, parameters, returnVars) = signature + + # ERC721 + if name == 'balanceOf' and parameters == ['address'] and returnVars != ['uint256']: + return True + if name == 'ownerOf' and parameters == ['uint256'] and returnVars != ['address']: + return True + if name == 'safeTransferFrom' and parameters == ['address', 'address', 'uint256', 'bytes'] and returnVars != []: + return True + if name == 'safeTransferFrom' and parameters == ['address', 'address', 'uint256'] and returnVars != []: + return True + if name == 'transferFrom' and parameters == ['address', 'address', 'uint256'] and returnVars != []: + return True + if name == 'approve' and parameters == ['address', 'uint256'] and returnVars != []: + return True + if name == 'setApprovalForAll' and parameters == ['address', 'bool'] and returnVars != []: + return True + if name == 'getApproved' and parameters == ['uint256'] and returnVars != ['address']: + return True + if name == 'isApprovedForAll' and parameters == ['address', 'address'] and returnVars != ['bool']: + return True + + # ERC165 (dependency) + if name == 'supportsInterface' and parameters == ['bytes4'] and returnVars != ['bool']: + return True + + return False + + @staticmethod + def detect_incorrect_erc721_interface(contract): + """ Detect incorrect ERC721 interface + + Returns: + list(str) : list of incorrect function signatures + """ + + # Verify this is an ERC721 contract. + if not is_possible_erc721(contract) or not is_possible_erc20(contract): + return [] + + functions = [f for f in contract.functions if IncorrectERC721InterfaceDetection.incorrect_erc721_interface(f.signature)] + return functions + + def _detect(self): + """ Detect incorrect erc721 interface + + Returns: + dict: [contract name] = set(str) events + """ + results = [] + for c in self.contracts: + functions = IncorrectERC721InterfaceDetection.detect_incorrect_erc721_interface(c) + if functions: + info = "{} ({}) has incorrect ERC721 function interface(s):\n" + info = info.format(c.name, + c.source_mapping_str) + for function in functions: + info += "\t-{} ({})\n".format(function.name, function.source_mapping_str) + json = self.generate_json_result(info) + self.add_functions_to_json(functions, json) + results.append(json) + + return results diff --git a/slither/detectors/erc20/unindexed_event_parameters.py b/slither/detectors/erc/unindexed_event_parameters.py similarity index 100% rename from slither/detectors/erc20/unindexed_event_parameters.py rename to slither/detectors/erc/unindexed_event_parameters.py diff --git a/tests/incorrect_erc20_interface.sol b/tests/incorrect_erc20_interface.sol index e082b77ef..6fdcb71a2 100644 --- a/tests/incorrect_erc20_interface.sol +++ b/tests/incorrect_erc20_interface.sol @@ -1,7 +1,10 @@ pragma solidity ^0.4.24; contract Token{ - function transfer(address to, uint value) external; - + function approve(address spender, uint value) external; + function transferFrom(address from, address to, uint value) external; + function totalSupply() external; + function balanceOf(address who) external; + function allowance(address owner, address spender) external; } diff --git a/tests/incorrect_erc721_interface.sol b/tests/incorrect_erc721_interface.sol new file mode 100644 index 000000000..a8e3e607b --- /dev/null +++ b/tests/incorrect_erc721_interface.sol @@ -0,0 +1,16 @@ +pragma solidity ^0.4.24; + +interface IERC165 { + function supportsInterface(bytes4 interfaceID) external; +} +contract Token is IERC165{ + function balanceOf(address _owner) external; + function ownerOf(uint256 _tokenId) external; + function safeTransferFrom(address _from, address _to, uint256 _tokenId, bytes data) external returns (bool); + function safeTransferFrom(address _from, address _to, uint256 _tokenId) external returns (bool); + function transferFrom(address _from, address _to, uint256 _tokenId) external returns (bool); + function approve(address _approved, uint256 _tokenId) external returns (bool); + function setApprovalForAll(address _operator, bool _approved) external returns (bool); + function getApproved(uint256 _tokenId) external; + function isApprovedForAll(address _owner, address _operator) external; +} From 5e1eb423113d5524ad044f0b59e82dd700fa2593 Mon Sep 17 00:00:00 2001 From: Josselin Date: Mon, 6 May 2019 17:53:51 +0100 Subject: [PATCH 3/7] ERC20/721 detector: move is_possible_* function to contract.has_an_erc*_function Add contract.is_erc721 --- scripts/tests_generate_expected_json_4.sh | 1 + scripts/travis_test_4.sh | 1 + slither/core/declarations/contract.py | 46 ++++ .../erc/incorrect_erc20_interface.py | 18 +- .../erc/incorrect_erc721_interface.py | 19 +- ...rrect_erc20_interface.erc20-interface.json | 218 +++++++++++++++++- ...orrect_erc20_interface.erc20-interface.txt | 9 +- 7 files changed, 271 insertions(+), 41 deletions(-) diff --git a/scripts/tests_generate_expected_json_4.sh b/scripts/tests_generate_expected_json_4.sh index e69148347..184c199f1 100755 --- a/scripts/tests_generate_expected_json_4.sh +++ b/scripts/tests_generate_expected_json_4.sh @@ -23,6 +23,7 @@ generate_expected_json(){ #generate_expected_json tests/deprecated_calls.sol "deprecated-standards" #generate_expected_json tests/erc20_indexed.sol "erc20-indexed" #generate_expected_json tests/incorrect_erc20_interface.sol "erc20-interface" +#generate_expected_json tests/incorrect_erc721_interface.sol "erc721-interface" #generate_expected_json tests/uninitialized.sol "uninitialized-state" #generate_expected_json tests/backdoor.sol "backdoor" #generate_expected_json tests/backdoor.sol "suicidal" diff --git a/scripts/travis_test_4.sh b/scripts/travis_test_4.sh index 5150a92f6..171079d9e 100755 --- a/scripts/travis_test_4.sh +++ b/scripts/travis_test_4.sh @@ -72,6 +72,7 @@ test_slither(){ test_slither tests/deprecated_calls.sol "deprecated-standards" test_slither tests/erc20_indexed.sol "erc20-indexed" test_slither tests/incorrect_erc20_interface.sol "erc20-interface" +test_slither tests/incorrect_erc721_interface.sol "erc721-interface" test_slither tests/uninitialized.sol "uninitialized-state" test_slither tests/backdoor.sol "backdoor" test_slither tests/backdoor.sol "suicidal" diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index 8a3fcbca9..f53e2a717 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -527,6 +527,14 @@ class Contract(ChildSlither, SourceMapping): """ return all((not f.is_implemented) for f in self.functions) + # endregion + ################################################################################### + ################################################################################### + # region ERC conformance + ################################################################################### + ################################################################################### + + def is_erc20(self): """ Check if the contract is an erc20 token @@ -540,6 +548,44 @@ class Contract(ChildSlither, SourceMapping): 'transferFrom(address,address,uint256)' in full_names and\ 'approve(address,uint256)' in full_names + def is_erc721(self): + full_names = set([f.full_name for f in self.functions]) + return self.is_erc20() and\ + 'ownerOf(uint256)' in full_names and\ + 'safeTransferFrom(address,address,uint256,bytes)' in full_names and\ + 'safeTransferFrom(address,address,uint256)' in full_names and\ + 'setApprovalForAll(address,bool)' in full_names and\ + 'getApproved(uint256)' in full_names and\ + 'isApprovedForAll(address,address)' in full_names + + def has_an_erc20_function(self): + """ + Checks if the provided contract could be attempting to implement ERC20 standards. + :param contract: The contract to check for token compatibility. + :return: Returns a boolean indicating if the provided contract met the token standard. + """ + full_names = set([f.full_name for f in self.functions]) + return 'transfer(address,uint256)' in full_names or \ + 'transferFrom(address,address,uint256)' in full_names or \ + 'approve(address,uint256)' in full_names + + def has_an_erc721_function(self): + """ + Checks if the provided contract could be attempting to implement ERC721 standards. + :param contract: The contract to check for token compatibility. + :return: Returns a boolean indicating if the provided contract met the token standard. + """ + full_names = set([f.full_name for f in self.functions]) + return self.has_an_erc20_function() and \ + ('ownerOf(uint256)' in full_names or + 'safeTransferFrom(address,address,uint256,bytes)' in full_names or + 'safeTransferFrom(address,address,uint256)' in full_names or + 'setApprovalForAll(address,bool)' in full_names or + 'getApproved(uint256)' in full_names or + 'isApprovedForAll(address,address)' in full_names) + + + # endregion ################################################################################### ################################################################################### diff --git a/slither/detectors/erc/incorrect_erc20_interface.py b/slither/detectors/erc/incorrect_erc20_interface.py index 2dd4ad3e0..ab577fbab 100644 --- a/slither/detectors/erc/incorrect_erc20_interface.py +++ b/slither/detectors/erc/incorrect_erc20_interface.py @@ -5,18 +5,6 @@ Some contracts do not return a bool on transfer/transferFrom/approve, which may from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification -def is_possible_erc20(contract): - """ - Checks if the provided contract could be attempting to implement ERC20 standards. - :param contract: The contract to check for token compatibility. - :return: Returns a boolean indicating if the provided contract met the token standard. - """ - full_names = set([f.full_name for f in contract.functions]) - return 'transfer(address,uint256)' in full_names or \ - 'transferFrom(address,address,uint256)' in full_names or \ - 'approve(address,uint256)' in full_names - - class IncorrectERC20InterfaceDetection(AbstractDetector): """ Incorrect ERC20 Interface @@ -74,14 +62,12 @@ contract Token{ list(str) : list of incorrect function signatures """ # Verify this is an ERC20 contract. - if not is_possible_erc20(contract): + if not contract.has_an_erc20_function(): return [] # If this contract implements a function from ERC721, we can assume it is an ERC721 token. These tokens # offer functions which are similar to ERC20, but are not compatible. - # NOTE: This detector is dependent on this one, so we import here to avoid errors. - from slither.detectors.erc.incorrect_erc721_interface import is_possible_erc721 - if is_possible_erc721(contract): + if contract.has_an_erc721_function(): return [] functions = [f for f in contract.functions if IncorrectERC20InterfaceDetection.incorrect_erc20_interface(f.signature)] diff --git a/slither/detectors/erc/incorrect_erc721_interface.py b/slither/detectors/erc/incorrect_erc721_interface.py index 900666674..5ddea37ff 100644 --- a/slither/detectors/erc/incorrect_erc721_interface.py +++ b/slither/detectors/erc/incorrect_erc721_interface.py @@ -2,23 +2,6 @@ Detect incorrect erc721 interface. """ from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification -from slither.detectors.erc.incorrect_erc20_interface import is_possible_erc20 - - -def is_possible_erc721(contract): - """ - Checks if the provided contract could be attempting to implement ERC721 standards. - :param contract: The contract to check for token compatibility. - :return: Returns a boolean indicating if the provided contract met the token standard. - """ - full_names = set([f.full_name for f in contract.functions]) - return is_possible_erc20(contract) and \ - ('ownerOf(uint256)' in full_names or - 'safeTransferFrom(address,address,uint256,bytes)' in full_names or - 'safeTransferFrom(address,address,uint256)' in full_names or - 'setApprovalForAll(address,bool)' in full_names or - 'getApproved(uint256)' in full_names or - 'isApprovedForAll(address,address)' in full_names) class IncorrectERC721InterfaceDetection(AbstractDetector): @@ -85,7 +68,7 @@ contract Token{ """ # Verify this is an ERC721 contract. - if not is_possible_erc721(contract) or not is_possible_erc20(contract): + if not contract.has_an_erc721_function() or not contract.has_an_erc20_function(): return [] functions = [f for f in contract.functions if IncorrectERC721InterfaceDetection.incorrect_erc721_interface(f.signature)] diff --git a/tests/expected_json/incorrect_erc20_interface.erc20-interface.json b/tests/expected_json/incorrect_erc20_interface.erc20-interface.json index 180113da4..dceec9be7 100644 --- a/tests/expected_json/incorrect_erc20_interface.erc20-interface.json +++ b/tests/expected_json/incorrect_erc20_interface.erc20-interface.json @@ -3,20 +3,184 @@ "check": "erc20-interface", "impact": "Medium", "confidence": "High", - "description": "Token (tests/incorrect_erc20_interface.sol#3-7) has incorrect ERC20 function interface(s):\n\t-transfer (tests/incorrect_erc20_interface.sol#5)\n", + "description": "Token (tests/incorrect_erc20_interface.sol#3-10) has incorrect ERC20 function interface(s):\n\t-transfer (tests/incorrect_erc20_interface.sol#4)\n\t-approve (tests/incorrect_erc20_interface.sol#5)\n\t-transferFrom (tests/incorrect_erc20_interface.sol#6)\n\t-totalSupply (tests/incorrect_erc20_interface.sol#7)\n\t-balanceOf (tests/incorrect_erc20_interface.sol#8)\n\t-allowance (tests/incorrect_erc20_interface.sol#9)\n", "elements": [ + { + "type": "function", + "name": "allowance", + "source_mapping": { + "start": 319, + "length": 60, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_relative": "tests/incorrect_erc20_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_short": "tests/incorrect_erc20_interface.sol", + "lines": [ + 9 + ], + "starting_column": 5, + "ending_column": 65 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 26, + "length": 355, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_relative": "tests/incorrect_erc20_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_short": "tests/incorrect_erc20_interface.sol", + "lines": [ + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "approve", + "source_mapping": { + "start": 102, + "length": 55, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_relative": "tests/incorrect_erc20_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_short": "tests/incorrect_erc20_interface.sol", + "lines": [ + 5 + ], + "starting_column": 5, + "ending_column": 60 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 26, + "length": 355, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_relative": "tests/incorrect_erc20_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_short": "tests/incorrect_erc20_interface.sol", + "lines": [ + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "balanceOf", + "source_mapping": { + "start": 273, + "length": 41, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_relative": "tests/incorrect_erc20_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_short": "tests/incorrect_erc20_interface.sol", + "lines": [ + 8 + ], + "starting_column": 5, + "ending_column": 46 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 26, + "length": 355, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_relative": "tests/incorrect_erc20_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_short": "tests/incorrect_erc20_interface.sol", + "lines": [ + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "totalSupply", + "source_mapping": { + "start": 236, + "length": 32, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_relative": "tests/incorrect_erc20_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_short": "tests/incorrect_erc20_interface.sol", + "lines": [ + 7 + ], + "starting_column": 5, + "ending_column": 37 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 26, + "length": 355, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_relative": "tests/incorrect_erc20_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_short": "tests/incorrect_erc20_interface.sol", + "lines": [ + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, { "type": "function", "name": "transfer", "source_mapping": { - "start": 47, + "start": 46, "length": 51, "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", "filename_relative": "tests/incorrect_erc20_interface.sol", "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", "filename_short": "tests/incorrect_erc20_interface.sol", "lines": [ - 5 + 4 ], "starting_column": 5, "ending_column": 56 @@ -26,7 +190,48 @@ "name": "Token", "source_mapping": { "start": 26, - "length": 75, + "length": 355, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_relative": "tests/incorrect_erc20_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_short": "tests/incorrect_erc20_interface.sol", + "lines": [ + 3, + 4, + 5, + 6, + 7, + 8, + 9, + 10 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "transferFrom", + "source_mapping": { + "start": 162, + "length": 69, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_relative": "tests/incorrect_erc20_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", + "filename_short": "tests/incorrect_erc20_interface.sol", + "lines": [ + 6 + ], + "starting_column": 5, + "ending_column": 74 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 26, + "length": 355, "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", "filename_relative": "tests/incorrect_erc20_interface.sol", "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc20_interface.sol", @@ -36,7 +241,10 @@ 4, 5, 6, - 7 + 7, + 8, + 9, + 10 ], "starting_column": 1, "ending_column": 2 diff --git a/tests/expected_json/incorrect_erc20_interface.erc20-interface.txt b/tests/expected_json/incorrect_erc20_interface.erc20-interface.txt index 4e9c44437..acff5f223 100644 --- a/tests/expected_json/incorrect_erc20_interface.erc20-interface.txt +++ b/tests/expected_json/incorrect_erc20_interface.erc20-interface.txt @@ -1,5 +1,10 @@ INFO:Detectors: -Token (tests/incorrect_erc20_interface.sol#3-7) has incorrect ERC20 function interface(s): - -transfer (tests/incorrect_erc20_interface.sol#5) +Token (tests/incorrect_erc20_interface.sol#3-10) has incorrect ERC20 function interface(s): + -transfer (tests/incorrect_erc20_interface.sol#4) + -approve (tests/incorrect_erc20_interface.sol#5) + -transferFrom (tests/incorrect_erc20_interface.sol#6) + -totalSupply (tests/incorrect_erc20_interface.sol#7) + -balanceOf (tests/incorrect_erc20_interface.sol#8) + -allowance (tests/incorrect_erc20_interface.sol#9) Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-erc20-interface INFO:Slither:tests/incorrect_erc20_interface.sol analyzed (1 contracts), 1 result(s) found From 1d4d681e2c90b6e4020e341f5024b10818196d24 Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 7 May 2019 09:33:42 +0100 Subject: [PATCH 4/7] Add missing files --- ...ect_erc721_interface.erc721-interface.json | 442 ++++++++++++++++++ ...rect_erc721_interface.erc721-interface.txt | 14 + 2 files changed, 456 insertions(+) create mode 100644 tests/expected_json/incorrect_erc721_interface.erc721-interface.json create mode 100644 tests/expected_json/incorrect_erc721_interface.erc721-interface.txt diff --git a/tests/expected_json/incorrect_erc721_interface.erc721-interface.json b/tests/expected_json/incorrect_erc721_interface.erc721-interface.json new file mode 100644 index 000000000..9f48193b7 --- /dev/null +++ b/tests/expected_json/incorrect_erc721_interface.erc721-interface.json @@ -0,0 +1,442 @@ +[ + { + "check": "erc721-interface", + "impact": "Medium", + "confidence": "High", + "description": "Token (tests/incorrect_erc721_interface.sol#6-16) has incorrect ERC721 function interface(s):\n\t-supportsInterface (tests/incorrect_erc721_interface.sol#4)\n\t-balanceOf (tests/incorrect_erc721_interface.sol#7)\n\t-ownerOf (tests/incorrect_erc721_interface.sol#8)\n\t-safeTransferFrom (tests/incorrect_erc721_interface.sol#9)\n\t-safeTransferFrom (tests/incorrect_erc721_interface.sol#10)\n\t-transferFrom (tests/incorrect_erc721_interface.sol#11)\n\t-approve (tests/incorrect_erc721_interface.sol#12)\n\t-setApprovalForAll (tests/incorrect_erc721_interface.sol#13)\n\t-getApproved (tests/incorrect_erc721_interface.sol#14)\n\t-isApprovedForAll (tests/incorrect_erc721_interface.sol#15)\n", + "elements": [ + { + "type": "function", + "name": "approve", + "source_mapping": { + "start": 549, + "length": 78, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 12 + ], + "starting_column": 5, + "ending_column": 83 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 109, + "length": 739, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "balanceOf", + "source_mapping": { + "start": 140, + "length": 44, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 7 + ], + "starting_column": 5, + "ending_column": 49 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 109, + "length": 739, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "getApproved", + "source_mapping": { + "start": 723, + "length": 48, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 14 + ], + "starting_column": 5, + "ending_column": 53 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 109, + "length": 739, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "isApprovedForAll", + "source_mapping": { + "start": 776, + "length": 70, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 15 + ], + "starting_column": 5, + "ending_column": 75 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 109, + "length": 739, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "ownerOf", + "source_mapping": { + "start": 189, + "length": 44, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 8 + ], + "starting_column": 5, + "ending_column": 49 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 109, + "length": 739, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "safeTransferFrom", + "source_mapping": { + "start": 238, + "length": 108, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 9 + ], + "starting_column": 5, + "ending_column": 113 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 109, + "length": 739, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "safeTransferFrom", + "source_mapping": { + "start": 351, + "length": 96, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 10 + ], + "starting_column": 5, + "ending_column": 101 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 109, + "length": 739, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "setApprovalForAll", + "source_mapping": { + "start": 632, + "length": 86, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 13 + ], + "starting_column": 5, + "ending_column": 91 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 109, + "length": 739, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "supportsInterface", + "source_mapping": { + "start": 50, + "length": 56, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 4 + ], + "starting_column": 5, + "ending_column": 61 + }, + "contract": { + "type": "contract", + "name": "IERC165", + "source_mapping": { + "start": 26, + "length": 82, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 3, + 4, + 5 + ], + "starting_column": 1, + "ending_column": 2 + } + } + }, + { + "type": "function", + "name": "transferFrom", + "source_mapping": { + "start": 452, + "length": 92, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 11 + ], + "starting_column": 5, + "ending_column": 97 + }, + "contract": { + "type": "contract", + "name": "Token", + "source_mapping": { + "start": 109, + "length": 739, + "filename_used": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_relative": "tests/incorrect_erc721_interface.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/incorrect_erc721_interface.sol", + "filename_short": "tests/incorrect_erc721_interface.sol", + "lines": [ + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16 + ], + "starting_column": 1, + "ending_column": 2 + } + } + } + ] + } +] \ No newline at end of file diff --git a/tests/expected_json/incorrect_erc721_interface.erc721-interface.txt b/tests/expected_json/incorrect_erc721_interface.erc721-interface.txt new file mode 100644 index 000000000..76530c072 --- /dev/null +++ b/tests/expected_json/incorrect_erc721_interface.erc721-interface.txt @@ -0,0 +1,14 @@ +INFO:Detectors: +Token (tests/incorrect_erc721_interface.sol#6-16) has incorrect ERC721 function interface(s): + -supportsInterface (tests/incorrect_erc721_interface.sol#4) + -balanceOf (tests/incorrect_erc721_interface.sol#7) + -ownerOf (tests/incorrect_erc721_interface.sol#8) + -safeTransferFrom (tests/incorrect_erc721_interface.sol#9) + -safeTransferFrom (tests/incorrect_erc721_interface.sol#10) + -transferFrom (tests/incorrect_erc721_interface.sol#11) + -approve (tests/incorrect_erc721_interface.sol#12) + -setApprovalForAll (tests/incorrect_erc721_interface.sol#13) + -getApproved (tests/incorrect_erc721_interface.sol#14) + -isApprovedForAll (tests/incorrect_erc721_interface.sol#15) +Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-erc721-interface +INFO:Slither:tests/incorrect_erc721_interface.sol analyzed (2 contracts), 1 result(s) found From 3876b2b479afa391f23262591a152f20498479f5 Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 7 May 2019 13:45:19 +0100 Subject: [PATCH 5/7] Improvements of the human summary printer. Several of the new features are integrated within slither.core or slither.utils - Add state_variable.signature function(s) to translate a state variable to a function signature - Add several contract.is_erc_* methods - Add contract.is_from_dependency that checks if the contract is from a dependency - Add utils.erc with standard ERCs signatuers - Add utils.standard_libraries with standard libraries Several of these methods require crytic_compile 3ed5160924d08a4d3c86d7cddf9f1e17dfe51808 --- slither/core/declarations/contract.py | 122 +++++++++-- slither/core/variables/state_variable.py | 45 ++++ .../erc/incorrect_erc20_interface.py | 4 +- .../erc/incorrect_erc721_interface.py | 2 +- slither/printers/summary/human_summary.py | 83 +++++++- slither/slither.py | 9 +- slither/utils/erc.py | 71 +++++++ slither/utils/standard_libraries.py | 193 ++++++++++++++++++ slither/utils/type.py | 3 + 9 files changed, 497 insertions(+), 35 deletions(-) create mode 100644 slither/utils/erc.py create mode 100644 slither/utils/standard_libraries.py diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index f53e2a717..f06bc3031 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -5,6 +5,9 @@ import logging from slither.core.children.child_slither import ChildSlither from slither.core.source_mapping.source_mapping import SourceMapping from slither.core.declarations.function import Function +from slither.utils.erc import ERC20_signatures, \ + ERC165_signatures, ERC223_signatures, ERC721_signatures, \ + ERC1820_signatures, ERC777_signatures logger = logging.getLogger("Contract") @@ -35,6 +38,8 @@ class Contract(ChildSlither, SourceMapping): self._using_for = {} self._kind = None + self._signatures = None + self._initial_state_variables = [] # ssa @@ -212,6 +217,20 @@ class Contract(ChildSlither, SourceMapping): ################################################################################### ################################################################################### + @property + def functions_signatures(self): + """ + Return the signatures of all the public/eterxnal functions/state variables + :return: list(string) the signatures of all the functions that can be called + """ + if self._signatures == None: + sigs = [v.full_name for v in self.state_variables if v.visibility in ['public', + 'external']] + + sigs += set([f.full_name for f in self.functions if f.visibility in ['public', 'external']]) + self._signatures = list(set(sigs)) + return self._signatures + @property def functions(self): ''' @@ -534,50 +553,102 @@ class Contract(ChildSlither, SourceMapping): ################################################################################### ################################################################################### + def ercs(self): + """ + Return the ERC implemented + :return: list of string + """ + all = [('ERC20', lambda x: x.is_erc20()), + ('ERC165', lambda x: x.is_erc165()), + ('ERC1820', lambda x: x.is_erc1820()), + ('ERC223', lambda x: x.is_erc223()), + ('ERC721', lambda x: x.is_erc721()), + ('ERC777', lambda x: x.is_erc777())] + + return [erc[0] for erc in all if erc[1](self)] def is_erc20(self): """ Check if the contract is an erc20 token Note: it does not check for correct return values - Returns: - bool + :return: Returns a true if the contract is an erc20 """ - full_names = set([f.full_name for f in self.functions]) - return 'transfer(address,uint256)' in full_names and\ - 'transferFrom(address,address,uint256)' in full_names and\ - 'approve(address,uint256)' in full_names + full_names = self.functions_signatures + return all((s in full_names for s in ERC20_signatures)) + + def is_erc165(self): + """ + Check if the contract is an erc165 token + + Note: it does not check for correct return values + :return: Returns a true if the contract is an erc165 + """ + full_names = self.functions_signatures + return all((s in full_names for s in ERC165_signatures)) + + def is_erc1820(self): + """ + Check if the contract is an erc1820 + + Note: it does not check for correct return values + :return: Returns a true if the contract is an erc165 + """ + full_names = self.functions_signatures + return all((s in full_names for s in ERC1820_signatures)) + + def is_erc223(self): + """ + Check if the contract is an erc223 token + + Note: it does not check for correct return values + :return: Returns a true if the contract is an erc223 + """ + full_names = self.functions_signatures + return all((s in full_names for s in ERC223_signatures)) def is_erc721(self): - full_names = set([f.full_name for f in self.functions]) - return self.is_erc20() and\ - 'ownerOf(uint256)' in full_names and\ - 'safeTransferFrom(address,address,uint256,bytes)' in full_names and\ - 'safeTransferFrom(address,address,uint256)' in full_names and\ - 'setApprovalForAll(address,bool)' in full_names and\ - 'getApproved(uint256)' in full_names and\ - 'isApprovedForAll(address,address)' in full_names + """ + Check if the contract is an erc721 token + + Note: it does not check for correct return values + :return: Returns a true if the contract is an erc721 + """ + full_names = self.functions_signatures + return all((s in full_names for s in ERC721_signatures)) + + def is_erc777(self): + """ + Check if the contract is an erc777 - def has_an_erc20_function(self): + Note: it does not check for correct return values + :return: Returns a true if the contract is an erc165 + """ + full_names = self.functions_signatures + return all((s in full_names for s in ERC777_signatures)) + + def is_possible_erc20(self): """ Checks if the provided contract could be attempting to implement ERC20 standards. :param contract: The contract to check for token compatibility. :return: Returns a boolean indicating if the provided contract met the token standard. """ - full_names = set([f.full_name for f in self.functions]) + # We do not check for all the functions, as name(), symbol(), might give too many FPs + full_names = self.functions_signatures return 'transfer(address,uint256)' in full_names or \ 'transferFrom(address,address,uint256)' in full_names or \ 'approve(address,uint256)' in full_names - def has_an_erc721_function(self): + def is_possible_erc721(self): """ Checks if the provided contract could be attempting to implement ERC721 standards. :param contract: The contract to check for token compatibility. :return: Returns a boolean indicating if the provided contract met the token standard. """ - full_names = set([f.full_name for f in self.functions]) - return self.has_an_erc20_function() and \ - ('ownerOf(uint256)' in full_names or + # We do not check for all the functions, as name(), symbol(), might give too many FPs + full_names = self.functions_signatures + return ('approve(address,uint256)' in full_names or + 'ownerOf(uint256)' in full_names or 'safeTransferFrom(address,address,uint256,bytes)' in full_names or 'safeTransferFrom(address,address,uint256)' in full_names or 'setApprovalForAll(address,bool)' in full_names or @@ -585,6 +656,17 @@ class Contract(ChildSlither, SourceMapping): 'isApprovedForAll(address,address)' in full_names) + # endregion + ################################################################################### + ################################################################################### + # region Dependencies + ################################################################################### + ################################################################################### + + def is_from_dependency(self): + if self.slither.crytic_compile is None: + return False + return self.slither.crytic_compile.is_dependency(self.source_mapping['filename_absolute']) # endregion ################################################################################### diff --git a/slither/core/variables/state_variable.py b/slither/core/variables/state_variable.py index b8f46482e..e85ca2e09 100644 --- a/slither/core/variables/state_variable.py +++ b/slither/core/variables/state_variable.py @@ -1,8 +1,53 @@ from .variable import Variable from slither.core.children.child_contract import ChildContract +from slither.utils.type import export_nested_types_from_variable class StateVariable(ChildContract, Variable): + ################################################################################### + ################################################################################### + # region Signature + ################################################################################### + ################################################################################### + + @property + def signature(self): + """ + Return the signature of the state variable as a function signature + :return: (str, list(str), list(str)), as (name, list parameters type, list return values type) + """ + return self.name, [str(x) for x in export_nested_types_from_variable(self)], self.type + + @property + def signature_str(self): + """ + Return the signature of the state variable as a function signature + :return: str: func_name(type1,type2) returns(type3) + """ + name, parameters, returnVars = self.signature + return name+'('+','.join(parameters)+') returns('+','.join(returnVars)+')' + + # endregion + ################################################################################### + ################################################################################### + # region Name + ################################################################################### + ################################################################################### + @property def canonical_name(self): return '{}:{}'.format(self.contract.name, self.name) + + @property + def full_name(self): + """ + Return the name of the state variable as a function signaure + str: func_name(type1,type2) + :return: the function signature without the return values + """ + name, parameters, _ = self.signature + return name+'('+','.join(parameters)+')' + + # endregion + ################################################################################### + ################################################################################### diff --git a/slither/detectors/erc/incorrect_erc20_interface.py b/slither/detectors/erc/incorrect_erc20_interface.py index ab577fbab..5333f089c 100644 --- a/slither/detectors/erc/incorrect_erc20_interface.py +++ b/slither/detectors/erc/incorrect_erc20_interface.py @@ -62,12 +62,12 @@ contract Token{ list(str) : list of incorrect function signatures """ # Verify this is an ERC20 contract. - if not contract.has_an_erc20_function(): + if not contract.is_possible_erc20(): return [] # If this contract implements a function from ERC721, we can assume it is an ERC721 token. These tokens # offer functions which are similar to ERC20, but are not compatible. - if contract.has_an_erc721_function(): + if contract.is_possible_erc721(): return [] functions = [f for f in contract.functions if IncorrectERC20InterfaceDetection.incorrect_erc20_interface(f.signature)] diff --git a/slither/detectors/erc/incorrect_erc721_interface.py b/slither/detectors/erc/incorrect_erc721_interface.py index 5ddea37ff..336d549db 100644 --- a/slither/detectors/erc/incorrect_erc721_interface.py +++ b/slither/detectors/erc/incorrect_erc721_interface.py @@ -68,7 +68,7 @@ contract Token{ """ # Verify this is an ERC721 contract. - if not contract.has_an_erc721_function() or not contract.has_an_erc20_function(): + if not contract.is_possible_erc721() or not contract.is_possible_erc20(): return [] functions = [f for f in contract.functions if IncorrectERC721InterfaceDetection.incorrect_erc721_interface(f.signature)] diff --git a/slither/printers/summary/human_summary.py b/slither/printers/summary/human_summary.py index e82b7fef7..625a0aca9 100644 --- a/slither/printers/summary/human_summary.py +++ b/slither/printers/summary/human_summary.py @@ -6,7 +6,7 @@ import logging from slither.printers.abstract_printer import AbstractPrinter from slither.utils.code_complexity import compute_cyclomatic_complexity from slither.utils.colors import green, red, yellow - +from slither.utils.standard_libraries import is_standard_library class PrinterHumanSummary(AbstractPrinter): ARGUMENT = 'human-summary' @@ -88,8 +88,14 @@ class PrinterHumanSummary(AbstractPrinter): issues_informational, issues_low, issues_medium, issues_high = self._get_detectors_result() txt = "Number of informational issues: {}\n".format(green(issues_informational)) txt += "Number of low issues: {}\n".format(green(issues_low)) - txt += "Number of medium issues: {}\n".format(yellow(issues_medium)) - txt += "Number of high issues: {}\n".format(red(issues_high)) + if issues_medium > 0: + txt += "Number of medium issues: {}\n".format(yellow(issues_medium)) + else: + txt += "Number of medium issues: {}\n".format(green(issues_medium)) + if issues_high > 0: + txt += "Number of high issues: {}\n".format(red(issues_high)) + else: + txt += "Number of high issues: {}\n\n".format(green(issues_high)) return txt @@ -119,6 +125,49 @@ class PrinterHumanSummary(AbstractPrinter): def _number_functions(contract): return len(contract.functions) + def _lines_number(self): + if not self.slither.source_code: + return None + total_dep_lines = 0 + total_lines = 0 + for filename, source_code in self.slither.source_code.items(): + lines = len(source_code.splitlines()) + is_dep = False + if self.slither.crytic_compile: + is_dep = self.slither.crytic_compile.is_dependency(filename) + if is_dep: + total_dep_lines += lines + else: + total_lines += lines + return total_lines, total_dep_lines + + def _compilation_type(self): + if self.slither.crytic_compile is None: + return 'Compilation non standard\n' + return f'Compiled with {self.slither.crytic_compile.type}\n' + + def _number_contracts(self): + if self.slither.crytic_compile is None: + len(self.slither.contracts), 0 + deps = [c for c in self.slither.contracts if c.is_from_dependency()] + contracts = [c for c in self.slither.contracts if not c.is_from_dependency()] + return len(contracts), len(deps) + + def _standard_libraries(self): + libraries = [] + for contract in self.contracts: + lib = is_standard_library(contract) + if lib: + libraries.append(lib) + + return libraries + + def _ercs(self): + ercs = [] + for contract in self.contracts: + ercs += contract.ercs() + return list(set(ercs)) + def output(self, _filename): """ _filename is not used @@ -126,15 +175,37 @@ class PrinterHumanSummary(AbstractPrinter): _filename(string) """ - txt = "Analyze of {}\n".format(self.slither.filename) + txt = "\n" + txt += self._compilation_type() + + lines_number = self._lines_number() + if lines_number: + total_lines, total_dep_lines = lines_number + txt += f'Number of lines: {total_lines} (+ {total_dep_lines} in dependencies)\n' + + number_contracts, number_contracts_deps = self._number_contracts() + txt += f'Number of contracts: {number_contracts} (+ {number_contracts_deps} in dependencies) \n\n' + txt += self.get_detectors_result() + + libs = self._standard_libraries() + if libs: + txt += f'\nUse: {", ".join(libs)}\n' + + ercs = self._ercs() + if ercs: + txt += f'ERCs: {", ".join(ercs)}\n' + for contract in self.slither.contracts_derived: txt += "\nContract {}\n".format(contract.name) txt += self.is_complex_code(contract) + txt += '\tNumber of functions: {}\n'.format(self._number_functions(contract)) + ercs = contract.ercs() + if ercs: + txt += '\tERCs: ' + ','.join(ercs) + '\n' is_erc20 = contract.is_erc20() - txt += '\tNumber of functions:{}'.format(self._number_functions(contract)) - txt += "\tIs ERC20 token: {}\n".format(contract.is_erc20()) if is_erc20: + txt += '\tERC20 info:\n' txt += self.get_summary_erc20(contract) self.info(txt) diff --git a/slither/slither.py b/slither/slither.py index d30099426..acb25d619 100644 --- a/slither/slither.py +++ b/slither/slither.py @@ -45,9 +45,6 @@ class Slither(SlitherSolc): ''' - truffle_ignore = kwargs.get('truffle_ignore', False) - embark_ignore = kwargs.get('embark_ignore', False) - # list of files provided (see --splitted option) if isinstance(contract, list): self._init_from_list(contract) @@ -56,13 +53,13 @@ class Slither(SlitherSolc): else: super(Slither, self).__init__('') try: - cryticCompile = CryticCompile(contract, **kwargs) - self._crytic_compile = cryticCompile + crytic_compile = CryticCompile(contract, **kwargs) + self._crytic_compile = crytic_compile except InvalidCompilation as e: logger.error('Invalid compilation') logger.error(e) exit(-1) - for path, ast in cryticCompile.asts.items(): + for path, ast in crytic_compile.asts.items(): self._parse_contracts_from_loaded_json(ast, path) self._add_source_code(path) diff --git a/slither/utils/erc.py b/slither/utils/erc.py new file mode 100644 index 000000000..08aa7ad0d --- /dev/null +++ b/slither/utils/erc.py @@ -0,0 +1,71 @@ + +def erc_to_signatures(erc): + return [f'{e[0]}({",".join(e[1])})' for e in erc] + + +# Final +# https://eips.ethereum.org/EIPS/eip-20 +ERC20 = [('name', [], 'string'), + ('symbol', [], 'string'), + ('decimals', [], 'uint8'), + ('totalSupply', [], 'uint256'), + ('balanceOf', ['address'], 'uint256'), + ('transfer', ['address', 'uint256'], 'bool'), + ('transferFrom', ['address', 'address', 'uint256'], 'bool'), + ('approve', ['address', 'uint256'], 'bool'), + ('allowance', ['address', 'address'], 'uint256')] +ERC20_signatures = erc_to_signatures(ERC20) + +# Draft +# https://github.com/ethereum/eips/issues/223 +ERC223 = [('name', [], 'string'), + ('symbol', [], 'string'), + ('decimals', [], 'uint8'), + ('totalSupply', [], 'uint256'), + ('balanceOf', ['address'], 'uint256'), + ('transfer', ['address', 'uint256'], 'bool'), + ('transfer', ['address', 'uint256', 'bytes'], 'bool'), + ('transfer', ['address', 'uint256', 'bytes', 'string'], 'bool')] +ERC223_signatures = erc_to_signatures(ERC223) + +# Final +# https://eips.ethereum.org/EIPS/eip-165 +ERC165 = [('supportsInterface', ['bytes4'], 'bool')] +ERC165_signatures = erc_to_signatures(ERC165) + +# Final +# https://eips.ethereum.org/EIPS/eip-721 +# Must have ERC165 +# name, symbol, tokenURI are optionals +ERC721 = [('balanceOf', ['address'], 'uint256'), + ('ownerOf', ['uint256'], 'address'), + ('safeTransferFrom', ['address', 'address', 'uint256', 'bytes'], ''), + ('safeTransferFrom', ['address', 'address', 'uint256'], ''), + ('transferFrom', ['address', 'address', 'uint256'], ''), + ('approve', ['address', 'uint256'], ''), + ('setApprovalForAll', ['address', 'bool'], ''), + ('getApproved', ['uint256'], 'address'), + ('isApprovedForAll', ['address', 'address'], 'bool')] + ERC165 +ERC721_signatures = erc_to_signatures(ERC721) + +# Final +# https://eips.ethereum.org/EIPS/eip-1820 +ERC1820 = [('canImplementInterfaceForAddress', ['bytes32', 'address'], 'bytes32')] +ERC1820_signatures = erc_to_signatures(ERC1820) + +# Last Call +# https://eips.ethereum.org/EIPS/eip-777 +ERC777 = [('name', [], 'string'), + ('symbol', [], 'string'), + ('totalSupply', [], 'uint256'), + ('balanceOf', ['address'], 'uint256'), + ('granularity', [], 'uint256'), + ('defaultOperators', [], 'address[]'), + ('isOperatorFor', ['address', 'address'], 'bool'), + ('authorizeOperator', ['address'], ''), + ('revokeOperator', ['address'], ''), + ('send', ['address', 'uint256', 'bytes'], ''), + ('operatorSend', ['address', 'address', 'uint256', 'bytes', 'bytes'], ''), + ('burn', ['uint256', 'bytes'] , ''), + ('operatorBurn', ['address', 'uint256', 'bytes', 'bytes'] , '')] +ERC777_signatures = erc_to_signatures(ERC777) \ No newline at end of file diff --git a/slither/utils/standard_libraries.py b/slither/utils/standard_libraries.py new file mode 100644 index 000000000..c73a29b93 --- /dev/null +++ b/slither/utils/standard_libraries.py @@ -0,0 +1,193 @@ +from pathlib import Path + + +libraries = { + 'Openzeppelin-SafeMath': lambda x: is_openzepellin_safemath(x), + 'Openzeppelin-ECRecovery': lambda x: is_openzepellin_ecrecovery(x), + 'Openzeppelin-Ownable': lambda x: is_openzepellin_ownable(x), + 'Openzeppelin-ERC20': lambda x: is_openzepellin_erc20(x), + 'Openzeppelin-ERC721': lambda x: is_openzepellin_erc721(x), + 'Zos-Upgrade': lambda x: is_zos_initializable(x), + 'Dapphub-DSAuth': lambda x: is_dapphub_ds_auth(x), + 'Dapphub-DSMath': lambda x: is_dapphub_ds_math(x), + 'Dapphub-DSToken': lambda x: is_dapphub_ds_token(x), + 'Dapphub-DSProxy': lambda x: is_dapphub_ds_proxy(x), + 'Dapphub-DSGroup': lambda x: is_dapphub_ds_group(x), +} + +def is_standard_library(contract): + for name, is_lib in libraries.items(): + if is_lib(contract): + return name + return None + + +################################################################################### +################################################################################### +# region General libraries +################################################################################### +################################################################################### + + +def is_openzepellin(contract): + if not contract.is_from_dependency(): + return False + return 'openzeppelin-solidity' in Path(contract.source_mapping['filename_absolute']).parts + + +def is_zos(contract): + if not contract.is_from_dependency(): + return False + return 'zos-lib' in Path(contract.source_mapping['filename_absolute']).parts + + +# endregion +################################################################################### +################################################################################### +# region SafeMath +################################################################################### +################################################################################### + + +def is_safemath(contract): + return contract.name == "SafeMath" + + +def is_openzepellin_safemath(contract): + return is_safemath(contract) and is_openzepellin(contract) + +# endregion +################################################################################### +################################################################################### +# region ECRecovery +################################################################################### +################################################################################### + + +def is_ecrecovery(contract): + return contract.name == 'ECRecovery' + + +def is_openzepellin_ecrecovery(contract): + return is_ecrecovery(contract) and is_openzepellin(contract) + + +# endregion +################################################################################### +################################################################################### +# region Ownable +################################################################################### +################################################################################### + + +def is_ownable(contract): + return contract.name == 'Ownable' + + +def is_openzepellin_ownable(contract): + return is_ownable(contract) and is_openzepellin(contract) + + +# endregion +################################################################################### +################################################################################### +# region ERC20 +################################################################################### +################################################################################### + + +def is_erc20(contract): + return contract.name == 'ERC20' + + +def is_openzepellin_erc20(contract): + return is_erc20(contract) and is_openzepellin(contract) + + +# endregion +################################################################################### +################################################################################### +# region ERC721 +################################################################################### +################################################################################### + + +def is_erc721(contract): + return contract.name == 'ERC721' + + +def is_openzepellin_erc721(contract): + return is_erc721(contract) and is_openzepellin(contract) + + +# endregion +################################################################################### +################################################################################### +# region Zos Initializable +################################################################################### +################################################################################### + + +def is_initializable(contract): + return contract.name == 'Initializable' + + +def is_zos_initializable(contract): + return is_initializable(contract) and is_zos(contract) + + +# endregion +################################################################################### +################################################################################### +# region DappHub +################################################################################### +################################################################################### + +dapphubs = { + 'DSAuth': 'ds-auth', + 'DSMath': 'ds-math', + 'DSToken': 'ds-token', + 'DSProxy': 'ds-proxy', + 'DSGroup': 'ds-group', +} + + +def _is_ds(contract, name): + return contract.name == name + +def _is_dappdhub_ds(contract, name): + if not contract.is_from_dependency(): + return False + if not dapphubs[name] in Path(contract.source_mapping['filename_absolute']).parts: + return False + return _is_ds(contract, name) + +def is_ds_auth(contract): + return _is_ds(contract, 'DSAuth') + +def is_dapphub_ds_auth(contract): + return _is_dappdhub_ds(contract, 'DSAuth') + +def is_ds_math(contract): + return _is_ds(contract, 'DSMath') + +def is_dapphub_ds_math(contract): + return _is_dappdhub_ds(contract, 'DSMath') + +def is_ds_token(contract): + return _is_ds(contract, 'DSToken') + +def is_dapphub_ds_token(contract): + return _is_dappdhub_ds(contract, 'DSToken') + +def is_ds_proxy(contract): + return _is_ds(contract, 'DSProxy') + +def is_dapphub_ds_proxy(contract): + return _is_dappdhub_ds(contract, 'DSProxy') + +def is_ds_group(contract): + return _is_ds(contract, 'DSGroup') + +def is_dapphub_ds_group(contract): + return _is_dappdhub_ds(contract, 'DSGroup') diff --git a/slither/utils/type.py b/slither/utils/type.py index d5bca3720..3b0577987 100644 --- a/slither/utils/type.py +++ b/slither/utils/type.py @@ -1,16 +1,19 @@ from slither.core.solidity_types import (ArrayType, MappingType, ElementaryType) + def _add_mapping_parameter(t, l): while isinstance(t, MappingType): l.append(t.type_from) t = t.type_to _add_array_parameter(t, l) + def _add_array_parameter(t, l): while isinstance(t, ArrayType): l.append(ElementaryType('uint256')) t = t.type + def export_nested_types_from_variable(variable): """ Export the list of nested types (mapping/array) From 0beb48146a169779f76943f3ff74d06dba766478 Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 7 May 2019 13:51:14 +0100 Subject: [PATCH 6/7] Update travis install --- scripts/travis_install.sh | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/scripts/travis_install.sh b/scripts/travis_install.sh index acd47eaba..77d84e296 100755 --- a/scripts/travis_install.sh +++ b/scripts/travis_install.sh @@ -1,4 +1,12 @@ #!/usr/bin/env bash + +# TODO: temporary until the next crytic-compile release +git clone https://github.com/crytic/crytic-compile +cd crytic-compile +git checkout dev +python setup.py install +cd .. + python setup.py install # Used by travis_test.sh pip install deepdiff From e6c9d186e331f82c2d9143eb3f2f195dc8e36983 Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 7 May 2019 17:55:13 +0100 Subject: [PATCH 7/7] Remove optional ERC20 functions --- slither/core/declarations/contract.py | 3 +-- slither/utils/erc.py | 6 ++---- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index f06bc3031..cbfe723f7 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -647,8 +647,7 @@ class Contract(ChildSlither, SourceMapping): """ # We do not check for all the functions, as name(), symbol(), might give too many FPs full_names = self.functions_signatures - return ('approve(address,uint256)' in full_names or - 'ownerOf(uint256)' in full_names or + return ('ownerOf(uint256)' in full_names or 'safeTransferFrom(address,address,uint256,bytes)' in full_names or 'safeTransferFrom(address,address,uint256)' in full_names or 'setApprovalForAll(address,bool)' in full_names or diff --git a/slither/utils/erc.py b/slither/utils/erc.py index 08aa7ad0d..21c2c53a5 100644 --- a/slither/utils/erc.py +++ b/slither/utils/erc.py @@ -5,10 +5,8 @@ def erc_to_signatures(erc): # Final # https://eips.ethereum.org/EIPS/eip-20 -ERC20 = [('name', [], 'string'), - ('symbol', [], 'string'), - ('decimals', [], 'uint8'), - ('totalSupply', [], 'uint256'), +# name, symbolc, decimals are optionals +ERC20 = [('totalSupply', [], 'uint256'), ('balanceOf', ['address'], 'uint256'), ('transfer', ['address', 'uint256'], 'bool'), ('transferFrom', ['address', 'address', 'uint256'], 'bool'),