feat: add arbitrary-send-erc20 and arbitrary-send-erc20-permit detect…

…ors (#1025)

Add arbitrary-send-erc20 and arbitrary-send-erc20-permit detectors

slither/detectors/all_detectors.py

@@ -6,7 +6,9 @@
 from .attributes.constant_pragma import ConstantPragma
 from .attributes.incorrect_solc import IncorrectSolc
 from .attributes.locked_ether import LockedEther
-from .functions.arbitrary_send import ArbitrarySend
+from .functions.arbitrary_send_eth import ArbitrarySendEth
+from .erc.erc20.arbitrary_send_erc20_no_permit import ArbitrarySendErc20NoPermit
+from .erc.erc20.arbitrary_send_erc20_permit import ArbitrarySendErc20Permit
 from .functions.suicidal import Suicidal
 
 # from .functions.complex_function import ComplexFunction
@@ -34,7 +36,7 @@
 from .operations.block_timestamp import Timestamp
 from .statements.calls_in_loop import MultipleCallsInLoop
 from .statements.incorrect_strict_equality import IncorrectStrictEquality
-from .erc.incorrect_erc20_interface import IncorrectERC20InterfaceDetection
+from .erc.erc20.incorrect_erc20_interface import IncorrectERC20InterfaceDetection
 from .erc.incorrect_erc721_interface import IncorrectERC721InterfaceDetection
 from .erc.unindexed_event_parameters import UnindexedERC20EventParameters
 from .statements.deprecated_calls import DeprecatedStandards

slither/detectors/erc/erc20/arbitrary_send_erc20.py

@@ -0,0 +1,95 @@
+from typing import List
+from slither.core.cfg.node import Node
+from slither.core.declarations.solidity_variables import SolidityVariable
+from slither.slithir.operations import HighLevelCall, LibraryCall
+from slither.core.declarations import Contract, Function, SolidityVariableComposed
+from slither.analyses.data_dependency.data_dependency import is_dependent
+from slither.core.compilation_unit import SlitherCompilationUnit
+
+
+class ArbitrarySendErc20:
+    """Detects instances where ERC20 can be sent from an arbitrary from address."""
+
+    def __init__(self, compilation_unit: SlitherCompilationUnit):
+        self._compilation_unit = compilation_unit
+        self._no_permit_results: List[Node] = []
+        self._permit_results: List[Node] = []
+
+    @property
+    def compilation_unit(self) -> SlitherCompilationUnit:
+        return self._compilation_unit
+
+    @property
+    def no_permit_results(self) -> List[Node]:
+        return self._no_permit_results
+
+    @property
+    def permit_results(self) -> List[Node]:
+        return self._permit_results
+
+    def _detect_arbitrary_from(self, contract: Contract):
+        for f in contract.functions:
+            all_high_level_calls = [
+                f_called[1].solidity_signature
+                for f_called in f.high_level_calls
+                if isinstance(f_called[1], Function)
+            ]
+            all_library_calls = [f_called[1].solidity_signature for f_called in f.library_calls]
+            if (
+                "transferFrom(address,address,uint256)" in all_high_level_calls
+                or "safeTransferFrom(address,address,address,uint256)" in all_library_calls
+            ):
+                if (
+                    "permit(address,address,uint256,uint256,uint8,bytes32,bytes32)"
+                    in all_high_level_calls
+                ):
+                    ArbitrarySendErc20._arbitrary_from(f.nodes, self._permit_results)
+                else:
+                    ArbitrarySendErc20._arbitrary_from(f.nodes, self._no_permit_results)
+
+    @staticmethod
+    def _arbitrary_from(nodes: List[Node], results: List[Node]):
+        """Finds instances of (safe)transferFrom that do not use msg.sender or address(this) as from parameter."""
+        for node in nodes:
+            for ir in node.irs:
+                if (
+                    isinstance(ir, HighLevelCall)
+                    and isinstance(ir.function, Function)
+                    and ir.function.solidity_signature == "transferFrom(address,address,uint256)"
+                    and not (
+                        is_dependent(
+                            ir.arguments[0],
+                            SolidityVariableComposed("msg.sender"),
+                            node.function.contract,
+                        )
+                        or is_dependent(
+                            ir.arguments[0],
+                            SolidityVariable("this"),
+                            node.function.contract,
+                        )
+                    )
+                ):
+                    results.append(ir.node)
+                elif (
+                    isinstance(ir, LibraryCall)
+                    and ir.function.solidity_signature
+                    == "safeTransferFrom(address,address,address,uint256)"
+                    and not (
+                        is_dependent(
+                            ir.arguments[1],
+                            SolidityVariableComposed("msg.sender"),
+                            node.function.contract,
+                        )
+                        or is_dependent(
+                            ir.arguments[1],
+                            SolidityVariable("this"),
+                            node.function.contract,
+                        )
+                    )
+                ):
+                    results.append(ir.node)
+
+    def detect(self):
+        """Detect transfers that use arbitrary `from` parameter."""
+        for c in self.compilation_unit.contracts_derived:
+            self._detect_arbitrary_from(c)

slither/detectors/erc/erc20/arbitrary_send_erc20_no_permit.py

@@ -0,0 +1,45 @@
+from typing import List
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.utils.output import Output
+from .arbitrary_send_erc20 import ArbitrarySendErc20
+
+
+class ArbitrarySendErc20NoPermit(AbstractDetector):
+    """
+    Detect when `msg.sender` is not used as `from` in transferFrom
+    """
+
+    ARGUMENT = "arbitrary-send-erc20"
+    HELP = "transferFrom uses arbitrary `from`"
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.HIGH
+
+    WIKI = "https://github.com/trailofbits/slither/wiki/Detector-Documentation#arbitrary-send-erc20"
+
+    WIKI_TITLE = "Arbitrary `from` in transferFrom"
+    WIKI_DESCRIPTION = "Detect when `msg.sender` is not used as `from` in transferFrom."
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+    function a(address from, address to, uint256 amount) public {
+        erc20.transferFrom(from, to, am);
+    }
+```
+Alice approves this contract to spend her ERC20 tokens. Bob can call `a` and specify Alice's address as the `from` parameter in `transferFrom`, allowing him to transfer Alice's tokens to himself."""
+
+    WIKI_RECOMMENDATION = """
+Use `msg.sender` as `from` in transferFrom.
+"""
+
+    def _detect(self) -> List[Output]:
+        """"""
+        results: List[Output] = []
+
+        arbitrary_sends = ArbitrarySendErc20(self.compilation_unit)
+        arbitrary_sends.detect()
+        for node in arbitrary_sends.no_permit_results:
+            func = node.function
+            info = [func, " uses arbitrary from in transferFrom: ", node, "\n"]
+            res = self.generate_result(info)
+            results.append(res)
+
+        return results

slither/detectors/erc/erc20/arbitrary_send_erc20_permit.py

@@ -0,0 +1,53 @@
+from typing import List
+from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification
+from slither.utils.output import Output
+from .arbitrary_send_erc20 import ArbitrarySendErc20
+
+
+class ArbitrarySendErc20Permit(AbstractDetector):
+    """
+    Detect when `msg.sender` is not used as `from` in transferFrom along with the use of permit.
+    """
+
+    ARGUMENT = "arbitrary-send-erc20-permit"
+    HELP = "transferFrom uses arbitrary from with permit"
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = "https://github.com/trailofbits/slither/wiki/Detector-Documentation#arbitrary-send-erc20-permit"
+
+    WIKI_TITLE = "Arbitrary `from` in transferFrom used with permit"
+    WIKI_DESCRIPTION = (
+        "Detect when `msg.sender` is not used as `from` in transferFrom and permit is used."
+    )
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+    function bad(address from, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s, address to) public {
+        erc20.permit(from, address(this), value, deadline, v, r, s);
+        erc20.transferFrom(from, to, value);
+    }
+```
+If an ERC20 token does not implement permit and has a fallback function e.g. WETH, transferFrom allows an attacker to transfer all tokens approved for this contract."""
+
+    WIKI_RECOMMENDATION = """
+Ensure that the underlying ERC20 token correctly implements a permit function.
+"""
+
+    def _detect(self) -> List[Output]:
+        """"""
+        results: List[Output] = []
+
+        arbitrary_sends = ArbitrarySendErc20(self.compilation_unit)
+        arbitrary_sends.detect()
+        for node in arbitrary_sends.permit_results:
+            func = node.function
+            info = [
+                func,
+                " uses arbitrary from in transferFrom in combination with permit: ",
+                node,
+                "\n",
+            ]
+            res = self.generate_result(info)
+            results.append(res)
+
+        return results

slither/detectors/functions/arbitrary_send.py → slither/detectors/functions/arbitrary_send_eth.py

@@ -90,8 +90,8 @@ def detect_arbitrary_send(contract: Contract):
     return ret
 
 
-class ArbitrarySend(AbstractDetector):
-    ARGUMENT = "arbitrary-send"
+class ArbitrarySendEth(AbstractDetector):
+    ARGUMENT = "arbitrary-send-eth"
     HELP = "Functions that send Ether to arbitrary destinations"
     IMPACT = DetectorClassification.HIGH
     CONFIDENCE = DetectorClassification.MEDIUM
@@ -104,7 +104,7 @@ class ArbitrarySend(AbstractDetector):
     # region wiki_exploit_scenario
     WIKI_EXPLOIT_SCENARIO = """
 ```solidity
-contract ArbitrarySend{
+contract ArbitrarySendEth{
     address destination;
     function setDestination(){
         destination = msg.sender;

tests/detectors/arbitrary-send-erc20-permit/0.4.25/arbitrary_send_erc20_permit.sol

@@ -0,0 +1,57 @@
+pragma solidity 0.4.25;
+
+library SafeERC20 {
+    function safeTransferFrom(IERC20 token, address from, address to, uint256 value) internal {}
+}
+
+interface IERC20 {
+    function transferFrom(address, address, uint256) external returns(bool);
+    function permit(address, address, uint256, uint256, uint8, bytes32, bytes32) external;
+}
+
+contract ERC20 is IERC20 {
+    function transferFrom(address from, address to, uint256 amount) external returns(bool) {
+        return true;
+    }
+    function permit(address owner, address spender, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s) external {}
+}
+
+contract C {
+    using SafeERC20 for IERC20;
+
+    IERC20 erc20;
+    address notsend;
+    address send;
+
+    constructor() public {
+        erc20 = new ERC20();
+        notsend = address(0x3);
+        send = msg.sender;
+    }
+
+    function bad1(address from, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s, address to) public {
+        erc20.permit(from, address(this), value, deadline, v, r, s);
+        erc20.transferFrom(from, to, value);
+    }
+
+    // This is not detected
+    function bad2(address from, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s, address to) public {
+        int_transferFrom(from,value, deadline, v, r, s, to);
+    }
+
+    function int_transferFrom(address from, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s, address to) internal {
+        erc20.permit(from, address(this), value, deadline, v, r, s);
+        erc20.transferFrom(from, to, value);
+    }
+
+    function bad3(address from, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s, address to) external {
+        erc20.permit(from, address(this), value, deadline, v, r, s);
+        erc20.safeTransferFrom(from, to, value);
+    }    
+
+    function bad4(address from, uint256 value, uint256 deadline, uint8 v, bytes32 r, bytes32 s, address to) external {
+        erc20.permit(from, address(this), value, deadline, v, r, s);
+        SafeERC20.safeTransferFrom(erc20, from, to, value);
+    }
+
+}