add unused-return-transfers detector + update unused-return

slither/detectors/all_detectors.py

@@ -21,6 +21,7 @@
 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

slither/detectors/operations/unchecked_low_level_return_values.py

@@ -34,7 +34,5 @@ class UncheckedLowLevel(UnusedReturnValues):
 
     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)

slither/detectors/operations/unchecked_send_return_value.py

@@ -35,7 +35,5 @@ class UncheckedSend(UnusedReturnValues):
 
     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)

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)"]
+        )

slither/detectors/operations/unused_return_values.py

@@ -5,6 +5,7 @@
 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 @@ class UnusedReturnValues(AbstractDetector):
 
     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
         """

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();
+    }
+}