Merge pull request #97 from pessimistic-io/develop

v0.4.1

.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

MANIFEST.in

@@ -0,0 +1 @@
+recursive-include slitherin *

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/)
 

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`

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
 

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,
 )

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

slither_pess/detectors/arbitrary_call.py → slither_pess/detectors/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