Merge branch 'benstew-unused-return-values' into dev

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
+11 | `unused-return` | [Unused return values](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-return) | Low | Medium
-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
+12 | `assembly` | [Assembly usage](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#assembly-usage) | 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
+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 | `low-level-calls` | [Low level calls](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#low-level-calls) | 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 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#conformance-to-solidity-naming-conventions) | Informational | High
+15 | `low-level-calls` | [Low level calls](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#low-level-calls) | 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
+16 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#conformance-to-solidity-naming-conventions) | 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
+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 | `unused-state` | [Unused state variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-state-variables) | 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.
 

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"

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
 

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]

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

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
 

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
+

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

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"
+    }
+]

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);
+    }
+}