From 0e078d0d5dbef8c3df6d98d93092c941615b634f Mon Sep 17 00:00:00 2001 From: rajeevgopalakrishna Date: Wed, 22 May 2019 16:33:46 +0530 Subject: [PATCH 1/4] Fixes false alert in naming-convention detector on empty string parameter name. --- slither/detectors/naming_convention/naming_convention.py | 3 +++ tests/naming_convention.sol | 7 +++++++ 2 files changed, 10 insertions(+) diff --git a/slither/detectors/naming_convention/naming_convention.py b/slither/detectors/naming_convention/naming_convention.py index 61d917b26..9c69e41ba 100644 --- a/slither/detectors/naming_convention/naming_convention.py +++ b/slither/detectors/naming_convention/naming_convention.py @@ -104,6 +104,9 @@ Solidity defines a [naming convention](https://solidity.readthedocs.io/en/v0.4.2 results.append(json) for argument in func.parameters: + # Ignore parameter names that are not specified i.e. empty strings + if argument.name == "": + continue if argument in func.variables_read_or_written: correct_naming = self.is_mixed_case(argument.name) else: diff --git a/tests/naming_convention.sol b/tests/naming_convention.sol index cc3596f6d..841c9f743 100644 --- a/tests/naming_convention.sol +++ b/tests/naming_convention.sol @@ -66,3 +66,10 @@ contract T { uint l = 1; } + +contract ParameterNameEmptyString { + + function foo (string) { + } + +} From 37d1fa0729ca5687658bdbdadf57823928b3d526 Mon Sep 17 00:00:00 2001 From: Eric Rafaloff Date: Tue, 28 May 2019 16:46:14 -0400 Subject: [PATCH 2/4] Add 'optimization' as a new detector category Apply to the following detectors: - constable-states - external-function --- README.md | 20 +++++++++---------- slither/detectors/abstract_detector.py | 14 +++++++++---- .../detectors/functions/external_function.py | 2 +- .../possible_const_state_variables.py | 2 +- ...onst_state_variables.constable-states.json | 12 +++++------ .../external_function.external-function.json | 8 ++++---- 6 files changed, 32 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 43f853120..193076eb7 100644 --- a/README.md +++ b/README.md @@ -65,16 +65,16 @@ Num | Detector | What it Detects | Impact | Confidence 24 | `reentrancy-benign` | [Benign reentrancy vulnerabilities](https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-2) | Low | Medium 25 | `timestamp` | [Dangerous usage of `block.timestamp`](https://github.com/crytic/slither/wiki/Detector-Documentation#block-timestamp) | Low | Medium 26 | `assembly` | [Assembly usage](https://github.com/crytic/slither/wiki/Detector-Documentation#assembly-usage) | Informational | High -27 | `constable-states` | [State variables that could be declared constant](https://github.com/crytic/slither/wiki/Detector-Documentation#state-variables-that-could-be-declared-constant) | Informational | High -28 | `deprecated-standards` | [Deprecated Solidity Standards](https://github.com/crytic/slither/wiki/Detector-Documentation#deprecated-standards) | Informational | High -29 | `erc20-indexed` | [Un-indexed ERC20 event parameters](https://github.com/crytic/slither/wiki/Detector-Documentation#unindexed-erc20-event-parameters) | Informational | High -30 | `external-function` | [Public function that could be declared as external](https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-as-external) | Informational | High -31 | `low-level-calls` | [Low level calls](https://github.com/crytic/slither/wiki/Detector-Documentation#low-level-calls) | Informational | High -32 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions) | Informational | High -33 | `pragma` | [If different pragma directives are used](https://github.com/crytic/slither/wiki/Detector-Documentation#different-pragma-directives-are-used) | Informational | High -34 | `solc-version` | [Incorrect Solidity version (< 0.4.24 or complex pragma)](https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-version-of-solidity) | Informational | High -35 | `unused-state` | [Unused state variables](https://github.com/crytic/slither/wiki/Detector-Documentation#unused-state-variables) | Informational | High -36 | `too-many-digits` | [Conformance to numeric notation best practices](https://github.com/crytic/slither/wiki/Detector-Documentation#too-many-digits) | Informational | Medium +27 | `deprecated-standards` | [Deprecated Solidity Standards](https://github.com/crytic/slither/wiki/Detector-Documentation#deprecated-standards) | Informational | High +28 | `erc20-indexed` | [Un-indexed ERC20 event parameters](https://github.com/crytic/slither/wiki/Detector-Documentation#unindexed-erc20-event-parameters) | Informational | High +29 | `low-level-calls` | [Low level calls](https://github.com/crytic/slither/wiki/Detector-Documentation#low-level-calls) | Informational | High +30 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions) | Informational | High +31 | `pragma` | [If different pragma directives are used](https://github.com/crytic/slither/wiki/Detector-Documentation#different-pragma-directives-are-used) | Informational | High +32 | `solc-version` | [Incorrect Solidity version (< 0.4.24 or complex pragma)](https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-version-of-solidity) | Informational | High +33 | `unused-state` | [Unused state variables](https://github.com/crytic/slither/wiki/Detector-Documentation#unused-state-variables) | Informational | High +34 | `too-many-digits` | [Conformance to numeric notation best practices](https://github.com/crytic/slither/wiki/Detector-Documentation#too-many-digits) | Informational | Medium +35 | `constable-states` | [State variables that could be declared constant](https://github.com/crytic/slither/wiki/Detector-Documentation#state-variables-that-could-be-declared-constant) | Optimization | High +36 | `external-function` | [Public function that could be declared as external](https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-as-external) | Optimization | High [Contact us](https://www.trailofbits.com/contact/) to get access to additional detectors. diff --git a/slither/detectors/abstract_detector.py b/slither/detectors/abstract_detector.py index 39651c12a..d05b11417 100644 --- a/slither/detectors/abstract_detector.py +++ b/slither/detectors/abstract_detector.py @@ -14,17 +14,20 @@ class DetectorClassification: MEDIUM = 1 LOW = 2 INFORMATIONAL = 3 + OPTIMIZATION = 4 classification_colors = { DetectorClassification.INFORMATIONAL: green, + DetectorClassification.OPTIMIZATION: green, DetectorClassification.LOW: green, DetectorClassification.MEDIUM: yellow, - DetectorClassification.HIGH: red, + DetectorClassification.HIGH: red } classification_txt = { DetectorClassification.INFORMATIONAL: 'Informational', + DetectorClassification.OPTIMIZATION: 'Optimization', DetectorClassification.LOW: 'Low', DetectorClassification.MEDIUM: 'Medium', DetectorClassification.HIGH: 'High', @@ -65,7 +68,8 @@ class AbstractDetector(metaclass=abc.ABCMeta): if not self.WIKI_DESCRIPTION: raise IncorrectDetectorInitialization('WIKI_DESCRIPTION is not initialized {}'.format(self.__class__.__name__)) - if not self.WIKI_EXPLOIT_SCENARIO and self.IMPACT != DetectorClassification.INFORMATIONAL: + if not self.WIKI_EXPLOIT_SCENARIO and self.IMPACT not in [DetectorClassification.INFORMATIONAL, + DetectorClassification.OPTIMIZATION]: raise IncorrectDetectorInitialization('WIKI_EXPLOIT_SCENARIO is not initialized {}'.format(self.__class__.__name__)) if not self.WIKI_RECOMMENDATION: @@ -77,13 +81,15 @@ class AbstractDetector(metaclass=abc.ABCMeta): if self.IMPACT not in [DetectorClassification.LOW, DetectorClassification.MEDIUM, DetectorClassification.HIGH, - DetectorClassification.INFORMATIONAL]: + DetectorClassification.INFORMATIONAL, + DetectorClassification.OPTIMIZATION]: raise IncorrectDetectorInitialization('IMPACT is not initialized {}'.format(self.__class__.__name__)) if self.CONFIDENCE not in [DetectorClassification.LOW, DetectorClassification.MEDIUM, DetectorClassification.HIGH, - DetectorClassification.INFORMATIONAL]: + DetectorClassification.INFORMATIONAL, + DetectorClassification.OPTIMIZATION]: raise IncorrectDetectorInitialization('CONFIDENCE is not initialized {}'.format(self.__class__.__name__)) diff --git a/slither/detectors/functions/external_function.py b/slither/detectors/functions/external_function.py index 9c95bde31..4b4a40f5f 100644 --- a/slither/detectors/functions/external_function.py +++ b/slither/detectors/functions/external_function.py @@ -13,7 +13,7 @@ class ExternalFunction(AbstractDetector): ARGUMENT = 'external-function' HELP = 'Public function that could be declared as external' - IMPACT = DetectorClassification.INFORMATIONAL + IMPACT = DetectorClassification.OPTIMIZATION CONFIDENCE = DetectorClassification.HIGH WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#public-function-that-could-be-declared-as-external' diff --git a/slither/detectors/variables/possible_const_state_variables.py b/slither/detectors/variables/possible_const_state_variables.py index d509f4569..f062a0c7a 100644 --- a/slither/detectors/variables/possible_const_state_variables.py +++ b/slither/detectors/variables/possible_const_state_variables.py @@ -18,7 +18,7 @@ class ConstCandidateStateVars(AbstractDetector): ARGUMENT = 'constable-states' HELP = 'State variables that could be declared constant' - IMPACT = DetectorClassification.INFORMATIONAL + IMPACT = DetectorClassification.OPTIMIZATION CONFIDENCE = DetectorClassification.HIGH WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#state-variables-that-could-be-declared-constant' diff --git a/tests/expected_json/const_state_variables.constable-states.json b/tests/expected_json/const_state_variables.constable-states.json index 55cb25ee9..8eb180ab5 100644 --- a/tests/expected_json/const_state_variables.constable-states.json +++ b/tests/expected_json/const_state_variables.constable-states.json @@ -5,7 +5,7 @@ "detectors": [ { "check": "constable-states", - "impact": "Informational", + "impact": "Optimization", "confidence": "High", "description": "A.myFriendsAddress should be constant (tests/const_state_variables.sol#7)\n", "elements": [ @@ -66,7 +66,7 @@ }, { "check": "constable-states", - "impact": "Informational", + "impact": "Optimization", "confidence": "High", "description": "A.test should be constant (tests/const_state_variables.sol#10)\n", "elements": [ @@ -127,7 +127,7 @@ }, { "check": "constable-states", - "impact": "Informational", + "impact": "Optimization", "confidence": "High", "description": "A.text2 should be constant (tests/const_state_variables.sol#14)\n", "elements": [ @@ -188,7 +188,7 @@ }, { "check": "constable-states", - "impact": "Informational", + "impact": "Optimization", "confidence": "High", "description": "B.mySistersAddress should be constant (tests/const_state_variables.sol#26)\n", "elements": [ @@ -245,7 +245,7 @@ }, { "check": "constable-states", - "impact": "Informational", + "impact": "Optimization", "confidence": "High", "description": "MyConc.should_be_constant should be constant (tests/const_state_variables.sol#42)\n", "elements": [ @@ -302,7 +302,7 @@ }, { "check": "constable-states", - "impact": "Informational", + "impact": "Optimization", "confidence": "High", "description": "MyConc.should_be_constant_2 should be constant (tests/const_state_variables.sol#43)\n", "elements": [ diff --git a/tests/expected_json/external_function.external-function.json b/tests/expected_json/external_function.external-function.json index c5f667494..b4a803034 100644 --- a/tests/expected_json/external_function.external-function.json +++ b/tests/expected_json/external_function.external-function.json @@ -5,7 +5,7 @@ "detectors": [ { "check": "external-function", - "impact": "Informational", + "impact": "Optimization", "confidence": "High", "description": "ContractWithFunctionNotCalled.funcNotCalled3() (tests/external_function.sol#13-15) should be declared external\n", "elements": [ @@ -70,7 +70,7 @@ }, { "check": "external-function", - "impact": "Informational", + "impact": "Optimization", "confidence": "High", "description": "ContractWithFunctionNotCalled.funcNotCalled2() (tests/external_function.sol#17-19) should be declared external\n", "elements": [ @@ -135,7 +135,7 @@ }, { "check": "external-function", - "impact": "Informational", + "impact": "Optimization", "confidence": "High", "description": "ContractWithFunctionNotCalled.funcNotCalled() (tests/external_function.sol#21-23) should be declared external\n", "elements": [ @@ -200,7 +200,7 @@ }, { "check": "external-function", - "impact": "Informational", + "impact": "Optimization", "confidence": "High", "description": "ContractWithFunctionNotCalled2.funcNotCalled() (tests/external_function.sol#32-39) should be declared external\n", "elements": [ From 0a28154bf3c28f9be4e53847e4cb16c4fc19d57f Mon Sep 17 00:00:00 2001 From: Josselin Date: Thu, 30 May 2019 13:23:59 +0100 Subject: [PATCH 3/4] Update etherscan test --- scripts/travis_test_etherscan.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/travis_test_etherscan.sh b/scripts/travis_test_etherscan.sh index 43c8a1e4a..c96eca12e 100755 --- a/scripts/travis_test_etherscan.sh +++ b/scripts/travis_test_etherscan.sh @@ -10,7 +10,7 @@ chmod +x solc-0.4.25 slither 0x7F37f78cBD74481E593F9C737776F7113d76B315 --solc "./solc-0.4.25" -if [ $? -ne 6 ] +if [ $? -ne 5 ] then echo "Etherscan test failed" exit -1 From db2b22d7572f0a912820c4848fcf96297a20b7ca Mon Sep 17 00:00:00 2001 From: Josselin Date: Thu, 30 May 2019 18:29:56 +0100 Subject: [PATCH 4/4] Fix travis --- scripts/tests_generate_expected_json_4.sh | 70 +++++++++---------- .../naming_convention.naming-convention.txt | 2 +- tests/naming_convention.sol | 2 +- 3 files changed, 37 insertions(+), 37 deletions(-) diff --git a/scripts/tests_generate_expected_json_4.sh b/scripts/tests_generate_expected_json_4.sh index 74050acd1..4e1def80c 100755 --- a/scripts/tests_generate_expected_json_4.sh +++ b/scripts/tests_generate_expected_json_4.sh @@ -21,38 +21,38 @@ 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" -generate_expected_json tests/pragma.0.4.24.sol "pragma" -generate_expected_json tests/old_solc.sol.json "solc-version" -generate_expected_json tests/reentrancy.sol "reentrancy-eth" -generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage" -generate_expected_json tests/tx_origin.sol "tx-origin" -generate_expected_json tests/unused_state.sol "unused-state" -generate_expected_json tests/locked_ether.sol "locked-ether" -generate_expected_json tests/arbitrary_send.sol "arbitrary-send" -generate_expected_json tests/inline_assembly_contract.sol "assembly" -generate_expected_json tests/inline_assembly_library.sol "assembly" -generate_expected_json tests/low_level_calls.sol "low-level-calls" -generate_expected_json tests/const_state_variables.sol "constable-states" -generate_expected_json tests/external_function.sol "external-function" -generate_expected_json tests/external_function_2.sol "external-function" -generate_expected_json tests/naming_convention.sol "naming-convention" -generate_expected_json tests/uninitialized_local_variable.sol "uninitialized-local" -generate_expected_json tests/controlled_delegatecall.sol "controlled-delegatecall" -generate_expected_json tests/constant.sol "constant-function" -generate_expected_json tests/unused_return.sol "unused-return" -generate_expected_json tests/shadowing_state_variable.sol "shadowing-state" -generate_expected_json tests/shadowing_abstract.sol "shadowing-abstract" -generate_expected_json tests/timestamp.sol "timestamp" -generate_expected_json tests/multiple_calls_in_loop.sol "calls-loop" -generate_expected_json tests/shadowing_builtin_symbols.sol "shadowing-builtin" -generate_expected_json tests/shadowing_local_variable.sol "shadowing-local" -generate_expected_json tests/solc_version_incorrect.sol "solc-version" -generate_expected_json tests/right_to_left_override.sol "rtlo" -generate_expected_json tests/unchecked_lowlevel.sol "unchecked-lowlevel" +#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" +#generate_expected_json tests/pragma.0.4.24.sol "pragma" +#generate_expected_json tests/old_solc.sol.json "solc-version" +#generate_expected_json tests/reentrancy.sol "reentrancy-eth" +#generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage" +#generate_expected_json tests/tx_origin.sol "tx-origin" +#generate_expected_json tests/unused_state.sol "unused-state" +#generate_expected_json tests/locked_ether.sol "locked-ether" +#generate_expected_json tests/arbitrary_send.sol "arbitrary-send" +#generate_expected_json tests/inline_assembly_contract.sol "assembly" +#generate_expected_json tests/inline_assembly_library.sol "assembly" +#generate_expected_json tests/low_level_calls.sol "low-level-calls" +#generate_expected_json tests/const_state_variables.sol "constable-states" +#generate_expected_json tests/external_function.sol "external-function" +#generate_expected_json tests/external_function_2.sol "external-function" +#generate_expected_json tests/naming_convention.sol "naming-convention" +#generate_expected_json tests/uninitialized_local_variable.sol "uninitialized-local" +#generate_expected_json tests/controlled_delegatecall.sol "controlled-delegatecall" +#generate_expected_json tests/constant.sol "constant-function" +#generate_expected_json tests/unused_return.sol "unused-return" +#generate_expected_json tests/shadowing_state_variable.sol "shadowing-state" +#generate_expected_json tests/shadowing_abstract.sol "shadowing-abstract" +#generate_expected_json tests/timestamp.sol "timestamp" +#generate_expected_json tests/multiple_calls_in_loop.sol "calls-loop" +#generate_expected_json tests/shadowing_builtin_symbols.sol "shadowing-builtin" +#generate_expected_json tests/shadowing_local_variable.sol "shadowing-local" +#generate_expected_json tests/solc_version_incorrect.sol "solc-version" +#generate_expected_json tests/right_to_left_override.sol "rtlo" +#generate_expected_json tests/unchecked_lowlevel.sol "unchecked-lowlevel" diff --git a/tests/expected_json/naming_convention.naming-convention.txt b/tests/expected_json/naming_convention.naming-convention.txt index 500b50836..25be91780 100644 --- a/tests/expected_json/naming_convention.naming-convention.txt +++ b/tests/expected_json/naming_convention.naming-convention.txt @@ -12,4 +12,4 @@ Parameter '_used' of _used (tests/naming_convention.sol#59) is not in mixedCase Variable 'T._myPublicVar' (tests/naming_convention.sol#56) is not in mixedCase Variable 'T.l' (tests/naming_convention.sol#67) used l, O, I, which should not be used Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#conformance-to-solidity-naming-conventions -INFO:Slither:tests/naming_convention.sol analyzed (3 contracts), 12 result(s) found +INFO:Slither:tests/naming_convention.sol analyzed (4 contracts), 12 result(s) found diff --git a/tests/naming_convention.sol b/tests/naming_convention.sol index 841c9f743..6c4b2f936 100644 --- a/tests/naming_convention.sol +++ b/tests/naming_convention.sol @@ -69,7 +69,7 @@ contract T { contract ParameterNameEmptyString { - function foo (string) { + function foo (uint) public { } }