diff --git a/slither/solc_parsing/cfg/node.py b/slither/solc_parsing/cfg/node.py index e1cc3c05d7..d60025e615 100644 --- a/slither/solc_parsing/cfg/node.py +++ b/slither/solc_parsing/cfg/node.py @@ -63,9 +63,9 @@ def analyze_expressions(self, caller_context: Union["FunctionSolc", "ModifierSol find_call = FindCalls(expression) self._node.calls_as_expression = find_call.result() - # Classify calls into internal and external - # Internal: direct calls, Solidity built-ins (abi.encode, etc.), library calls - # External: calls to external contracts + # Classify calls into internal and external. + # Note: using-for library calls are re-classified later in + # _analyze_using_for once the directives are available. internal, external = classify_calls(self._node.calls_as_expression) self._node.internal_calls_as_expressions = internal self._node.external_calls_as_expressions = external diff --git a/slither/solc_parsing/slither_compilation_unit_solc.py b/slither/solc_parsing/slither_compilation_unit_solc.py index 783acb0519..b4d4058b41 100644 --- a/slither/solc_parsing/slither_compilation_unit_solc.py +++ b/slither/solc_parsing/slither_compilation_unit_solc.py @@ -701,6 +701,8 @@ def _analyze_third_part( def _analyze_using_for( self, contracts_to_be_analyzed: list[ContractSolc], libraries: list[ContractSolc] ) -> None: + from slither.visitors.expression.call_classification import classify_calls + self._analyze_top_level_using_for() for lib in libraries: @@ -721,6 +723,25 @@ def _analyze_using_for( else: contracts_to_be_analyzed += [contract] + # Re-classify calls now that using-for directives are available. + # The initial classification in NodeSolc.analyze_expressions runs + # before using-for is parsed, so library calls via ``using L for T`` + # are incorrectly marked external. Re-running with the complete + # using-for map fixes this. + for contract in self._underlying_contract_to_parser: + using_for = contract.using_for_complete + if not using_for: + continue + for func in contract.functions_declared + contract.modifiers_declared: + for node in func.nodes: + if not node.calls_as_expression: + continue + internal, external = classify_calls( + node.calls_as_expression, using_for=using_for + ) + node.internal_calls_as_expressions = internal + node.external_calls_as_expressions = external + def _analyze_enums(self, contract: ContractSolc) -> None: # Enum must be analyzed first contract.analyze_enums() diff --git a/slither/visitors/expression/call_classification.py b/slither/visitors/expression/call_classification.py index 7b4c2a2237..c1bf494828 100644 --- a/slither/visitors/expression/call_classification.py +++ b/slither/visitors/expression/call_classification.py @@ -1,19 +1,62 @@ from __future__ import annotations from collections.abc import Callable +from typing import TYPE_CHECKING from slither.core.declarations import Contract from slither.core.declarations.solidity_variables import SolidityVariable from slither.core.expressions.call_expression import CallExpression from slither.core.expressions.identifier import Identifier from slither.core.expressions.member_access import MemberAccess +from slither.core.variables.variable import Variable + +if TYPE_CHECKING: + from slither.utils.using_for import USING_FOR ExternalIdentifierPredicate = Callable[[Identifier], bool] +def _is_using_for_library_call( + variable: Variable, + member_name: str, + using_for: USING_FOR, +) -> bool: + """Return True when *member_name* resolves to a library function + attached to *variable*'s type via a ``using … for`` directive. + """ + from slither.core.solidity_types.user_defined_type import ( + UserDefinedType, + ) + + var_type = variable.type + if var_type is None: + return False + + # Collect candidate library entries for the variable's type. + # Keys can be Type objects, the "*" wildcard, or string representations. + candidates = [] + for key, items in using_for.items(): + if key == "*" or key == var_type or str(key) == str(var_type): + candidates.extend(items) + + for item in candidates: + # ``using LibContract for SomeType`` → item is UserDefinedType + if isinstance(item, UserDefinedType): + lib = item.type + if isinstance(lib, Contract) and lib.is_library: + if any(f.name == member_name for f in lib.functions): + return True + # ``using {freeFunc} for SomeType`` → item is Function + if hasattr(item, "name") and item.name == member_name: + return True + + return False + + def classify_calls( calls: list[CallExpression], is_external_identifier: ExternalIdentifierPredicate | None = None, + using_for: USING_FOR | None = None, ) -> tuple[list[CallExpression], list[CallExpression]]: """ Classify call expressions into internal and external calls. @@ -22,11 +65,14 @@ def classify_calls( Internal calls include: - Direct function calls (e.g., myFunc()) - Solidity built-in calls (e.g., abi.encode(), abi.decode()) - - Library calls (e.g., SafeMath.add()) + - Library calls (e.g., SafeMath.add(), or addr.sendValue() via using-for) Args: calls: List of CallExpression to classify - is_external_identifier: Optional predicate to mark Identifier calls as external + is_external_identifier: Optional predicate to mark Identifier calls + as external + using_for: Optional mapping from ``contract.using_for_complete`` + used to recognise ``using Library for Type`` calls as internal Returns: Tuple of (internal_calls, external_calls) @@ -51,19 +97,32 @@ def classify_calls( base_value = base_expr.value # Solidity built-ins (abi, msg, block, tx, etc.) - # Note: "this" is a SolidityVariable but this.foo() is an external call - if isinstance(base_value, SolidityVariable) and base_value.name != "this": + # Note: "this" is a SolidityVariable but this.foo() is + # an external call + if ( + isinstance(base_value, SolidityVariable) + and base_value.name != "this" + ): internal_calls.append(call) - # Library calls + # Library calls (e.g., SafeMath.add()) elif isinstance(base_value, Contract) and base_value.is_library: internal_calls.append(call) + # using-for library calls (e.g., addr.sendValue()) + elif ( + using_for + and isinstance(base_value, Variable) + and _is_using_for_library_call( + base_value, called.member_name, using_for + ) + ): + internal_calls.append(call) else: external_calls.append(call) else: external_calls.append(call) continue - # Other cases (e.g., complex expressions) - treat as external to be safe + # Other cases (e.g., complex expressions) — treat as external external_calls.append(call) return internal_calls, external_calls diff --git a/tests/unit/core/test_external_calls_classification.py b/tests/unit/core/test_external_calls_classification.py index 7b3a4fca3d..dd7bee4709 100644 --- a/tests/unit/core/test_external_calls_classification.py +++ b/tests/unit/core/test_external_calls_classification.py @@ -1,5 +1,261 @@ -from slither.core.declarations import SolidityFunction +from unittest.mock import MagicMock, PropertyMock + +import pytest + +from slither.core.declarations import Contract, SolidityFunction +from slither.core.declarations.solidity_variables import SolidityVariable +from slither.core.expressions.call_expression import CallExpression from slither.core.expressions.identifier import Identifier +from slither.core.expressions.member_access import MemberAccess +from slither.core.solidity_types.elementary_type import ElementaryType +from slither.core.solidity_types.user_defined_type import UserDefinedType +from slither.core.variables.local_variable import LocalVariable +from slither.core.variables.state_variable import StateVariable +from slither.visitors.expression.call_classification import classify_calls + + +# --------------------------------------------------------------------------- +# Helpers +# --------------------------------------------------------------------------- + + +def _make_member_call(base_value, member_name="foo"): + """Build a minimal CallExpression whose callee is a MemberAccess on base_value.""" + base_expr = MagicMock(spec=Identifier) + base_expr.value = base_value + called = MagicMock(spec=MemberAccess) + called.expression = base_expr + called.member_name = member_name + call = MagicMock(spec=CallExpression) + call.called = called + return call + + +def _make_identifier_call(identifier_value): + """Build a minimal CallExpression whose callee is a plain Identifier.""" + called = MagicMock(spec=Identifier) + called.value = identifier_value + call = MagicMock(spec=CallExpression) + call.called = called + return call + + +# --------------------------------------------------------------------------- +# Unit tests for classify_calls (no Solidity compiler required) +# --------------------------------------------------------------------------- + + +class TestClassifyCalls: + """Tests for classify_calls with mocked expression objects. + + Covers the cases from https://github.com/crytic/slither/issues/2073: + Solidity built-ins, library calls, struct/variable member accesses, and + genuine external contract calls should all be classified correctly. + """ + + def test_abi_encode_is_internal(self): + """abi.encode() must not appear in external calls (abi is a SolidityVariable).""" + call = _make_member_call(SolidityVariable("abi")) + internal, external = classify_calls([call]) + assert call in internal + assert call not in external + + def test_msg_sender_call_is_internal(self): + """msg.sender.call() — msg is a SolidityVariable, so the member access is internal.""" + call = _make_member_call(SolidityVariable("msg")) + internal, external = classify_calls([call]) + assert call in internal + assert call not in external + + def test_this_call_is_external(self): + """this.foo() must be classified as external (re-entrant call through the ABI).""" + call = _make_member_call(SolidityVariable("this")) + internal, external = classify_calls([call]) + assert call in external + assert call not in internal + + def test_library_call_is_internal(self): + """MyLib.foo() must be internal — library calls are inlined at compile time.""" + lib = MagicMock(spec=Contract) + lib.is_library = True + call = _make_member_call(lib) + internal, external = classify_calls([call]) + assert call in internal + assert call not in external + + def test_non_library_contract_call_is_external(self): + """ExternalContract.foo() must be external.""" + contract = MagicMock(spec=Contract) + contract.is_library = False + call = _make_member_call(contract) + internal, external = classify_calls([call]) + assert call in external + assert call not in internal + + def test_state_variable_member_call_is_external(self): + """token.transfer() where token is a state variable must be external.""" + state_var = MagicMock(spec=StateVariable) + call = _make_member_call(state_var) + internal, external = classify_calls([call]) + assert call in external + assert call not in internal + + def test_plain_identifier_call_is_internal(self): + """Direct function calls (no member access) must be internal by default.""" + func = MagicMock(spec=SolidityFunction) + call = _make_identifier_call(func) + internal, external = classify_calls([call]) + assert call in internal + assert call not in external + + def test_plain_identifier_call_marked_external(self): + """is_external_identifier predicate should override identifier classification.""" + func = MagicMock(spec=SolidityFunction) + call = _make_identifier_call(func) + called_id = call.called + internal, external = classify_calls([call], is_external_identifier=lambda x: x is called_id) + assert call in external + assert call not in internal + + def test_empty_call_list(self): + """classify_calls on an empty list must return two empty lists.""" + internal, external = classify_calls([]) + assert internal == [] + assert external == [] + + def test_mixed_calls(self): + """Verify that a mixed list of calls is split correctly.""" + lib = MagicMock(spec=Contract) + lib.is_library = True + lib_call = _make_member_call(lib) + + state_var = MagicMock(spec=StateVariable) + ext_call = _make_member_call(state_var) + + abi_call = _make_member_call(SolidityVariable("abi")) + + internal, external = classify_calls([lib_call, ext_call, abi_call]) + assert lib_call in internal + assert abi_call in internal + assert ext_call in external + assert len(internal) == 2 + assert len(external) == 1 + + +# --------------------------------------------------------------------------- +# using-for library call classification +# --------------------------------------------------------------------------- + + +def _make_using_for(var_type, library_contract): + """Build a USING_FOR dict: {var_type: [UserDefinedType(library)]}.""" + udt = MagicMock(spec=UserDefinedType) + udt.type = library_contract + return {var_type: [udt]} + + +def _make_library(name, function_names): + """Create a mock library Contract with the given function names.""" + lib = MagicMock(spec=Contract) + lib.is_library = True + lib.name = name + funcs = [] + for fn in function_names: + f = MagicMock() + f.name = fn + funcs.append(f) + lib.functions = funcs + return lib + + +class TestUsingForClassification: + """Tests for using-for library call classification (issue #2073).""" + + def test_using_for_library_call_is_internal(self): + """addr.sendValue() via 'using Address for address' must be internal.""" + addr_type = ElementaryType("address") + lib = _make_library("Address", ["sendValue", "functionCall"]) + using_for = _make_using_for(addr_type, lib) + + local_var = MagicMock(spec=LocalVariable) + type(local_var).type = PropertyMock(return_value=addr_type) + call = _make_member_call(local_var, member_name="sendValue") + + internal, external = classify_calls([call], using_for=using_for) + assert call in internal + assert call not in external + + def test_using_for_non_matching_member_is_external(self): + """addr.nonExistent() must stay external even with using-for.""" + addr_type = ElementaryType("address") + lib = _make_library("Address", ["sendValue"]) + using_for = _make_using_for(addr_type, lib) + + local_var = MagicMock(spec=LocalVariable) + type(local_var).type = PropertyMock(return_value=addr_type) + call = _make_member_call(local_var, member_name="nonExistent") + + internal, external = classify_calls([call], using_for=using_for) + assert call in external + assert call not in internal + + def test_using_for_wrong_type_is_external(self): + """uint256.sendValue() must be external when using-for is on address.""" + addr_type = ElementaryType("address") + uint_type = ElementaryType("uint256") + lib = _make_library("Address", ["sendValue"]) + using_for = _make_using_for(addr_type, lib) + + local_var = MagicMock(spec=LocalVariable) + type(local_var).type = PropertyMock(return_value=uint_type) + call = _make_member_call(local_var, member_name="sendValue") + + internal, external = classify_calls([call], using_for=using_for) + assert call in external + assert call not in internal + + def test_using_for_wildcard(self): + """'using Lib for *' must match any variable type.""" + lib = _make_library("SafeMath", ["add", "sub"]) + udt = MagicMock(spec=UserDefinedType) + udt.type = lib + using_for = {"*": [udt]} + + local_var = MagicMock(spec=LocalVariable) + type(local_var).type = PropertyMock( + return_value=ElementaryType("uint256") + ) + call = _make_member_call(local_var, member_name="add") + + internal, external = classify_calls([call], using_for=using_for) + assert call in internal + assert call not in external + + def test_using_for_without_using_for_dict(self): + """Without using_for, variable member calls must remain external.""" + local_var = MagicMock(spec=LocalVariable) + type(local_var).type = PropertyMock( + return_value=ElementaryType("address") + ) + call = _make_member_call(local_var, member_name="sendValue") + + internal, external = classify_calls([call]) + assert call in external + assert call not in internal + + def test_using_for_state_variable(self): + """State variable member calls via using-for must also be internal.""" + addr_type = ElementaryType("address") + lib = _make_library("Address", ["sendValue"]) + using_for = _make_using_for(addr_type, lib) + + state_var = MagicMock(spec=StateVariable) + type(state_var).type = PropertyMock(return_value=addr_type) + call = _make_member_call(state_var, member_name="sendValue") + + internal, external = classify_calls([call], using_for=using_for) + assert call in internal + assert call not in external def test_vyper_external_calls_classification(slither_from_vyper_source):