Add two new detectors:

- unchecked send
- unchecked low level
Add Internal/InternalDyanmicCall to unused return value detector

Close 224

scripts/tests_generate_expected_json_4.sh

@@ -54,3 +54,4 @@ generate_expected_json(){
 #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"

scripts/tests_generate_expected_json_5.sh

@@ -35,4 +35,6 @@ generate_expected_json(){
 #generate_expected_json tests/constant-0.5.1.sol "constant-function"
 #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"
+generate_expected_json tests/unchecked_send-0.5.1.sol "unchecked-send"
 

scripts/travis_test_4.sh

@@ -69,6 +69,7 @@ test_slither(){
 }
 
 
+test_slither tests/unchecked_lowlevel.sol "unchecked-lowlevel"
 test_slither tests/deprecated_calls.sol "deprecated-standards"
 test_slither tests/erc20_indexed.sol "erc20-indexed"
 test_slither tests/incorrect_erc20_interface.sol "erc20-interface"

scripts/travis_test_5.sh

@@ -69,6 +69,8 @@ test_slither(){
 }
 
 
+test_slither tests/unchecked_lowlevel-0.5.1.sol "unchecked-lowlevel"
+test_slither tests/unchecked_send-0.5.1.sol "unchecked-send"
 test_slither tests/uninitialized-0.5.1.sol "uninitialized-state"
 test_slither tests/backdoor.sol "backdoor"
 test_slither tests/backdoor.sol "suicidal"

slither/detectors/all_detectors.py

@@ -34,5 +34,7 @@ from .erc.unindexed_event_parameters import UnindexedERC20EventParameters
 from .statements.deprecated_calls import DeprecatedStandards
 from .source.rtlo import RightToLeftOverride
 from .statements.too_many_digits import TooManyDigits
+from .operations.unchecked_low_level_return_values import UncheckedLowLevel
+from .operations.unchecked_send_return_value import UncheckedSend
 #
 #

slither/detectors/operations/unchecked_low_level_return_values.py

@@ -0,0 +1,43 @@
+"""
+Module detecting unused return values from low level
+"""
+from slither.detectors.abstract_detector import DetectorClassification
+from .unused_return_values import UnusedReturnValues
+from slither.slithir.operations import LowLevelCall
+
+class UncheckedLowLevel(UnusedReturnValues):
+    """
+    If the return value of a send is not checked, it might lead to losing ether
+    """
+
+    ARGUMENT = 'unchecked-lowlevel'
+    HELP = 'Unchecked low-level calls'
+    IMPACT = DetectorClassification.MEDIUM
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#unchecked-low-level'
+
+    WIKI_TITLE = 'Unchecked low-level calls'
+    WIKI_DESCRIPTION = 'The return value of a low-level call is not checked.'
+    WIKI_EXPLOIT_SCENARIO = '''
+```solidity
+contract MyConc{
+    function my_func(address payable dst) public payable{
+        dst.call.value(msg.value)("");
+    }
+}
+```
+The return value of the low-level call is not checked. As a result if the callfailed, the ether will be locked in the contract.
+If the low level is used to prevent blocking operations, consider logging failed calls.
+    '''
+
+    WIKI_RECOMMENDATION = 'Ensure that the return value of low-level call is checked or logged.'
+
+    _txt_description = "low-level calls"
+
+    def _is_instance(self, ir):
+        return isinstance(ir, LowLevelCall)
+
+
+
+

slither/detectors/operations/unchecked_send_return_value.py

@@ -0,0 +1,40 @@
+"""
+Module detecting unused return values from send
+"""
+
+from slither.detectors.abstract_detector import DetectorClassification
+from .unused_return_values import UnusedReturnValues
+from slither.slithir.operations import Send
+
+class UncheckedSend(UnusedReturnValues):
+    """
+    If the return value of a send is not checked, it might lead to losing ether
+    """
+
+    ARGUMENT = 'unchecked-send'
+    HELP = 'Unchecked send'
+    IMPACT = DetectorClassification.MEDIUM
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#unchecked-send'
+
+    WIKI_TITLE = 'Unchecked Send'
+    WIKI_DESCRIPTION = 'The return value of a send is not checked.'
+    WIKI_EXPLOIT_SCENARIO = '''
+```solidity
+contract MyConc{
+    function my_func(address payable dst) public payable{
+        dst.send(msg.value);
+    }
+}
+```
+The return value of `send` is not checked. As a result if the send failed, the ether will be locked in the contract.
+If `send` is used to prevent blocking operations, consider logging the failed sent.
+    '''
+
+    WIKI_RECOMMENDATION = 'Ensure that the return value of send is checked or logged.'
+
+    _txt_description = "send calls"
+
+    def _is_instance(self, ir):
+        return isinstance(ir, Send)

slither/detectors/operations/unused_return_values.py

@@ -2,9 +2,8 @@
 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.slithir.operations import HighLevelCall, InternalCall, InternalDynamicCall
 from slither.core.variables.state_variable import StateVariable
 
 class UnusedReturnValues(AbstractDetector):
@@ -19,7 +18,6 @@ class UnusedReturnValues(AbstractDetector):
 
     WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#unused-return'
 
-
     WIKI_TITLE = 'Unused return'
     WIKI_DESCRIPTION = 'The return value of an external call is not stored in a local or state variable.'
     WIKI_EXPLOIT_SCENARIO = '''
@@ -33,7 +31,12 @@ contract MyConc{
 ```
 `MyConc` calls `add` of SafeMath, but does not store the result in `a`. As a result, the computation has no effect.'''
 
-    WIKI_RECOMMENDATION = 'Ensure that all the return values of the function calls are stored in a local or state variable.'
+    WIKI_RECOMMENDATION = 'Ensure that all the return values of the function calls are used.'
+
+    _txt_description = "external functions"
+
+    def _is_instance(self, ir):
+        return isinstance(ir, HighLevelCall)
 
     def detect_unused_return_values(self, f):
         """
@@ -47,7 +50,7 @@ contract MyConc{
         nodes_origin = {}
         for n in f.nodes:
             for ir in n.irs:
-                if isinstance(ir, HighLevelCall):
+                if self._is_instance(ir):
                     # 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)
@@ -68,10 +71,11 @@ contract MyConc{
                     continue
                 unused_return = self.detect_unused_return_values(f)
                 if unused_return:
-                    info = "{}.{} ({}) does not use the value returned by external calls:\n"
+                    info = "{}.{} ({}) does not use the value returned by {}:\n"
                     info = info.format(f.contract.name,
                                        f.name,
-                                       f.source_mapping_str)
+                                       f.source_mapping_str,
+                                       self._txt_description)
                     for node in unused_return:
                         info += "\t-{} ({})\n".format(node.expression, node.source_mapping_str)
 
@@ -81,3 +85,4 @@ contract MyConc{
                     results.append(json)
 
         return results
+

slither/slither.py

@@ -55,7 +55,7 @@ class Slither(SlitherSolc):
                 crytic_compile = CryticCompile(contract, **kwargs)
                 self._crytic_compile = crytic_compile
             except InvalidCompilation as e:
-                raise SlitherError('Invalid compilation: '+e)
+                raise SlitherError('Invalid compilation: \n'+str(e))
             for path, ast in crytic_compile.asts.items():
                 self._parse_contracts_from_loaded_json(ast, path)
                 self._add_source_code(path)
@@ -160,7 +160,7 @@ class Slither(SlitherSolc):
                 )
             )
 
-        if any(isinstance(obj, cls) for obj in instances_list):
+        if any(type(obj) == cls for obj in instances_list):
             raise Exception(
                 "You can't register {!r} twice.".format(cls)
             )

tests/expected_json/unchecked_lowlevel-0.5.1.unchecked-lowlevel.json

@@ -0,0 +1,73 @@
+[
+  {
+    "check": "unchecked-lowlevel",
+    "impact": "Medium",
+    "confidence": "Medium",
+    "description": "MyConc.bad (tests/unchecked_lowlevel-0.5.1.sol#2-4) does not use the value returned by low-level calls:\n\t-dst.call.value(msg.value)() (tests/unchecked_lowlevel-0.5.1.sol#3)\n",
+    "elements": [
+      {
+        "type": "function",
+        "name": "bad",
+        "source_mapping": {
+          "start": 21,
+          "length": 96,
+          "filename_used": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel-0.5.1.sol",
+          "filename_relative": "tests/unchecked_lowlevel-0.5.1.sol",
+          "filename_absolute": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel-0.5.1.sol",
+          "filename_short": "tests/unchecked_lowlevel-0.5.1.sol",
+          "lines": [
+            2,
+            3,
+            4
+          ],
+          "starting_column": 5,
+          "ending_column": 6
+        },
+        "contract": {
+          "type": "contract",
+          "name": "MyConc",
+          "source_mapping": {
+            "start": 0,
+            "length": 274,
+            "filename_used": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel-0.5.1.sol",
+            "filename_relative": "tests/unchecked_lowlevel-0.5.1.sol",
+            "filename_absolute": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel-0.5.1.sol",
+            "filename_short": "tests/unchecked_lowlevel-0.5.1.sol",
+            "lines": [
+              1,
+              2,
+              3,
+              4,
+              5,
+              6,
+              7,
+              8,
+              9,
+              10,
+              11
+            ],
+            "starting_column": 1,
+            "ending_column": 2
+          }
+        }
+      },
+      {
+        "type": "expression",
+        "expression": "dst.call.value(msg.value)()",
+        "source_mapping": {
+          "start": 81,
+          "length": 29,
+          "filename_used": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel-0.5.1.sol",
+          "filename_relative": "tests/unchecked_lowlevel-0.5.1.sol",
+          "filename_absolute": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel-0.5.1.sol",
+          "filename_short": "tests/unchecked_lowlevel-0.5.1.sol",
+          "lines": [
+            3
+          ],
+          "starting_column": 9,
+          "ending_column": 38
+        }
+      }
+    ]
+  }
+]

tests/expected_json/unchecked_lowlevel-0.5.1.unchecked-lowlevel.txt

@@ -0,0 +1,6 @@
+INFO:Detectors:
+MyConc.bad (tests/unchecked_lowlevel-0.5.1.sol#2-4) does not use the value returned by low-level calls:
+	-dst.call.value(msg.value)() (tests/unchecked_lowlevel-0.5.1.sol#3)
+Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unchecked-low-level
+INFO:Slither:/home/monty/Private/tob/tools/slither-public/scripts/../tests/expected_json/unchecked_lowlevel-0.5.1.unchecked-lowlevel.json exists already, the overwrite is prevented
+INFO:Slither:tests/unchecked_lowlevel-0.5.1.sol analyzed (1 contracts), 1 result(s) found

tests/expected_json/unchecked_lowlevel.unchecked-lowlevel.json

@@ -0,0 +1,72 @@
+[
+  {
+    "check": "unchecked-lowlevel",
+    "impact": "Medium",
+    "confidence": "Medium",
+    "description": "MyConc.bad (tests/unchecked_lowlevel.sol#2-4) does not use the value returned by low-level calls:\n\t-dst.call.value(msg.value)() (tests/unchecked_lowlevel.sol#3)\n",
+    "elements": [
+      {
+        "type": "function",
+        "name": "bad",
+        "source_mapping": {
+          "start": 21,
+          "length": 88,
+          "filename_used": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel.sol",
+          "filename_relative": "tests/unchecked_lowlevel.sol",
+          "filename_absolute": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel.sol",
+          "filename_short": "tests/unchecked_lowlevel.sol",
+          "lines": [
+            2,
+            3,
+            4
+          ],
+          "starting_column": 5,
+          "ending_column": 6
+        },
+        "contract": {
+          "type": "contract",
+          "name": "MyConc",
+          "source_mapping": {
+            "start": 0,
+            "length": 214,
+            "filename_used": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel.sol",
+            "filename_relative": "tests/unchecked_lowlevel.sol",
+            "filename_absolute": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel.sol",
+            "filename_short": "tests/unchecked_lowlevel.sol",
+            "lines": [
+              1,
+              2,
+              3,
+              4,
+              5,
+              6,
+              7,
+              8,
+              9,
+              10
+            ],
+            "starting_column": 1,
+            "ending_column": 2
+          }
+        }
+      },
+      {
+        "type": "expression",
+        "expression": "dst.call.value(msg.value)()",
+        "source_mapping": {
+          "start": 73,
+          "length": 29,
+          "filename_used": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel.sol",
+          "filename_relative": "tests/unchecked_lowlevel.sol",
+          "filename_absolute": "/home/travis/build/crytic/slither/tests/unchecked_lowlevel.sol",
+          "filename_short": "tests/unchecked_lowlevel.sol",
+          "lines": [
+            3
+          ],
+          "starting_column": 9,
+          "ending_column": 38
+        }
+      }
+    ]
+  }
+]

tests/expected_json/unchecked_lowlevel.unchecked-lowlevel.txt

@@ -0,0 +1,5 @@
+INFO:Detectors:
+MyConc.bad (tests/unchecked_lowlevel.sol#2-4) does not use the value returned by low-level calls:
+	-dst.call.value(msg.value)() (tests/unchecked_lowlevel.sol#3)
+Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unchecked-low-level
+INFO:Slither:tests/unchecked_lowlevel.sol analyzed (1 contracts), 1 result(s) found

tests/expected_json/unchecked_send-0.5.1.unchecked-send.json

@@ -0,0 +1,80 @@
+[
+  {
+    "check": "unchecked-send",
+    "impact": "Medium",
+    "confidence": "Medium",
+    "description": "MyConc.bad (tests/unchecked_send-0.5.1.sol#2-4) does not use the value returned by send calls:\n\t-dst.send(msg.value) (tests/unchecked_send-0.5.1.sol#3)\n",
+    "elements": [
+      {
+        "type": "function",
+        "name": "bad",
+        "source_mapping": {
+          "start": 21,
+          "length": 86,
+          "filename_used": "/home/travis/build/crytic/slither/tests/unchecked_send-0.5.1.sol",
+          "filename_relative": "tests/unchecked_send-0.5.1.sol",
+          "filename_absolute": "/home/travis/build/crytic/slither/tests/unchecked_send-0.5.1.sol",
+          "filename_short": "tests/unchecked_send-0.5.1.sol",
+          "lines": [
+            2,
+            3,
+            4
+          ],
+          "starting_column": 5,
+          "ending_column": 6
+        },
+        "contract": {
+          "type": "contract",
+          "name": "MyConc",
+          "source_mapping": {
+            "start": 0,
+            "length": 419,
+            "filename_used": "/home/travis/build/crytic/slither/tests/unchecked_send-0.5.1.sol",
+            "filename_relative": "tests/unchecked_send-0.5.1.sol",
+            "filename_absolute": "/home/travis/build/crytic/slither/tests/unchecked_send-0.5.1.sol",
+            "filename_short": "tests/unchecked_send-0.5.1.sol",
+            "lines": [
+              1,
+              2,
+              3,
+              4,
+              5,
+              6,
+              7,
+              8,
+              9,
+              10,
+              11,
+              12,
+              13,
+              14,
+              15,
+              16,
+              17,
+              18
+            ],
+            "starting_column": 1,
+            "ending_column": 2
+          }
+        }
+      },
+      {
+        "type": "expression",
+        "expression": "dst.send(msg.value)",
+        "source_mapping": {
+          "start": 81,
+          "length": 19,
+          "filename_used": "/home/travis/build/crytic/slither/tests/unchecked_send-0.5.1.sol",
+          "filename_relative": "tests/unchecked_send-0.5.1.sol",
+          "filename_absolute": "/home/travis/build/crytic/slither/tests/unchecked_send-0.5.1.sol",
+          "filename_short": "tests/unchecked_send-0.5.1.sol",
+          "lines": [
+            3
+          ],
+          "starting_column": 9,
+          "ending_column": 28
+        }
+      }
+    ]
+  }
+]

tests/expected_json/unchecked_send-0.5.1.unchecked-send.txt

@@ -0,0 +1,6 @@
+INFO:Detectors:
+MyConc.bad (tests/unchecked_send-0.5.1.sol#2-4) does not use the value returned by send calls:
+	-dst.send(msg.value) (tests/unchecked_send-0.5.1.sol#3)
+Reference: https://github.com/crytic/slither/wiki/Detector-Documentation#unchecked-send
+INFO:Slither:/home/monty/Private/tob/tools/slither-public/scripts/../tests/expected_json/unchecked_send-0.5.1.unchecked-send.json exists already, the overwrite is prevented
+INFO:Slither:tests/unchecked_send-0.5.1.sol analyzed (1 contracts), 1 result(s) found

tests/unchecked_lowlevel-0.5.1.sol

@@ -0,0 +1,11 @@
+contract MyConc{
+    function bad(address payable dst) external payable{
+        dst.call.value(msg.value)("");
+    }
+
+    function good(address payable dst) external payable{
+        (bool ret, bytes memory _) = dst.call.value(msg.value)("");
+        require(ret);
+    }
+
+}

tests/unchecked_lowlevel.sol

@@ -0,0 +1,10 @@
+contract MyConc{
+    function bad(address dst) external payable{
+        dst.call.value(msg.value)("");
+    }
+
+    function good(address dst) external payable{
+        require(dst.call.value(msg.value)());
+    }
+
+}

tests/unchecked_send-0.5.1.sol

@@ -0,0 +1,18 @@
+contract MyConc{
+    function bad(address payable dst) external payable{
+        dst.send(msg.value);
+    }
+
+    function good(address payable dst) external payable{
+        require(dst.send(msg.value));
+    }
+
+    function good2(address payable dst) external payable{
+        bool res = dst.send(msg.value);
+        if(!res){
+            emit Failed(dst, msg.value);
+        }
+    }
+
+    event Failed(address, uint);
+}