diff --git a/scripts/tests_generate_expected_json_4.sh b/scripts/tests_generate_expected_json_4.sh index c44898d9f..302f318b3 100755 --- a/scripts/tests_generate_expected_json_4.sh +++ b/scripts/tests_generate_expected_json_4.sh @@ -46,7 +46,8 @@ generate_expected_json tests/reentrancy.sol "reentrancy-unlimited-gas" #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/constant.sol "constant-function-asm" +#generate_expected_json tests/constant.sol "constant-function-state" #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" diff --git a/scripts/tests_generate_expected_json_5.sh b/scripts/tests_generate_expected_json_5.sh index 620fc0b0c..8804974a5 100755 --- a/scripts/tests_generate_expected_json_5.sh +++ b/scripts/tests_generate_expected_json_5.sh @@ -32,7 +32,8 @@ generate_expected_json(){ #generate_expected_json tests/arbitrary_send-0.5.1.sol "arbitrary-send" #generate_expected_json tests/inline_assembly_contract-0.5.1.sol "assembly" #generate_expected_json tests/inline_assembly_library-0.5.1.sol "assembly" -#generate_expected_json tests/constant-0.5.1.sol "constant-function" +#generate_expected_json tests/constant-0.5.1.sol "constant-function-asm" +#generate_expected_json tests/constant-0.5.1.sol "constant-function-state" #generate_expected_json tests/incorrect_equality.sol "incorrect-equality" #generate_expected_json tests/too_many_digits.sol "too-many-digits" #generate_expected_json tests/unchecked_lowlevel-0.5.1.sol "unchecked-lowlevel" diff --git a/scripts/travis_test_4.sh b/scripts/travis_test_4.sh index 01e5088df..aae98669e 100755 --- a/scripts/travis_test_4.sh +++ b/scripts/travis_test_4.sh @@ -95,7 +95,8 @@ test_slither tests/naming_convention.sol "naming-convention" #test_slither tests/complex_func.sol "complex-function" test_slither tests/controlled_delegatecall.sol "controlled-delegatecall" test_slither tests/uninitialized_local_variable.sol "uninitialized-local" -test_slither tests/constant.sol "constant-function" +test_slither tests/constant.sol "constant-function-asm" +test_slither tests/constant.sol "constant-function-state" test_slither tests/unused_return.sol "unused-return" test_slither tests/shadowing_abstract.sol "shadowing-abstract" test_slither tests/shadowing_state_variable.sol "shadowing-state" diff --git a/scripts/travis_test_5.sh b/scripts/travis_test_5.sh index 4b3928b5c..d0f91b1b4 100755 --- a/scripts/travis_test_5.sh +++ b/scripts/travis_test_5.sh @@ -91,7 +91,8 @@ test_slither tests/external_function_2.sol "external-function" test_slither tests/naming_convention.sol "naming-convention" #test_slither tests/complex_func.sol "complex-function" test_slither tests/controlled_delegatecall.sol "controlled-delegatecall" -test_slither tests/constant-0.5.1.sol "constant-function" +test_slither tests/constant-0.5.1.sol "constant-function-asm" +test_slither tests/constant-0.5.1.sol "constant-function-state" test_slither tests/unused_return.sol "unused-return" test_slither tests/timestamp.sol "timestamp" test_slither tests/incorrect_equality.sol "incorrect-equality" diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index e2f1dc368..8e5412773 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -22,7 +22,8 @@ from .operations.unused_return_values import UnusedReturnValues from .naming_convention.naming_convention import NamingConvention from .functions.external_function import ExternalFunction from .statements.controlled_delegatecall import ControlledDelegateCall -from .attributes.const_functions import ConstantFunctions +from .attributes.const_functions_asm import ConstantFunctionsAsm +from .attributes.const_functions_state import ConstantFunctionsState from .shadowing.abstract import ShadowingAbstractDetection from .shadowing.state import StateShadowing from .shadowing.local import LocalShadowing diff --git a/slither/detectors/attributes/const_functions_asm.py b/slither/detectors/attributes/const_functions_asm.py new file mode 100644 index 000000000..5bf0bacfe --- /dev/null +++ b/slither/detectors/attributes/const_functions_asm.py @@ -0,0 +1,71 @@ +""" +Module detecting constant functions +Recursively check the called functions +""" +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.formatters.attributes.const_functions import format + + +class ConstantFunctionsAsm(AbstractDetector): + """ + Constant function detector + """ + + ARGUMENT = 'constant-function-asm' # run the detector with slither.py --ARGUMENT + HELP = 'Constant functions changing the state' # help information + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#constant-functions-changing-the-state' + + WIKI_TITLE = 'Constant functions using assembly code' + WIKI_DESCRIPTION = ''' +Functions declared as `constant`/`pure`/`view` using assembly code. + +`constant`/`pure`/`view` was not enforced prior Solidity 0.5. +Starting from Solidity 0.5, a call to a `constant`/`pure`/`view` function uses the `STATICCALL` opcode, which reverts in case of state modification. + +As a result, a call to an [incorrectly labeled function may trap a contract compiled with Solidity 0.5](https://solidity.readthedocs.io/en/develop/050-breaking-changes.html#interoperability-with-older-contracts).''' + + WIKI_EXPLOIT_SCENARIO = ''' +```solidity +contract Constant{ + uint counter; + function get() public view returns(uint){ + counter = counter +1; + return counter + } +} +``` +`Constant` was deployed with Solidity 0.4.25. Bob writes a smart contract interacting with `Constant` in Solidity 0.5.0. +All the calls to `get` revert, breaking Bob's smart contract execution.''' + + WIKI_RECOMMENDATION = 'Ensure that the attributes of contracts compiled prior to Solidity 0.5.0 are correct.' + + def _detect(self): + """ Detect the constant function using assembly code + + Recursively visit the calls + Returns: + list: {'vuln', 'filename,'contract','func','#varsWritten'} + """ + results = [] + if self.slither._solc_version < "0.5.0": + for c in self.contracts: + for f in c.functions: + if f.contract_declarer != c: + continue + if f.view or f.pure: + if f.contains_assembly: + attr = 'view' if f.view else 'pure' + + info = [f, f' is declared {attr} but contains assembly code\n'] + res = self.generate_result(info, {'contains_assembly': True}) + + results.append(res) + + return results + + @staticmethod + def _format(slither, result): + format(slither, result) diff --git a/slither/detectors/attributes/const_functions.py b/slither/detectors/attributes/const_functions_state.py similarity index 62% rename from slither/detectors/attributes/const_functions.py rename to slither/detectors/attributes/const_functions_state.py index 6e892a626..0bf8fdd5f 100644 --- a/slither/detectors/attributes/const_functions.py +++ b/slither/detectors/attributes/const_functions_state.py @@ -6,12 +6,12 @@ from slither.detectors.abstract_detector import AbstractDetector, DetectorClassi from slither.formatters.attributes.const_functions import format -class ConstantFunctions(AbstractDetector): +class ConstantFunctionsState(AbstractDetector): """ Constant function detector """ - ARGUMENT = 'constant-function' # run the detector with slither.py --ARGUMENT + ARGUMENT = 'constant-function-state' # run the detector with slither.py --ARGUMENT HELP = 'Constant functions changing the state' # help information IMPACT = DetectorClassification.MEDIUM CONFIDENCE = DetectorClassification.MEDIUM @@ -20,7 +20,7 @@ class ConstantFunctions(AbstractDetector): WIKI_TITLE = 'Constant functions changing the state' WIKI_DESCRIPTION = ''' -Functions declared as `constant`/`pure`/`view` changing the state or using assembly code. +Functions declared as `constant`/`pure`/`view` changing the state. `constant`/`pure`/`view` was not enforced prior Solidity 0.5. Starting from Solidity 0.5, a call to a `constant`/`pure`/`view` function uses the `STATICCALL` opcode, which reverts in case of state modification. @@ -50,31 +50,24 @@ All the calls to `get` revert, breaking Bob's smart contract execution.''' list: {'vuln', 'filename,'contract','func','#varsWritten'} """ results = [] - for c in self.contracts: - for f in c.functions: - if f.contract_declarer != c: - continue - if f.view or f.pure: - if f.contains_assembly: - attr = 'view' if f.view else 'pure' + if self.slither._solc_version < "0.5.0": + for c in self.contracts: + for f in c.functions: + if f.contract_declarer != c: + continue + if f.view or f.pure: + variables_written = f.all_state_variables_written() + if variables_written: + attr = 'view' if f.view else 'pure' - info = [f, f' is declared {attr} but contains assembly code\n'] - res = self.generate_result(info, {'contains_assembly': True}) + info = [f, f' is declared {attr} but changes state variables:\n'] - results.append(res) + for variable_written in variables_written: + info += ['\t- ', variable_written, '\n'] - variables_written = f.all_state_variables_written() - if variables_written: - attr = 'view' if f.view else 'pure' + res = self.generate_result(info, {'contains_assembly': False}) - info = [f, f' is declared {attr} but changes state variables:\n'] - - for variable_written in variables_written: - info += ['\t- ', variable_written, '\n'] - - res = self.generate_result(info, {'contains_assembly': False}) - - results.append(res) + results.append(res) return results diff --git a/slither/tools/slither_format/__main__.py b/slither/tools/slither_format/__main__.py index 82256a5fa..66787ee0e 100644 --- a/slither/tools/slither_format/__main__.py +++ b/slither/tools/slither_format/__main__.py @@ -16,7 +16,8 @@ available_detectors = ["unused-state", "naming-convention", "external-function", "constable-states", - "constant-function"] + "constant-function-asm", + "constatnt-function-state"] detectors_to_run = [] diff --git a/slither/tools/slither_format/slither_format.py b/slither/tools/slither_format/slither_format.py index dd9a745ee..597c17753 100644 --- a/slither/tools/slither_format/slither_format.py +++ b/slither/tools/slither_format/slither_format.py @@ -6,7 +6,8 @@ from slither.detectors.attributes.constant_pragma import ConstantPragma from slither.detectors.naming_convention.naming_convention import NamingConvention from slither.detectors.functions.external_function import ExternalFunction from slither.detectors.variables.possible_const_state_variables import ConstCandidateStateVars -from slither.detectors.attributes.const_functions import ConstantFunctions +from slither.detectors.attributes.const_functions_asm import ConstantFunctionsAsm +from slither.detectors.attributes.const_functions_state import ConstantFunctionsState from slither.utils.colors import yellow logging.basicConfig(level=logging.INFO) @@ -19,7 +20,8 @@ all_detectors = { 'naming-convention': NamingConvention, 'external-function': ExternalFunction, 'constable-states' : ConstCandidateStateVars, - 'constant-function': ConstantFunctions + 'constant-function-asm': ConstantFunctionsAsm, + 'constant-functions-state': ConstantFunctionsState } def slither_format(slither, **kwargs): diff --git a/tests/constant-0.5.1.sol b/tests/constant-0.5.1.sol index 3293ab45d..fd3a4a192 100644 --- a/tests/constant-0.5.1.sol +++ b/tests/constant-0.5.1.sol @@ -1,3 +1,5 @@ +pragma solidity 0.5.1; + contract Constant { uint a; diff --git a/tests/expected_json/constant-0.5.1.constant-function-asm.json b/tests/expected_json/constant-0.5.1.constant-function-asm.json new file mode 100644 index 000000000..f8c07c1d8 --- /dev/null +++ b/tests/expected_json/constant-0.5.1.constant-function-asm.json @@ -0,0 +1,5 @@ +{ + "success": true, + "error": null, + "results": {} +} \ No newline at end of file diff --git a/tests/expected_json/constant-0.5.1.constant-function-asm.txt b/tests/expected_json/constant-0.5.1.constant-function-asm.txt new file mode 100644 index 000000000..e69de29bb diff --git a/tests/expected_json/constant-0.5.1.constant-function-state.json b/tests/expected_json/constant-0.5.1.constant-function-state.json new file mode 100644 index 000000000..f8c07c1d8 --- /dev/null +++ b/tests/expected_json/constant-0.5.1.constant-function-state.json @@ -0,0 +1,5 @@ +{ + "success": true, + "error": null, + "results": {} +} \ No newline at end of file diff --git a/tests/expected_json/constant-0.5.1.constant-function-state.txt b/tests/expected_json/constant-0.5.1.constant-function-state.txt new file mode 100644 index 000000000..e69de29bb diff --git a/tests/expected_json/constant-0.5.1.constant-function.txt b/tests/expected_json/constant-0.5.1.constant-function.txt deleted file mode 100644 index 1fa22f64d..000000000 --- a/tests/expected_json/constant-0.5.1.constant-function.txt +++ /dev/null @@ -1,4 +0,0 @@ - -Constant.test_assembly_bug() (tests/constant-0.5.1.sol#15-17) is declared view but contains assembly code -Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#constant-functions-changing-the-state -tests/constant-0.5.1.sol analyzed (1 contracts with 1 detectors), 1 result(s) found diff --git a/tests/expected_json/constant-0.5.1.constant-function.json b/tests/expected_json/constant.constant-function-asm.json similarity index 70% rename from tests/expected_json/constant-0.5.1.constant-function.json rename to tests/expected_json/constant.constant-function-asm.json index dab6537ea..bc754a1ac 100644 --- a/tests/expected_json/constant-0.5.1.constant-function.json +++ b/tests/expected_json/constant.constant-function-asm.json @@ -9,17 +9,17 @@ "type": "function", "name": "test_assembly_bug", "source_mapping": { - "start": 185, + "start": 324, "length": 66, - "filename_used": "/home/travis/build/crytic/slither/tests/constant-0.5.1.sol", - "filename_relative": "tests/constant-0.5.1.sol", - "filename_absolute": "/home/travis/build/crytic/slither/tests/constant-0.5.1.sol", - "filename_short": "tests/constant-0.5.1.sol", + "filename_used": "/home/travis/build/crytic/slither/tests/constant.sol", + "filename_relative": "tests/constant.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/constant.sol", + "filename_short": "tests/constant.sol", "is_dependency": false, "lines": [ - 15, - 16, - 17 + 22, + 23, + 24 ], "starting_column": 5, "ending_column": 6 @@ -30,11 +30,11 @@ "name": "Constant", "source_mapping": { "start": 0, - "length": 253, - "filename_used": "/home/travis/build/crytic/slither/tests/constant-0.5.1.sol", - "filename_relative": "tests/constant-0.5.1.sol", - "filename_absolute": "/home/travis/build/crytic/slither/tests/constant-0.5.1.sol", - "filename_short": "tests/constant-0.5.1.sol", + "length": 392, + "filename_used": "/home/travis/build/crytic/slither/tests/constant.sol", + "filename_relative": "tests/constant.sol", + "filename_absolute": "/home/travis/build/crytic/slither/tests/constant.sol", + "filename_short": "tests/constant.sol", "is_dependency": false, "lines": [ 1, @@ -54,7 +54,14 @@ 15, 16, 17, - 18 + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25 ], "starting_column": 1, "ending_column": 2 @@ -64,13 +71,13 @@ } } ], - "description": "Constant.test_assembly_bug() (tests/constant-0.5.1.sol#15-17) is declared view but contains assembly code\n", - "markdown": "[Constant.test_assembly_bug()](tests/constant-0.5.1.sol#L15-L17) is declared view but contains assembly code\n", + "description": "Constant.test_assembly_bug() (tests/constant.sol#22-24) is declared view but contains assembly code\n", + "markdown": "[Constant.test_assembly_bug()](tests/constant.sol#L22-L24) is declared view but contains assembly code\n", "id": "1f892cae08b89096bdc4d6ecdf55a3adc4b4314390e054fe2547d9c8e9f76e23", "additional_fields": { "contains_assembly": true }, - "check": "constant-function", + "check": "constant-function-asm", "impact": "Medium", "confidence": "Medium" } diff --git a/tests/expected_json/constant.constant-function-asm.txt b/tests/expected_json/constant.constant-function-asm.txt new file mode 100644 index 000000000..19daeb137 --- /dev/null +++ b/tests/expected_json/constant.constant-function-asm.txt @@ -0,0 +1,4 @@ + +Constant.test_assembly_bug() (tests/constant.sol#22-24) is declared view but contains assembly code +Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#constant-functions-changing-the-state +tests/constant.sol analyzed (1 contracts with 1 detectors), 3 result(s) found diff --git a/tests/expected_json/constant.constant-function.json b/tests/expected_json/constant.constant-function-state.json similarity index 77% rename from tests/expected_json/constant.constant-function.json rename to tests/expected_json/constant.constant-function-state.json index 40c0c626e..ff6a558b3 100644 --- a/tests/expected_json/constant.constant-function.json +++ b/tests/expected_json/constant.constant-function-state.json @@ -139,7 +139,7 @@ "additional_fields": { "contains_assembly": false }, - "check": "constant-function", + "check": "constant-function-state", "impact": "Medium", "confidence": "Medium" }, @@ -279,85 +279,7 @@ "additional_fields": { "contains_assembly": false }, - "check": "constant-function", - "impact": "Medium", - "confidence": "Medium" - }, - { - "elements": [ - { - "type": "function", - "name": "test_assembly_bug", - "source_mapping": { - "start": 324, - "length": 66, - "filename_used": "/home/travis/build/crytic/slither/tests/constant.sol", - "filename_relative": "tests/constant.sol", - "filename_absolute": "/home/travis/build/crytic/slither/tests/constant.sol", - "filename_short": "tests/constant.sol", - "is_dependency": false, - "lines": [ - 22, - 23, - 24 - ], - "starting_column": 5, - "ending_column": 6 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "Constant", - "source_mapping": { - "start": 0, - "length": 392, - "filename_used": "/home/travis/build/crytic/slither/tests/constant.sol", - "filename_relative": "tests/constant.sol", - "filename_absolute": "/home/travis/build/crytic/slither/tests/constant.sol", - "filename_short": "tests/constant.sol", - "is_dependency": false, - "lines": [ - 1, - 2, - 3, - 4, - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "test_assembly_bug()" - } - } - ], - "description": "Constant.test_assembly_bug() (tests/constant.sol#22-24) is declared view but contains assembly code\n", - "markdown": "[Constant.test_assembly_bug()](tests/constant.sol#L22-L24) is declared view but contains assembly code\n", - "id": "1f892cae08b89096bdc4d6ecdf55a3adc4b4314390e054fe2547d9c8e9f76e23", - "additional_fields": { - "contains_assembly": true - }, - "check": "constant-function", + "check": "constant-function-state", "impact": "Medium", "confidence": "Medium" } diff --git a/tests/expected_json/constant.constant-function.txt b/tests/expected_json/constant.constant-function-state.txt similarity index 82% rename from tests/expected_json/constant.constant-function.txt rename to tests/expected_json/constant.constant-function-state.txt index f2b695232..5c58c0f83 100644 --- a/tests/expected_json/constant.constant-function.txt +++ b/tests/expected_json/constant.constant-function-state.txt @@ -3,6 +3,5 @@ Constant.test_view_bug() (tests/constant.sol#5-7) is declared view but changes s - Constant.a (tests/constant.sol#3) Constant.test_constant_bug() (tests/constant.sol#9-11) is declared view but changes state variables: - Constant.a (tests/constant.sol#3) -Constant.test_assembly_bug() (tests/constant.sol#22-24) is declared view but contains assembly code Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#constant-functions-changing-the-state tests/constant.sol analyzed (1 contracts with 1 detectors), 3 result(s) found