Merge pull request #170 from pessimistic-io/develop

v0.7.1

.github/workflows/release.yml

@@ -16,10 +16,11 @@ jobs:
  run: |
  python -m pip install --upgrade pip
  pip install setuptools wheel twine
+ python -m pip install solc-select
+ python -m pip install slither-analyzer
  - name: Build and publish
  env:
- TWINE_USERNAME: ${{ secrets.PYPI_USERNAME }}
- TWINE_PASSWORD: ${{ secrets.PYPI_PASSWORD }}
+ TWINE_TOKEN: ${{ secrets.PYPI_TOKEN }}
  run: |
  python setup.py sdist bdist_wheel
  twine upload dist/*

docs/call_forward_to_protected.md

@@ -6,6 +6,7 @@
 * Confidence: `Low` 
 
 ## Description
+**The detector is obsolete since Slitherin 0.7.1.**
 Sees if a contract function has low level calls to a custom address.
 
 ## Vulnerable Scenario

docs/dubious_typecast.md

@@ -8,7 +8,7 @@
 
 ## Description
 
-Highlights nonstandard typecasts. E.g: `uint256(uint8(K))`
+Highlights explicit typecasts, where the result value can differ from the original one. E.g., `uint8(uint256(1e18))`, `uint256(int256(-1))`.
 
 ## Vulnerable Scenario
 

docs/readonly_reentrancy.md

@@ -6,6 +6,7 @@
 * Confidence: `Low`
 
 ## Description
+**The detector is obsolete since Slitherin 0.7.1.**
 Highlights the use of getter functions that return a value that theoretically could be manipulated during the execution.
 
 ## Vulnerable Scenario

slitherin/__init__.py

@@ -1,3 +1,5 @@
+import os
+
 from slitherin.detectors.arbitrary_call.arbitrary_call import ArbitraryCall
 from slitherin.detectors.double_entry_token_possibility import (
  DoubleEntryTokenPossiblity,
@@ -9,12 +11,14 @@
 from slitherin.detectors.unprotected_setter import UnprotectedSetter
 from slitherin.detectors.nft_approve_warning import NftApproveWarning
 from slitherin.detectors.inconsistent_nonreentrant import InconsistentNonreentrant
-from slitherin.detectors.call_forward_to_protected import CallForwardToProtected
+from slitherin.detectors.obsolete.call_forward_to_protected import (
+ CallForwardToProtected,
+)
 from slitherin.detectors.multiple_storage_read import MultipleStorageRead
 from slitherin.detectors.timelock_controller import TimelockController
 from slitherin.detectors.tx_gasprice_warning import TxGaspriceWarning
 from slitherin.detectors.unprotected_initialize import UnprotectedInitialize
-from slitherin.detectors.read_only_reentrancy import ReadOnlyReentrancy
+from slitherin.detectors.obsolete.read_only_reentrancy import ReadOnlyReentrancy
 from slitherin.detectors.event_setter import EventSetter
 from slitherin.detectors.before_token_transfer import BeforeTokenTransfer
 from slitherin.detectors.uni_v2 import UniswapV2
@@ -29,19 +33,28 @@
 from slitherin.detectors.arbitrum.block_number_timestamp import (
  ArbitrumBlockNumberTimestamp,
 )
-from slitherin.detectors.arbitrum.arbitrum_chainlink_price_feed import ArbitrumChainlinkPriceFeed
+from slitherin.detectors.arbitrum.arbitrum_chainlink_price_feed import (
+ ArbitrumChainlinkPriceFeed,
+)
 from slitherin.detectors.potential_arith_overflow import PotentialArithmOverflow
 from slitherin.detectors.curve.curve_readonly_reentrancy import CurveReadonlyReentrancy
-from slitherin.detectors.balancer.balancer_readonly_reentrancy import BalancerReadonlyReentrancy
-from slitherin.detectors.vyper.reentrancy_curve_vyper_version import CurveVyperReentrancy
+from slitherin.detectors.balancer.balancer_readonly_reentrancy import (
+ BalancerReadonlyReentrancy,
+)
+from slitherin.detectors.vyper.reentrancy_curve_vyper_version import (
+ CurveVyperReentrancy,
+)
 from slitherin.detectors.price_manipulation import PriceManipulationDetector
+from .consts import OBSOLETE_FLAG
 
 artbitrum_detectors = [
  ArbitrumPrevrandaoDifficulty,
  ArbitrumBlockNumberTimestamp,
- ArbitrumChainlinkPriceFeed
+ ArbitrumChainlinkPriceFeed,
 ]
 
+obsolete_detectors = [CallForwardToProtected, ReadOnlyReentrancy]
+
 plugin_detectors = artbitrum_detectors + [
  DoubleEntryTokenPossiblity,
  UnprotectedSetter,
@@ -51,12 +64,10 @@
  OnlyEOACheck,
  MagicNumber,
  DubiousTypecast,
- CallForwardToProtected,
  MultipleStorageRead,
  TimelockController,
  TxGaspriceWarning,
  UnprotectedInitialize,
- ReadOnlyReentrancy,
  EventSetter,
  BeforeTokenTransfer,
  UniswapV2,
@@ -70,8 +81,12 @@
  CurveReadonlyReentrancy,
  BalancerReadonlyReentrancy,
  CurveVyperReentrancy,
- PriceManipulationDetector
+ PriceManipulationDetector,
 ]
+
+if os.getenv(OBSOLETE_FLAG):
+ plugin_detectors += obsolete_detectors
+
 plugin_printers = []
 
 

slitherin/consts.py

@@ -1,2 +1,3 @@
 ARBITRUM_KEY = "SLITHERIN_ARBITRUM"
-SLITHERIN_VERSION = "0.7.0"
+OBSOLETE_FLAG = "SLITHERIN_OBSOLETE"
+SLITHERIN_VERSION = "0.7.1"

slitherin/detectors/dubious_typecast.py

@@ -71,6 +71,8 @@ class DubiousTypecast(AbstractDetector):
  )
  WIKI_RECOMMENDATION = "Use clear constants"
 
+ WHITELIST = ["SafeCast", "SignedMath"] # OZ
+
  def analyze_irs(self, irs: List[Operation]) -> List[Tuple[str, str]]:
  results = []
  for i in irs:
@@ -98,6 +100,8 @@ def get_dubious_typecasts(self, fun: FunctionContract, params=None):
  def _detect(self):
  results = []
  for contract in self.compilation_unit.contracts_derived:
+ if contract.name in self.WHITELIST:
+ continue
  for f in contract.functions:
  func_res = self.get_dubious_typecasts(f)
  if func_res:

slitherin/detectors/event_setter.py

@@ -10,36 +10,44 @@ class EventSetter(AbstractDetector):
  Sees if contract setters do not emit events
  """
 
- ARGUMENT = 'pess-event-setter' # slither will launch the detector with slither.py --detect mydetector
- HELP = 'Contract function does not emit event after the value is set'
+ ARGUMENT = "pess-event-setter"  # slither will launch the detector with slither.py --detect mydetector
+ HELP = "Contract function does not emit event after the value is set"
  IMPACT = DetectorClassification.LOW
  CONFIDENCE = DetectorClassification.MEDIUM
 
- WIKI = 'https://github.com/pessimistic-io/slitherin/blob/master/docs/event_setter.md'
- WIKI_TITLE = 'Missing Event Setter'
+ WIKI = (
+ "https://github.com/pessimistic-io/slitherin/blob/master/docs/event_setter.md"
+ )
+ WIKI_TITLE = "Missing Event Setter"
  WIKI_DESCRIPTION = "Setter-functions must emit events"
- WIKI_EXPLOIT_SCENARIO = 'N/A'
- WIKI_RECOMMENDATION = 'Emit events in setter functions'
+ WIKI_EXPLOIT_SCENARIO = "N/A"
+ WIKI_RECOMMENDATION = "Emit events in setter functions"
 
-
- def _emits_event(self, fun: Function) -> bool: 
+ def _emits_event(self, fun: Function) -> bool:
  """Checks if function has multiple storage read"""
- if isinstance(fun, Function): # check for a correct function type
- if any(ir for node in fun.nodes for ir in node.irs if isinstance(ir, EventCall)):
+ if isinstance(fun, Function): # check for a correct function type
+ if any(
+ ir for node in fun.nodes for ir in node.irs if isinstance(ir, EventCall)
+ ):
  return True
  return False
 
  def _detect(self) -> List[Output]:
  """Main function"""
  res = []
  for contract in self.compilation_unit.contracts_derived:
- if not contract.is_interface:
+ if not contract.is_interface and not contract.is_library:
  for f in contract.functions_and_modifiers_declared:
  if f.name.startswith("set"):
  x = self._emits_event(f)
  if not x:
- res.append(self.generate_result([
- "Setter function ",
- f, ' does not emit an event'
- '\n']))
+ res.append(
+ self.generate_result(
+ [
+ "Setter function ",
+ f,
+ " does not emit an event" "\n",
+ ]
+ )
+ )
  return res

slitherin/detectors/magic_number.py

@@ -26,6 +26,7 @@ class MagicNumber(AbstractDetector):
 
  EXCEPTION = {"0", "1", "2", "1000", "1e18"}
  used_count = defaultdict(lambda: {"count": 0, "nodes": []})
+ WHITELIST = ["SafeCast", "Math"]
 
  def _check_if_pow_10(self, str: str) -> bool:
  reg = re.fullmatch(r"^10*$|^10*e\d+$", str) # 1(0..) or 1(0..)eX
@@ -63,6 +64,8 @@ def _detect(self) -> List[Output]:
  res = []
  all_used_counts = []
  for contract in self.compilation_unit.contracts_derived:
+ if contract.name in self.WHITELIST:
+ continue
  self.used_count = defaultdict(lambda: {"count": 0, "nodes": []})
  for f in contract.functions:
  self._getLiterals(f)
@@ -71,7 +74,7 @@ def _detect(self) -> List[Output]:
  for contract_used_count in all_used_counts:
  for num, data in contract_used_count.items():
  if num:
- if data["count"] < 2:
+ if data["count"] <= 2:
  continue
  info = [f"Magic number {num} is used multiple times in:\n"]
  for n in data["nodes"]:

slitherin/detectors/obsolete/README.md

@@ -0,0 +1,4 @@
+# Obsolete detectors
+These detectors are deprecated.
+## How to still use it
+Set a flag `SLITHERIN_OBSOLETE=true`

slitherin/detectors/call_forward_to_protected.py → slitherin/detectors/obsolete/call_forward_to_protected.py

@@ -50,7 +50,7 @@ def _detect_low_level_custom_address_call(self, fun: Function) -> bool:
  return True
  return False
 
- def _pess_is_excluded_from_detector(self, contract: "Contract") -> bool:
+ def _pess_is_excluded_from_detector(self, contract) -> bool:
  path = Path(contract.source_mapping.filename.absolute).parts
  is_zep = "openzeppelin-solidity" in path or \
  ("@openzeppelin" in path and path[path.index("@openzeppelin") + 1] == "contracts") or \

slitherin/detectors/strange_setter.py

@@ -5,6 +5,7 @@
 import slither.core.expressions.new_array as na
 import slither.core.expressions.new_contract as nc
 from slither.analyses.data_dependency.data_dependency import is_dependent
+from slither.slithir.operations.internal_call import InternalCall
 
 
 class StrangeSetter(AbstractDetector):
@@ -21,7 +22,9 @@ class StrangeSetter(AbstractDetector):
  "https://github.com/pessimistic-io/slitherin/blob/master/docs/strange_setter.md"
  )
  WIKI_TITLE = "Strange Setter"
- WIKI_DESCRIPTION = "Setter must write to storage variables or pass arguments to external calls"
+ WIKI_DESCRIPTION = (
+ "Setter must write to storage variables or pass arguments to external calls"
+ )
  WIKI_EXPLOIT_SCENARIO = "-"
  WIKI_RECOMMENDATION = "Make sure that your setter actually sets something"
 
@@ -34,26 +37,26 @@ def _is_strange_setter(self, fun: Function) -> bool:
  # nothing is in the params, so we don't care
  return False
  used_params = set()
- for (
- fin
- ) in fun.internal_calls: # branch with for-loop for setters in internal calls
- if isinstance(fin, Function):
- for param in fin.parameters:
- for n in fin.nodes:
- if n.state_variables_written and str(param) in str(
- n
- ): # check if there's a state variable setter using function parameters
- used_params.add(param)
+
+ for node in fun.nodes:
+ for ir in node.irs:
+ if isinstance(ir, InternalCall):
+ for arg in ir.arguments:
+ used_params.add(arg)
+
  for param in fun.parameters:
  if fun.state_variables_written:
  for n in fun.nodes:
  if str(param) in str(n):
  used_params.add(param)
+
  for param in fun.parameters:
  for external in fun.external_calls_as_expressions:
  if isinstance(external._called, na.NewArray):
  continue
- if isinstance(external._called, nc.NewContract): # skip new contract calls, idk how to get arguments passed to creation
+ if isinstance(
+ external._called, nc.NewContract
+ ): # skip new contract calls, idk how to get arguments passed to creation
  continue
  for arg in [*external.arguments, external._called._expression]:
  if str(arg) == str(param):
@@ -76,7 +79,7 @@ def _detect(self) -> List[Output]:
  """Main function"""
  res = []
  for contract in self.compilation_unit.contracts_derived:
- if not contract.is_interface:
+ if not contract.is_interface and not contract.is_library:
  overriden_funtions = [] # functions that are overridden
  for f in contract.functions:
  if f.parameters:

tests/strange_setter_test.sol

@@ -44,18 +44,28 @@ contract StrangeSetter {
  vulnurable_internal(x);
  }
 
- function setSwapEnabledExternal_ok(ExternalContract target, bool swapEnabled) external onlyOwner {
+ function setSwapEnabledExternal_ok(
+ ExternalContract target,
+ bool swapEnabled
+ ) external onlyOwner {
  target.set(swapEnabled);
  }
 
- function setUseOnlyOneArg_vulnerable(uint256 arg1, bool isProtectedArg) external onlyOwner {
+ function setUseOnlyOneArg_vulnerable(
+ uint256 arg1,
+ bool isProtectedArg
+ ) external onlyOwner {
  isProtected = isProtectedArg;
  }
 
  function set_ok(uint256 setter) public onlyOwner {
  toSet = setter;
  }
 
+ function set_ok2(uint val) public onlyOwner {
+ vulnurable_internal(val);
+ }
+
  function set_ok_with_temp_war(uint256 setter) public onlyOwner {
  uint k = setter * 100;
  toSet = k;
@@ -88,12 +98,12 @@ contract OkConstructor {
  }
 }
 
-contract TestInheritance is StrangeSetter{
+contract TestInheritance is StrangeSetter {
  constructor(uint256 _toSet) StrangeSetter(_toSet) {}
 }
 
 contract TestNewContract {
  constructor(uint256 _toSet) {
  new TestInheritance(_toSet);
  }
-}
+}