Merge pull request #117 from trailofbits/dev-reentrancy

Refactor reentrancy detector into three variants
pull/125/head
Feist Josselin 6 years ago committed by GitHub
commit c9d18ab4c8
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
  1. 31
      README.md
  2. 4
      scripts/tests_generate_expected_json_4.sh
  3. 1
      scripts/tests_generate_expected_json_5.sh
  4. 2
      scripts/travis_test_4.sh
  5. 2
      scripts/travis_test_5.sh
  6. 8
      slither/__main__.py
  7. 226
      slither/detectors/reentrancy/reentrancy_benign.py
  8. 30
      slither/detectors/reentrancy/reentrancy_eth.py
  9. 216
      slither/detectors/reentrancy/reentrancy_read_before_write.py
  10. 1
      tests/expected_json/reentrancy-0.5.1.reentrancy-eth.json
  11. 1
      tests/expected_json/reentrancy-0.5.1.reentrancy.json
  12. 1
      tests/expected_json/reentrancy.reentrancy-eth.json
  13. 1
      tests/expected_json/reentrancy.reentrancy.json
  14. 21
      tests/reentrancy.sol

@ -59,24 +59,25 @@ Num | Detector | What it Detects | Impact | Confidence
4 | `uninitialized-storage` | [Uninitialized storage variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-storage-variables) | High | High 4 | `uninitialized-storage` | [Uninitialized storage variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-storage-variables) | High | High
5 | `arbitrary-send` | [Functions that send ether to arbitrary destinations](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations) | High | Medium 5 | `arbitrary-send` | [Functions that send ether to arbitrary destinations](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#functions-that-send-ether-to-arbitrary-destinations) | High | Medium
6 | `controlled-delegatecall` | [Controlled delegatecall destination](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#controlled-delegatecall) | High | Medium 6 | `controlled-delegatecall` | [Controlled delegatecall destination](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#controlled-delegatecall) | High | Medium
7 | `reentrancy` | [Reentrancy vulnerabilities](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities) | High | Medium 7 | `reentrancy-eth` | [Reentrancy vulnerabilities (theft of ethers)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities) | High | Medium
8 | `locked-ether` | [Contracts that lock ether](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#contracts-that-lock-ether) | Medium | High 8 | `locked-ether` | [Contracts that lock ether](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#contracts-that-lock-ether) | Medium | High
9 | `shadowing-abstract` | [State variables shadowing from abstract contracts](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variable-shadowing-from-abstract-contracts) | Medium | High 9 | `shadowing-abstract` | [State variables shadowing from abstract contracts](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variable-shadowing-from-abstract-contracts) | Medium | High
10 | `constant-function` | [Constant functions changing the state](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#constant-functions-changing-the-state) | Medium | Medium 10 | `constant-function` | [Constant functions changing the state](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#constant-functions-changing-the-state) | Medium | Medium
11 | `tx-origin` | [Dangerous usage of `tx.origin`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-usage-of-txorigin) | Medium | Medium 11 | `reentrancy-no-eth` | [Reentrancy vulnerabilities (no theft of ethers)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities) | Medium | Medium
12 | `uninitialized-local` | [Uninitialized local variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-local-variables) | Medium | Medium 12 | `tx-origin` | [Dangerous usage of `tx.origin`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#dangerous-usage-of-txorigin) | Medium | Medium
13 | `unused-return` | [Unused return values](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-return) | Medium | Medium 13 | `uninitialized-local` | [Uninitialized local variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#uninitialized-local-variables) | Medium | Medium
14 | `calls-loop` | [Multiple calls in a loop](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#calls-inside-a-loop) | Low | Medium 14 | `unused-return` | [Unused return values](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-return) | Medium | Medium
15 | `timestamp` | [Dangerous usage of `block.timestamp`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#block-timestamp) | Low | Medium 15 | `calls-loop` | [Multiple calls in a loop](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description/_edit#calls-inside-a-loop) | Low | Medium
16 | `assembly` | [Assembly usage](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#assembly-usage) | Informational | High 16 | `reentrancy-benign` | [Benign reentrancy vulnerabilities](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities) | Low | Medium
17 | `constable-states` | [State variables that could be declared constant](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variables-that-could-be-declared-constant) | Informational | High 17 | `timestamp` | [Dangerous usage of `block.timestamp`](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#block-timestamp) | Low | Medium
18 | `external-function` | [Public function that could be declared as external](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#public-function-that-could-be-declared-as-external) | Informational | High 18 | `assembly` | [Assembly usage](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#assembly-usage) | Informational | High
19 | `low-level-calls` | [Low level calls](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#low-level-calls) | Informational | High 19 | `constable-states` | [State variables that could be declared constant](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#state-variables-that-could-be-declared-constant) | Informational | High
20 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#conformance-to-solidity-naming-conventions) | Informational | High 20 | `external-function` | [Public function that could be declared as external](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#public-function-that-could-be-declared-as-external) | Informational | High
21 | `pragma` | [If different pragma directives are used](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#different-pragma-directives-are-used) | Informational | High 21 | `low-level-calls` | [Low level calls](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#low-level-calls) | Informational | High
22 | `solc-version` | [Old versions of Solidity (< 0.4.23)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#old-versions-of-solidity) | Informational | High 22 | `naming-convention` | [Conformance to Solidity naming conventions](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#conformance-to-solidity-naming-conventions) | Informational | High
23 | `unused-state` | [Unused state variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-state-variables) | Informational | High 23 | `pragma` | [If different pragma directives are used](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#different-pragma-directives-are-used) | Informational | High
24 | `solc-version` | [Old versions of Solidity (< 0.4.23)](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#old-versions-of-solidity) | Informational | High
25 | `unused-state` | [Unused state variables](https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#unused-state-variables) | Informational | High
[Contact us](https://www.trailofbits.com/contact/) to get access to additional detectors. [Contact us](https://www.trailofbits.com/contact/) to get access to additional detectors.

@ -19,7 +19,7 @@ generate_expected_json(){
#generate_expected_json tests/backdoor.sol "suicidal" #generate_expected_json tests/backdoor.sol "suicidal"
#generate_expected_json tests/pragma.0.4.24.sol "pragma" #generate_expected_json tests/pragma.0.4.24.sol "pragma"
#generate_expected_json tests/old_solc.sol.json "solc-version" #generate_expected_json tests/old_solc.sol.json "solc-version"
#generate_expected_json tests/reentrancy.sol "reentrancy" #generate_expected_json tests/reentrancy.sol "reentrancy-eth"
#generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage" #generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage"
#generate_expected_json tests/tx_origin.sol "tx-origin" #generate_expected_json tests/tx_origin.sol "tx-origin"
#generate_expected_json tests/unused_state.sol "unused-state" #generate_expected_json tests/unused_state.sol "unused-state"
@ -30,7 +30,7 @@ generate_expected_json(){
#generate_expected_json tests/low_level_calls.sol "low-level-calls" #generate_expected_json tests/low_level_calls.sol "low-level-calls"
#generate_expected_json tests/const_state_variables.sol "constable-states" #generate_expected_json tests/const_state_variables.sol "constable-states"
#generate_expected_json tests/external_function.sol "external-function" #generate_expected_json tests/external_function.sol "external-function"
generate_expected_json tests/external_function_2.sol "external-function" #generate_expected_json tests/external_function_2.sol "external-function"
#generate_expected_json tests/naming_convention.sol "naming-convention" #generate_expected_json tests/naming_convention.sol "naming-convention"
#generate_expected_json tests/uninitialized_local_variable.sol "uninitialized-local" #generate_expected_json tests/uninitialized_local_variable.sol "uninitialized-local"
#generate_expected_json tests/controlled_delegatecall.sol "controlled-delegatecall" #generate_expected_json tests/controlled_delegatecall.sol "controlled-delegatecall"

@ -19,6 +19,7 @@ generate_expected_json(){
#generate_expected_json tests/backdoor.sol "suicidal" #generate_expected_json tests/backdoor.sol "suicidal"
#generate_expected_json tests/pragma.0.4.24.sol "pragma" #generate_expected_json tests/pragma.0.4.24.sol "pragma"
#generate_expected_json tests/old_solc.sol.json "solc-version" #generate_expected_json tests/old_solc.sol.json "solc-version"
#generate_expected_json tests/reentrancy-0.5.1.sol "reentrancy-eth"
#generate_expected_json tests/reentrancy-0.5.1.sol "reentrancy" #generate_expected_json tests/reentrancy-0.5.1.sol "reentrancy"
#generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage" #generate_expected_json tests/uninitialized_storage_pointer.sol "uninitialized-storage"
#generate_expected_json tests/tx_origin-0.5.1.sol "tx-origin" #generate_expected_json tests/tx_origin-0.5.1.sol "tx-origin"

@ -70,7 +70,7 @@ test_slither tests/backdoor.sol "backdoor"
test_slither tests/backdoor.sol "suicidal" test_slither tests/backdoor.sol "suicidal"
test_slither tests/pragma.0.4.24.sol "pragma" test_slither tests/pragma.0.4.24.sol "pragma"
test_slither tests/old_solc.sol.json "solc-version" test_slither tests/old_solc.sol.json "solc-version"
test_slither tests/reentrancy.sol "reentrancy" test_slither tests/reentrancy.sol "reentrancy-eth"
test_slither tests/uninitialized_storage_pointer.sol "uninitialized-storage" test_slither tests/uninitialized_storage_pointer.sol "uninitialized-storage"
test_slither tests/tx_origin.sol "tx-origin" test_slither tests/tx_origin.sol "tx-origin"
test_slither tests/unused_state.sol "unused-state" test_slither tests/unused_state.sol "unused-state"

@ -69,7 +69,7 @@ test_slither tests/uninitialized-0.5.1.sol "uninitialized-state"
test_slither tests/backdoor.sol "backdoor" test_slither tests/backdoor.sol "backdoor"
test_slither tests/backdoor.sol "suicidal" test_slither tests/backdoor.sol "suicidal"
test_slither tests/old_solc.sol.json "solc-version" test_slither tests/old_solc.sol.json "solc-version"
test_slither tests/reentrancy-0.5.1.sol "reentrancy" test_slither tests/reentrancy-0.5.1.sol "reentrancy-eth"
test_slither tests/tx_origin-0.5.1.sol "tx-origin" test_slither tests/tx_origin-0.5.1.sol "tx-origin"
test_slither tests/unused_state.sol "unused-state" test_slither tests/unused_state.sol "unused-state"
test_slither tests/locked_ether-0.5.1.sol "locked-ether" test_slither tests/locked_ether-0.5.1.sol "locked-ether"

@ -117,7 +117,9 @@ def get_detectors_and_printers():
from slither.detectors.functions.arbitrary_send import ArbitrarySend from slither.detectors.functions.arbitrary_send import ArbitrarySend
from slither.detectors.functions.suicidal import Suicidal from slither.detectors.functions.suicidal import Suicidal
from slither.detectors.functions.complex_function import ComplexFunction from slither.detectors.functions.complex_function import ComplexFunction
from slither.detectors.reentrancy.reentrancy import Reentrancy from slither.detectors.reentrancy.reentrancy_benign import ReentrancyBenign
from slither.detectors.reentrancy.reentrancy_read_before_write import ReentrancyReadBeforeWritten
from slither.detectors.reentrancy.reentrancy_eth import ReentrancyEth
from slither.detectors.variables.unused_state_variables import UnusedStateVars from slither.detectors.variables.unused_state_variables import UnusedStateVars
from slither.detectors.variables.possible_const_state_variables import ConstCandidateStateVars from slither.detectors.variables.possible_const_state_variables import ConstCandidateStateVars
from slither.detectors.statements.tx_origin import TxOrigin from slither.detectors.statements.tx_origin import TxOrigin
@ -140,7 +142,9 @@ def get_detectors_and_printers():
UninitializedLocalVars, UninitializedLocalVars,
ConstantPragma, ConstantPragma,
OldSolc, OldSolc,
Reentrancy, ReentrancyBenign,
ReentrancyReadBeforeWritten,
ReentrancyEth,
LockedEther, LockedEther,
ArbitrarySend, ArbitrarySend,
Suicidal, Suicidal,

@ -0,0 +1,226 @@
""""
Re-entrancy detection
Based on heuristics, it may lead to FP and FN
Iterate over all the nodes of the graph until reaching a fixpoint
"""
from slither.core.cfg.node import NodeType
from slither.core.declarations import Function, SolidityFunction
from slither.core.expressions import UnaryOperation, UnaryOperationType
from slither.detectors.abstract_detector import (AbstractDetector,
DetectorClassification)
from slither.visitors.expression.export_values import ExportValues
from slither.slithir.operations import (HighLevelCall, LowLevelCall,
LibraryCall,
Send, Transfer)
class ReentrancyBenign(AbstractDetector):
ARGUMENT = 'reentrancy-benign'
HELP = 'Benign reentrancy vulnerabilities'
IMPACT = DetectorClassification.LOW
CONFIDENCE = DetectorClassification.MEDIUM
WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities'
key = 'REENTRANCY-BENIGN'
@staticmethod
def _can_callback(node):
"""
Detect if the node contains a call that can
be used to re-entrance
Consider as valid target:
- low level call
- high level call
Do not consider Send/Transfer as there is not enough gas
"""
for ir in node.irs:
if isinstance(ir, LowLevelCall):
return True
if isinstance(ir, HighLevelCall) and not isinstance(ir, LibraryCall):
return True
return False
@staticmethod
def _can_send_eth(node):
"""
Detect if the node can send eth
"""
for ir in node.irs:
if isinstance(ir, (HighLevelCall, LowLevelCall, Transfer, Send)):
if ir.call_value:
return True
return False
def _filter_if(self, node):
"""
Check if the node is a condtional node where
there is an external call checked
Heuristic:
- The call is a IF node
- It contains a, external call
- The condition is the negation (!)
This will work only on naive implementation
"""
return isinstance(node.expression, UnaryOperation)\
and node.expression.type == UnaryOperationType.BANG
def _explore(self, node, visited, skip_father=None):
"""
Explore the CFG and look for re-entrancy
Heuristic: There is a re-entrancy if a state variable is written
after an external call
node.context will contains the external calls executed
It contains the calls executed in father nodes
if node.context is not empty, and variables are written, a re-entrancy is possible
"""
if node in visited:
return
visited = visited + [node]
# First we add the external calls executed in previous nodes
# send_eth returns the list of calls sending value
# calls returns the list of calls that can callback
# read returns the variable read
fathers_context = {'send_eth':[], 'calls':[], 'read':[]}
for father in node.fathers:
if self.key in father.context:
fathers_context['send_eth'] += [s for s in father.context[self.key]['send_eth'] if s!=skip_father]
fathers_context['calls'] += [c for c in father.context[self.key]['calls'] if c!=skip_father]
fathers_context['read'] += father.context[self.key]['read']
# Exclude path that dont bring further information
if node in self.visited_all_paths:
if all(call in self.visited_all_paths[node]['calls'] for call in fathers_context['calls']):
if all(send in self.visited_all_paths[node]['send_eth'] for send in fathers_context['send_eth']):
if all(read in self.visited_all_paths[node]['read'] for read in fathers_context['read']):
return
else:
self.visited_all_paths[node] = {'send_eth':[], 'calls':[], 'read':[]}
self.visited_all_paths[node]['send_eth'] = list(set(self.visited_all_paths[node]['send_eth'] + fathers_context['send_eth']))
self.visited_all_paths[node]['calls'] = list(set(self.visited_all_paths[node]['calls'] + fathers_context['calls']))
self.visited_all_paths[node]['read'] = list(set(self.visited_all_paths[node]['read'] + fathers_context['read']))
node.context[self.key] = fathers_context
contains_call = False
if self._can_callback(node):
node.context[self.key]['calls'] = list(set(node.context[self.key]['calls'] + [node]))
contains_call = True
if self._can_send_eth(node):
node.context[self.key]['send_eth'] = list(set(node.context[self.key]['send_eth'] + [node]))
# All the state variables written
state_vars_written = node.state_variables_written
# Add the state variables written in internal calls
for internal_call in node.internal_calls:
# Filter to Function, as internal_call can be a solidity call
if isinstance(internal_call, Function):
state_vars_written += internal_call.all_state_variables_written()
not_read_then_written = [(v, node) for v in state_vars_written if v not in node.context[self.key]['read']]
node.context[self.key]['read'] = list(set(node.context[self.key]['read'] + node.state_variables_read))
# If a state variables was read and is then written, there is a dangerous call and
# ether were sent
# We found a potential re-entrancy bug
if (not_read_then_written and
node.context[self.key]['calls']):
# calls are ordered
finding_key = (node.function,
tuple(set(node.context[self.key]['calls'])),
tuple(set(node.context[self.key]['send_eth'])))
finding_vars = not_read_then_written
if finding_key not in self.result:
self.result[finding_key] = []
self.result[finding_key] = list(set(self.result[finding_key] + finding_vars))
sons = node.sons
if contains_call and node.type in [NodeType.IF, NodeType.IFLOOP]:
if self._filter_if(node):
son = sons[0]
self._explore(son, visited, node)
sons = sons[1:]
else:
son = sons[1]
self._explore(son, visited, node)
sons = [sons[0]]
for son in sons:
self._explore(son, visited)
def detect_reentrancy(self, contract):
"""
"""
for function in contract.functions:
if function.is_implemented:
self._explore(function.entry_point, [])
def detect(self):
"""
"""
self.result = {}
# if a node was already visited by another path
# we will only explore it if the traversal brings
# new variables written
# This speedup the exploration through a light fixpoint
# Its particular useful on 'complex' functions with several loops and conditions
self.visited_all_paths = {}
for c in self.contracts:
self.detect_reentrancy(c)
results = []
result_sorted = sorted(list(self.result.items()), key=lambda x:x[0][0].name)
for (func, calls, send_eth), varsWritten in result_sorted:
calls = list(set(calls))
send_eth = list(set(send_eth))
info = 'Reentrancy in {}.{} ({}):\n'
info = info.format(func.contract.name, func.name, func.source_mapping_str)
info += '\tExternal calls:\n'
for call_info in calls:
info += '\t- {} ({})\n'.format(call_info.expression, call_info.source_mapping_str)
if calls != send_eth:
info += '\tExternal calls sending eth:\n'
for call_info in send_eth:
info += '\t- {} ({})\n'.format(call_info.expression, call_info.source_mapping_str)
info += '\tState variables written after the call(s):\n'
for (v, node) in varsWritten:
info += '\t- {} ({})\n'.format(v, node.source_mapping_str)
self.log(info)
sending_eth_json = []
if calls != send_eth:
sending_eth_json = [{'type' : 'external_calls_sending_eth',
'expression': str(call_info.expression),
'source_mapping': call_info.source_mapping}
for call_info in send_eth]
json = self.generate_json_result(info)
self.add_function_to_json(func, json)
json['elements'] += [{'type': 'external_calls',
'expression': str(call_info.expression),
'source_mapping': call_info.source_mapping}
for call_info in calls]
json['elements'] += sending_eth_json
json['elements'] += [{'type':'variables_written',
'name': v.name,
'expression': str(node.expression),
'source_mapping': node.source_mapping}
for (v, node) in varsWritten]
results.append(json)
return results

@ -14,15 +14,15 @@ from slither.slithir.operations import (HighLevelCall, LowLevelCall,
LibraryCall, LibraryCall,
Send, Transfer) Send, Transfer)
class Reentrancy(AbstractDetector): class ReentrancyEth(AbstractDetector):
ARGUMENT = 'reentrancy' ARGUMENT = 'reentrancy-eth'
HELP = 'Reentrancy vulnerabilities' HELP = 'Reentrancy vulnerabilities (theft of ethers)'
IMPACT = DetectorClassification.HIGH IMPACT = DetectorClassification.HIGH
CONFIDENCE = DetectorClassification.MEDIUM CONFIDENCE = DetectorClassification.MEDIUM
WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities' WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities'
key = 'REENTRANCY' key = 'REENTRANCY-ETHERS'
@staticmethod @staticmethod
def _can_callback(node): def _can_callback(node):
@ -54,7 +54,7 @@ class Reentrancy(AbstractDetector):
return True return True
return False return False
def _check_on_call_returned(self, node): def _filter_if(self, node):
""" """
Check if the node is a condtional node where Check if the node is a condtional node where
there is an external call checked there is an external call checked
@ -68,7 +68,7 @@ class Reentrancy(AbstractDetector):
return isinstance(node.expression, UnaryOperation)\ return isinstance(node.expression, UnaryOperation)\
and node.expression.type == UnaryOperationType.BANG and node.expression.type == UnaryOperationType.BANG
def _explore(self, node, visited): def _explore(self, node, visited, skip_father=None):
""" """
Explore the CFG and look for re-entrancy Explore the CFG and look for re-entrancy
Heuristic: There is a re-entrancy if a state variable is written Heuristic: There is a re-entrancy if a state variable is written
@ -92,8 +92,8 @@ class Reentrancy(AbstractDetector):
for father in node.fathers: for father in node.fathers:
if self.key in father.context: if self.key in father.context:
fathers_context['send_eth'] += father.context[self.key]['send_eth'] fathers_context['send_eth'] += [s for s in father.context[self.key]['send_eth'] if s!=skip_father]
fathers_context['calls'] += father.context[self.key]['calls'] fathers_context['calls'] += [c for c in father.context[self.key]['calls'] if c!=skip_father]
fathers_context['read'] += father.context[self.key]['read'] fathers_context['read'] += father.context[self.key]['read']
# Exclude path that dont bring further information # Exclude path that dont bring further information
@ -146,8 +146,16 @@ class Reentrancy(AbstractDetector):
self.result[finding_key] = list(set(self.result[finding_key] + finding_vars)) self.result[finding_key] = list(set(self.result[finding_key] + finding_vars))
sons = node.sons sons = node.sons
if contains_call and self._check_on_call_returned(node): if contains_call and node.type in [NodeType.IF, NodeType.IFLOOP]:
sons = sons[1:] if self._filter_if(node):
son = sons[0]
self._explore(son, visited, node)
sons = sons[1:]
else:
son = sons[1]
self._explore(son, visited, node)
sons = [sons[0]]
for son in sons: for son in sons:
self._explore(son, visited) self._explore(son, visited)
@ -203,7 +211,7 @@ class Reentrancy(AbstractDetector):
sending_eth_json = [{'type' : 'external_calls_sending_eth', sending_eth_json = [{'type' : 'external_calls_sending_eth',
'expression': str(call_info.expression), 'expression': str(call_info.expression),
'source_mapping': call_info.source_mapping} 'source_mapping': call_info.source_mapping}
for call_info in calls] for call_info in send_eth]
json = self.generate_json_result(info) json = self.generate_json_result(info)
self.add_function_to_json(func, json) self.add_function_to_json(func, json)

@ -0,0 +1,216 @@
""""
Re-entrancy detection
Based on heuristics, it may lead to FP and FN
Iterate over all the nodes of the graph until reaching a fixpoint
"""
from slither.core.cfg.node import NodeType
from slither.core.declarations import Function, SolidityFunction
from slither.core.expressions import UnaryOperation, UnaryOperationType
from slither.detectors.abstract_detector import (AbstractDetector,
DetectorClassification)
from slither.visitors.expression.export_values import ExportValues
from slither.slithir.operations import (HighLevelCall, LowLevelCall,
LibraryCall,
Send, Transfer)
class ReentrancyReadBeforeWritten(AbstractDetector):
ARGUMENT = 'reentrancy-no-eth'
HELP = 'Reentrancy vulnerabilities (no theft of ethers)'
IMPACT = DetectorClassification.MEDIUM
CONFIDENCE = DetectorClassification.MEDIUM
WIKI = 'https://github.com/trailofbits/slither/wiki/Vulnerabilities-Description#reentrancy-vulnerabilities'
key = 'REENTRANCY-NO-ETHER'
@staticmethod
def _can_callback(node):
"""
Detect if the node contains a call that can
be used to re-entrance
Consider as valid target:
- low level call
- high level call
Do not consider Send/Transfer as there is not enough gas
"""
for ir in node.irs:
if isinstance(ir, LowLevelCall):
return True
if isinstance(ir, HighLevelCall) and not isinstance(ir, LibraryCall):
return True
return False
@staticmethod
def _can_send_eth(node):
"""
Detect if the node can send eth
"""
for ir in node.irs:
if isinstance(ir, (HighLevelCall, LowLevelCall, Transfer, Send)):
if ir.call_value:
return True
return False
def _filter_if(self, node):
"""
Check if the node is a condtional node where
there is an external call checked
Heuristic:
- The call is a IF node
- It contains a, external call
- The condition is the negation (!)
This will work only on naive implementation
"""
return isinstance(node.expression, UnaryOperation)\
and node.expression.type == UnaryOperationType.BANG
def _explore(self, node, visited, skip_father=None):
"""
Explore the CFG and look for re-entrancy
Heuristic: There is a re-entrancy if a state variable is written
after an external call
node.context will contains the external calls executed
It contains the calls executed in father nodes
if node.context is not empty, and variables are written, a re-entrancy is possible
"""
if node in visited:
return
visited = visited + [node]
# First we add the external calls executed in previous nodes
# send_eth returns the list of calls sending value
# calls returns the list of calls that can callback
# read returns the variable read
fathers_context = {'send_eth':[], 'calls':[], 'read':[]}
for father in node.fathers:
if self.key in father.context:
fathers_context['send_eth'] += [s for s in father.context[self.key]['send_eth'] if s!=skip_father]
fathers_context['calls'] += [c for c in father.context[self.key]['calls'] if c!=skip_father]
fathers_context['read'] += father.context[self.key]['read']
# Exclude path that dont bring further information
if node in self.visited_all_paths:
if all(call in self.visited_all_paths[node]['calls'] for call in fathers_context['calls']):
if all(send in self.visited_all_paths[node]['send_eth'] for send in fathers_context['send_eth']):
if all(read in self.visited_all_paths[node]['read'] for read in fathers_context['read']):
return
else:
self.visited_all_paths[node] = {'send_eth':[], 'calls':[], 'read':[]}
self.visited_all_paths[node]['send_eth'] = list(set(self.visited_all_paths[node]['send_eth'] + fathers_context['send_eth']))
self.visited_all_paths[node]['calls'] = list(set(self.visited_all_paths[node]['calls'] + fathers_context['calls']))
self.visited_all_paths[node]['read'] = list(set(self.visited_all_paths[node]['read'] + fathers_context['read']))
node.context[self.key] = fathers_context
contains_call = False
if self._can_callback(node):
node.context[self.key]['calls'] = list(set(node.context[self.key]['calls'] + [node]))
contains_call = True
if self._can_send_eth(node):
node.context[self.key]['send_eth'] = list(set(node.context[self.key]['send_eth'] + [node]))
# All the state variables written
state_vars_written = node.state_variables_written
# Add the state variables written in internal calls
for internal_call in node.internal_calls:
# Filter to Function, as internal_call can be a solidity call
if isinstance(internal_call, Function):
state_vars_written += internal_call.all_state_variables_written()
read_then_written = [(v, node) for v in state_vars_written if v in node.context[self.key]['read']]
node.context[self.key]['read'] = list(set(node.context[self.key]['read'] + node.state_variables_read))
# If a state variables was read and is then written, there is a dangerous call and
# ether were sent
# We found a potential re-entrancy bug
if (read_then_written and
node.context[self.key]['calls'] and
not node.context[self.key]['send_eth']):
# calls are ordered
finding_key = (node.function,
tuple(set(node.context[self.key]['calls'])))
finding_vars = read_then_written
if finding_key not in self.result:
self.result[finding_key] = []
self.result[finding_key] = list(set(self.result[finding_key] + finding_vars))
sons = node.sons
if contains_call and node.type in [NodeType.IF, NodeType.IFLOOP]:
if self._filter_if(node):
son = sons[0]
self._explore(son, visited, node)
sons = sons[1:]
else:
son = sons[1]
self._explore(son, visited, node)
sons = [sons[0]]
for son in sons:
self._explore(son, visited)
def detect_reentrancy(self, contract):
"""
"""
for function in contract.functions:
if function.is_implemented:
self._explore(function.entry_point, [])
def detect(self):
"""
"""
self.result = {}
# if a node was already visited by another path
# we will only explore it if the traversal brings
# new variables written
# This speedup the exploration through a light fixpoint
# Its particular useful on 'complex' functions with several loops and conditions
self.visited_all_paths = {}
for c in self.contracts:
self.detect_reentrancy(c)
results = []
result_sorted = sorted(list(self.result.items()), key=lambda x:x[0][0].name)
for (func, calls), varsWritten in result_sorted:
calls = list(set(calls))
info = 'Reentrancy in {}.{} ({}):\n'
info = info.format(func.contract.name, func.name, func.source_mapping_str)
info += '\tExternal calls:\n'
for call_info in calls:
info += '\t- {} ({})\n'.format(call_info.expression, call_info.source_mapping_str)
info += '\tState variables written after the call(s):\n'
for (v, node) in varsWritten:
info += '\t- {} ({})\n'.format(v, node.source_mapping_str)
self.log(info)
sending_eth_json = []
json = self.generate_json_result(info)
self.add_function_to_json(func, json)
json['elements'] += [{'type': 'external_calls',
'expression': str(call_info.expression),
'source_mapping': call_info.source_mapping}
for call_info in calls]
json['elements'] += sending_eth_json
json['elements'] += [{'type':'variables_written',
'name': v.name,
'expression': str(node.expression),
'source_mapping': node.source_mapping}
for (v, node) in varsWritten]
results.append(json)
return results

@ -0,0 +1 @@
[{"check": "reentrancy-eth", "impact": "High", "confidence": "Medium", "description": "Reentrancy in Reentrancy.withdrawBalance (tests/reentrancy-0.5.1.sol#14-22):\n\tExternal calls:\n\t- (ret,mem) = msg.sender.call.value(userBalance[msg.sender])() (tests/reentrancy-0.5.1.sol#17)\n\tState variables written after the call(s):\n\t- userBalance (tests/reentrancy-0.5.1.sol#21)\n", "elements": [{"type": "function", "name": "withdrawBalance", "source_mapping": {"start": 298, "length": 357, "filename": "tests/reentrancy-0.5.1.sol", "lines": [14, 15, 16, 17, 18, 19, 20, 21, 22]}, "contract": {"type": "contract", "name": "Reentrancy", "source_mapping": {"start": 25, "length": 1807, "filename": "tests/reentrancy-0.5.1.sol", "lines": [3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54]}}}, {"type": "external_calls", "expression": "(ret,mem) = msg.sender.call.value(userBalance[msg.sender])()", "source_mapping": {"start": 477, "length": 81, "filename": "tests/reentrancy-0.5.1.sol", "lines": [17]}}, {"type": "variables_written", "name": "userBalance", "expression": "userBalance[msg.sender] = 0", "source_mapping": {"start": 621, "length": 27, "filename": "tests/reentrancy-0.5.1.sol", "lines": [21]}}]}, {"check": "reentrancy-eth", "impact": "High", "confidence": "Medium", "description": "Reentrancy in Reentrancy.withdrawBalance_fixed_3 (tests/reentrancy-0.5.1.sol#44-53):\n\tExternal calls:\n\t- (ret,mem) = msg.sender.call.value(amount)() (tests/reentrancy-0.5.1.sol#49)\n\tState variables written after the call(s):\n\t- userBalance (tests/reentrancy-0.5.1.sol#51)\n", "elements": [{"type": "function", "name": "withdrawBalance_fixed_3", "source_mapping": {"start": 1434, "length": 393, "filename": "tests/reentrancy-0.5.1.sol", "lines": [44, 45, 46, 47, 48, 49, 50, 51, 52, 53]}, "contract": {"type": "contract", "name": "Reentrancy", "source_mapping": {"start": 25, "length": 1807, "filename": "tests/reentrancy-0.5.1.sol", "lines": [3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54]}}}, {"type": "external_calls", "expression": "(ret,mem) = msg.sender.call.value(amount)()", "source_mapping": {"start": 1679, "length": 64, "filename": "tests/reentrancy-0.5.1.sol", "lines": [49]}}, {"type": "variables_written", "name": "userBalance", "expression": "userBalance[msg.sender] = amount", "source_mapping": {"start": 1778, "length": 32, "filename": "tests/reentrancy-0.5.1.sol", "lines": [51]}}]}]

@ -1 +0,0 @@
[{"check": "reentrancy", "impact": "High", "confidence": "Medium", "description": "Reentrancy in Reentrancy.withdrawBalance (tests/reentrancy-0.5.1.sol#14-22):\n\tExternal calls:\n\t- (ret,mem) = msg.sender.call.value(userBalance[msg.sender])() (tests/reentrancy-0.5.1.sol#17)\n\tState variables written after the call(s):\n\t- userBalance (tests/reentrancy-0.5.1.sol#21)\n", "elements": [{"type": "function", "name": "withdrawBalance", "source_mapping": {"start": 298, "length": 357, "filename": "tests/reentrancy-0.5.1.sol", "lines": [14, 15, 16, 17, 18, 19, 20, 21, 22]}, "contract": {"type": "contract", "name": "Reentrancy", "source_mapping": {"start": 25, "length": 1807, "filename": "tests/reentrancy-0.5.1.sol", "lines": [3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54]}}}, {"type": "external_calls", "expression": "(ret,mem) = msg.sender.call.value(userBalance[msg.sender])()", "source_mapping": {"start": 477, "length": 81, "filename": "tests/reentrancy-0.5.1.sol", "lines": [17]}}, {"type": "variables_written", "name": "userBalance", "expression": "userBalance[msg.sender] = 0", "source_mapping": {"start": 621, "length": 27, "filename": "tests/reentrancy-0.5.1.sol", "lines": [21]}}]}, {"check": "reentrancy", "impact": "High", "confidence": "Medium", "description": "Reentrancy in Reentrancy.withdrawBalance_fixed_3 (tests/reentrancy-0.5.1.sol#44-53):\n\tExternal calls:\n\t- (ret,mem) = msg.sender.call.value(amount)() (tests/reentrancy-0.5.1.sol#49)\n\tState variables written after the call(s):\n\t- userBalance (tests/reentrancy-0.5.1.sol#51)\n", "elements": [{"type": "function", "name": "withdrawBalance_fixed_3", "source_mapping": {"start": 1434, "length": 393, "filename": "tests/reentrancy-0.5.1.sol", "lines": [44, 45, 46, 47, 48, 49, 50, 51, 52, 53]}, "contract": {"type": "contract", "name": "Reentrancy", "source_mapping": {"start": 25, "length": 1807, "filename": "tests/reentrancy-0.5.1.sol", "lines": [3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54]}}}, {"type": "external_calls", "expression": "(ret,mem) = msg.sender.call.value(amount)()", "source_mapping": {"start": 1679, "length": 64, "filename": "tests/reentrancy-0.5.1.sol", "lines": [49]}}, {"type": "variables_written", "name": "userBalance", "expression": "userBalance[msg.sender] = amount", "source_mapping": {"start": 1778, "length": 32, "filename": "tests/reentrancy-0.5.1.sol", "lines": [51]}}]}]

@ -0,0 +1 @@
[{"check": "reentrancy-eth", "impact": "High", "confidence": "Medium", "description": "Reentrancy in Reentrancy.withdrawBalance (tests/reentrancy.sol#14-21):\n\tExternal calls:\n\t- ! (msg.sender.call.value(userBalance[msg.sender])()) (tests/reentrancy.sol#17-19)\n\tState variables written after the call(s):\n\t- userBalance (tests/reentrancy.sol#20)\n", "elements": [{"type": "function", "name": "withdrawBalance", "source_mapping": {"start": 299, "length": 314, "filename": "tests/reentrancy.sol", "lines": [14, 15, 16, 17, 18, 19, 20, 21]}, "contract": {"type": "contract", "name": "Reentrancy", "source_mapping": {"start": 26, "length": 2334, "filename": "tests/reentrancy.sol", "lines": [3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72]}}}, {"type": "external_calls", "expression": "! (msg.sender.call.value(userBalance[msg.sender])())", "source_mapping": {"start": 478, "length": 92, "filename": "tests/reentrancy.sol", "lines": [17, 18, 19]}}, {"type": "variables_written", "name": "userBalance", "expression": "userBalance[msg.sender] = 0", "source_mapping": {"start": 579, "length": 27, "filename": "tests/reentrancy.sol", "lines": [20]}}]}, {"check": "reentrancy-eth", "impact": "High", "confidence": "Medium", "description": "Reentrancy in Reentrancy.withdrawBalance_nested (tests/reentrancy.sol#64-70):\n\tExternal calls:\n\t- msg.sender.call.value(amount / 2)() (tests/reentrancy.sol#67)\n\tState variables written after the call(s):\n\t- userBalance (tests/reentrancy.sol#68)\n", "elements": [{"type": "function", "name": "withdrawBalance_nested", "source_mapping": {"start": 2108, "length": 246, "filename": "tests/reentrancy.sol", "lines": [64, 65, 66, 67, 68, 69, 70]}, "contract": {"type": "contract", "name": "Reentrancy", "source_mapping": {"start": 26, "length": 2334, "filename": "tests/reentrancy.sol", "lines": [3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51, 52, 53, 54, 55, 56, 57, 58, 59, 60, 61, 62, 63, 64, 65, 66, 67, 68, 69, 70, 71, 72]}}}, {"type": "external_calls", "expression": "msg.sender.call.value(amount / 2)()", "source_mapping": {"start": 2263, "length": 33, "filename": "tests/reentrancy.sol", "lines": [67]}}, {"type": "variables_written", "name": "userBalance", "expression": "userBalance[msg.sender] = 0", "source_mapping": {"start": 2310, "length": 27, "filename": "tests/reentrancy.sol", "lines": [68]}}]}]

@ -1 +0,0 @@
[{"check": "reentrancy", "impact": "High", "confidence": "Medium", "description": "Reentrancy in Reentrancy.withdrawBalance (tests/reentrancy.sol#14-21):\n\tExternal calls:\n\t- ! (msg.sender.call.value(userBalance[msg.sender])()) (tests/reentrancy.sol#17-19)\n\tState variables written after the call(s):\n\t- userBalance (tests/reentrancy.sol#20)\n", "elements": [{"type": "function", "name": "withdrawBalance", "source_mapping": {"start": 299, "length": 314, "filename": "tests/reentrancy.sol", "lines": [14, 15, 16, 17, 18, 19, 20, 21]}, "contract": {"type": "contract", "name": "Reentrancy", "source_mapping": {"start": 26, "length": 1678, "filename": "tests/reentrancy.sol", "lines": [3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23, 24, 25, 26, 27, 28, 29, 30, 31, 32, 33, 34, 35, 36, 37, 38, 39, 40, 41, 42, 43, 44, 45, 46, 47, 48, 49, 50, 51]}}}, {"type": "external_calls", "expression": "! (msg.sender.call.value(userBalance[msg.sender])())", "source_mapping": {"start": 478, "length": 92, "filename": "tests/reentrancy.sol", "lines": [17, 18, 19]}}, {"type": "variables_written", "name": "userBalance", "expression": "userBalance[msg.sender] = 0", "source_mapping": {"start": 579, "length": 27, "filename": "tests/reentrancy.sol", "lines": [20]}}]}]

@ -48,4 +48,25 @@ contract Reentrancy {
userBalance[msg.sender] = amount; userBalance[msg.sender] = amount;
} }
} }
function withdrawBalance_fixed_4() public{
// The state can be changed
// But it is fine, as it can only occur if the transaction fails
uint amount = userBalance[msg.sender];
userBalance[msg.sender] = 0;
if( (msg.sender.call.value(amount)() ) ){
return;
}
else{
userBalance[msg.sender] = amount;
}
}
function withdrawBalance_nested() public{
uint amount = userBalance[msg.sender];
if( ! (msg.sender.call.value(amount/2)() ) ){
msg.sender.call.value(amount/2)();
userBalance[msg.sender] = 0;
}
}
} }

Loading…
Cancel
Save