Merge pull request #97 from pessimistic-io/develop

v0.4.1
pessimistic-io · Nov 1, 2023 · 1a8b988 · 1a8b988
2 parents f458eb8 + 8d4bf53
commit 1a8b988
Show file tree

Hide file tree

Showing 40 changed files with 637 additions and 47 deletions.
diff --git a/.github/workflows/notification.yml b/.github/workflows/notification.yml
@@ -15,4 +15,4 @@ jobs:
         to: ${{ secrets.TELEGRAM_TO }}
         token: ${{ secrets.BOT_TOKEN }}
         message: |
-         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
+         New version of Slitherin got released. 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/MANIFEST.in b/MANIFEST.in
@@ -0,0 +1 @@
+recursive-include slitherin *
diff --git a/README.md b/README.md
@@ -38,8 +38,7 @@ To install Pessimistic Detectors:
 python3 setup.py develop
 ```
 > Keep in mind that you don't have to reinstall the plugin after changes in the repository!
-4. Run the original Slither as usual.
-5. Dependencies must be installed in order to test the detectors on our test contracts:
+4. Dependencies must be installed in order to test the detectors on our test contracts:
 ```bash
 npm install
 ```
@@ -49,7 +48,25 @@ npm install
 ```bash
 pip install slitherin
 ```
-3. Run the original Slither as usual.
+
+## Usage
+### Slitherin-cli (Recommended)
+Use Slitherin-cli to run detectors on a Hardhat/Foundry/Dapp/Brownie application. You have the following options:
+* Run ONLY Slitherin detectors:
+```bash
+slitherin . --pess
+```
+* Run ONLY Slither detectors:
+```bash
+slitherin . --slither
+```
+* Run Slither detectors, then Slitherin detectors:
+```bash
+slitherin . --separated
+```
+> Keep in mind that Slitherin-cli supports all Slither run options.
+### Slither
+Slitherin detectors are included into original Slither after the installation. You can use Slither [as usual](https://github.com/crytic/slither#usage).
 
 ## **Detectors Table**
 
@@ -161,6 +178,7 @@ Our team would like to express our deepest gratitude to the [Slither tool](https
 - [Blockthreat](https://newsletter.blockthreat.io/p/blockthreat-week-16-2023#:~:text=Slitherin%20a%20collection%20of%20Slither%20detection%20by%20Pessimistic.io%20team)
 - [Release article by officercia.eth](https://officercia.mirror.xyz/ucWYWnhBXmkKq54BIdJcH5GnrAB-nQkUsZ2F-ytEsR4)
 - [Defillama](https://t.me/defillama_tg/842)
+- [Essential Tools for Auditing Smart Contracts by Hexens](https://hexens.io/blog/toolkit-for-web3-security-engineers)
 - [ETH Belgrade](https://www.youtube.com/watch?v=CU9JAqGY5h8&t=3s) talk
 - Integrated into [AuditWizard](https://app.auditwizard.io/)
 

diff --git a/docs/aave/flashloan_callback.md b/docs/aave/flashloan_callback.md
@@ -0,0 +1,19 @@
+# AAVE Flashloan callback detector
+
+## Configuration
+
+- Check: `pess-aave-flashloan-callback`
+- Severity: `High`
+- Confidence: `High`
+
+## Description
+
+It is important to validate `initiator` and `msg.sender` in `executeOperation` callback
+
+## Vulnerable Scenario
+
+[test scenarios](../tests/AaveFlashloanCallback.sol)
+
+## Recommendation
+
+Validate `initiator` and `msg.sender`
diff --git a/docs/arbitrary_call.md b/docs/arbitrary_call.md
@@ -2,17 +2,30 @@
 
 ## Configuration
 
-- Check: `pess-arbitrary-call`
-- Severity: `High`
-- Confidence: `Low`
+- - Check: `pess-arbitrary-call`
+  - Severity: `High`
+  - Confidence: `High`
+
+* - Check: `pess-arbitrary-call-with-stored-erc20-approves`
+  - Severity: `High`
+  - Confidence: `High`
+
+- - Check: `pess-arbitrary-call-destination-tainted`
+  - Severity: `Medium`
+  - Confidence: `Medium`
+
+* - Check: `pess-arbitrary-call-calldata-tainted`
+  - Severity: `Medium`
+  - Confidence: `Medium`
 
 ## Description
 
 The detector iterates over all low-level calls, checks if the destination or calldata could be tainted(manipulated).
+This detector consists of multiple detectors, which will run with this detector.
 
 ### Potential Improvement
 
-Filter out role protected calls, divide detector to multiple detectors with different severity and confidence
+Filter out proxy results
 
 ## Vulnerable Scenario
 

diff --git a/setup.py b/setup.py
@@ -1,4 +1,5 @@
 from setuptools import setup, find_packages
+from slitherin.cli import SLITHERIN_VERSION
 
 with open("./README.md", "r") as f:
     long_description = f.read()
@@ -8,18 +9,27 @@
     description="Pessimistic security Slither detectors",
     long_description=long_description,
     long_description_content_type="text/markdown",
+    package_data={"slitherin": ["py.typed"]},
     url="https://github.com/pessimistic-io/slitherin",
     author="Pessimistic.io",
-    version="0.4.0",
-    package_dir={"":"."},
+    author_email="[email protected]",
+    classifiers=[
+        "Development Status :: 3 - Alpha",
+        "Programming Language :: Python",
+        "License :: OSI Approved :: Apache Software License",
+        "Topic :: Software Development :: Libraries",
+    ],
+    version=SLITHERIN_VERSION,
     packages=find_packages(),
     license="AGPL-3.0",
     python_requires=">=3.8",
-    install_requires=["slither-analyzer>=0.8.3"],
+    install_requires=["slither-analyzer>=0.9.3"],
     extras_requires={
         "dev": ["twine>=4.0.2"],
     },
     entry_points={
-        "slither_analyzer.plugin": "slither my-plugin=slither_pess:make_plugin",
+        "slither_analyzer.plugin": "slither slitherin-plugins=slither_pess:make_plugin",
+        "console_scripts": ["slitherin=slither_pess.cli:main"],
     },
+    include_package_data=True,
 )
diff --git a/slither_pess/__init__.py b/slither_pess/__init__.py
@@ -1,4 +1,4 @@
-from slither_pess.detectors.arbitrary_call import ArbitraryCall
+from slither_pess.detectors.arbitrary_call.arbitrary_call import ArbitraryCall
 from slither_pess.detectors.double_entry_token_possibility import (
     DoubleEntryTokenPossiblity,
 )
@@ -22,33 +22,36 @@
 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
+from slither_pess.detectors.aave.flashloan_callback import AAVEFlashloanCallbackDetector
 
 
-def make_plugin():
-    plugin_detectors = [
-        DoubleEntryTokenPossiblity,
-        UnprotectedSetter,
-        NftApproveWarning,
-        InconsistentNonreentrant,
-        StrangeSetter,
-        OnlyEOACheck,
-        MagicNumber,
-        DubiousTypecast,
-        CallForwardToProtected,
-        MultipleStorageRead,
-        TimelockController,
-        TxGaspriceWarning,
-        UnprotectedInitialize,
-        ReadOnlyReentrancy,
-        EventSetter,
-        BeforeTokenTransfer,
-        UniswapV2,
-        TokenFallback,
-        ForContinueIncrement,
-        ArbitraryCall,
-        Ecrecover,
-        PublicVsExternal,
-    ]
-    plugin_printers = []
+plugin_detectors = [
+    DoubleEntryTokenPossiblity,
+    UnprotectedSetter,
+    NftApproveWarning,
+    InconsistentNonreentrant,
+    StrangeSetter,
+    OnlyEOACheck,
+    MagicNumber,
+    DubiousTypecast,
+    CallForwardToProtected,
+    MultipleStorageRead,
+    TimelockController,
+    TxGaspriceWarning,
+    UnprotectedInitialize,
+    ReadOnlyReentrancy,
+    EventSetter,
+    BeforeTokenTransfer,
+    UniswapV2,
+    TokenFallback,
+    ForContinueIncrement,
+    ArbitraryCall,
+    Ecrecover,
+    PublicVsExternal,
+    AAVEFlashloanCallbackDetector,
+]
+plugin_printers = []
 
+
+def make_plugin():
     return plugin_detectors, plugin_printers
diff --git a/slither_pess/detectors/arbitrary_call.py → ...etectors/arbitrary_call/arbitrary_call.py b/slither_pess/detectors/arbitrary_call.py → ...etectors/arbitrary_call/arbitrary_call.py
@@ -1,7 +1,13 @@
+from enum import Enum
 from typing import List, Tuple
+from collections import namedtuple
 
-from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
-from slither.slithir.operations import SolidityCall
+from slither.detectors.abstract_detector import (
+    AbstractDetector,
+    DetectorClassification,
+    classification_txt,
+)
+from slither.slithir.operations import SolidityCall, HighLevelCall
 from slither.core.declarations import (
     Contract,
     SolidityVariableComposed,
@@ -16,6 +22,33 @@
 # look for transferFroms if there is any transferFrom and contract contains whole arbitrary call - it is screwed
 # Filter out role protected
 
+DetectorParams = namedtuple(
+    "DetectorParams", ["argument_suffix", "impact", "confidence"]
+)
+
+
+class ArbitraryCallDetectors:
+    ArbitraryCallWithApproveStored = DetectorParams(
+        "-with-stored-erc20-approves",
+        DetectorClassification.HIGH,
+        DetectorClassification.HIGH,
+    )
+    ArbitraryCall = DetectorParams(
+        "",
+        DetectorClassification.HIGH,
+        DetectorClassification.HIGH,
+    )
+    ArbitraryCallDestinationTainted = DetectorParams(
+        "-destination-tainted",
+        DetectorClassification.MEDIUM,
+        DetectorClassification.MEDIUM,
+    )
+    ArbitraryCallCalldataTainted = DetectorParams(
+        "-calldata-tainted",
+        DetectorClassification.MEDIUM,
+        DetectorClassification.MEDIUM,
+    )
+
 
 class ArbitraryCall(AbstractDetector):
     """
@@ -36,10 +69,19 @@ class ArbitraryCall(AbstractDetector):
     WIKI_EXPLOIT_SCENARIO = "Attacker can manipulate on inputs"
     WIKI_RECOMMENDATION = "Do not allow arbitrary calls"
 
+    def _is_role_protected(self, function: FunctionContract):
+        for m in function.modifiers:
+            if m.name.startswith("only"):
+                return True
+        return False
+
     def analyze_function(
         self, function: FunctionContract
-    ) -> List[Tuple[FunctionContract, Node, LowLevelCall, bool, bool]]:
+    ) -> Tuple[
+        List[Tuple[FunctionContract, Node, LowLevelCall, bool, bool]], bool
+    ]:  # TODO(yhtiyar): make return type as named tuple/class
         results = []
+        stores_approve = False
         for node in function.nodes:
             for ir in node.irs:
                 try:
@@ -76,6 +118,12 @@ def analyze_function(
                         destination_tainted = is_tainted(ir.arguments[1], node, True)
                         args_tainted = is_tainted(ir.arguments[3], node, True)
 
+                    elif (
+                        isinstance(ir, HighLevelCall)
+                        and ir.function.name == "transferFrom"
+                    ):
+                        stores_approve = True
+
                     if args_tainted or destination_tainted:
                         results.append(
                             (
@@ -90,7 +138,7 @@ def analyze_function(
                     print("ArbitraryCall:Failed to check types", e)
                     print(ir)
 
-        return results
+        return (results, stores_approve)
 
     def analyze_contract(self, contract: Contract):
         stores_approve = False
@@ -99,7 +147,8 @@ def analyze_contract(self, contract: Contract):
         ] = []
         results = []
         for f in contract.functions:
-            res = self.analyze_function(f)
+            (res, _stores_approve) = self.analyze_function(f)
+            stores_approve |= _stores_approve
             if res:
                 all_tainted_calls.extend(res)
 
@@ -115,6 +164,8 @@ def analyze_contract(self, contract: Contract):
             for f in contract.functions:
                 if f.visibility not in ["external", "public"]:
                     continue
+                if self._is_role_protected(f):
+                    continue
 
                 fn_taints_args = False
                 fn_taints_destination = False
@@ -137,15 +188,40 @@ def analyze_contract(self, contract: Contract):
                 if not (fn_taints_args or fn_taints_destination):
                     continue
 
+                detectorParams: DetectorParams = None
                 if fn_taints_args and fn_taints_destination:
-                    text = "The call could be fully manipulated (arbitrary call)"
+                    if stores_approve:
+                        text = "The call could be fully manipulated (arbitrary call). This contract also STORES APPROVES!!!"
+                        detectorParams = (
+                            ArbitraryCallDetectors.ArbitraryCallWithApproveStored
+                        )
+
+                    else:
+                        text = "The call could be fully manipulated (arbitrary call)"
+                        detectorParams = ArbitraryCallDetectors.ArbitraryCall
                 else:
-                    part = "calldata" if fn_taints_args else "destination"
+                    if fn_taints_args:
+                        part = "calldata"
+                        detectorParams = (
+                            ArbitraryCallDetectors.ArbitraryCallCalldataTainted
+                        )
+
+                    else:
+                        part = "destination"
+                        detectorParams = (
+                            ArbitraryCallDetectors.ArbitraryCallDestinationTainted
+                        )
+
                     text = f"The {part} could be manipulated"
                 info += [f"\t{text} through ", f, "\n"]
 
             res = self.generate_result(info)
             res.add(node)
+
+            res.data["check"] = self.ARGUMENT + detectorParams.argument_suffix
+            res.data["impact"] = classification_txt[detectorParams.impact]
+            res.data["confidence"] = classification_txt[detectorParams.confidence]
+
             results.append(res)
         return results