strange setter fix

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);
  }
-}
+}