TheDude
/
slither
mirror of https://github.com/crytic/slither
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

add unused-return-transfers detector + exclude transfer/transferFrom from unused-return detector

Browse Source
pull/822/head
Alexander Remie 4 years ago
parent cf07c59516
commit c1aca4c956
8 changed files with 941 additions and 1 deletions
Show all changes Ignore whitespace when comparing lines Ignore changes in amount of whitespace Ignore changes in whitespace at EOL
Show Stats Download Patch File Download Diff File
  1. 1
      slither/detectors/all_detectors.py
  2. 8
      slither/detectors/operations/unused_return_values.py
  3. 96
      slither/detectors/operations/unused_return_values_transfers.py
  4. 63
      tests/detectors/unused-return-transfers/unused_return_transfers.sol
  5. 436
      tests/detectors/unused-return-transfers/unused_return_transfers.sol.0.7.6.UnusedReturnValuesTransfers.json
  6. 78
      tests/detectors/unused-return/unused_return-sol7.sol
  7. 250
      tests/detectors/unused-return/unused_return-sol7.sol.0.7.6.UnusedReturnValues.json
  8. 10
      tests/test_detectors.py

1
slither/detectors/all_detectors.py
Unescape View File

@ -21,6 +21,7 @@ from .statements.tx_origin import TxOrigin
from .statements.assembly import Assembly
from .operations.low_level_calls import LowLevelCalls
from .operations.unused_return_values import UnusedReturnValues
from .operations.unused_return_values_transfers import UnusedReturnValuesTransfers
from .naming_convention.naming_convention import NamingConvention
from .functions.external_function import ExternalFunction
from .statements.controlled_delegatecall import ControlledDelegateCall

8
slither/detectors/operations/unused_return_values.py
Unescape View File

@ -5,6 +5,7 @@ Module detecting unused return values from external calls
from slither.core.variables.state_variable import StateVariable
from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
from slither.slithir.operations import HighLevelCall
from slither.core.declarations import Function
class UnusedReturnValues(AbstractDetector):
@ -39,7 +40,12 @@ contract MyConc{
_txt_description = "external calls"
def _is_instance(self, ir): # pylint: disable=no-self-use
return isinstance(ir, HighLevelCall)
return (
isinstance(ir, HighLevelCall)
and isinstance(ir.function, Function)
and ir.function.solidity_signature
not in ["transfer(address,uint256)", "transferFrom(address,address,uint256)"]
)
def detect_unused_return_values(self, f): # pylint: disable=no-self-use
"""

96
slither/detectors/operations/unused_return_values_transfers.py
Unescape View File

@ -0,0 +1,96 @@
"""
Module detecting unused transfer/transferFrom return values from external calls
"""
from slither.core.variables.state_variable import StateVariable
from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
from slither.slithir.operations import HighLevelCall
from slither.core.declarations import Function
class UnusedReturnValuesTransfers(AbstractDetector):
"""
If the return value of a transfer/transferFrom function is never used, it's likely to be bug
"""
ARGUMENT = "unused-return-transfers"
HELP = "Unused transfer/transferFrom return values"
IMPACT = DetectorClassification.HIGH
CONFIDENCE = DetectorClassification.MEDIUM
WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#unused-return-transfers"
WIKI_TITLE = "Unused return"
WIKI_DESCRIPTION = (
"The return value of an external transfer/transferFrom call is not used"
)
WIKI_EXPLOIT_SCENARIO = """
```solidity
contract Token {
function transfer(address _to, uint256 _value) public returns (bool success);
function transferFrom(address _from, address _to, uint256 _value) public returns (bool success);
}
contract MyConc{
function my_func1(Token tok, address to) public{
tok.transfer(to, 1 ether);
}
function my_func2(Token tok, address to) public{
tok.transferFrom(address(this), to, 1 ether);
}
}
```
`MyConc` calls `transfer` or `transferFrom` on a token contract but does not check the return value. As a result, transfers that do not revert on failure will appear to have succeeded."""
WIKI_RECOMMENDATION = "Ensure that the returned boolean of all transfer and transferFrom function calls is checked."
_txt_description = "external transfer calls"
def _is_instance(self, ir): # pylint: disable=no-self-use
return (
isinstance(ir, HighLevelCall)
and isinstance(ir.function, Function)
and ir.function.solidity_signature
in ["transfer(address,uint256)", "transferFrom(address,address,uint256)"]
)
def detect_unused_return_values(self, f): # pylint: disable=no-self-use
"""
Return the nodes where the return value of a call is unused
Args:
f (Function)
Returns:
list(Node)
"""
values_returned = []
nodes_origin = {}
for n in f.nodes:
for ir in n.irs:
if self._is_instance(ir):
# if a return value is stored in a state variable, it's ok
if ir.lvalue and not isinstance(ir.lvalue, StateVariable):
values_returned.append(ir.lvalue)
nodes_origin[ir.lvalue] = ir
for read in ir.read:
if read in values_returned:
values_returned.remove(read)
return [nodes_origin[value].node for value in values_returned]
def _detect(self):
"""Detect external transfer/transferFrom calls whose return value is not checked"""
results = []
for c in self.slither.contracts:
for f in c.functions + c.modifiers:
if f.contract_declarer != c:
continue
unused_return = self.detect_unused_return_values(f)
if unused_return:
for node in unused_return:
info = [f, " ignores return value of ", node, "\n"]
res = self.generate_result(info)
results.append(res)
return results

63
tests/detectors/unused-return-transfers/unused_return_transfers.sol
Unescape View File

@ -0,0 +1,63 @@
contract Token {
function transfer(address _to, uint256 _value) public returns (bool success) {
return true;
}
function transferFrom(address _from, address _to, uint256 _value) public returns (bool success) {
return true;
}
function other() public returns (bool) {
return true;
}
}
contract C {
Token t;
constructor() public {
t = new Token();
}
// calling the transfer function
function bad0() public{
t.transfer(address(0), 1 ether);
}
function good0() public{
bool a = t.transfer(address(0), 1 ether);
}
function good1() public{
require(t.transfer(address(0), 1 ether), "failed");
}
function good2() public{
assert(t.transfer(address(0), 1 ether));
}
function good3() public returns (bool) {
return t.transfer(address(0), 1 ether);
}
function good4() public returns (bool ret) {
ret = t.transfer(address(0), 1 ether);
}
// calling the transferFrom function
function bad1() public {
t.transferFrom(address(this), address(0), 1 ether);
}
function good5() public{
bool a = t.transferFrom(address(this), address(0), 1 ether);
}
function good6() public{
require(t.transferFrom(address(this), address(0), 1 ether), "failed");
}
function good7() public{
assert(t.transferFrom(address(this), address(0), 1 ether));
}
function good8() public returns (bool) {
return t.transferFrom(address(this), address(0), 1 ether);
}
function good9() public returns (bool ret) {
ret = t.transferFrom(address(this), address(0), 1 ether);
}
// calling the other function
function good10() public {
t.other();
}
}

436
tests/detectors/unused-return-transfers/unused_return_transfers.sol.0.7.6.UnusedReturnValuesTransfers.json
Unescape View File

@ -0,0 +1,436 @@
[
[
{
"elements": [
{
"type": "function",
"name": "bad0",
"source_mapping": {
"start": 461,
"length": 70,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"is_dependency": false,
"lines": [
20,
21,
22
],
"starting_column": 5,
"ending_column": 6
},
"type_specific_fields": {
"parent": {
"type": "contract",
"name": "C",
"source_mapping": {
"start": 330,
"length": 1456,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"is_dependency": false,
"lines": [
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
],
"starting_column": 1,
"ending_column": 0
}
},
"signature": "bad0()"
}
},
{
"type": "node",
"name": "t.transfer(address(0),1000000000000000000)",
"source_mapping": {
"start": 493,
"length": 31,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"is_dependency": false,
"lines": [
21
],
"starting_column": 9,
"ending_column": 40
},
"type_specific_fields": {
"parent": {
"type": "function",
"name": "bad0",
"source_mapping": {
"start": 461,
"length": 70,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"is_dependency": false,
"lines": [
20,
21,
22
],
"starting_column": 5,
"ending_column": 6
},
"type_specific_fields": {
"parent": {
"type": "contract",
"name": "C",
"source_mapping": {
"start": 330,
"length": 1456,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"is_dependency": false,
"lines": [
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
],
"starting_column": 1,
"ending_column": 0
}
},
"signature": "bad0()"
}
}
}
}
],
"description": "C.bad0() (tests/detectors/unused-return-transfers/unused_return_transfers.sol#20-22) ignores return value of t.transfer(address(0),1000000000000000000) (tests/detectors/unused-return-transfers/unused_return_transfers.sol#21)\n",
"markdown": "[C.bad0()](tests/detectors/unused-return-transfers/unused_return_transfers.sol#L20-L22) ignores return value of [t.transfer(address(0),1000000000000000000)](tests/detectors/unused-return-transfers/unused_return_transfers.sol#L21)\n",
"id": "273f513954167c12160962f06a02a10088834be02327cd8d27edd32f28e6b930",
"check": "unused-return-transfers",
"impact": "High",
"confidence": "Medium"
},
{
"elements": [
{
"type": "function",
"name": "bad1",
"source_mapping": {
"start": 1043,
"length": 90,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"is_dependency": false,
"lines": [
40,
41,
42
],
"starting_column": 5,
"ending_column": 6
},
"type_specific_fields": {
"parent": {
"type": "contract",
"name": "C",
"source_mapping": {
"start": 330,
"length": 1456,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"is_dependency": false,
"lines": [
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
],
"starting_column": 1,
"ending_column": 0
}
},
"signature": "bad1()"
}
},
{
"type": "node",
"name": "t.transferFrom(address(this),address(0),1000000000000000000)",
"source_mapping": {
"start": 1076,
"length": 50,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"is_dependency": false,
"lines": [
41
],
"starting_column": 9,
"ending_column": 59
},
"type_specific_fields": {
"parent": {
"type": "function",
"name": "bad1",
"source_mapping": {
"start": 1043,
"length": 90,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"is_dependency": false,
"lines": [
40,
41,
42
],
"starting_column": 5,
"ending_column": 6
},
"type_specific_fields": {
"parent": {
"type": "contract",
"name": "C",
"source_mapping": {
"start": 330,
"length": 1456,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"is_dependency": false,
"lines": [
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
],
"starting_column": 1,
"ending_column": 0
}
},
"signature": "bad1()"
}
}
}
}
],
"description": "C.bad1() (tests/detectors/unused-return-transfers/unused_return_transfers.sol#40-42) ignores return value of t.transferFrom(address(this),address(0),1000000000000000000) (tests/detectors/unused-return-transfers/unused_return_transfers.sol#41)\n",
"markdown": "[C.bad1()](tests/detectors/unused-return-transfers/unused_return_transfers.sol#L40-L42) ignores return value of [t.transferFrom(address(this),address(0),1000000000000000000)](tests/detectors/unused-return-transfers/unused_return_transfers.sol#L41)\n",
"id": "41be6c49206e7b40a8d6ac10260a60bff5f876839d951ac20b3a38ac5185c0ae",
"check": "unused-return-transfers",
"impact": "High",
"confidence": "Medium"
}
]
]

78
tests/detectors/unused-return/unused_return-sol7.sol
Unescape View File

@ -0,0 +1,78 @@
contract Token {
function transfer(address _to, uint256 _value) public returns (bool success) {
return true;
}
function transferFrom(address _from, address _to, uint256 _value) public returns (bool success) {
return true;
}
function other() public returns (bool) {
return true;
}
}
contract C {
Token t;
constructor() public {
t = new Token();
}
// calling the transfer function
function good0() public{
t.transfer(address(0), 1 ether);
}
function good1() public{
bool a = t.transfer(address(0), 1 ether);
}
function good2() public{
require(t.transfer(address(0), 1 ether), "failed");
}
function good3() public{
assert(t.transfer(address(0), 1 ether));
}
function good4() public returns (bool) {
return t.transfer(address(0), 1 ether);
}
function good5() public returns (bool ret) {
ret = t.transfer(address(0), 1 ether);
}
// calling the transferFrom function
function good6() public {
t.transferFrom(address(this), address(0), 1 ether);
}
function good7() public{
bool a = t.transferFrom(address(this), address(0), 1 ether);
}
function good8() public{
require(t.transferFrom(address(this), address(0), 1 ether), "failed");
}
function good9() public{
assert(t.transferFrom(address(this), address(0), 1 ether));
}
function good10() public returns (bool) {
return t.transferFrom(address(this), address(0), 1 ether);
}
function good11() public returns (bool ret) {
ret = t.transferFrom(address(this), address(0), 1 ether);
}
// calling the other function
function bad0() public {
t.other();
}
function good12() public{
bool a = t.other();
}
function good13() public{
require(t.other(), "failed");
}
function good14() public{
assert(t.other());
}
function good15() public returns (bool) {
return t.other();
}
function good16() public returns (bool ret) {
ret = t.other();
}
}

250
tests/detectors/unused-return/unused_return-sol7.sol.0.7.6.UnusedReturnValues.json
Unescape View File

@ -0,0 +1,250 @@
[
[
{
"elements": [
{
"type": "function",
"name": "bad0",
"source_mapping": {
"start": 1737,
"length": 49,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return/unused_return-sol7.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return/unused_return-sol7.sol",
"is_dependency": false,
"lines": [
60,
61,
62
],
"starting_column": 5,
"ending_column": 6
},
"type_specific_fields": {
"parent": {
"type": "contract",
"name": "C",
"source_mapping": {
"start": 330,
"length": 1818,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return/unused_return-sol7.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return/unused_return-sol7.sol",
"is_dependency": false,
"lines": [
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,
73,
74,
75,
76,
77,
78,
79
],
"starting_column": 1,
"ending_column": 0
}
},
"signature": "bad0()"
}
},
{
"type": "node",
"name": "t.other()",
"source_mapping": {
"start": 1770,
"length": 9,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return/unused_return-sol7.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return/unused_return-sol7.sol",
"is_dependency": false,
"lines": [
61
],
"starting_column": 9,
"ending_column": 18
},
"type_specific_fields": {
"parent": {
"type": "function",
"name": "bad0",
"source_mapping": {
"start": 1737,
"length": 49,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return/unused_return-sol7.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return/unused_return-sol7.sol",
"is_dependency": false,
"lines": [
60,
61,
62
],
"starting_column": 5,
"ending_column": 6
},
"type_specific_fields": {
"parent": {
"type": "contract",
"name": "C",
"source_mapping": {
"start": 330,
"length": 1818,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unused-return/unused_return-sol7.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unused-return/unused_return-sol7.sol",
"is_dependency": false,
"lines": [
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,
73,
74,
75,
76,
77,
78,
79
],
"starting_column": 1,
"ending_column": 0
}
},
"signature": "bad0()"
}
}
}
}
],
"description": "C.bad0() (tests/detectors/unused-return/unused_return-sol7.sol#60-62) ignores return value by t.other() (tests/detectors/unused-return/unused_return-sol7.sol#61)\n",
"markdown": "[C.bad0()](tests/detectors/unused-return/unused_return-sol7.sol#L60-L62) ignores return value by [t.other()](tests/detectors/unused-return/unused_return-sol7.sol#L61)\n",
"id": "f9924f5b012de6c1e91b68996bc7944585b0daab740b86c89cd157b0d22d0092",
"check": "unused-return",
"impact": "Medium",
"confidence": "Medium"
}
]
]

10
tests/test_detectors.py
Unescape View File

@ -251,6 +251,16 @@ ALL_TESTS = [
Test(
all_detectors.UnusedReturnValues, "tests/detectors/unused-return/unused_return.sol", "0.5.1"
),
Test(
all_detectors.UnusedReturnValues,
"tests/detectors/unused-return/unused_return-sol7.sol",
"0.7.6",
),
Test(
all_detectors.UnusedReturnValuesTransfers,
"tests/detectors/unused-return-transfers/unused_return_transfers.sol",
"0.7.6",
),
Test(
all_detectors.ShadowingAbstractDetection,
"tests/detectors/shadowing-abstract/shadowing_abstract.sol",

Write Preview
Loading…
Cancel
Save