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

Merge pull request #822 from crytic/add-unused-return-transfers-detector

Browse Source 
add unused-return-transfers detector + update unused-return
pull/828/head
Feist Josselin 4 years ago committed by GitHub
parent cf07c59516 c2f7773aa7
commit 3b7710e434
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
10 changed files with 898 additions and 7 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. 2
      slither/detectors/operations/unchecked_low_level_return_values.py
  3. 2
      slither/detectors/operations/unchecked_send_return_value.py
  4. 51
      slither/detectors/operations/unchecked_transfer.py
  5. 12
      slither/detectors/operations/unused_return_values.py
  6. 63
      tests/detectors/unchecked-transfer/unused_return_transfers.sol
  7. 436
      tests/detectors/unchecked-transfer/unused_return_transfers.sol.0.7.6.UncheckedTransfer.json
  8. 78
      tests/detectors/unused-return/unused_return-sol7.sol
  9. 250
      tests/detectors/unused-return/unused_return-sol7.sol.0.7.6.UnusedReturnValues.json
  10. 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.unchecked_transfer import UncheckedTransfer
from .naming_convention.naming_convention import NamingConvention
from .functions.external_function import ExternalFunction
from .statements.controlled_delegatecall import ControlledDelegateCall

2
slither/detectors/operations/unchecked_low_level_return_values.py
Unescape View File

@ -34,7 +34,5 @@ If the low level is used to prevent blocking operations, consider logging failed
WIKI_RECOMMENDATION = "Ensure that the return value of a low-level call is checked or logged."
_txt_description = "low-level calls"
def _is_instance(self, ir): # pylint: disable=no-self-use
return isinstance(ir, LowLevelCall)

2
slither/detectors/operations/unchecked_send_return_value.py
Unescape View File

@ -35,7 +35,5 @@ If `send` is used to prevent blocking operations, consider logging the failed `s
WIKI_RECOMMENDATION = "Ensure that the return value of `send` is checked or logged."
_txt_description = "send calls"
def _is_instance(self, ir): # pylint: disable=no-self-use
return isinstance(ir, Send)

51
slither/detectors/operations/unchecked_transfer.py
Unescape View File

@ -0,0 +1,51 @@
"""
Module detecting unused transfer/transferFrom return values from external calls
"""
from slither.core.declarations import Function
from slither.detectors.abstract_detector import DetectorClassification
from slither.detectors.operations.unused_return_values import UnusedReturnValues
from slither.slithir.operations import HighLevelCall
class UncheckedTransfer(UnusedReturnValues):
"""
If the return value of a transfer/transferFrom function is never used, it's likely to be bug
"""
ARGUMENT = "unchecked-transfer"
HELP = "Unchecked tokens transfer"
IMPACT = DetectorClassification.HIGH
CONFIDENCE = DetectorClassification.MEDIUM
WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#unchecked-transfer"
WIKI_TITLE = "Unchecked transfer"
WIKI_DESCRIPTION = "The return value of an external transfer/transferFrom call is not checked"
WIKI_EXPLOIT_SCENARIO = """
```solidity
contract Token {
function transferFrom(address _from, address _to, uint256 _value) public returns (bool success);
}
contract MyBank{
mapping(address => uint) balances;
Token token;
function deposit(uint amount) public{
token.transferFrom(msg.sender, address(this), amount);
balances[msg.sender] += amount;
}
}
```
Several tokens do not revert in case of failure and return false. If one of these tokens is used in `MyBank`, `deposit` will not revert if the transfer fails, and an attacker can call `deposit` for free.."""
WIKI_RECOMMENDATION = (
"Use `SafeERC20`, or ensure that the transfer/transferFrom return value is checked."
)
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)"]
)

12
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):
@ -36,10 +37,15 @@ contract MyConc{
WIKI_RECOMMENDATION = "Ensure that all the return values of the function calls are used."
_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)"]
)
or not isinstance(ir.function, Function)
)
def detect_unused_return_values(self, f): # pylint: disable=no-self-use
"""

63
tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol.0.7.6.UncheckedTransfer.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/unchecked-transfer/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol#20-22) ignores return value by t.transfer(address(0),1000000000000000000) (tests/detectors/unchecked-transfer/unused_return_transfers.sol#21)\n",
"markdown": "[C.bad0()](tests/detectors/unchecked-transfer/unused_return_transfers.sol#L20-L22) ignores return value by [t.transfer(address(0),1000000000000000000)](tests/detectors/unchecked-transfer/unused_return_transfers.sol#L21)\n",
"id": "c999626efabdf72014bbebbdecd64178176d168947ae803ebbbd873941a6ed7e",
"check": "unchecked-transfer",
"impact": "High",
"confidence": "Medium"
},
{
"elements": [
{
"type": "function",
"name": "bad1",
"source_mapping": {
"start": 1043,
"length": 90,
"filename_used": "/GENERIC_PATH",
"filename_relative": "tests/detectors/unchecked-transfer/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/unchecked-transfer/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/unchecked-transfer/unused_return_transfers.sol#40-42) ignores return value by t.transferFrom(address(this),address(0),1000000000000000000) (tests/detectors/unchecked-transfer/unused_return_transfers.sol#41)\n",
"markdown": "[C.bad1()](tests/detectors/unchecked-transfer/unused_return_transfers.sol#L40-L42) ignores return value by [t.transferFrom(address(this),address(0),1000000000000000000)](tests/detectors/unchecked-transfer/unused_return_transfers.sol#L41)\n",
"id": "d4c9ecd3753df951f9b2c890f54f981285d42500cd41e5dfc2f46f2feb1dbe6e",
"check": "unchecked-transfer",
"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.UncheckedTransfer,
"tests/detectors/unchecked-transfer/unused_return_transfers.sol",
"0.7.6",
),
Test(
all_detectors.ShadowingAbstractDetection,
"tests/detectors/shadowing-abstract/shadowing_abstract.sol",

Write Preview
Loading…
Cancel
Save