From 510aa29b9952315eb8a0f1ad6ced6f15c49439a7 Mon Sep 17 00:00:00 2001 From: David Pokora Date: Sun, 6 Jan 2019 13:00:02 -0500 Subject: [PATCH 1/2] Rewrote external-function detector to handle false-positives as a result of inheritance. --- .../detectors/functions/external_function.py | 138 +++++++++++++++--- tests/external_function_test_3.sol | 55 +++++++ 2 files changed, 176 insertions(+), 17 deletions(-) create mode 100644 tests/external_function_test_3.sol diff --git a/slither/detectors/functions/external_function.py b/slither/detectors/functions/external_function.py index 006cb0192..6b39f6273 100644 --- a/slither/detectors/functions/external_function.py +++ b/slither/detectors/functions/external_function.py @@ -27,7 +27,10 @@ class ExternalFunction(AbstractDetector): (list): List of all InternallCall, SolidityCall """ result = [] + + # Obtain all functions reachable by this contract. for func in contract.all_functions_called: + # Loop through all nodes in the function, add all calls to a list. for node in func.nodes: for ir in node.irs: if isinstance(ir, (InternalCall, SolidityCall)): @@ -36,6 +39,12 @@ class ExternalFunction(AbstractDetector): @staticmethod def _contains_internal_dynamic_call(contract): + """ + Checks if a contract contains a dynamic call either in a direct definition, or through inheritance. + + Returns: + (boolean): True if this contract contains a dynamic call (including through inheritance). + """ for func in contract.all_functions_called: for node in func.nodes: for ir in node.irs: @@ -43,31 +52,126 @@ class ExternalFunction(AbstractDetector): return True return False + @staticmethod + def get_base_most_function(function): + """ + Obtains the base function definition for the provided function. This could be used to obtain the original + definition of a function, if the provided function is an override. + + Returns: + (function): Returns the base-most function of a provided function. (The original definition). + """ + # Loop through the list of inherited contracts and this contract, to find the first function instance which + # matches this function's signature. Note here that `inheritance` is in order from most basic to most extended. + for contract in function.contract.inheritance + [function.contract]: + + # Loop through the functions not inherited (explicitly defined in this contract). + for f in contract.functions_not_inherited: + + # If it matches names, this is the base most function. + if f.full_name == function.full_name: + return f + + # Somehow we couldn't resolve it, which shouldn't happen, as the provided function should be found if we could + # not find some any more basic. + raise Exception("Could not resolve the base-most function for the provided function.") + + @staticmethod + def get_all_function_definitions(base_most_function): + """ + Obtains all function definitions given a base-most function. This includes the provided function, plus any + overrides of that function. + + Returns: + (list): Returns any the provided function and any overriding functions defined for it. + """ + # We assume the provided function is the base-most function, so we check all derived contracts + # for a redefinition + return [base_most_function] + [function for derived_contract in base_most_function.contract.derived_contracts + for function in derived_contract.functions + if function.full_name == base_most_function.full_name] + def detect(self): results = [] - public_function_calls = [] all_info = '' - for contract in self.slither.contracts_derived: + # Create a set to track contracts with dynamic calls. All contracts with dynamic calls could potentially be + # calling functions internally, and thus we can't assume any function in such contracts isn't called by them. + dynamic_call_contracts = set() + + # Create a completed functions set to skip over functions already processed (any functions which are the base + # of, or override hierarchically are processed together). + completed_functions = set() + + # First we build our set of all contracts with dynamic calls + for contract in self.contracts: if self._contains_internal_dynamic_call(contract): + dynamic_call_contracts.add(contract) + + # Loop through all not-inherited contracts. + for contract in self.slither.contracts_derived: + + # Filter false-positives: Immediately filter this contract if it's in blacklist + if contract in dynamic_call_contracts: continue - func_list = self.detect_functions_called(contract) - public_function_calls.extend(func_list) - - for func in [f for f in contract.functions if f.visibility == 'public' and\ - not f in public_function_calls and\ - not f.is_constructor]: - txt = "{}.{} ({}) should be declared external\n" - info = txt.format(func.contract.name, - func.name, - func.source_mapping_str) - all_info += info - - json = self.generate_json_result(info) - self.add_function_to_json(func, json) - results.append(json) + # Next we'll want to loop through all functions defined directly in this contract. + for function in contract.functions_not_inherited: + + # If the function is a constructor, or is public, we skip it. + if function.is_constructor or function.visibility != "public": + continue + + # Optimization: If this function has already been processed, we stop. + if function in completed_functions: + continue + + # Get the base-most function to know our origin of this function. + base_most_function = self.get_base_most_function(function) + + # Get all possible contracts which can call this function (or an override). + all_possible_sources = [base_most_function.contract] + base_most_function.contract.derived_contracts + + # Get all function signatures (overloaded and not), mark as completed and we process them now. + # Note: We mark all function definitions as the same, as they must all share visibility to override. + all_function_definitions = set(self.get_all_function_definitions(base_most_function)) + completed_functions = completed_functions.union(all_function_definitions) + + # Filter false-positives: Determine if any of these sources have dynamic calls, if so, flag all of these + # function definitions, and then flag all functions in all contracts that make dynamic calls. + sources_with_dynamic_calls = set(all_possible_sources) & dynamic_call_contracts + if sources_with_dynamic_calls: + functions_in_dynamic_call_sources = set([f for dyn_contract in sources_with_dynamic_calls + for f in dyn_contract if not f.is_constructor]) + completed_functions = completed_functions.union(functions_in_dynamic_call_sources) + continue + + # Detect all functions called in each source, if any match our current signature, we skip + # otherwise, this is a candidate (in all sources) to be changed visibility for. + is_called = False + for possible_source in all_possible_sources: + functions_called = self.detect_functions_called(possible_source) + if set(functions_called) & all_function_definitions: + is_called = True + break + + # If any of this function's definitions are called, we skip it. + if is_called: + continue + + # Loop for each function definition, and recommend it be declared external. + for function_definition in all_function_definitions: + txt = "{}.{} ({}) should be declared external\n" + info = txt.format(function_definition.contract.name, + function_definition.name, + function_definition.source_mapping_str) + all_info += info + + json = self.generate_json_result(info) + self.add_function_to_json(function_definition, json) + results.append(json) + if all_info != '': self.log(all_info) return results diff --git a/tests/external_function_test_3.sol b/tests/external_function_test_3.sol new file mode 100644 index 000000000..07dab167c --- /dev/null +++ b/tests/external_function_test_3.sol @@ -0,0 +1,55 @@ +// This tests against false-positives. This test should output no recommendations from the external-function detector. + + +contract ContractWithBaseFunctionCalled { + function getsCalledByBase() public; + function callsOverrideMe() external { + getsCalledByBase(); + } +} + + +contract DerivingContractWithBaseCalled is ContractWithBaseFunctionCalled { + function getsCalledByBase() public { + // This should not be recommended to be marked external because it is called by the base class. + } +} + + +// All the contracts below should not recommend changing to external since inherited contracts have dynamic calls. +contract ContractWithDynamicCall { + function() returns(uint) ptr; + + function test1() public returns(uint){ + return 1; + } + + function test2() public returns(uint){ + return 2; + } + + function set_test1() external{ + ptr = test1; + } + + function set_test2() external{ + ptr = test2; + } + + function exec() external returns(uint){ + return ptr(); + } +} + +contract DerivesFromDynamicCall is ContractWithDynamicCall{ + function getsCalledDynamically() public returns (uint){ + // This should not be recommended because it is called dynamically. + return 3; + } + function set_test3() public { + // This should not be recommended because we inherit from a contract that calls dynamically, and we cannot be + // sure it did not somehow call this function. + + ptr = getsCalledDynamically; + } +} \ No newline at end of file From af646da29ed40f8602a9755cf90cc20f1b8669dd Mon Sep 17 00:00:00 2001 From: Josselin Date: Mon, 7 Jan 2019 09:13:54 +0000 Subject: [PATCH 2/2] Update travis tests --- scripts/tests_generate_expected_json_4.sh | 51 ++++++++++--------- scripts/tests_generate_expected_json_5.sh | 16 +++--- scripts/travis_test_4.sh | 1 + scripts/travis_test_5.sh | 3 +- ...external_function_2.external-function.json | 1 + tests/external_function.sol | 2 +- ...ion_test_3.sol => external_function_2.sol} | 8 +-- ...est_2.sol => external_function_import.sol} | 2 +- 8 files changed, 44 insertions(+), 40 deletions(-) create mode 100644 tests/expected_json/external_function_2.external-function.json rename tests/{external_function_test_3.sol => external_function_2.sol} (93%) rename tests/{external_function_test_2.sol => external_function_import.sol} (68%) diff --git a/scripts/tests_generate_expected_json_4.sh b/scripts/tests_generate_expected_json_4.sh index 5bedd2f69..00d0b2b14 100755 --- a/scripts/tests_generate_expected_json_4.sh +++ b/scripts/tests_generate_expected_json_4.sh @@ -14,28 +14,29 @@ generate_expected_json(){ } -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" -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/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/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" +#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" diff --git a/scripts/tests_generate_expected_json_5.sh b/scripts/tests_generate_expected_json_5.sh index a6b0feea2..63a60a668 100755 --- a/scripts/tests_generate_expected_json_5.sh +++ b/scripts/tests_generate_expected_json_5.sh @@ -14,16 +14,16 @@ generate_expected_json(){ } -generate_expected_json tests/uninitialized-0.5.1.sol "uninitialized-state" +#generate_expected_json tests/uninitialized-0.5.1.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-0.5.1.sol "reentrancy" +#generate_expected_json tests/reentrancy-0.5.1.sol "reentrancy" #generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage" -generate_expected_json tests/tx_origin-0.5.1.sol "tx-origin" -generate_expected_json tests/locked_ether-0.5.1.sol "locked-ether" -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/tx_origin-0.5.1.sol "tx-origin" +#generate_expected_json tests/locked_ether-0.5.1.sol "locked-ether" +#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" diff --git a/scripts/travis_test_4.sh b/scripts/travis_test_4.sh index 6395537a8..29b9bceef 100755 --- a/scripts/travis_test_4.sh +++ b/scripts/travis_test_4.sh @@ -81,6 +81,7 @@ test_slither tests/inline_assembly_library.sol "assembly" test_slither tests/low_level_calls.sol "low-level-calls" test_slither tests/const_state_variables.sol "constable-states" test_slither tests/external_function.sol "external-function" +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" diff --git a/scripts/travis_test_5.sh b/scripts/travis_test_5.sh index d7af73720..24f258f19 100755 --- a/scripts/travis_test_5.sh +++ b/scripts/travis_test_5.sh @@ -79,8 +79,9 @@ test_slither tests/inline_assembly_library-0.5.1.sol "assembly" test_slither tests/low_level_calls.sol "low-level-calls" test_slither tests/const_state_variables.sol "constable-states" test_slither tests/external_function.sol "external-function" +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/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/unused_return.sol "unused-return" diff --git a/tests/expected_json/external_function_2.external-function.json b/tests/expected_json/external_function_2.external-function.json new file mode 100644 index 000000000..0637a088a --- /dev/null +++ b/tests/expected_json/external_function_2.external-function.json @@ -0,0 +1 @@ +[] \ No newline at end of file diff --git a/tests/external_function.sol b/tests/external_function.sol index 714e6997f..1ed7c70c0 100644 --- a/tests/external_function.sol +++ b/tests/external_function.sol @@ -1,6 +1,6 @@ //pragma solidity ^0.4.24; -import "./external_function_test_2.sol"; +import "./external_function_import.sol"; contract ContractWithFunctionCalledSuper is ContractWithFunctionCalled { function callWithSuper() public { diff --git a/tests/external_function_test_3.sol b/tests/external_function_2.sol similarity index 93% rename from tests/external_function_test_3.sol rename to tests/external_function_2.sol index 07dab167c..97eed10a0 100644 --- a/tests/external_function_test_3.sol +++ b/tests/external_function_2.sol @@ -28,11 +28,11 @@ contract ContractWithDynamicCall { return 2; } - function set_test1() external{ + function setTest1() external{ ptr = test1; } - function set_test2() external{ + function setTest2() external{ ptr = test2; } @@ -46,10 +46,10 @@ contract DerivesFromDynamicCall is ContractWithDynamicCall{ // This should not be recommended because it is called dynamically. return 3; } - function set_test3() public { + function setTest3() public { // This should not be recommended because we inherit from a contract that calls dynamically, and we cannot be // sure it did not somehow call this function. ptr = getsCalledDynamically; } -} \ No newline at end of file +} diff --git a/tests/external_function_test_2.sol b/tests/external_function_import.sol similarity index 68% rename from tests/external_function_test_2.sol rename to tests/external_function_import.sol index ae7388993..efddea28e 100644 --- a/tests/external_function_test_2.sol +++ b/tests/external_function_import.sol @@ -1,4 +1,4 @@ -//pragma solidity ^0.4.24; +// This file is imported by external_function.sol contract ContractWithFunctionCalled { function funcCalled() external {