Merge pull request #2156 from crytic/new-detectors

Release new detectors

slither/detectors/all_detectors.py

@@ -92,3 +92,8 @@
 from .operations.cache_array_length import CacheArrayLength
 from .statements.incorrect_using_for import IncorrectUsingFor
 from .operations.encode_packed import EncodePackedCollision
+from .assembly.incorrect_return import IncorrectReturn
+from .assembly.return_instead_of_leave import ReturnInsteadOfLeave
+from .operations.incorrect_exp import IncorrectOperatorExponentiation
+from .statements.tautological_compare import TautologicalCompare
+from .statements.return_bomb import ReturnBomb

slither/detectors/assembly/incorrect_return.py

@@ -0,0 +1,91 @@
+from typing import List, Optional
+
+from slither.core.declarations import SolidityFunction, Function
+from slither.detectors.abstract_detector import (
+    AbstractDetector,
+    DetectorClassification,
+    DETECTOR_INFO,
+)
+from slither.slithir.operations import SolidityCall
+from slither.utils.output import Output
+
+
+def _assembly_node(function: Function) -> Optional[SolidityCall]:
+    """
+    Check if there is a node that use return in assembly
+
+    Args:
+        function:
+
+    Returns:
+
+    """
+
+    for ir in function.all_slithir_operations():
+        if isinstance(ir, SolidityCall) and ir.function == SolidityFunction(
+            "return(uint256,uint256)"
+        ):
+            return ir
+    return None
+
+
+class IncorrectReturn(AbstractDetector):
+    """
+    Check for cases where a return(a,b) is used in an assembly function
+    """
+
+    ARGUMENT = "incorrect-return"
+    HELP = "If a `return` is incorrectly used in assembly mode."
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-assembly-return"
+
+    WIKI_TITLE = "Incorrect return in assembly"
+    WIKI_DESCRIPTION = "Detect if `return` in an assembly block halts unexpectedly the execution."
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+contract C {
+    function f() internal returns (uint a, uint b) {
+        assembly {
+            return (5, 6)
+        }
+    }
+
+    function g() returns (bool){
+        f();
+        return true;
+    }
+}
+```
+The return statement in `f` will cause execution in `g` to halt.
+The function will return 6 bytes starting from offset 5, instead of returning a boolean."""
+
+    WIKI_RECOMMENDATION = "Use the `leave` statement."
+
+    # pylint: disable=too-many-nested-blocks
+    def _detect(self) -> List[Output]:
+        results: List[Output] = []
+        for c in self.contracts:
+            for f in c.functions_and_modifiers_declared:
+
+                for node in f.nodes:
+                    if node.sons:
+                        for function_called in node.internal_calls:
+                            if isinstance(function_called, Function):
+                                found = _assembly_node(function_called)
+                                if found:
+
+                                    info: DETECTOR_INFO = [
+                                        f,
+                                        " calls ",
+                                        function_called,
+                                        " which halt the execution ",
+                                        found.node,
+                                        "\n",
+                                    ]
+                                    json = self.generate_result(info)
+
+                                    results.append(json)
+
+        return results

slither/detectors/assembly/return_instead_of_leave.py

@@ -0,0 +1,68 @@
+from typing import List
+
+from slither.core.declarations import SolidityFunction, Function
+from slither.detectors.abstract_detector import (
+    AbstractDetector,
+    DetectorClassification,
+    DETECTOR_INFO,
+)
+from slither.slithir.operations import SolidityCall
+from slither.utils.output import Output
+
+
+class ReturnInsteadOfLeave(AbstractDetector):
+    """
+    Check for cases where a return(a,b) is used in an assembly function that also returns two variables
+    """
+
+    ARGUMENT = "return-leave"
+    HELP = "If a `return` is used instead of a `leave`."
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-assembly-return"
+
+    WIKI_TITLE = "Return instead of leave in assembly"
+    WIKI_DESCRIPTION = "Detect if a `return` is used where a `leave` should be used."
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+contract C {
+    function f() internal returns (uint a, uint b) {
+        assembly {
+            return (5, 6)
+        }
+    }
+
+}
+```
+The function will halt the execution, instead of returning a two uint."""
+
+    WIKI_RECOMMENDATION = "Use the `leave` statement."
+
+    def _check_function(self, f: Function) -> List[Output]:
+        results: List[Output] = []
+
+        for node in f.nodes:
+            for ir in node.irs:
+                if isinstance(ir, SolidityCall) and ir.function == SolidityFunction(
+                    "return(uint256,uint256)"
+                ):
+                    info: DETECTOR_INFO = [f, " contains an incorrect call to return: ", node, "\n"]
+                    json = self.generate_result(info)
+
+                    results.append(json)
+        return results
+
+    def _detect(self) -> List[Output]:
+        results: List[Output] = []
+        for c in self.contracts:
+            for f in c.functions_declared:
+
+                if (
+                    len(f.returns) == 2
+                    and f.contains_assembly
+                    and f.visibility not in ["public", "external"]
+                ):
+                    results += self._check_function(f)
+
+        return results

slither/detectors/operations/incorrect_exp.py

@@ -0,0 +1,93 @@
+"""
+Module detecting incorrect operator usage for exponentiation where bitwise xor '^' is used instead of '**'
+"""
+from typing import Tuple, List, Union
+
+from slither.core.cfg.node import Node
+from slither.core.declarations import Contract, Function
+from slither.detectors.abstract_detector import (
+    AbstractDetector,
+    DetectorClassification,
+    DETECTOR_INFO,
+)
+from slither.slithir.operations import Binary, BinaryType, Operation
+from slither.slithir.utils.utils import RVALUE
+from slither.slithir.variables.constant import Constant
+from slither.utils.output import Output
+
+
+def _is_constant_candidate(var: Union[RVALUE, Function]) -> bool:
+    """
+    Check if the variable is a constant.
+    Do not consider variable that are expressed with hexadecimal.
+    Something like 2^0xf is likely to be a correct bitwise operator
+    :param var:
+    :return:
+    """
+    return isinstance(var, Constant) and not var.original_value.startswith("0x")
+
+
+def _is_bitwise_xor_on_constant(ir: Operation) -> bool:
+    return (
+        isinstance(ir, Binary)
+        and ir.type == BinaryType.CARET
+        and (_is_constant_candidate(ir.variable_left) or _is_constant_candidate(ir.variable_right))
+    )
+
+
+def _detect_incorrect_operator(contract: Contract) -> List[Tuple[Function, Node]]:
+    ret: List[Tuple[Function, Node]] = []
+    f: Function
+    for f in contract.functions + contract.modifiers:  # type:ignore
+        # Heuristic: look for binary expressions with ^ operator where at least one of the operands is a constant, and
+        # the constant is not in hex, because hex typically is used with bitwise xor and not exponentiation
+        nodes = [node for node in f.nodes for ir in node.irs if _is_bitwise_xor_on_constant(ir)]
+        for node in nodes:
+            ret.append((f, node))
+    return ret
+
+
+# pylint: disable=too-few-public-methods
+class IncorrectOperatorExponentiation(AbstractDetector):
+    """
+    Incorrect operator usage of bitwise xor mistaking it for exponentiation
+    """
+
+    ARGUMENT = "incorrect-exp"
+    HELP = "Incorrect exponentiation"
+    IMPACT = DetectorClassification.HIGH
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-exponentiation"
+
+    WIKI_TITLE = "Incorrect exponentiation"
+    WIKI_DESCRIPTION = "Detect use of bitwise `xor ^` instead of exponential `**`"
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+contract Bug{
+    uint UINT_MAX = 2^256 - 1;
+    ...
+}
+```
+Alice deploys a contract in which `UINT_MAX` incorrectly uses `^` operator instead of `**` for exponentiation"""
+
+    WIKI_RECOMMENDATION = "Use the correct operator `**` for exponentiation."
+
+    def _detect(self) -> List[Output]:
+        """Detect the incorrect operator usage for exponentiation where bitwise xor ^ is used instead of **
+
+        Returns:
+            list: (function, node)
+        """
+        results: List[Output] = []
+        for c in self.compilation_unit.contracts_derived:
+            res = _detect_incorrect_operator(c)
+            for (func, node) in res:
+                info: DETECTOR_INFO = [
+                    func,
+                    " has bitwise-xor operator ^ instead of the exponentiation operator **: \n",
+                ]
+                info += ["\t - ", node, "\n"]
+                results.append(self.generate_result(info))
+
+        return results

slither/detectors/statements/return_bomb.py

@@ -0,0 +1,123 @@
+from typing import List
+
+from slither.core.cfg.node import Node
+from slither.core.declarations import Contract
+from slither.core.declarations.function import Function
+from slither.core.solidity_types import Type
+from slither.detectors.abstract_detector import (
+    AbstractDetector,
+    DetectorClassification,
+    DETECTOR_INFO,
+)
+from slither.slithir.operations import LowLevelCall, HighLevelCall
+from slither.analyses.data_dependency.data_dependency import is_tainted
+from slither.utils.output import Output
+
+
+class ReturnBomb(AbstractDetector):
+
+    ARGUMENT = "return-bomb"
+    HELP = "A low level callee may consume all callers gas unexpectedly."
+    IMPACT = DetectorClassification.LOW
+    CONFIDENCE = DetectorClassification.MEDIUM
+
+    WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#return-bomb"
+
+    WIKI_TITLE = "Return Bomb"
+    WIKI_DESCRIPTION = "A low level callee may consume all callers gas unexpectedly."
+    WIKI_EXPLOIT_SCENARIO = """
+```solidity
+//Modified from https://github.com/nomad-xyz/ExcessivelySafeCall
+contract BadGuy {
+    function youveActivateMyTrapCard() external pure returns (bytes memory) {
+        assembly{
+            revert(0, 1000000)
+        }
+    }
+}
+
+contract Mark {
+    function oops(address badGuy) public{
+        bool success;
+        bytes memory ret;
+
+        // Mark pays a lot of gas for this copy
+        //(success, ret) = badGuy.call{gas:10000}(
+        (success, ret) = badGuy.call(
+            abi.encodeWithSelector(
+                BadGuy.youveActivateMyTrapCard.selector
+            )
+        );
+
+        // Mark may OOG here, preventing local state changes
+        //importantCleanup();
+    }
+}
+
+```
+After Mark calls BadGuy bytes are copied from returndata to memory, the memory expansion cost is paid. This means that when using a standard solidity call, the callee can "returnbomb" the caller, imposing an arbitrary gas cost. 
+Callee unexpectedly makes the caller OOG. 
+"""
+
+    WIKI_RECOMMENDATION = "Avoid unlimited implicit decoding of returndata."
+
+    @staticmethod
+    def is_dynamic_type(ty: Type) -> bool:
+        # ty.is_dynamic ?
+        name = str(ty)
+        if "[]" in name or name in ("bytes", "string"):
+            return True
+        return False
+
+    def get_nodes_for_function(self, function: Function, contract: Contract) -> List[Node]:
+        nodes = []
+        for node in function.nodes:
+            for ir in node.irs:
+                if isinstance(ir, (HighLevelCall, LowLevelCall)):
+                    if not is_tainted(ir.destination, contract):  # type:ignore
+                        # Only interested if the target address is controlled/tainted
+                        continue
+
+                    if isinstance(ir, HighLevelCall) and isinstance(ir.function, Function):
+                        # in normal highlevel calls return bombs are _possible_
+                        # if the return type is dynamic and the caller tries to copy and decode large data
+                        has_dyn = False
+                        if ir.function.return_type:
+                            has_dyn = any(
+                                self.is_dynamic_type(ty) for ty in ir.function.return_type
+                            )
+
+                        if not has_dyn:
+                            continue
+
+                    # If a gas budget was specified then the
+                    # user may not know about the return bomb
+                    if ir.call_gas is None:
+                        # if a gas budget was NOT specified then the caller
+                        # may already suspect the call may spend all gas?
+                        continue
+
+                    nodes.append(node)
+                # TODO: check that there is some state change after the call
+
+        return nodes
+
+    def _detect(self) -> List[Output]:
+        results = []
+
+        for contract in self.compilation_unit.contracts:
+            for function in contract.functions_declared:
+                nodes = self.get_nodes_for_function(function, contract)
+                if nodes:
+                    info: DETECTOR_INFO = [
+                        function,
+                        " tries to limit the gas of an external call that controls implicit decoding\n",
+                    ]
+
+                    for node in sorted(nodes, key=lambda x: x.node_id):
+                        info += ["\t", node, "\n"]
+
+                    res = self.generate_result(info)
+                    results.append(res)
+
+        return results