diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index e94eab330..488071ea5 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.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 diff --git a/slither/detectors/operations/unused_return_values.py b/slither/detectors/operations/unused_return_values.py index 4d98f71da..8bbcf7988 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): @@ -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 """ diff --git a/slither/detectors/operations/unused_return_values_transfers.py b/slither/detectors/operations/unused_return_values_transfers.py new file mode 100644 index 000000000..ea54a1786 --- /dev/null +++ b/slither/detectors/operations/unused_return_values_transfers.py @@ -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 diff --git a/tests/detectors/unused-return-transfers/unused_return_transfers.sol b/tests/detectors/unused-return-transfers/unused_return_transfers.sol new file mode 100644 index 000000000..dc16943df --- /dev/null +++ b/tests/detectors/unused-return-transfers/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/unused-return-transfers/unused_return_transfers.sol.0.7.6.UnusedReturnValuesTransfers.json b/tests/detectors/unused-return-transfers/unused_return_transfers.sol.0.7.6.UnusedReturnValuesTransfers.json new file mode 100644 index 000000000..69542c9cd --- /dev/null +++ b/tests/detectors/unused-return-transfers/unused_return_transfers.sol.0.7.6.UnusedReturnValuesTransfers.json @@ -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" + } + ] +] \ 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..6fd733adf 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.UnusedReturnValuesTransfers, + "tests/detectors/unused-return-transfers/unused_return_transfers.sol", + "0.7.6", + ), Test( all_detectors.ShadowingAbstractDetection, "tests/detectors/shadowing-abstract/shadowing_abstract.sol",