diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index e94eab330..bbf9464d3 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -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 diff --git a/slither/detectors/operations/unchecked_low_level_return_values.py b/slither/detectors/operations/unchecked_low_level_return_values.py index c2ac7f43b..8ecec9f1d 100644 --- a/slither/detectors/operations/unchecked_low_level_return_values.py +++ b/slither/detectors/operations/unchecked_low_level_return_values.py @@ -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) diff --git a/slither/detectors/operations/unchecked_send_return_value.py b/slither/detectors/operations/unchecked_send_return_value.py index 1c9ba11c0..d98ec3317 100644 --- a/slither/detectors/operations/unchecked_send_return_value.py +++ b/slither/detectors/operations/unchecked_send_return_value.py @@ -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) diff --git a/slither/detectors/operations/unchecked_transfer.py b/slither/detectors/operations/unchecked_transfer.py new file mode 100644 index 000000000..36456e3e0 --- /dev/null +++ b/slither/detectors/operations/unchecked_transfer.py @@ -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)"] + ) diff --git a/slither/detectors/operations/unused_return_values.py b/slither/detectors/operations/unused_return_values.py index 4d98f71da..7ba4211d8 100644 --- a/slither/detectors/operations/unused_return_values.py +++ b/slither/detectors/operations/unused_return_values.py @@ -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 """ diff --git a/tests/detectors/unchecked-transfer/unused_return_transfers.sol b/tests/detectors/unchecked-transfer/unused_return_transfers.sol new file mode 100644 index 000000000..dc16943df --- /dev/null +++ b/tests/detectors/unchecked-transfer/unused_return_transfers.sol @@ -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(); + } +} \ No newline at end of file diff --git a/tests/detectors/unchecked-transfer/unused_return_transfers.sol.0.7.6.UncheckedTransfer.json b/tests/detectors/unchecked-transfer/unused_return_transfers.sol.0.7.6.UncheckedTransfer.json new file mode 100644 index 000000000..9de6e71b1 --- /dev/null +++ b/tests/detectors/unchecked-transfer/unused_return_transfers.sol.0.7.6.UncheckedTransfer.json @@ -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" + } + ] +] \ No newline at end of file diff --git a/tests/detectors/unused-return/unused_return-sol7.sol b/tests/detectors/unused-return/unused_return-sol7.sol new file mode 100644 index 000000000..ddbc127da --- /dev/null +++ b/tests/detectors/unused-return/unused_return-sol7.sol @@ -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(); + } +} \ No newline at end of file diff --git a/tests/detectors/unused-return/unused_return-sol7.sol.0.7.6.UnusedReturnValues.json b/tests/detectors/unused-return/unused_return-sol7.sol.0.7.6.UnusedReturnValues.json new file mode 100644 index 000000000..c17b605ec --- /dev/null +++ b/tests/detectors/unused-return/unused_return-sol7.sol.0.7.6.UnusedReturnValues.json @@ -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" + } + ] +] \ No newline at end of file diff --git a/tests/test_detectors.py b/tests/test_detectors.py index b3bb4a50e..5b707d7bf 100644 --- a/tests/test_detectors.py +++ b/tests/test_detectors.py @@ -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",