Merge pull request #88 from pessimistic-io/develop

ndkirillov · web-flow · commit e80099d3ef4d · 2023-10-12T13:22:31.000+05:00
Slitherin 0.4.0
diff --git a/.github/workflows/notification.yml b/.github/workflows/notification.yml
@@ -1,8 +1,8 @@
 name: notification
 
 on:
-  push:
-    branches: [ master ]
+  release:
+    types: [ published ]
 
 jobs:
 
@@ -15,4 +15,4 @@ jobs:
         to: ${{ secrets.TELEGRAM_TO }}
         token: ${{ secrets.BOT_TOKEN }}
         message: |
-         Master branch received updates. Pull new features from https://github.com/pessimistic-io/slitherin.
+         New version of Slitherin got realeased. Pull updates from here: https://github.com/pessimistic-io/slitherin or update a Python package: https://pypi.org/project/slitherin/. Release note: https://github.com/pessimistic-io/slitherin/releases
diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml
@@ -5,17 +5,17 @@ on:
     types: [published]
 
 jobs:
-  publish:
+  deploy:
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@v2
       - uses: actions/setup-python@v2
         with:
-          python-version: '3.8'
+          python-version: '3.x'
       - name: Install dependencies
         run: |
           python -m pip install --upgrade pip
-          pip install -e .[distribute]
+          pip install setuptools wheel twine
       - name: Build and publish
         env:
           TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }}
diff --git a/README.md b/README.md
@@ -79,7 +79,7 @@ pip install slitherin
 
 - *Valid - issues included in reports and fixed by developers (January 2023 - June 2023).
 
-- There is one detector that is disabled by default: [pess-uni-v2](https://github.com/pessimistic-io/slitherin/blob/master/slither_pess/detectors/uni_v2.py). **It is recommended to run it only on projects that integrate [Uniswap V2](https://betterprogramming.pub/uniswap-v2-in-depth-98075c826254)!**
+- There is one integration detector which has several checks inside: [pess-uni-v2](https://github.com/pessimistic-io/slitherin/blob/master/slither_pess/detectors/uni_v2.py). **It runs only on projects that integrate [Uniswap V2](https://betterprogramming.pub/uniswap-v2-in-depth-98075c826254)!**
 
 ## Enhancements & New Detectors
 
diff --git a/docs/ecrecover.md b/docs/ecrecover.md
@@ -0,0 +1,23 @@
+# Ecrecover
+
+## Configuration
+
+- Check: `pess-ecrecover`
+- Severity: `High`
+- Confidence: `Medium`
+
+## Description
+
+`ecrecover` functions returns `0` on error. It is important to check the result for `0`.
+
+### Potential Improvement
+
+As for now, the detector might not work on asm level.
+
+## Vulnerable Scenario
+
+[test scenarios](../tests/ecrecover.sol)
+
+## Recommendation
+
+Check the result of `ecrecover` or use OZ ECDSA library.
diff --git a/docs/integration_uniswapV2.md b/docs/integration_uniswapV2.md
@@ -1,6 +1,6 @@
 # UniswapV2 Integration
 
-Disabled by default. Use `--detect pess-uni-v2` to enable the detector. 
+Looks for contracts inheritance. Use `--detect pess-uni-v2` to forcefully enable the detector. 
 
 ## Configuration
 * Check: `pess-uni-v2`
diff --git a/docs/public_vs_external.md b/docs/public_vs_external.md
@@ -0,0 +1,23 @@
+# Public vs External
+
+## Configuration
+
+- Check: `pess-public-vs-external`
+- Severity: `Low`
+- Confidence: `Medium`
+
+## Description
+
+Detects functions that have `public` modifiers and could be turned into `external` (not used in the contract)
+
+### Potential Improvement
+
+There could be FP's because of inheritance
+
+## Vulnerable Scenario
+
+[test scenarios](../tests/public_vs_external_test.sol)
+
+## Recommendation
+
+Mark `public` functions as `external` where it is possible to enhance control-flow readability.
diff --git a/setup.py b/setup.py
@@ -10,7 +10,7 @@
     long_description_content_type="text/markdown",
     url="https://github.com/pessimistic-io/slitherin",
     author="Pessimistic.io",
-    version="0.3.0",
+    version="0.4.0",
     package_dir={"":"."},
     packages=find_packages(),
     license="AGPL-3.0",
diff --git a/slither_pess/__init__.py b/slither_pess/__init__.py
@@ -20,6 +20,8 @@
 from slither_pess.detectors.uni_v2 import UniswapV2
 from slither_pess.detectors.token_fallback import TokenFallback
 from slither_pess.detectors.for_continue_increment import ForContinueIncrement
+from slither_pess.detectors.ecrecover import Ecrecover
+from slither_pess.detectors.public_vs_external import PublicVsExternal
 
 
 def make_plugin():
@@ -44,6 +46,8 @@ def make_plugin():
         TokenFallback,
         ForContinueIncrement,
         ArbitraryCall,
+        Ecrecover,
+        PublicVsExternal,
     ]
     plugin_printers = []
 
diff --git a/slither_pess/detectors/arbitrary_call.py b/slither_pess/detectors/arbitrary_call.py
@@ -1,9 +1,7 @@
-from collections import namedtuple
-from dataclasses import dataclass
-from typing import Dict, List, Optional, Tuple, Set
+from typing import List, Tuple
 
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
-from slither.slithir.operations import TypeConversion, Operation, SolidityCall
+from slither.slithir.operations import SolidityCall
 from slither.core.declarations import (
     Contract,
     SolidityVariableComposed,
diff --git a/slither_pess/detectors/before_token_transfer.py b/slither_pess/detectors/before_token_transfer.py
@@ -1,7 +1,6 @@
 from typing import List
 from slither.utils.output import Output
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
-from slither.core.declarations import Function
 
 
 class BeforeTokenTransfer(AbstractDetector):
diff --git a/slither_pess/detectors/call_forward_to_protected.py b/slither_pess/detectors/call_forward_to_protected.py
@@ -27,7 +27,7 @@ def _contains_low_level_calls(self, node) -> bool:
 
     def _detect_low_level_custom_address_call(self, fun: Function) -> bool:
         address_parameters = [
-            parameter for parameter in fun.parameters if str(parameter.type) == "address"
+            parameter for parameter in fun.parameters if str(parameter.type) == "address" and parameter.name
         ]
         for node in fun.nodes:
             if self._contains_low_level_calls(node):
diff --git a/slither_pess/detectors/double_entry_token_possibility.py b/slither_pess/detectors/double_entry_token_possibility.py
@@ -1,4 +1,3 @@
-from slither.core.cfg.node import NodeType
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
 
 
diff --git a/slither_pess/detectors/dubious_typecast.py b/slither_pess/detectors/dubious_typecast.py
@@ -1,4 +1,4 @@
-from typing import List, Optional, Tuple
+from typing import List, Tuple
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
 from slither.slithir.operations import TypeConversion, Operation
 from slither.core.declarations import FunctionContract
diff --git a/slither_pess/detectors/ecrecover.py b/slither_pess/detectors/ecrecover.py
@@ -0,0 +1,87 @@
+import re
+from typing import List, Tuple
+
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.slithir.operations import (
+    SolidityCall,
+    Condition,
+)
+from slither.core.declarations import (
+    FunctionContract,
+)
+from slither.core.cfg.node import Node
+from slither.slithir.operations import LowLevelCall
+from slither.analyses.data_dependency.data_dependency import is_dependent
+
+
+class Ecrecover(AbstractDetector):
+    """
+    Detects not checked results of ecrecover
+    """
+
+    ARGUMENT = "pess-ecrecover"  # slither will launch the detector with slither.py --detect mydetector
+    HELP = "signer = ecrecover(hash, v, r, s)"
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = "https://github.com/pessimistic-io/slitherin/blob/master/docs/ecrecover.md"
+    WIKI_TITLE = "Ecrecover"
+    WIKI_DESCRIPTION = "Check docs"
+    WIKI_EXPLOIT_SCENARIO = "Attacker can validate signatures from 0x0 address"
+    WIKI_RECOMMENDATION = "Check the result of ecrecover"
+
+    def analyze_function(
+        self, function: FunctionContract
+    ) -> List[Tuple[FunctionContract, Node, LowLevelCall, bool, bool]]:
+        unchecked_results = set()
+        var_to_node = {}
+        for node in function.nodes:
+            for ir in node.irs:
+                try:
+                    node_contains_0 = re.search(
+                        r"address\((0|0x0*)\)", str(node)
+                    )  # check if the node contains address(0|0x0..)
+                    if isinstance(ir, SolidityCall):
+                        if (
+                            ir.function.name
+                            == "ecrecover(bytes32,uint8,bytes32,bytes32)"
+                        ):
+                            unchecked_results.add(ir.lvalue)
+                            var_to_node[ir.lvalue] = node
+                        elif (
+                            ir.function.name == "require(bool,string)"
+                            or ir.function.name == "assert(bool)"
+                        ):
+                            if not node_contains_0:  # does not contain 0 check
+                                continue
+                            checking_var = ir.arguments[0]
+                            for ur in unchecked_results:
+                                if is_dependent(checking_var, ur, node):
+                                    unchecked_results.remove(ur)
+                                    break
+                    elif isinstance(ir, Condition):
+                        # this is copypaste, for now, couldn't figure out how to make this better without overcomplicating
+                        if not node_contains_0:  # does not contain 0 check
+                            continue
+                        for ur in unchecked_results:
+                            if is_dependent(ir.value, ur, node):
+                                unchecked_results.remove(ur)
+                                break
+
+                except Exception as e:
+                    print("failed", e)
+
+        return [var_to_node[ur] for ur in unchecked_results]
+
+    def _detect(self):
+        results = []
+        for contract in self.compilation_unit.contracts_derived:
+            for f in contract.functions:
+                res = self.analyze_function(f)
+                if res:
+                    for r in res:
+                        info = ["Unchecked result of ecrecover for 0:\n\t", r, "\n"]
+                        tres = self.generate_result(info)
+                        tres.add(r)
+                        results.append(tres)
+        return results
diff --git a/slither_pess/detectors/falsy_only_eoa_modifier.py b/slither_pess/detectors/falsy_only_eoa_modifier.py
@@ -1,6 +1,6 @@
 from slither.core.cfg.node import NodeType
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
-from slither.core.declarations import Contract, Function, SolidityVariableComposed
+from slither.core.declarations import SolidityVariableComposed
 from slither.analyses.data_dependency.data_dependency import is_dependent
 
 
diff --git a/slither_pess/detectors/for_continue_increment.py b/slither_pess/detectors/for_continue_increment.py
@@ -1,4 +1,3 @@
-import copy
 from typing import List, Optional
 from slither.core.cfg.node import NodeType, Node
 from slither.core.declarations import Contract, Function
diff --git a/slither_pess/detectors/public_vs_external.py b/slither_pess/detectors/public_vs_external.py
@@ -0,0 +1,52 @@
+from typing import List
+from slither.utils.output import Output
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.core.declarations import Contract
+
+
+class PublicVsExternal(AbstractDetector):
+    """
+    Detects functions that have "public" modifiers and not used in the contract
+    """
+
+    ARGUMENT = "pess-public-vs-external"  # slither will launch the detector with slither.py --detect mydetector
+    HELP = "mark public functions as external where possible, to enhance contract's control-flow readability"
+    IMPACT = DetectorClassification.LOW
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = "https://github.com/pessimistic-io/slitherin/blob/master/docs/public_vs_external.md"
+    WIKI_TITLE = "Public vs External"
+    WIKI_DESCRIPTION = "Check docs"
+    WIKI_EXPLOIT_SCENARIO = "No exploits, just readability enhancement"
+    WIKI_RECOMMENDATION = "Mark public functions as external where it is possible"
+
+    def _analyze_contract(self, contract: Contract) -> list:
+        res = []
+        used_functions = set()
+        for f in contract.functions_and_modifiers_declared:
+            for node in f.nodes:
+                for call in node.internal_calls:
+                    used_functions.add(call.name)
+
+        for f in contract.functions_and_modifiers_declared:
+            if f.visibility == "public" and f.name not in used_functions:
+                res.append(f)
+        return res
+
+    def _detect(self) -> List[Output]:
+        """Main function"""
+        results = []
+        for contract in self.compilation_unit.contracts_derived:
+            res = self._analyze_contract(contract)
+            if res:
+                info = [
+                    "The following public functions could be turned into external in ",
+                    contract,
+                    " contract:\n",
+                ]
+                for r in res:
+                    info += ["\t", r, "\n"]
+                contract_result = self.generate_result(info)
+                results.append(contract_result)
+
+        return results
diff --git a/slither_pess/detectors/read_only_reentrancy.py b/slither_pess/detectors/read_only_reentrancy.py
@@ -16,10 +16,8 @@
     union_dict,
     _filter_if,
     is_subset,
-    dict_are_equal,
 )
-from slither.slithir.operations import Send, Transfer, EventCall
-from slither.slithir.operations import Call
+from slither.slithir.operations import EventCall
 
 FindingKey = namedtuple("FindingKey", ["function", "calls"])
 FindingValue = namedtuple("FindingValue", ["variable", "written_at", "node", "nodes"])
diff --git a/slither_pess/detectors/uni_v2.py b/slither_pess/detectors/uni_v2.py
@@ -8,12 +8,12 @@
 from slither.core.compilation_unit import SlitherCompilationUnit
 from slither.utils.output import Output
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
-from slither.core.declarations import Function, Contract, FunctionContract
+from slither.core.declarations import Function, Contract
 from slither.core.expressions.type_conversion import TypeConversion
-from slither.core.cfg.node import NodeType, Node
+from slither.core.cfg.node import Node
 from slither.core.variables.local_variable import LocalVariable
 from slither.core.solidity_types.array_type import ArrayType
-from slither.slithir.operations import SolidityCall, InternalCall, HighLevelCall
+from slither.slithir.operations import InternalCall, HighLevelCall
 from slither.slithir.operations.assignment import Assignment
 from slither.analyses.data_dependency.data_dependency import is_dependent
 
diff --git a/slither_pess/detectors/unprotected_initialize.py b/slither_pess/detectors/unprotected_initialize.py
@@ -2,6 +2,7 @@
 from slither.utils.output import Output
 from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
 from slither.core.declarations import Function
+from slither.core.variables.variable import Variable
 
 
 class UnprotectedInitialize(AbstractDetector):
@@ -38,8 +39,9 @@ def _has_require(self, fun: Function) -> bool:
         for node in fun.nodes:
             if "require" in str(node):
                 for variable in node.variables_read:
-                    if str(variable.type) == "address":
-                        return True
+                    if isinstance(variable, Variable):
+                        if str(variable.type) == "address":
+                            return True
         return False
 
     def _detect(self) -> List[Output]:
diff --git a/slither_pess/detectors/unprotected_setter.py b/slither_pess/detectors/unprotected_setter.py
@@ -30,12 +30,13 @@ def is_setter(self, fun, params=None):
                         left = lr[0]
                         right = lr[1]
                         for p in params:
-                            if "." in left:
-                                continue
-                            if "[" in left:
-                                continue
-                            if right == str(p):
-                                return left
+                            if p.name:
+                                if "." in left:
+                                    continue
+                                if "[" in left:
+                                    continue
+                                if right == str(p):
+                                    return left
         return None
 
     def has_access_control(self, fun):
diff --git a/tests/ecrecover.sol b/tests/ecrecover.sol
diff --git a/tests/public_vs_external_test.sol b/tests/public_vs_external_test.sol
