diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 57f44bc56e..bb74b9c09e 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -75,6 +75,7 @@ from .compiler_bugs.multiple_constructor_schemes import MultipleConstructorSchemes from .compiler_bugs.public_mapping_nested import PublicMappingNested from .compiler_bugs.reused_base_constructor import ReusedBaseConstructor +from .statements.wrong_encode_with_selector import WrongEncodeWithSelector from .operations.missing_events_access_control import MissingEventsAccessControl from .operations.missing_events_arithmetic import MissingEventsArithmetic from .functions.modifier import ModifierDefaultDetection diff --git a/slither/detectors/statements/wrong_encode_with_selector.py b/slither/detectors/statements/wrong_encode_with_selector.py new file mode 100644 index 0000000000..cb255be243 --- /dev/null +++ b/slither/detectors/statements/wrong_encode_with_selector.py @@ -0,0 +1,145 @@ +from typing import List, Dict, Tuple +from slither.core.declarations.contract import Contract +from slither.core.cfg.node import Node +from slither.core.declarations.function import Function +from slither.slithir.operations.operation import Operation +from slither.slithir.operations import TypeConversion +from slither.core.expressions.literal import Literal +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.slithir.operations import SolidityCall +from slither.core.declarations.solidity_variables import SolidityFunction +from slither.slithir.operations.assignment import Assignment +from slither.slithir.variables.reference import ReferenceVariable +from slither.slithir.variables.constant import Constant +from slither.utils.function import get_function_id +from slither.utils.output import Output + + +def get_signatures(c: Contract) -> Dict[str, str]: + """Build a dictionary mapping function ids to signature, name, arguments for a contract""" + result = {} + functions = c.functions + for f in functions: + if f.visibility not in ["public", "external"]: + continue + if f.is_constructor or f.is_fallback: + continue + result[get_function_id(f.full_name)] = (f.full_name, *f.signature[:2]) + + variables = c.state_variables + for variable in variables: + if variable.visibility not in ["public"]: + continue + name = variable.full_name + result[get_function_id(name)] = (name, (), ()) + + return result + + +class WrongEncodeWithSelector(AbstractDetector): + """ + Detect calls to abi.encodeWithSelector that may result in unexpected calldata encodings + """ + + ARGUMENT = "wrong-encode-selector" + HELP = "abi.encodeWithSelector with unexpected arguments" + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.HIGH + + WIKI = "https://github.com/trailofbits/slither/wiki/encode-with-selector" + WIKI_TITLE = "Parameters of incorrect type or number in abi.encodeWithSelector" + WIKI_DESCRIPTION = "Wrong number or type of arguments in abi.encodeWithSelector" + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract Test { + event Val(uint, uint); + function f(uint a, uint b) public { + emit Val(a, b); + } +} +contract D { + function g() public { + Test t = new Test(); + address(t).call(abi.encodeWithSelector(Test.f.selector, "test")); + } +} +``` +abi.encodeWithSelector's arguments do not match the types expected in the function signature. +function signature. +""" + WIKI_RECOMMENDATION = "Make sure that the type and number of arguments passed to abi.encodeWithSelector are the same as the target function signature." + + def _detect(self) -> List[Output]: + # gather all known funcids + func_ids = {} + for contract in self.compilation_unit.contracts_derived: + func_ids.update(get_signatures(contract)) + # todo: include func_ids from the public db + + results = [] + for contract in self.compilation_unit.contracts_derived: + for func, node in check_contract(contract, func_ids): + info = [ + func, + " calls abi.encodeWithSelector() with arguments of incorrect type or with an incorrect number of arguments:", + node, + ] + json = self.generate_result(info) + results.append(json) + + return results + + +def check_wrong_selector_encoding( + function: Function, node: Node, ir: Operation, func_ids: Dict[str, str] +) -> List[Tuple[Function, Node]]: + result = [] + if isinstance(ir, SolidityCall) and ir.function == SolidityFunction("abi.encodeWithSelector()"): + # build reference bindings dict + assignments = {} + for ir1 in node.irs: + if isinstance(ir1, Assignment): + assignments[ir1.lvalue.name] = ir1.rvalue + + # if the selector is a reference, deref + selector = ir.arguments[0] + if isinstance(selector, ReferenceVariable): + selector = assignments[selector.name] + + # Does not handle LocalVariables + if not isinstance(selector, Constant): + if isinstance(selector.expression, TypeConversion) and isinstance( + selector.expression.expression, Literal + ): + selector = selector.expression.expression.value + else: + return result + + _, _, argument_types = func_ids[selector.value] + arguments = ir.arguments[1:] + + # todo: add check for user defined types + if len(argument_types) != len(arguments): + result.append((function, node)) + else: + for idx, expected in enumerate(argument_types): + if expected != str(arguments[idx].type): + result.append((function, node)) + break + + return result + + +def check_contract(contract: Contract, func_ids: Dict[str, str]) -> List[Tuple[Function, Node]]: + """Check contract's usage of abi.encodeWithSelector to ensure that the number of arguments + and their type math the function signature of the given selector. + """ + result = [] + for function in contract.functions_and_modifiers_declared: + if SolidityFunction("abi.encodeWithSelector()") in function.solidity_calls: + for node in function.nodes: + if SolidityFunction("abi.encodeWithSelector()") in node.solidity_calls: + for ir in node.irs: + result += check_wrong_selector_encoding(function, node, ir, func_ids) + + return result diff --git a/tests/e2e/detectors/snapshots/detectors__detector_WrongEncodeWithSelector_0_8_0_wrong_encode_selector_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_WrongEncodeWithSelector_0_8_0_wrong_encode_selector_sol__0.txt new file mode 100644 index 0000000000..3202d442d7 --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_WrongEncodeWithSelector_0_8_0_wrong_encode_selector_sol__0.txt @@ -0,0 +1,4 @@ +D.bad_elementaryTypes() (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol#40-45) calls abi.encodeWithSelector() with arguments of incorrect type or with an incorrect number of arguments:address(t).call(abi.encodeWithSelector(Test.f.selector,test,test)) (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol#42-44) +D.bad_userDefined() (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol#47-56) calls abi.encodeWithSelector() with arguments of incorrect type or with an incorrect number of arguments:address(t).call(abi.encodeWithSelector(Test.f2.selector,badUserDefined(test,1),badUserDefined(test,1))) (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol#49-55) +D.bad_array() (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol#58-64) calls abi.encodeWithSelector() with arguments of incorrect type or with an incorrect number of arguments:address(t).call(abi.encodeWithSelector(Test.f3.selector,arr)) (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol#63) +D.bad_numArgs() (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol#35-38) calls abi.encodeWithSelector() with arguments of incorrect type or with an incorrect number of arguments:address(t).call(abi.encodeWithSelector(Test.f.selector,test)) (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol#37) diff --git a/tests/e2e/detectors/snapshots/detectors__detector_WrongEncodeWithSelector_0_8_15_wrong_encode_selector_sol__0.txt b/tests/e2e/detectors/snapshots/detectors__detector_WrongEncodeWithSelector_0_8_15_wrong_encode_selector_sol__0.txt new file mode 100644 index 0000000000..c04d14ec70 --- /dev/null +++ b/tests/e2e/detectors/snapshots/detectors__detector_WrongEncodeWithSelector_0_8_15_wrong_encode_selector_sol__0.txt @@ -0,0 +1,4 @@ +D.bad_array() (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol#58-64) calls abi.encodeWithSelector() with arguments of incorrect type or with an incorrect number of arguments:address(t).call(abi.encodeWithSelector(Test.f3.selector,arr)) (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol#63) +D.bad_elementaryTypes() (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol#40-45) calls abi.encodeWithSelector() with arguments of incorrect type or with an incorrect number of arguments:address(t).call(abi.encodeWithSelector(Test.f.selector,test,test)) (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol#42-44) +D.bad_numArgs() (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol#35-38) calls abi.encodeWithSelector() with arguments of incorrect type or with an incorrect number of arguments:address(t).call(abi.encodeWithSelector(Test.f.selector,test)) (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol#37) +D.bad_userDefined() (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol#47-56) calls abi.encodeWithSelector() with arguments of incorrect type or with an incorrect number of arguments:address(t).call(abi.encodeWithSelector(Test.f2.selector,badUserDefined(test,1),badUserDefined(test,1))) (tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol#49-55) diff --git a/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol b/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol new file mode 100644 index 0000000000..2d739c39df --- /dev/null +++ b/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol @@ -0,0 +1,78 @@ +contract Test { + struct UserDefined { + uint256 a; + string b; + } + + event Val(uint, uint); + event Val2(UserDefined, UserDefined); + event Val3(uint256[]); + + function f(uint a, uint b) public { + emit Val(a, b); + } + + function f2(UserDefined memory a, UserDefined memory b) public { + emit Val2(a, b); + } + + function f3(uint256[] memory a) public { + emit Val3(a); + } +} + +contract D { + struct UserDefined { + uint256 a; + string b; + } + + struct badUserDefined { + string a; + uint256 b; + } + + function bad_numArgs() public { + Test t = new Test(); + address(t).call(abi.encodeWithSelector(Test.f.selector, "test")); + } + + function bad_elementaryTypes() public { + Test t = new Test(); + address(t).call( + abi.encodeWithSelector(Test.f.selector, "test", "test") + ); + } + + function bad_userDefined() public { + Test t = new Test(); + address(t).call( + abi.encodeWithSelector( + Test.f2.selector, + badUserDefined({a: "test", b: 1}), + badUserDefined({a: "test", b: 1}) + ) + ); + } + + function bad_array() public { + Test t = new Test(); + uint32[] memory arr = new uint32[](2); + arr[0] = 1; + arr[1] = 2; + address(t).call(abi.encodeWithSelector(Test.f3.selector, arr)); + } + + function good() public { + Test t = new Test(); + address(t).call(abi.encodeWithSelector(Test.f.selector, 1, 2)); + } + + function good_array() public { + Test t = new Test(); + uint256[] memory arr = new uint256[](2); + arr[0] = 1; + arr[1] = 2; + address(t).call(abi.encodeWithSelector(Test.f3.selector, arr)); + } +} diff --git a/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol-0.8.0.zip b/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol-0.8.0.zip new file mode 100644 index 0000000000..16aca9b248 Binary files /dev/null and b/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.0/wrong-encode-selector.sol-0.8.0.zip differ diff --git a/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol b/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol new file mode 100644 index 0000000000..2d739c39df --- /dev/null +++ b/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol @@ -0,0 +1,78 @@ +contract Test { + struct UserDefined { + uint256 a; + string b; + } + + event Val(uint, uint); + event Val2(UserDefined, UserDefined); + event Val3(uint256[]); + + function f(uint a, uint b) public { + emit Val(a, b); + } + + function f2(UserDefined memory a, UserDefined memory b) public { + emit Val2(a, b); + } + + function f3(uint256[] memory a) public { + emit Val3(a); + } +} + +contract D { + struct UserDefined { + uint256 a; + string b; + } + + struct badUserDefined { + string a; + uint256 b; + } + + function bad_numArgs() public { + Test t = new Test(); + address(t).call(abi.encodeWithSelector(Test.f.selector, "test")); + } + + function bad_elementaryTypes() public { + Test t = new Test(); + address(t).call( + abi.encodeWithSelector(Test.f.selector, "test", "test") + ); + } + + function bad_userDefined() public { + Test t = new Test(); + address(t).call( + abi.encodeWithSelector( + Test.f2.selector, + badUserDefined({a: "test", b: 1}), + badUserDefined({a: "test", b: 1}) + ) + ); + } + + function bad_array() public { + Test t = new Test(); + uint32[] memory arr = new uint32[](2); + arr[0] = 1; + arr[1] = 2; + address(t).call(abi.encodeWithSelector(Test.f3.selector, arr)); + } + + function good() public { + Test t = new Test(); + address(t).call(abi.encodeWithSelector(Test.f.selector, 1, 2)); + } + + function good_array() public { + Test t = new Test(); + uint256[] memory arr = new uint256[](2); + arr[0] = 1; + arr[1] = 2; + address(t).call(abi.encodeWithSelector(Test.f3.selector, arr)); + } +} diff --git a/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol-0.8.15.zip b/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol-0.8.15.zip new file mode 100644 index 0000000000..ba1e252026 Binary files /dev/null and b/tests/e2e/detectors/test_data/wrong-encode-selector/0.8.15/wrong-encode-selector.sol-0.8.15.zip differ diff --git a/tests/e2e/detectors/test_detectors.py b/tests/e2e/detectors/test_detectors.py index 6fc04e4e1f..37217c5848 100644 --- a/tests/e2e/detectors/test_detectors.py +++ b/tests/e2e/detectors/test_detectors.py @@ -1533,6 +1533,16 @@ def id_test(test_item: Test): "arbitrary_send_erc20_permit.sol", "0.8.0", ), + Test( + all_detectors.WrongEncodeWithSelector, + "wrong-encode-selector.sol", + "0.8.15", + ), + Test( + all_detectors.WrongEncodeWithSelector, + "wrong-encode-selector.sol", + "0.8.0", + ), Test( all_detectors.DomainSeparatorCollision, "permit_domain_collision.sol", @@ -1660,6 +1670,7 @@ def id_test(test_item: Test): TEST_DATA_DIR = Path(__file__).resolve().parent / "test_data" + # pylint: disable=too-many-locals @pytest.mark.parametrize("test_item", ALL_TESTS, ids=id_test) def test_detector(test_item: Test, snapshot):