diff --git a/README.md b/README.md index 5ae83bf05..04e7501e5 100644 --- a/README.md +++ b/README.md @@ -60,18 +60,18 @@ Num | Detector | What it Detects | Impact | Confidence 5 | `controlled-delegatecall` | [Controlled delegatecall destination](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#controlled-delegatecall) | High | Medium 6 | `reentrancy` | [Reentrancy vulnerabilities](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities) | High | Medium 7 | `locked-ether` | [Contracts that lock ether](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#contracts-that-lock-ether) | Medium | High -8 | `constant-function` | [Constant functions changing the state](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#constant-functions-changing-the-state) | Medium | Medium +8 | `constant-func` | [Constant functions changing the state](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#constant-functions-changing-the-state) | Medium | Medium 9 | `tx-origin` | [Dangerous usage of `tx.origin`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-usage-of-txorigin) | Medium | Medium 10 | `uninitialized-local` | [Uninitialized local variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-local-variables) | Medium | Medium -11 | `assembly` | [Assembly usage](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#assembly-usage) | Informational | High -12 | `constable-states` | [State variables that could be declared constant](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variables-that-could-be-declared-constant) | Informational | High -13 | `external-function` | [Public function that could be declared as external](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#public-function-that-could-be-declared-as-external) | Informational | High -14 | `low-level-calls` | [Low level calls](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#low-level-calls) | Informational | High -15 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#conformance-to-solidity-naming-conventions) | Informational | High -16 | `pragma` | [If different pragma directives are used](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variables-that-could-be-declared-constant) | Informational | High -17 | `solc-version` | [Old versions of Solidity (< 0.4.23)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#old-versions-of-solidity) | Informational | High -18 | `unused-state` | [Unused state variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-state-variables) | Informational | High - +11 | `unused-return` | [Unused return values](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-return) | Low | Medium +12 | `assembly` | [Assembly usage](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#assembly-usage) | Informational | High +13 | `constable-states` | [State variables that could be declared constant](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variables-that-could-be-declared-constant) | Informational | High +14 | `external-function` | [Public function that could be declared as external](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#public-function-that-could-be-declared-as-external) | Informational | High +15 | `low-level-calls` | [Low level calls](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#low-level-calls) | Informational | High +16 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#conformance-to-solidity-naming-conventions) | Informational | High +17 | `pragma` | [If different pragma directives are used](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variables-that-could-be-declared-constant) | Informational | High +18 | `solc-version` | [Old versions of Solidity (< 0.4.23)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#old-versions-of-solidity) | Informational | High +19 | `unused-state` | [Unused state variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-state-variables) | Informational | High [Contact us](https://www.trailofbits.com/contact/) to get access to additional detectors. diff --git a/scripts/tests_generate_expected_json.sh b/scripts/tests_generate_expected_json.sh index 882b56e37..1474bf9b4 100755 --- a/scripts/tests_generate_expected_json.sh +++ b/scripts/tests_generate_expected_json.sh @@ -38,4 +38,5 @@ generate_expected_json(){ #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 "const-func" +#generate_expected_json tests/constant.sol "constant-func" +generate_expected_json tests/unused_return.sol "unused-return" diff --git a/scripts/travis_test.sh b/scripts/travis_test.sh index a48c05943..5582b1bf6 100755 --- a/scripts/travis_test.sh +++ b/scripts/travis_test.sh @@ -86,7 +86,10 @@ test_slither tests/external_function.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.sol "const-func" +test_slither tests/constant.sol "constant-function" +# TODO: update to the new testing framework +test_slither tests/unused_return.sol "unused-return" + ### Test scripts diff --git a/slither/__main__.py b/slither/__main__.py index bafb8a91b..94d1c6ab0 100644 --- a/slither/__main__.py +++ b/slither/__main__.py @@ -122,6 +122,7 @@ def get_detectors_and_printers(): from slither.detectors.statements.tx_origin import TxOrigin from slither.detectors.statements.assembly import Assembly from slither.detectors.operations.low_level_calls import LowLevelCalls + from slither.detectors.operations.unused_return_values import UnusedReturnValues from slither.detectors.naming_convention.naming_convention import NamingConvention from slither.detectors.functions.external_function import ExternalFunction from slither.detectors.statements.controlled_delegatecall import ControlledDelegateCall @@ -144,6 +145,7 @@ def get_detectors_and_printers(): NamingConvention, ConstCandidateStateVars, #ComplexFunction, + UnusedReturnValues, ExternalFunction, ControlledDelegateCall, ConstantFunctions] diff --git a/slither/detectors/operations/unused_return_values.py b/slither/detectors/operations/unused_return_values.py new file mode 100644 index 000000000..817767377 --- /dev/null +++ b/slither/detectors/operations/unused_return_values.py @@ -0,0 +1,68 @@ +""" +Module detecting unused return values from external calls +""" + +from collections import defaultdict +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.slithir.operations.high_level_call import HighLevelCall +from slither.core.variables.state_variable import StateVariable + +class UnusedReturnValues(AbstractDetector): + """ + If the return value of a function is never used, it's likely to be bug + """ + + ARGUMENT = 'unused-return' + HELP = 'Unused return values' + IMPACT = DetectorClassification.LOW + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-return' + + def detect_unused_return_values(self, f): + """ + Return the nodes where the return value of a call is unused + Args: + f (Function) + Returns: + list(Node) + """ + values_returned = [] + nodes_origin = {} + for n in f.nodes: + for ir in n.irs: + if isinstance(ir, HighLevelCall): + # if a return value is stored in a state variable, it's ok + if ir.lvalue and not isinstance(ir.lvalue, StateVariable): + values_returned.append(ir.lvalue) + nodes_origin[ir.lvalue] = ir + for read in ir.read: + if read in values_returned: + values_returned.remove(read) + + return [nodes_origin[value].node for value in values_returned] + + def detect(self): + """ Detect unused high level calls that return a value but are never used + """ + results = [] + for c in self.slither.contracts: + for f in c.functions + c.modifiers: + unused_return = self.detect_unused_return_values(f) + if unused_return: + info = "{}.{} ({}) does not use the value returned by external calls:\n" + info = info.format(f.contract.name, + f.name, + f.source_mapping_str) + for node in unused_return: + info += "\t-{} ({})\n".format(node.expression, node.source_mapping_str) + self.log(info) + + sourceMapping = [v.source_mapping for v in unused_return] + + results.append({'vuln': 'UnusedReturn', + 'sourceMapping': sourceMapping, + 'filename': self.filename, + 'contract': c.name, + 'expressions':[str(n.expression) for n in unused_return]}) + return results diff --git a/slither/slithir/convert.py b/slither/slithir/convert.py index 283f9d4d7..b5149e038 100644 --- a/slither/slithir/convert.py +++ b/slither/slithir/convert.py @@ -124,6 +124,7 @@ def propage_type_and_convert_call(result, node): if isinstance(ins, TmpCall): new_ins = extract_tmp_call(ins) if new_ins: + new_ins.set_node(ins.node) ins = new_ins result[idx] = ins @@ -154,6 +155,7 @@ def propage_type_and_convert_call(result, node): new_ins = propagate_types(ins, node) if new_ins: + new_ins.set_node(ins.node) if isinstance(new_ins, (list,)): assert len(new_ins) == 2 result.insert(idx, new_ins[0]) @@ -250,6 +252,7 @@ def look_for_library(contract, ir, node, using_for, t): lib_call.arguments = [ir.destination] + ir.arguments new_ir = convert_type_library_call(lib_call, lib_contract) if new_ir: + new_ir.set_node(ir.node) return new_ir return None diff --git a/slither/slithir/operations/operation.py b/slither/slithir/operations/operation.py index c4c338c25..f957ed9be 100644 --- a/slither/slithir/operations/operation.py +++ b/slither/slithir/operations/operation.py @@ -1,5 +1,6 @@ import abc from slither.core.context.context import Context +from slither.core.children.child_node import ChildNode class AbstractOperation(abc.ABC): @@ -19,7 +20,7 @@ class AbstractOperation(abc.ABC): """ pass -class Operation(Context, AbstractOperation): +class Operation(Context, ChildNode, AbstractOperation): @property def used(self): @@ -27,3 +28,4 @@ class Operation(Context, AbstractOperation): By default used is all the variables read """ return self.read + diff --git a/slither/visitors/slithir/expression_to_slithir.py b/slither/visitors/slithir/expression_to_slithir.py index 77215da96..1b5c088a5 100644 --- a/slither/visitors/slithir/expression_to_slithir.py +++ b/slither/visitors/slithir/expression_to_slithir.py @@ -66,6 +66,8 @@ class ExpressionToSlithIR(ExpressionVisitor): self._node = node self._result = [] self._visit_expression(self.expression) + for ir in self._result: + ir.set_node(node) def result(self): return self._result diff --git a/tests/expected_json/unused_return.unused-return.json b/tests/expected_json/unused_return.unused-return.json new file mode 100644 index 000000000..91e0b8a27 --- /dev/null +++ b/tests/expected_json/unused_return.unused-return.json @@ -0,0 +1,29 @@ +[ + { + "contract": "User", + "expressions": [ + "a.add(0)", + "t.f()" + ], + "filename": "tests/unused_return.sol", + "sourceMapping": [ + { + "filename": "tests/unused_return.sol", + "length": 5, + "lines": [ + 18 + ], + "start": 263 + }, + { + "filename": "tests/unused_return.sol", + "length": 8, + "lines": [ + 22 + ], + "start": 337 + } + ], + "vuln": "UnusedReturn" + } +] \ No newline at end of file diff --git a/tests/unused_return.sol b/tests/unused_return.sol new file mode 100644 index 000000000..dab5ee766 --- /dev/null +++ b/tests/unused_return.sol @@ -0,0 +1,30 @@ +pragma solidity ^0.4.24; + +library SafeMath{ + function add(uint a, uint b) public returns(uint){ + return a+b; + } +} + +contract Target{ + function f() returns(uint); +} + +contract User{ + + using SafeMath for uint; + + function test(Target t){ + t.f(); + + // example with library usage + uint a; + a.add(0); + + // The value is not used + // But the detector should not detect it + // As the value returned by the call is stored + // (unused local variable should be another issue) + uint b = a.add(1); + } +}