From 825e53db52ffdf64bc00eb19bc10e9e3cf18578c Mon Sep 17 00:00:00 2001 From: edlsh Date: Sat, 20 Dec 2025 08:01:38 -0500 Subject: [PATCH] Fix function-summary printer incorrectly reporting internal calls as external Fixes #2073 The function-summary printer was incorrectly classifying all member access expressions (x.foo()) as external calls, including: - Solidity built-ins: abi.encode(), abi.decode(), etc. - Library calls: SafeMath.add(), both direct and via 'using X for Y' - Struct field access: data.value Changes: - solc_parsing/cfg/node.py: Add _classify_calls() to properly filter Solidity built-ins and direct library calls at parse time - core/declarations/function_contract.py: Use IR-based high_level_calls minus library_calls for accurate external call reporting in get_summary() - core/declarations/function_top_level.py: Same fix for top-level functions - tests/e2e/printers/: Add regression test covering all cases Note: this.foo() is correctly classified as external (uses CALL opcode). --- .../core/declarations/function_contract.py | 12 +- .../core/declarations/function_top_level.py | 12 +- slither/solc_parsing/cfg/node.py | 80 ++++++++++- .../test_function_summary/external_calls.sol | 135 ++++++++++++++++++ tests/e2e/printers/test_function_summary.py | 94 ++++++++++++ 5 files changed, 322 insertions(+), 11 deletions(-) create mode 100644 tests/e2e/printers/test_data/test_function_summary/external_calls.sol create mode 100644 tests/e2e/printers/test_function_summary.py diff --git a/slither/core/declarations/function_contract.py b/slither/core/declarations/function_contract.py index 2e8aa3efef..7ec4089732 100644 --- a/slither/core/declarations/function_contract.py +++ b/slither/core/declarations/function_contract.py @@ -103,8 +103,16 @@ def get_summary( Return the function summary Returns: (str, str, str, list(str), list(str), listr(str), list(str), list(str); - contract_name, name, visibility, modifiers, vars read, vars written, internal_calls, external_calls_as_expressions + contract_name, name, visibility, modifiers, vars read, vars written, internal_calls, external_calls """ + # Get true external calls by excluding library calls from high-level calls + # This is more accurate than external_calls_as_expressions which is set at parse time + # and cannot distinguish library calls via "using X for Y" syntax + library_call_irs = set(self.library_calls) + external_calls = [ + (contract, ir) for contract, ir in self.high_level_calls if ir not in library_call_irs + ] + return ( self.contract_declarer.name, self.full_name, @@ -113,7 +121,7 @@ def get_summary( [str(x) for x in self.state_variables_read + self.solidity_variables_read], [str(x) for x in self.state_variables_written], [str(x) for x in self.internal_calls], - [str(x) for x in self.external_calls_as_expressions], + [f"{contract.name}.{ir.function_name}()" for contract, ir in external_calls], compute_cyclomatic_complexity(self), ) diff --git a/slither/core/declarations/function_top_level.py b/slither/core/declarations/function_top_level.py index 39fa2d2bb7..62221364b7 100644 --- a/slither/core/declarations/function_top_level.py +++ b/slither/core/declarations/function_top_level.py @@ -74,8 +74,16 @@ def get_summary( Return the function summary Returns: (str, str, str, list(str), list(str), listr(str), list(str), list(str); - contract_name, name, visibility, modifiers, vars read, vars written, internal_calls, external_calls_as_expressions + contract_name, name, visibility, modifiers, vars read, vars written, internal_calls, external_calls """ + # Get true external calls by excluding library calls from high-level calls + # This is more accurate than external_calls_as_expressions which is set at parse time + # and cannot distinguish library calls via "using X for Y" syntax + library_call_irs = set(self.library_calls) + external_calls = [ + (contract, ir) for contract, ir in self.high_level_calls if ir not in library_call_irs + ] + return ( "", self.full_name, @@ -84,7 +92,7 @@ def get_summary( [str(x) for x in self.state_variables_read + self.solidity_variables_read], [str(x) for x in self.state_variables_written], [str(x) for x in self.internal_calls], - [str(x) for x in self.external_calls_as_expressions], + [f"{contract.name}.{ir.function_name}()" for contract, ir in external_calls], ) # endregion diff --git a/slither/solc_parsing/cfg/node.py b/slither/solc_parsing/cfg/node.py index b1380553b0..a91aa7bde0 100644 --- a/slither/solc_parsing/cfg/node.py +++ b/slither/solc_parsing/cfg/node.py @@ -1,12 +1,16 @@ -from typing import Union, Optional, Dict, TYPE_CHECKING +from typing import Union, Optional, Dict, List, Tuple, TYPE_CHECKING from slither.core.cfg.node import Node from slither.core.cfg.node import NodeType +from slither.core.declarations import Contract +from slither.core.declarations.solidity_variables import SolidityVariable from slither.core.expressions.assignment_operation import ( AssignmentOperation, AssignmentOperationType, ) +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.solc_parsing.expressions.expression_parsing import parse_expression from slither.visitors.expression.find_calls import FindCalls from slither.visitors.expression.read_var import ReadVar @@ -17,6 +21,67 @@ from slither.solc_parsing.declarations.modifier import ModifierSolc +def _classify_calls( + calls: List[CallExpression], +) -> Tuple[List[CallExpression], List[CallExpression]]: + """ + Classify call expressions into internal and external calls. + + External calls are calls to external contracts (e.g., token.transfer()). + 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()) + + Args: + calls: List of CallExpression to classify + + Returns: + Tuple of (internal_calls, external_calls) + """ + internal_calls: List[CallExpression] = [] + external_calls: List[CallExpression] = [] + + for call in calls: + called = call.called + + if isinstance(called, Identifier): + # Direct function call like myFunc() or require() + # These are internal calls (including Solidity built-ins accessed directly) + internal_calls.append(call) + + elif isinstance(called, MemberAccess): + # Member access like x.foo() + # Need to determine if x is: + # - A Solidity built-in (abi, msg, block, etc.) -> internal + # - A library contract -> internal + # - An external contract/address -> external + base_expr = called.expression + + if isinstance(base_expr, Identifier): + base_value = base_expr.value + + # Check if it's a Solidity built-in variable (abi, msg, block, tx, etc.) + # Note: "this" is a SolidityVariable but this.foo() is an external call + # (uses CALL opcode), so we exclude it from internal calls + if isinstance(base_value, SolidityVariable) and base_value.name != "this": + internal_calls.append(call) + # Check if it's a library contract + elif isinstance(base_value, Contract) and base_value.is_library: + internal_calls.append(call) + else: + # External contract call + external_calls.append(call) + else: + # Complex expression like getContract().foo() - treat as external + external_calls.append(call) + else: + # Other cases (e.g., complex expressions) - treat as external to be safe + external_calls.append(call) + + return internal_calls, external_calls + + class NodeSolc: def __init__(self, node: Node) -> None: self._unparsed_expression: Optional[Dict] = None @@ -62,9 +127,10 @@ def analyze_expressions(self, caller_context: Union["FunctionSolc", "ModifierSol find_call = FindCalls(expression) self._node.calls_as_expression = find_call.result() - self._node.external_calls_as_expressions = [ - c for c in self._node.calls_as_expression if not isinstance(c.called, Identifier) - ] - self._node.internal_calls_as_expressions = [ - c for c in self._node.calls_as_expression if isinstance(c.called, Identifier) - ] + + # Classify calls into internal and external + # Internal: direct calls, Solidity built-ins (abi.encode, etc.), library calls + # External: calls to external contracts + 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/tests/e2e/printers/test_data/test_function_summary/external_calls.sol b/tests/e2e/printers/test_data/test_function_summary/external_calls.sol new file mode 100644 index 0000000000..0d5192f23a --- /dev/null +++ b/tests/e2e/printers/test_data/test_function_summary/external_calls.sol @@ -0,0 +1,135 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.0; + +// Test case for issue #2073: function-summary printer incorrectly reports +// abi.encode, library calls, and struct field access as external calls + +// Library for testing library calls +library SafeMath { + function add(uint256 a, uint256 b) internal pure returns (uint256) { + return a + b; + } + + function sub(uint256 a, uint256 b) internal pure returns (uint256) { + return a - b; + } +} + +// Interface for external calls +interface IExternalContract { + function externalFunction() external returns (uint256); + function anotherExternal(uint256 x) external returns (uint256); +} + +// Contract to test Solidity built-in calls (should NOT be external calls) +contract TestBuiltins { + function useAbiEncode(uint256 a, address b) external pure returns (bytes memory) { + return abi.encode(a, b); + } + + function useAbiEncodePacked(uint256 a) external pure returns (bytes memory) { + return abi.encodePacked(a); + } + + function useAbiEncodeWithSelector(bytes4 selector, uint256 a) external pure returns (bytes memory) { + return abi.encodeWithSelector(selector, a); + } + + function useAbiDecode(bytes memory data) external pure returns (uint256, address) { + return abi.decode(data, (uint256, address)); + } + + function useKeccak256(bytes memory data) external pure returns (bytes32) { + return keccak256(data); + } +} + +// Contract to test library calls (should NOT be external calls) +contract TestLibraryCalls { + using SafeMath for uint256; + + function useLibraryWithUsing(uint256 a, uint256 b) external pure returns (uint256) { + return a.add(b); + } + + function useLibraryDirect(uint256 a, uint256 b) external pure returns (uint256) { + return SafeMath.add(a, b); + } + + function useMultipleLibraryCalls(uint256 a, uint256 b, uint256 c) external pure returns (uint256) { + uint256 sum = SafeMath.add(a, b); + return SafeMath.sub(sum, c); + } +} + +// Contract to test struct field access (should NOT be external calls) +contract TestStructAccess { + struct Data { + uint256 value; + address owner; + } + + Data private data; + + function setData(uint256 v, address o) external { + data.value = v; + data.owner = o; + } + + function getValue() external view returns (uint256) { + return data.value; + } + + function getOwner() external view returns (address) { + return data.owner; + } +} + +// Contract to test TRUE external calls (SHOULD be external calls) +contract TestExternalCalls { + IExternalContract private externalContract; + + constructor(IExternalContract _external) { + externalContract = _external; + } + + function callExternal() external returns (uint256) { + return externalContract.externalFunction(); + } + + function callExternalWithArg(uint256 x) external returns (uint256) { + return externalContract.anotherExternal(x); + } + + function callMultipleExternal(uint256 x) external returns (uint256) { + uint256 a = externalContract.externalFunction(); + uint256 b = externalContract.anotherExternal(x); + return a + b; + } +} + +// Mixed contract testing all patterns +contract TestMixed { + using SafeMath for uint256; + + IExternalContract private externalContract; + + struct Config { + uint256 threshold; + } + + Config private config; + + constructor(IExternalContract _external) { + externalContract = _external; + } + + // Should have 1 external call (externalContract.externalFunction) + // Should NOT count: abi.encode, SafeMath.add, config.threshold + function mixedOperations(uint256 a, uint256 b) external returns (bytes memory) { + uint256 sum = SafeMath.add(a, b); + uint256 external_val = externalContract.externalFunction(); + uint256 threshold = config.threshold; + return abi.encode(sum, external_val, threshold); + } +} diff --git a/tests/e2e/printers/test_function_summary.py b/tests/e2e/printers/test_function_summary.py new file mode 100644 index 0000000000..707f7a391d --- /dev/null +++ b/tests/e2e/printers/test_function_summary.py @@ -0,0 +1,94 @@ +""" +Tests for the function-summary printer, specifically for issue #2073. + +Issue #2073: The function-summary printer incorrectly reports all statements +with '.' as external calls, including: +- Solidity built-ins like abi.encode(), abi.decode() +- Library calls like SafeMath.add() +- Struct field access + +This test verifies that only true external contract calls are reported +in the "External Calls" column of the function-summary printer. +""" + +from pathlib import Path + +from crytic_compile import CryticCompile +from crytic_compile.platform.solc_standard_json import SolcStandardJson + +from slither import Slither + + +TEST_DATA_DIR = Path(__file__).resolve().parent / "test_data" / "test_function_summary" + + +def _get_external_calls_from_summary(func): + """Get external calls from function summary (index 7 in the tuple).""" + summary = func.get_summary() + return summary[7] # external_calls is at index 7 + + +def _assert_no_external_calls_in_summary(contract, contract_name): + """Helper to assert a contract's functions have no external calls in summary.""" + for func in contract.functions: + if func.is_constructor: + continue + external_calls = _get_external_calls_from_summary(func) + assert len(external_calls) == 0, ( + f"{contract_name}.{func.name} should have no external calls, " + f"but found: {external_calls}" + ) + + +def test_function_summary_external_calls(solc_binary_path) -> None: + """ + Test that the function-summary correctly classifies external calls. + + - Solidity built-ins (abi.encode, etc.) should NOT be external calls + - Library calls (SafeMath.add, etc.) should NOT be external calls + - Struct field access should NOT be external calls + - True external contract calls SHOULD be external calls + """ + solc_path = solc_binary_path("0.8.0") + standard_json = SolcStandardJson() + standard_json.add_source_file(Path(TEST_DATA_DIR, "external_calls.sol").as_posix()) + compilation = CryticCompile(standard_json, solc=solc_path) + slither = Slither(compilation) + + # Test TestBuiltins contract - should have NO external calls + test_builtins = slither.get_contract_from_name("TestBuiltins")[0] + _assert_no_external_calls_in_summary(test_builtins, "TestBuiltins") + + # Test TestLibraryCalls contract - should have NO external calls + test_library = slither.get_contract_from_name("TestLibraryCalls")[0] + _assert_no_external_calls_in_summary(test_library, "TestLibraryCalls") + + # Test TestStructAccess contract - should have NO external calls + test_struct = slither.get_contract_from_name("TestStructAccess")[0] + _assert_no_external_calls_in_summary(test_struct, "TestStructAccess") + + # Test TestExternalCalls contract - should have external calls + test_external = slither.get_contract_from_name("TestExternalCalls")[0] + + call_external = test_external.get_function_from_signature("callExternal()") + ext_calls = _get_external_calls_from_summary(call_external) + assert len(ext_calls) == 1, "TestExternalCalls.callExternal should have 1 external call" + + call_with_arg = test_external.get_function_from_signature("callExternalWithArg(uint256)") + ext_calls = _get_external_calls_from_summary(call_with_arg) + assert len(ext_calls) == 1, "TestExternalCalls.callExternalWithArg should have 1 external call" + + call_multiple = test_external.get_function_from_signature("callMultipleExternal(uint256)") + ext_calls = _get_external_calls_from_summary(call_multiple) + assert ( + len(ext_calls) == 2 + ), "TestExternalCalls.callMultipleExternal should have 2 external calls" + + # Test TestMixed contract - mixedOperations should have exactly 1 external call + test_mixed = slither.get_contract_from_name("TestMixed")[0] + mixed_ops = test_mixed.get_function_from_signature("mixedOperations(uint256,uint256)") + ext_calls = _get_external_calls_from_summary(mixed_ops) + assert len(ext_calls) == 1, ( + f"TestMixed.mixedOperations should have 1 external call, " + f"but found {len(ext_calls)}: {ext_calls}" + )