mirror of https://github.com/crytic/slither
- Clean Reentrancy based class - Add ReentrancyEvent to detect reentrancies leading to out of order Events - Add ReentrancyNoGas to detect reentrancies based on send/transfer Fix #236pull/375/head
parent
00faa0cbad
commit
1bb72898f6
@ -0,0 +1,112 @@ |
||||
"""" |
||||
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.detectors.abstract_detector import DetectorClassification |
||||
|
||||
|
||||
from .reentrancy import Reentrancy |
||||
class ReentrancyEvent(Reentrancy): |
||||
ARGUMENT = 'reentrancy-events' |
||||
HELP = 'Reentrancy vulnerabilities leading to out-of-order Events' |
||||
IMPACT = DetectorClassification.LOW |
||||
CONFIDENCE = DetectorClassification.MEDIUM |
||||
|
||||
WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-4' |
||||
|
||||
WIKI_TITLE = 'Reentrancy vulnerabilities' |
||||
WIKI_DESCRIPTION = ''' |
||||
Detection of the [re-entrancy bug](https://github.com/trailofbits/not-so-smart-contracts/tree/master/reentrancy). |
||||
Only report reentrancies leading to out-of-order Events''' |
||||
WIKI_EXPLOIT_SCENARIO = ''' |
||||
```solidity |
||||
function bug(Called d){ |
||||
counter += 1; |
||||
d.f(); |
||||
emit Counter(counter); |
||||
} |
||||
``` |
||||
|
||||
If `d.()` reenters, the `Counter` events will be showed in an incorrect order, which might lead to issues for third-parties.''' |
||||
|
||||
|
||||
WIKI_RECOMMENDATION = 'Apply the [check-effects-interactions pattern](http://solidity.readthedocs.io/en/v0.4.21/security-considerations.html#re-entrancy).' |
||||
|
||||
STANDARD_JSON = False |
||||
|
||||
def find_reentrancies(self): |
||||
result = {} |
||||
for contract in self.contracts: |
||||
for f in contract.functions_and_modifiers_declared: |
||||
for node in f.nodes: |
||||
# dead code |
||||
if not self.KEY in node.context: |
||||
continue |
||||
if node.context[self.KEY]['calls']: |
||||
if not any(n != node for n in node.context[self.KEY]['calls']): |
||||
continue |
||||
|
||||
# calls are ordered |
||||
finding_key = (node.function, |
||||
tuple(sorted(list(node.context[self.KEY]['calls']), key=lambda x: x.node_id)), |
||||
tuple(sorted(list(node.context[self.KEY]['send_eth']), key=lambda x: x.node_id))) |
||||
finding_vars = [(v, node) for v in node.context[self.KEY]['events']] |
||||
if finding_vars: |
||||
if finding_key not in result: |
||||
result[finding_key] = set() |
||||
result[finding_key] = set(result[finding_key] | set(finding_vars)) |
||||
return result |
||||
|
||||
def _detect(self): |
||||
""" |
||||
""" |
||||
super()._detect() |
||||
|
||||
reentrancies = self.find_reentrancies() |
||||
|
||||
results = [] |
||||
|
||||
result_sorted = sorted(list(reentrancies.items()), key=lambda x:x[0][0].name) |
||||
for (func, calls, send_eth), events in result_sorted: |
||||
calls = sorted(list(set(calls)), key=lambda x: x.node_id) |
||||
send_eth = sorted(list(set(send_eth)), key=lambda x: x.node_id) |
||||
|
||||
info = ['Reentrancy in ', func, ':\n'] |
||||
info += ['\tExternal calls:\n'] |
||||
for call_info in calls: |
||||
info += ['\t- ' , call_info, '\n'] |
||||
if calls != send_eth and send_eth: |
||||
info += ['\tExternal calls sending eth:\n'] |
||||
for call_info in send_eth: |
||||
info += ['\t- ', call_info, '\n'] |
||||
info += ['\tEvent emitted after the call(s):\n'] |
||||
for (e, node) in sorted(events, key=lambda x: (x[0], x[1].node_id)): |
||||
info += ['\t- ', e, ' in ', node, '\n'] |
||||
|
||||
# Create our JSON result |
||||
res = self.generate_result(info) |
||||
|
||||
# Add the function with the re-entrancy first |
||||
res.add(func) |
||||
|
||||
# Add all underlying calls in the function which are potentially problematic. |
||||
for call_info in calls: |
||||
res.add(call_info, { |
||||
"underlying_type": "external_calls" |
||||
}) |
||||
|
||||
# |
||||
|
||||
# If the calls are not the same ones that send eth, add the eth sending nodes. |
||||
if calls != send_eth: |
||||
for call_info in send_eth: |
||||
res.add(call_info, { |
||||
"underlying_type": "external_calls_sending_eth" |
||||
}) |
||||
|
||||
# Append our result |
||||
results.append(res) |
||||
|
||||
return results |
@ -0,0 +1,138 @@ |
||||
"""" |
||||
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.variables.variable import Variable |
||||
from slither.detectors.abstract_detector import DetectorClassification |
||||
from slither.slithir.operations import Send, Transfer |
||||
from .reentrancy import Reentrancy |
||||
|
||||
|
||||
class ReentrancyNoGas(Reentrancy): |
||||
KEY = 'REENTRANCY_NO_GAS' |
||||
|
||||
ARGUMENT = 'reentrancy-unlimited-gas' |
||||
HELP = 'Reentrancy vulnerabilities through send and transfer' |
||||
IMPACT = DetectorClassification.INFORMATIONAL |
||||
CONFIDENCE = DetectorClassification.MEDIUM |
||||
|
||||
WIKI = 'https://github.com/crytic/slither/wiki/Detector-Documentation#reentrancy-vulnerabilities-3' |
||||
|
||||
WIKI_TITLE = 'Reentrancy vulnerabilities' |
||||
WIKI_DESCRIPTION = ''' |
||||
Detection of the [re-entrancy bug](https://github.com/trailofbits/not-so-smart-contracts/tree/master/reentrancy). |
||||
Only report reentrancy that are based on `transfer` or `send`.''' |
||||
WIKI_EXPLOIT_SCENARIO = ''' |
||||
```solidity |
||||
function callme(){ |
||||
msg.sender.transfer(balances[msg.sender]): |
||||
balances[msg.sender] = 0; |
||||
} |
||||
``` |
||||
|
||||
`send` and `transfer` does not protect from reentrancies in case of gas-price change.''' |
||||
|
||||
WIKI_RECOMMENDATION = 'Apply the [check-effects-interactions pattern](http://solidity.readthedocs.io/en/v0.4.21/security-considerations.html#re-entrancy).' |
||||
|
||||
def _can_callback(self, irs): |
||||
""" |
||||
Same as Reentrancy, but also consider Send and Transfer |
||||
|
||||
""" |
||||
return any((isinstance(ir, (Send, Transfer)) for ir in irs)) |
||||
|
||||
STANDARD_JSON = False |
||||
|
||||
def find_reentrancies(self): |
||||
result = {} |
||||
for contract in self.contracts: |
||||
for f in contract.functions_and_modifiers_declared: |
||||
for node in f.nodes: |
||||
# dead code |
||||
if not self.KEY in node.context: |
||||
continue |
||||
if node.context[self.KEY]['calls']: |
||||
if not any(n != node for n in node.context[self.KEY]['calls']): |
||||
continue |
||||
|
||||
# calls are ordered |
||||
finding_key = (node.function, |
||||
tuple(sorted(list(node.context[self.KEY]['calls']), key=lambda x: x.node_id)), |
||||
tuple(sorted(list(node.context[self.KEY]['send_eth']), key=lambda x: x.node_id))) |
||||
finding_vars = [(v, node) for v in node.context[self.KEY]['written']] |
||||
finding_vars += [(v, node) for v in node.context[self.KEY]['events']] |
||||
if finding_vars: |
||||
if finding_key not in result: |
||||
result[finding_key] = set() |
||||
result[finding_key] = set(result[finding_key] | set(finding_vars)) |
||||
return result |
||||
|
||||
def _detect(self): |
||||
""" |
||||
""" |
||||
|
||||
super()._detect() |
||||
reentrancies = self.find_reentrancies() |
||||
|
||||
results = [] |
||||
|
||||
result_sorted = sorted(list(reentrancies.items()), key=lambda x: x[0][0].name) |
||||
for (func, calls, send_eth), varsWrittenOrEvent in result_sorted: |
||||
calls = sorted(list(set(calls)), key=lambda x: x.node_id) |
||||
send_eth = sorted(list(set(send_eth)), key=lambda x: x.node_id) |
||||
info = ['Reentrancy in ', func, ':\n'] |
||||
|
||||
info += ['\tExternal calls:\n'] |
||||
for call_info in calls: |
||||
info += ['\t- ', call_info, '\n'] |
||||
if calls != send_eth and send_eth: |
||||
info += ['\tExternal calls sending eth:\n'] |
||||
for call_info in send_eth: |
||||
info += ['\t- ', call_info, '\n'] |
||||
|
||||
varsWritten = [(v, node) for (v, node) in varsWrittenOrEvent if isinstance(v, Variable)] |
||||
if varsWritten: |
||||
info += ['\tState variables written after the call(s):\n'] |
||||
for (v, node) in sorted(varsWritten, key=lambda x: (x[0].name, x[1].node_id)): |
||||
info += ['\t- ', v, ' in ', node, '\n'] |
||||
|
||||
events = [(e, node) for (e, node) in varsWrittenOrEvent if isinstance(e, str)] |
||||
if events: |
||||
info += ['\tEvent emitted after the call(s):\n'] |
||||
for (e, node) in sorted(events, key=lambda x: (x[0], x[1].node_id)): |
||||
info += ['\t- ', e, ' in ', node, '\n'] |
||||
|
||||
# Create our JSON result |
||||
res = self.generate_result(info) |
||||
|
||||
# Add the function with the re-entrancy first |
||||
res.add(func) |
||||
|
||||
# Add all underlying calls in the function which are potentially problematic. |
||||
for call_info in calls: |
||||
res.add(call_info, { |
||||
"underlying_type": "external_calls" |
||||
}) |
||||
|
||||
# |
||||
|
||||
# If the calls are not the same ones that send eth, add the eth sending nodes. |
||||
if calls != send_eth: |
||||
for call_info in send_eth: |
||||
res.add(call_info, { |
||||
"underlying_type": "external_calls_sending_eth" |
||||
}) |
||||
|
||||
# Add all variables written via nodes which write them. |
||||
for (v, node) in varsWritten: |
||||
res.add(node, { |
||||
"underlying_type": "variables_written", |
||||
"variable_name": v.name |
||||
}) |
||||
|
||||
# Append our result |
||||
results.append(res) |
||||
|
||||
return results |
@ -0,0 +1,410 @@ |
||||
{ |
||||
"success": true, |
||||
"error": null, |
||||
"results": { |
||||
"detectors": [ |
||||
{ |
||||
"elements": [ |
||||
{ |
||||
"type": "function", |
||||
"name": "withdrawBalance_fixed_2", |
||||
"source_mapping": { |
||||
"start": 951, |
||||
"length": 386, |
||||
"filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_relative": "tests/reentrancy.sol", |
||||
"filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_short": "tests/reentrancy.sol", |
||||
"is_dependency": false, |
||||
"lines": [ |
||||
33, |
||||
34, |
||||
35, |
||||
36, |
||||
37, |
||||
38, |
||||
39, |
||||
40 |
||||
], |
||||
"starting_column": 5, |
||||
"ending_column": 6 |
||||
}, |
||||
"type_specific_fields": { |
||||
"parent": { |
||||
"type": "contract", |
||||
"name": "Reentrancy", |
||||
"source_mapping": { |
||||
"start": 26, |
||||
"length": 2334, |
||||
"filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_relative": "tests/reentrancy.sol", |
||||
"filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_short": "tests/reentrancy.sol", |
||||
"is_dependency": false, |
||||
"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 |
||||
], |
||||
"starting_column": 1, |
||||
"ending_column": 2 |
||||
} |
||||
}, |
||||
"signature": "withdrawBalance_fixed_2()" |
||||
} |
||||
}, |
||||
{ |
||||
"type": "node", |
||||
"name": "msg.sender.transfer(userBalance[msg.sender])", |
||||
"source_mapping": { |
||||
"start": 1249, |
||||
"length": 44, |
||||
"filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_relative": "tests/reentrancy.sol", |
||||
"filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_short": "tests/reentrancy.sol", |
||||
"is_dependency": false, |
||||
"lines": [ |
||||
38 |
||||
], |
||||
"starting_column": 9, |
||||
"ending_column": 53 |
||||
}, |
||||
"type_specific_fields": { |
||||
"parent": { |
||||
"type": "function", |
||||
"name": "withdrawBalance_fixed_2", |
||||
"source_mapping": { |
||||
"start": 951, |
||||
"length": 386, |
||||
"filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_relative": "tests/reentrancy.sol", |
||||
"filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_short": "tests/reentrancy.sol", |
||||
"is_dependency": false, |
||||
"lines": [ |
||||
33, |
||||
34, |
||||
35, |
||||
36, |
||||
37, |
||||
38, |
||||
39, |
||||
40 |
||||
], |
||||
"starting_column": 5, |
||||
"ending_column": 6 |
||||
}, |
||||
"type_specific_fields": { |
||||
"parent": { |
||||
"type": "contract", |
||||
"name": "Reentrancy", |
||||
"source_mapping": { |
||||
"start": 26, |
||||
"length": 2334, |
||||
"filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_relative": "tests/reentrancy.sol", |
||||
"filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_short": "tests/reentrancy.sol", |
||||
"is_dependency": false, |
||||
"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 |
||||
], |
||||
"starting_column": 1, |
||||
"ending_column": 2 |
||||
} |
||||
}, |
||||
"signature": "withdrawBalance_fixed_2()" |
||||
} |
||||
} |
||||
}, |
||||
"additional_fields": { |
||||
"underlying_type": "external_calls" |
||||
} |
||||
}, |
||||
{ |
||||
"type": "node", |
||||
"name": "userBalance[msg.sender] = 0", |
||||
"source_mapping": { |
||||
"start": 1303, |
||||
"length": 27, |
||||
"filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_relative": "tests/reentrancy.sol", |
||||
"filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_short": "tests/reentrancy.sol", |
||||
"is_dependency": false, |
||||
"lines": [ |
||||
39 |
||||
], |
||||
"starting_column": 9, |
||||
"ending_column": 36 |
||||
}, |
||||
"type_specific_fields": { |
||||
"parent": { |
||||
"type": "function", |
||||
"name": "withdrawBalance_fixed_2", |
||||
"source_mapping": { |
||||
"start": 951, |
||||
"length": 386, |
||||
"filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_relative": "tests/reentrancy.sol", |
||||
"filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_short": "tests/reentrancy.sol", |
||||
"is_dependency": false, |
||||
"lines": [ |
||||
33, |
||||
34, |
||||
35, |
||||
36, |
||||
37, |
||||
38, |
||||
39, |
||||
40 |
||||
], |
||||
"starting_column": 5, |
||||
"ending_column": 6 |
||||
}, |
||||
"type_specific_fields": { |
||||
"parent": { |
||||
"type": "contract", |
||||
"name": "Reentrancy", |
||||
"source_mapping": { |
||||
"start": 26, |
||||
"length": 2334, |
||||
"filename_used": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_relative": "tests/reentrancy.sol", |
||||
"filename_absolute": "/home/travis/build/crytic/slither/tests/reentrancy.sol", |
||||
"filename_short": "tests/reentrancy.sol", |
||||
"is_dependency": false, |
||||
"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 |
||||
], |
||||
"starting_column": 1, |
||||
"ending_column": 2 |
||||
} |
||||
}, |
||||
"signature": "withdrawBalance_fixed_2()" |
||||
} |
||||
} |
||||
}, |
||||
"additional_fields": { |
||||
"underlying_type": "variables_written", |
||||
"variable_name": "userBalance" |
||||
} |
||||
} |
||||
], |
||||
"description": "Reentrancy in Reentrancy.withdrawBalance_fixed_2() (tests/reentrancy.sol#33-40):\n\tExternal calls:\n\t- msg.sender.transfer(userBalance[msg.sender]) (tests/reentrancy.sol#38)\n\tState variables written after the call(s):\n\t- Reentrancy.userBalance (tests/reentrancy.sol#4) in userBalance[msg.sender] = 0 (tests/reentrancy.sol#39)\n", |
||||
"markdown": "Reentrancy in [Reentrancy.withdrawBalance_fixed_2()](tests/reentrancy.sol#L33-L40):\n\tExternal calls:\n\t- [msg.sender.transfer(userBalance[msg.sender])](tests/reentrancy.sol#L38)\n\tState variables written after the call(s):\n\t- [Reentrancy.userBalance](tests/reentrancy.sol#L4) in [userBalance[msg.sender] = 0](tests/reentrancy.sol#L39)\n", |
||||
"id": "ff12b17dedf35fcd1f5b1286653a53430009f3689c46c06c1aba1f2d4b17d713", |
||||
"check": "reentrancy-unlimited-gas", |
||||
"impact": "Informational", |
||||
"confidence": "Medium" |
||||
} |
||||
] |
||||
} |
||||
} |
Loading…
Reference in new issue