diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 97622f8878..fab9562d20 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -96,3 +96,4 @@ 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 diff --git a/slither/detectors/statements/return_bomb.py b/slither/detectors/statements/return_bomb.py new file mode 100644 index 0000000000..8b6cd07a29 --- /dev/null +++ b/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 diff --git a/tests/e2e/detectors/snapshots/detectors__detector_ReturnBomb_0_8_20_return_bomb_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_ReturnBomb_0_8_20_return_bomb_sol__0.txt new file mode 100644 index 0000000000..f53b5233fd --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_ReturnBomb_0_8_20_return_bomb_sol__0.txt @@ -0,0 +1,5 @@ +Mark.oops(address) (tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol#31-55) tries to limit the gas of an external call that controls implicit decoding + ret1 = BadGuy(badGuy).fbad{gas: 2000}() (tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol#42) + (x,str) = BadGuy(badGuy).fbad1{gas: 2000}() (tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol#44) + (success,ret) = badGuy.call{gas: 10000}(abi.encodeWithSelector(BadGuy.llbad.selector)) (tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol#47-51) + diff --git a/tests/e2e/detectors/snapshots/detectors__detector_TautologicalCompare_0_8_20_compare_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_TautologicalCompare_0_8_20_compare_sol__0.txt new file mode 100644 index 0000000000..76685043ce --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_TautologicalCompare_0_8_20_compare_sol__0.txt @@ -0,0 +1,3 @@ +A.check(uint256) (tests/e2e/detectors/test_data/tautological-compare/0.8.20/compare.sol#3-5) compares a variable to itself: + (a >= a) (tests/e2e/detectors/test_data/tautological-compare/0.8.20/compare.sol#4) + diff --git a/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol b/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol new file mode 100644 index 0000000000..76413fdcaa --- /dev/null +++ b/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol @@ -0,0 +1,57 @@ +contract BadGuy { + function llbad() external pure returns (bytes memory) { + assembly{ + revert(0, 1000000) + } + } + + function fgood() external payable returns (uint){ + assembly{ + return(0, 1000000) + } + } + + function fbad() external payable returns (uint[] memory){ + assembly{ + return(0, 1000000) + } + } + + function fbad1() external payable returns (uint, string memory){ + assembly{ + return(0, 1000000) + } + } + + +} + +contract Mark { + + function oops(address badGuy) public{ + bool success; + string memory str; + bytes memory ret; + uint x; + uint[] memory ret1; + + x = BadGuy(badGuy).fgood{gas:2000}(); + + ret1 = BadGuy(badGuy).fbad(); //good (no gas specified) + + ret1 = BadGuy(badGuy).fbad{gas:2000}(); + + (x, str) = BadGuy(badGuy).fbad1{gas:2000}(); + + // Mark pays a lot of gas for this copy 😬😬😬 + (success, ret) = badGuy.call{gas:10000}( + abi.encodeWithSelector( + BadGuy.llbad.selector + ) + ); + + // Mark may OOG here, preventing local state changes + //importantCleanup(); + } +} + diff --git a/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol-0.8.20.zip b/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol-0.8.20.zip new file mode 100644 index 0000000000..4c10ea6fea Binary files /dev/null and b/tests/e2e/detectors/test_data/return-bomb/0.8.20/return_bomb.sol-0.8.20.zip differ diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index 99e9095fff..28dcc5e755 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/tests/e2e/detectors/test_detectors.py @@ -1674,6 +1674,11 @@ def id_test(test_item: Test): "compare.sol", "0.8.20", ), + Test( + all_detectors.ReturnBomb, + "return_bomb.sol", + "0.8.20", + ), ] GENERIC_PATH = "/GENERIC_PATH"