From 96219063f7141941a6194ead79b9b0ba9daaf966 Mon Sep 17 00:00:00 2001 From: godsangsoo Date: Wed, 21 Dec 2022 14:19:04 +0000 Subject: [PATCH] dangerous-external-call --- slither/detectors/all_detectors.py | 1 + .../statements/dangerous_external_call.py | 233 ++++++++++++++++++ .../0.8.0/dangerous_external_call.sol | 8 + ..._call.sol.0.8.0.DangerousExternalCall.json | 59 +++++ tests/test_detectors.py | 5 + 5 files changed, 306 insertions(+) create mode 100644 slither/detectors/statements/dangerous_external_call.py create mode 100644 tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol create mode 100644 tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol.0.8.0.DangerousExternalCall.json diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 2c8d244281..f45b10be1d 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -85,3 +85,4 @@ from .statements.delegatecall_in_loop import DelegatecallInLoop from .functions.protected_variable import ProtectedVariables from .functions.permit_domain_signature_collision import DomainSeparatorCollision +from .statements.dangerous_external_call import DangerousExternalCall diff --git a/slither/detectors/statements/dangerous_external_call.py b/slither/detectors/statements/dangerous_external_call.py new file mode 100644 index 0000000000..4451489de3 --- /dev/null +++ b/slither/detectors/statements/dangerous_external_call.py @@ -0,0 +1,233 @@ +""" +Module detecting dangerous external call. + +""" + +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.slithir.operations import Binary, BinaryType +from slither.slithir.operations.solidity_call import SolidityCall +from slither.slithir.operations.high_level_call import HighLevelCall +from slither.slithir.operations.library_call import LibraryCall +from slither.slithir.operations.internal_call import InternalCall +from slither.core.declarations.function import FunctionType +from slither.analyses.data_dependency.data_dependency import get_all_dependencies +from slither.core.declarations.function import FunctionType +from slither.core.declarations.solidity_variables import SolidityVariableComposed +from slither.slithir.operations.index import Index +from slither.core.declarations.modifier import Modifier +import re + + +class DangerousExternalCall(AbstractDetector): + """ + Detect dangerous-external-call + """ + + ARGUMENT = "dangerous-external-call" + HELP = "External call by using unverified address" + IMPACT = DetectorClassification.HIGH + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#dangerous-external-call" + WIKI_TITLE = "DANGEROUS_EXTERNAL_CALL" + WIKI_DESCRIPTION = "DANGEROUS_EXTERNAL_CALL" + WIKI_EXPLOIT_SCENARIO = """ +```solidity +interface IERC20 { + function safeTransferFrom (address, address, uint) external; +} +contract A { + function depositFor(address token, uint _amount,address user ) public { + IERC20(token).safeTransferFrom(msg.sender, address(this), _amount); //vulnerable point + } +} +``` +https://ftmscan.com/address/0x660184ce8af80e0b1e5a1172a16168b15f4136bf#code#L1115 +https://rekt.news/grim-finance-rekt/ + +Attacker can input fake token address because there is no verification of token address. +""" + WIKI_RECOMMENDATION = "Add verification of address to function calling external call" + + # heuristic 1: Before calling, external call's address might be verfied. + def explore(self, _contract, _func, _visited: dict, _binary_operands: set): + + if _func in _visited: + return _visited[_func] + + _visited[_func] = False + + dependency = get_all_dependencies(_contract) + # _func is internal func or modifier? + # modifier does not external call. + if isinstance(_func, Modifier): + internal_calls = [] + for node in _func.nodes: + for ir in node.irs: + # not modifier + if isinstance(ir, InternalCall) and not isinstance(ir.function, Modifier): + internal_calls.append(ir.function) + + # collect binary operand + if isinstance(ir, Binary) and ir._type in [ + BinaryType.EQUAL, + BinaryType.NOT_EQUAL, + ]: + for op in ir.read: + _binary_operands.add(op) + + # access-control + # require(isExcluded[msg.sender]) + # require(msg.sender == owner) + if isinstance(ir, SolidityCall) and "require" in ir.function.full_name: + for op in ir.arguments: + if ( + op in dependency + and SolidityVariableComposed("msg.sender") in dependency[op] + ): + self.access_control = True + # print(node.expression) + + for mod in _func.modifiers: + _visited[_func] |= self.explore(_contract, mod, _visited, _binary_operands) + + for func in internal_calls: + _visited[_func] |= self.explore(_contract, func, _visited, _binary_operands) + + else: + + for mod in _func.modifiers: + _visited[_func] |= self.explore(_contract, mod, _visited, _binary_operands) + + for node in _func.nodes: + for ir in node.irs: + # not modifier + if isinstance(ir, InternalCall) and not isinstance(ir.function, Modifier): + _visited[_func] |= self.explore( + _contract, ir.function, _visited, _binary_operands + ) + + # collect binary operand + if isinstance(ir, Binary) and ir._type in [ + BinaryType.EQUAL, + BinaryType.NOT_EQUAL, + ]: + for op in ir.read: + _binary_operands.add(op) + + # access-control + # require(isExcluded[msg.sender]) + # require(msg.sender == owner) + if isinstance(ir, SolidityCall) and "require" in ir.function.full_name: + for op in ir.arguments: + if ( + op in dependency + and SolidityVariableComposed("msg.sender") in dependency[op] + ): + self.access_control = True + # print(node.expression) + + # external call + if isinstance(ir, HighLevelCall): + + # SafeErc + if ( + isinstance(ir, LibraryCall) + and ir._destination.name.lower() == "safemath" + ): + continue + + self.answer = False + + # dest is state variable? + dest = re.findall(r"\([a-zA-Z0-9_]+\)", str(ir.expression)) + dest2 = str(ir.expression).split(".") + + if ( + len(dest) > 0 + and any(vari.name == dest[0][1:-1] for vari in _contract.variables) + ) or ( + (len(dest2) > 0) + and any(vari.name == dest2[0] for vari in _contract.variables) + ): + continue + + data = ir.destination + for op in _binary_operands: + if op in dependency and data in dependency[op]: + self.answer = True + + if data in dependency: + if any(op == _data for _data in dependency[data]): + self.answer = True + + if op in dependency and data in dependency: + for _data in dependency[data]: + if _data in dependency[op]: + self.answer = True + + if self.answer == False and self.access_control == False: + info = [ + "Dangerous external call in ", + _func, + "\n", + ] + res = self.generate_result(info) + self.results.append(res) + return _visited[_func] + + def _detect(self): + + KEY_NON_SSA = "DATA_DEPENDENCY" + + for contract in self.compilation_unit.contracts: + t = get_all_dependencies(contract) + + for function in contract.functions + contract.modifiers: + for node in function.nodes: + for ir in node.irs: + # At a = b[index], add 'index' to 'data dependency of a' + if isinstance(ir, Index): + + # data_dependency[index] = index + # data_dependency[a] += index + if ir.variable_right not in t: + contract.context[KEY_NON_SSA][ir.lvalue].add(ir.variable_right) + contract.context[KEY_NON_SSA][ir.variable_right] = set( + [ir.variable_right] + ) + + # data_dependency[a] += data_dependency[index] + else: + for i in t[ir.variable_right]: + contract.context[KEY_NON_SSA][ir.lvalue].add(i) + + # if a in data_dependency[?] -> data_dependency[?] += data_dependency[index] + for key, value in t.items(): + if ir.lvalue in value: + for i in t[ir.variable_right]: + contract.context[KEY_NON_SSA][key].add(i) + self.results = [] + + for contract in self.compilation_unit.contracts: + for function in contract.functions: + + if "init" in function.name.lower() and any( + mod.name + in ["initializer", "reinitializer", "onlyInitializing", "_disableInitializers"] + for mod in function.modifiers + ): + continue + + if ( + function._pure == True + or function._view == True + or function._visibility in ["internal", "private"] + or function._function_type == FunctionType.CONSTRUCTOR + ): + continue + + self.access_control = False + self.explore(contract, function, {}, set()) + + return self.results diff --git a/tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol b/tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol new file mode 100644 index 0000000000..626a21048c --- /dev/null +++ b/tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol @@ -0,0 +1,8 @@ +interface IERC20 { + function safeTransferFrom (address, address, uint) external; +} +contract A { + function depositFor(address token, uint _amount,address user ) public { + IERC20(token).safeTransferFrom(msg.sender, address(this), _amount); //vulnerable point + } +} \ No newline at end of file diff --git a/tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol.0.8.0.DangerousExternalCall.json b/tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol.0.8.0.DangerousExternalCall.json new file mode 100644 index 0000000000..7290d29790 --- /dev/null +++ b/tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol.0.8.0.DangerousExternalCall.json @@ -0,0 +1,59 @@ +[ + [ + { + "elements": [ + { + "type": "function", + "name": "depositFor", + "source_mapping": { + "start": 103, + "length": 172, + "filename_relative": "tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "A", + "source_mapping": { + "start": 86, + "length": 191, + "filename_relative": "tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol", + "is_dependency": false, + "lines": [ + 4, + 5, + 6, + 7, + 8, + 9 + ], + "starting_column": 1, + "ending_column": 0 + } + }, + "signature": "depositFor(address,uint256,address)" + } + } + ], + "description": "Dangerous external call in A.depositFor(address,uint256,address) (tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol#5-7)\n", + "markdown": "Dangerous external call in [A.depositFor(address,uint256,address)](tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol#L5-L7)\n", + "first_markdown_element": "tests/detectors/dangerous-external-call/0.8.0/dangerous_external_call.sol#L5-L7", + "id": "f37363838be23f73fc91760f32b6f5900a02840dc797ba804b9f3bfcb84b7ae9", + "check": "dangerous-external-call", + "impact": "High", + "confidence": "Medium" + } + ] +] \ No newline at end of file diff --git a/tests/test_detectors.py b/tests/test_detectors.py index 810589bb06..8fad2a4efe 100644 --- a/tests/test_detectors.py +++ b/tests/test_detectors.py @@ -1557,6 +1557,11 @@ def id_test(test_item: Test): "permit_domain_state_var_collision.sol", "0.8.0", ), + Test( + all_detectors.DangerousExternalCall, + "dangerous_external_call.sol", + "0.8.0", + ), ]