From c307d1b0d00159f83a1670ed903d0631a6506b3b Mon Sep 17 00:00:00 2001 From: Cryptomental Date: Sat, 20 Oct 2018 12:13:53 +0200 Subject: [PATCH 1/4] detectors: Rewrite unitialized state variables with IR. Refs: https://github.com/trailofbits/slither/issues/30 --- scripts/travis_test.sh | 2 +- .../uninitialized_state_variables.py | 67 +++++++++++-------- tests/uninitialized.sol | 46 +++++++++++++ 3 files changed, 86 insertions(+), 29 deletions(-) diff --git a/scripts/travis_test.sh b/scripts/travis_test.sh index 0e4e4030a..754661234 100755 --- a/scripts/travis_test.sh +++ b/scripts/travis_test.sh @@ -4,7 +4,7 @@ slither tests/uninitialized.sol --disable-solc-warnings --detect-uninitialized-state -if [ $? -ne 1 ]; then +if [ $? -ne 3 ]; then exit 1 fi diff --git a/slither/detectors/variables/uninitialized_state_variables.py b/slither/detectors/variables/uninitialized_state_variables.py index 717425e26..e24fdf298 100644 --- a/slither/detectors/variables/uninitialized_state_variables.py +++ b/slither/detectors/variables/uninitialized_state_variables.py @@ -2,16 +2,20 @@ Module detecting state uninitialized variables Recursively check the called functions - The heuristic chekcs that: - - state variables are read or called - - the variables does not call push (avoid too many FP) + The heuristic checks: + - state variables including mappings/refs + - LibraryCalls, InternalCalls, InternalDynamicCalls with storage variables Only analyze "leaf" contracts (contracts that are not inherited by another contract) """ from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.core.variables.state_variable import StateVariable +from slither.slithir.variables import ReferenceVariable +from slither.slithir.operations.assignment import Assignment -from slither.visitors.expression.find_push import FindPush +from slither.slithir.operations import (OperationWithLValue, Index, Member, + InternalCall, InternalDynamicCall, LibraryCall) class UninitializedStateVarsDetection(AbstractDetector): @@ -24,29 +28,36 @@ class UninitializedStateVarsDetection(AbstractDetector): IMPACT = DetectorClassification.HIGH CONFIDENCE = DetectorClassification.HIGH - def detect_uninitialized(self, contract): - # get all the state variables read by all functions - var_read = [f.state_variables_read for f in contract.all_functions_called + contract.modifiers] - # flat list - var_read = [item for sublist in var_read for item in sublist] - # remove state variable that are initiliazed at contract construction - var_read = [v for v in var_read if v.uninitialized] - - # get all the state variables written by the functions - var_written = [f.state_variables_written for f in contract.all_functions_called + contract.modifiers] - # flat list - var_written = [item for sublist in var_written for item in sublist] - - all_push = [f.apply_visitor(FindPush) for f in contract.functions] - # flat list - all_push = [item for sublist in all_push for item in sublist] + @staticmethod + def written_variables(contract): + ret = [] + for f in contract.all_functions_called + contract.modifiers: + for n in f.nodes: + for ir in n.irs: + if isinstance(ir, (Index, Member)): + continue # Don't consider Member and Index operations -> ReferenceVariable + elif isinstance(ir, OperationWithLValue) and isinstance(ir.lvalue, StateVariable): + ret.append(ir.lvalue) + elif isinstance(ir, Assignment) and isinstance(ir.lvalue, ReferenceVariable): + dest = ir.lvalue + while isinstance(dest, ReferenceVariable): + dest = dest.points_to + ret.append(dest) + elif isinstance(ir, LibraryCall) \ + or isinstance(ir, InternalCall) \ + or isinstance(ir, InternalDynamicCall): + for v in ir.arguments: + ret.append(v) + for param in f.parameters: + if param.location == 'storage': + ret.append(param) + + return ret - uninitialized_vars = list(set([v for v in var_read if \ - v not in var_written and \ - v not in all_push and \ - v.type not in contract.using_for])) # Note: does not handle using X for * - - return [(v, contract.get_functions_reading_from_variable(v)) for v in uninitialized_vars] + def detect_uninitialized(self, contract): + written_variables = self.written_variables(contract) + return [(variable, contract.get_functions_reading_from_variable(variable)) + for variable in contract.state_variables if variable not in written_variables] def detect(self): """ Detect uninitialized state variables @@ -61,8 +72,8 @@ class UninitializedStateVarsDetection(AbstractDetector): for variable, functions in ret: info = "Uninitialized state variable in %s, " % self.filename + \ "Contract: %s, Variable: %s, Used in %s" % (c.name, - str(variable), - [str(f) for f in functions]) + str(variable), + [str(f) for f in functions]) self.log(info) source = [variable.source_mapping] diff --git a/tests/uninitialized.sol b/tests/uninitialized.sol index ff3f1addc..d31eb10bf 100644 --- a/tests/uninitialized.sol +++ b/tests/uninitialized.sol @@ -9,3 +9,49 @@ contract Uninitialized{ } } + + +contract Test { + mapping (address => uint) balances; + mapping (address => uint) balancesInitialized; + + + function init() { + balancesInitialized[msg.sender] = 0; + } + + function use() { + // random operation to use the mapping + require(balances[msg.sender] == balancesInitialized[msg.sender]); + } +} + +library Lib{ + + struct MyStruct{ + uint val; + } + + function set(MyStruct storage st){ + st.val = 4; + } + +} + + +contract Test2 { + using Lib for Lib.MyStruct; + + Lib.MyStruct st; + Lib.MyStruct stInitiliazed; + + function init(){ + stInitiliazed.set(); + } + + function use(){ + // random operation to use the structure + require(st.val == stInitiliazed.val); + } + +} \ No newline at end of file From 16e20508c03057dc4996077eec6f840b7c4863ea Mon Sep 17 00:00:00 2001 From: Cryptomental Date: Thu, 25 Oct 2018 18:34:34 +0200 Subject: [PATCH 2/4] detectors: Use new API for state vars written in uninitialized state detector. Refs: https://github.com/trailofbits/slither/issues/30 --- .../variables/uninitialized_state_variables.py | 12 ++---------- 1 file changed, 2 insertions(+), 10 deletions(-) diff --git a/slither/detectors/variables/uninitialized_state_variables.py b/slither/detectors/variables/uninitialized_state_variables.py index e24fdf298..152cbbead 100644 --- a/slither/detectors/variables/uninitialized_state_variables.py +++ b/slither/detectors/variables/uninitialized_state_variables.py @@ -33,17 +33,9 @@ class UninitializedStateVarsDetection(AbstractDetector): ret = [] for f in contract.all_functions_called + contract.modifiers: for n in f.nodes: + ret += n.state_variables_written for ir in n.irs: - if isinstance(ir, (Index, Member)): - continue # Don't consider Member and Index operations -> ReferenceVariable - elif isinstance(ir, OperationWithLValue) and isinstance(ir.lvalue, StateVariable): - ret.append(ir.lvalue) - elif isinstance(ir, Assignment) and isinstance(ir.lvalue, ReferenceVariable): - dest = ir.lvalue - while isinstance(dest, ReferenceVariable): - dest = dest.points_to - ret.append(dest) - elif isinstance(ir, LibraryCall) \ + if isinstance(ir, LibraryCall) \ or isinstance(ir, InternalCall) \ or isinstance(ir, InternalDynamicCall): for v in ir.arguments: From 6afb9101e96ef25661b47b995188c03d15820cce Mon Sep 17 00:00:00 2001 From: Cryptomental Date: Thu, 25 Oct 2018 18:42:40 +0200 Subject: [PATCH 3/4] travis_test: Increase detect unitialized state findings to three. The test case was extended with new edge cases. Refs: https://github.com/trailofbits/slither/issues/30 --- scripts/travis_test.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/travis_test.sh b/scripts/travis_test.sh index 6e788a30c..2fb3971dc 100755 --- a/scripts/travis_test.sh +++ b/scripts/travis_test.sh @@ -15,7 +15,7 @@ test_slither(){ fi } -test_slither tests/uninitialized.sol "--detect-uninitialized-state" 1 +test_slither tests/uninitialized.sol "--detect-uninitialized-state" 3 test_slither tests/backdoor.sol "--detect-backdoor" 1 test_slither tests/backdoor.sol "--detect-suicidal" 1 test_slither tests/pragma.0.4.24.sol "--detect-pragma" 1 From dece5ab023f0af1e02f3cde7a29786adf0133fce Mon Sep 17 00:00:00 2001 From: Josselin Date: Fri, 26 Oct 2018 11:15:59 +0100 Subject: [PATCH 4/4] UninitializedStateVarsDetection: Fix incorrect storage parameters add --- scripts/travis_test.sh | 2 +- .../detectors/variables/uninitialized_state_variables.py | 8 ++++---- tests/uninitialized.sol | 7 ++++--- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/scripts/travis_test.sh b/scripts/travis_test.sh index ccf46d177..b3378aaad 100755 --- a/scripts/travis_test.sh +++ b/scripts/travis_test.sh @@ -15,7 +15,7 @@ test_slither(){ fi } -test_slither tests/uninitialized.sol "uninitialized-state" 3 +test_slither tests/uninitialized.sol "uninitialized-state" 4 test_slither tests/backdoor.sol "backdoor" 1 test_slither tests/backdoor.sol "suicidal" 1 test_slither tests/pragma.0.4.24.sol "pragma" 1 diff --git a/slither/detectors/variables/uninitialized_state_variables.py b/slither/detectors/variables/uninitialized_state_variables.py index 152cbbead..8ec652782 100644 --- a/slither/detectors/variables/uninitialized_state_variables.py +++ b/slither/detectors/variables/uninitialized_state_variables.py @@ -38,11 +38,11 @@ class UninitializedStateVarsDetection(AbstractDetector): if isinstance(ir, LibraryCall) \ or isinstance(ir, InternalCall) \ or isinstance(ir, InternalDynamicCall): - for v in ir.arguments: - ret.append(v) - for param in f.parameters: + idx = 0 + for param in ir.function.parameters: if param.location == 'storage': - ret.append(param) + ret.append(ir.arguments[idx]) + idx = idx+1 return ret diff --git a/tests/uninitialized.sol b/tests/uninitialized.sol index d31eb10bf..2305483e6 100644 --- a/tests/uninitialized.sol +++ b/tests/uninitialized.sol @@ -32,7 +32,7 @@ library Lib{ uint val; } - function set(MyStruct storage st){ + function set(MyStruct storage st, uint v){ st.val = 4; } @@ -44,9 +44,10 @@ contract Test2 { Lib.MyStruct st; Lib.MyStruct stInitiliazed; + uint v; // v is used as parameter of the lib, but is never init function init(){ - stInitiliazed.set(); + stInitiliazed.set(v); } function use(){ @@ -54,4 +55,4 @@ contract Test2 { require(st.val == stInitiliazed.val); } -} \ No newline at end of file +}