From b9d68bea09a5fd448d514d179fbf48b4be0c46ab Mon Sep 17 00:00:00 2001 From: Alexis Date: Thu, 11 Apr 2024 17:15:25 +0200 Subject: [PATCH 1/5] Adds a new printer for external calls. --- slither/printers/all_printers.py | 1 + slither/printers/summary/external_calls.py | 59 ++++++++++++++ slither/solc_parsing/cfg/node.py | 22 ++++-- .../test_data/test_external_calls/A.sol | 30 +++++++ .../test_data/test_external_calls/IERC20.sol | 79 +++++++++++++++++++ 5 files changed, 185 insertions(+), 6 deletions(-) create mode 100644 slither/printers/summary/external_calls.py create mode 100644 tests/e2e/printers/test_data/test_external_calls/A.sol create mode 100644 tests/e2e/printers/test_data/test_external_calls/IERC20.sol diff --git a/slither/printers/all_printers.py b/slither/printers/all_printers.py index 3edd5325b6..446f0b9ecd 100644 --- a/slither/printers/all_printers.py +++ b/slither/printers/all_printers.py @@ -24,3 +24,4 @@ from .summary.declaration import Declaration from .functions.dominator import Dominator from .summary.martin import Martin +from .summary.external_calls import ExternalCallPrinter diff --git a/slither/printers/summary/external_calls.py b/slither/printers/summary/external_calls.py new file mode 100644 index 0000000000..6476418389 --- /dev/null +++ b/slither/printers/summary/external_calls.py @@ -0,0 +1,59 @@ +""" + Module printing the high level calls +""" +from slither.printers.abstract_printer import AbstractPrinter +from slither.utils.myprettytable import MyPrettyTable + + +class ExternalCallPrinter(AbstractPrinter): + + ARGUMENT = "external-calls" + HELP = "Print the external calls performed by each function" + + WIKI = "https://github.com/trailofbits/slither/wiki/Printer-documentation#external-calls" + + def output(self, _): + """Computes and returns the list of external calls performed.""" + + all_txt = "External calls" + + table = MyPrettyTable(["Source (Line)", "Destination", "Chain"]) + + # pylint: disable=too-many-nested-blocks + for contract in self.slither.contracts: + if contract.is_interface or contract.is_abstract: + continue + + for function in contract.functions: + # Bail out early if this function does not perform high level calls + if not function.all_high_level_calls(): + continue + + for node in function.nodes: + for target_contract, target_function in node.high_level_calls: + + row = [ + f"{contract.name}.{function.name} {node.source_mapping.lines}", + f"{target_contract.name}.{target_function}", + ] + + if function.all_reachable_from_functions: + + for source in function.all_reachable_from_functions: + chain = f"{source.contract.name}.{source.name} -> {contract.name}.{function.name}" + table.add_row( + [ + *row, + chain, + ] + ) + else: + table.add_row([*row, ""]) + + all_txt += "\n" + str(table) + self.info(all_txt) + + res = self.generate_output(all_txt) + res.add_pretty_table(table, "External Calls") + + return res diff --git a/slither/solc_parsing/cfg/node.py b/slither/solc_parsing/cfg/node.py index b1380553b0..a9dbc83eed 100644 --- a/slither/solc_parsing/cfg/node.py +++ b/slither/solc_parsing/cfg/node.py @@ -1,11 +1,14 @@ +from itertools import filterfalse from typing import Union, Optional, Dict, TYPE_CHECKING from slither.core.cfg.node import Node from slither.core.cfg.node import NodeType +from slither.core.declarations import SolidityVariable from slither.core.expressions.assignment_operation import ( AssignmentOperation, AssignmentOperationType, ) + from slither.core.expressions.identifier import Identifier from slither.solc_parsing.expressions.expression_parsing import parse_expression from slither.visitors.expression.find_calls import FindCalls @@ -15,6 +18,7 @@ if TYPE_CHECKING: from slither.solc_parsing.declarations.function import FunctionSolc from slither.solc_parsing.declarations.modifier import ModifierSolc + from slither.core.expressions.expression import Expression class NodeSolc: @@ -62,9 +66,15 @@ 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) - ] + + def is_external_call(element: "Expression") -> bool: + return not isinstance(element.called, Identifier) and not isinstance( + element.called.expression.value, SolidityVariable + ) + + self._node.external_calls_as_expressions = list( + filter(is_external_call, self._node.calls_as_expression) + ) + self._node.internal_calls_as_expressions = list( + filterfalse(is_external_call, self._node.calls_as_expression) + ) diff --git a/tests/e2e/printers/test_data/test_external_calls/A.sol b/tests/e2e/printers/test_data/test_external_calls/A.sol new file mode 100644 index 0000000000..4917ce7ccd --- /dev/null +++ b/tests/e2e/printers/test_data/test_external_calls/A.sol @@ -0,0 +1,30 @@ +// SPDX-License-Identifier: GPL3 +pragma solidity ^0.8.20; + +import "./IERC20.sol"; + +contract A { + IERC20 private token; + + function foo() view internal { + token.balanceOf(address(this)); + } + + function encodeData(uint256 number, string memory text) public pure returns (bytes memory) { + return abi.encode(number, text); + } +} + +contract B is A { + function bar() view public { + foo(); + } +} + + +contract C { + B public b; + function pop() view public { + b.bar(); + } +} \ No newline at end of file diff --git a/tests/e2e/printers/test_data/test_external_calls/IERC20.sol b/tests/e2e/printers/test_data/test_external_calls/IERC20.sol new file mode 100644 index 0000000000..bcd73a9b10 --- /dev/null +++ b/tests/e2e/printers/test_data/test_external_calls/IERC20.sol @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v5.0.0) (token/ERC20/IERC20.sol) + +pragma solidity ^0.8.20; + +/** + * @dev Interface of the ERC-20 standard as defined in the ERC. + */ +interface IERC20 { + /** + * @dev Emitted when `value` tokens are moved from one account (`from`) to + * another (`to`). + * + * Note that `value` may be zero. + */ + event Transfer(address indexed from, address indexed to, uint256 value); + + /** + * @dev Emitted when the allowance of a `spender` for an `owner` is set by + * a call to {approve}. `value` is the new allowance. + */ + event Approval(address indexed owner, address indexed spender, uint256 value); + + /** + * @dev Returns the value of tokens in existence. + */ + function totalSupply() external view returns (uint256); + + /** + * @dev Returns the value of tokens owned by `account`. + */ + function balanceOf(address account) external view returns (uint256); + + /** + * @dev Moves a `value` amount of tokens from the caller's account to `to`. + * + * Returns a boolean value indicating whether the operation succeeded. + * + * Emits a {Transfer} event. + */ + function transfer(address to, uint256 value) external returns (bool); + + /** + * @dev Returns the remaining number of tokens that `spender` will be + * allowed to spend on behalf of `owner` through {transferFrom}. This is + * zero by default. + * + * This value changes when {approve} or {transferFrom} are called. + */ + function allowance(address owner, address spender) external view returns (uint256); + + /** + * @dev Sets a `value` amount of tokens as the allowance of `spender` over the + * caller's tokens. + * + * Returns a boolean value indicating whether the operation succeeded. + * + * IMPORTANT: Beware that changing an allowance with this method brings the risk + * that someone may use both the old and the new allowance by unfortunate + * transaction ordering. One possible solution to mitigate this race + * condition is to first reduce the spender's allowance to 0 and set the + * desired value afterwards: + * https://github.com/ethereum/EIPs/issues/20#issuecomment-263524729 + * + * Emits an {Approval} event. + */ + function approve(address spender, uint256 value) external returns (bool); + + /** + * @dev Moves a `value` amount of tokens from `from` to `to` using the + * allowance mechanism. `value` is then deducted from the caller's + * allowance. + * + * Returns a boolean value indicating whether the operation succeeded. + * + * Emits a {Transfer} event. + */ + function transferFrom(address from, address to, uint256 value) external returns (bool); +} \ No newline at end of file From 227aec5856173a6527f5ebb964e9db82ba6bff82 Mon Sep 17 00:00:00 2001 From: Alexis Date: Thu, 11 Apr 2024 17:18:14 +0200 Subject: [PATCH 2/5] Add a simple test for the printer --- tests/e2e/printers/test_printers.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/tests/e2e/printers/test_printers.py b/tests/e2e/printers/test_printers.py index 26429d3381..3a6ec772b7 100644 --- a/tests/e2e/printers/test_printers.py +++ b/tests/e2e/printers/test_printers.py @@ -2,11 +2,12 @@ from collections import Counter from pathlib import Path -from crytic_compile import CryticCompile +from crytic_compile import CryticCompile, compile_all from crytic_compile.platform.solc_standard_json import SolcStandardJson from slither import Slither from slither.printers.inheritance.inheritance_graph import PrinterInheritanceGraph +from slither.printers.summary.external_calls import ExternalCallPrinter TEST_DATA_DIR = Path(__file__).resolve().parent / "test_data" @@ -34,3 +35,14 @@ def test_inheritance_printer(solc_binary_path) -> None: assert counter["B -> A"] == 2 assert counter["C -> A"] == 1 + + +def test_external_call_printers() -> None: + compilation = compile_all((TEST_DATA_DIR / "test_external_calls" / "A.sol").as_posix()).pop() + slither = Slither(compilation) + + printer = ExternalCallPrinter(slither, None) + output = printer.output("") + + # The test is not great here, but they will soon be moved to a snapshot based system + assert output is not None From b0d525bf5a5365f1c4929b472c92ec9c7119e057 Mon Sep 17 00:00:00 2001 From: Alexis Date: Fri, 12 Apr 2024 17:07:09 +0200 Subject: [PATCH 3/5] Fix code that was breaking the test. --- slither/solc_parsing/cfg/node.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/slither/solc_parsing/cfg/node.py b/slither/solc_parsing/cfg/node.py index a9dbc83eed..d77f410cb4 100644 --- a/slither/solc_parsing/cfg/node.py +++ b/slither/solc_parsing/cfg/node.py @@ -68,9 +68,13 @@ def analyze_expressions(self, caller_context: Union["FunctionSolc", "ModifierSol self._node.calls_as_expression = find_call.result() def is_external_call(element: "Expression") -> bool: - return not isinstance(element.called, Identifier) and not isinstance( - element.called.expression.value, SolidityVariable - ) + if not isinstance(element.called, Identifier): + try: + return not isinstance(element.called.expression.value, SolidityVariable) + except AttributeError: + return True + + return False self._node.external_calls_as_expressions = list( filter(is_external_call, self._node.calls_as_expression) From 6c310204f916c2df36c6cca765fbb78a0ef42d65 Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 16 Apr 2024 10:31:00 +0200 Subject: [PATCH 4/5] Improve output generation --- slither/printers/summary/external_calls.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/slither/printers/summary/external_calls.py b/slither/printers/summary/external_calls.py index 6476418389..03203e7bf7 100644 --- a/slither/printers/summary/external_calls.py +++ b/slither/printers/summary/external_calls.py @@ -33,14 +33,14 @@ def output(self, _): for target_contract, target_function in node.high_level_calls: row = [ - f"{contract.name}.{function.name} {node.source_mapping.lines}", + f"{function.canonical_name} {node.source_mapping.to_detailed_str()}", f"{target_contract.name}.{target_function}", ] if function.all_reachable_from_functions: for source in function.all_reachable_from_functions: - chain = f"{source.contract.name}.{source.name} -> {contract.name}.{function.name}" + chain = f"{source.canonical_name} -> {function.canonical_name}" table.add_row( [ *row, From fc6c4cabbc1bcd7a6a1b1d04ebb49b8a3d51f16f Mon Sep 17 00:00:00 2001 From: Alexis Date: Tue, 16 Apr 2024 10:52:30 +0200 Subject: [PATCH 5/5] Force solc version for tests. --- tests/e2e/printers/test_data/test_external_calls/A.sol | 2 +- .../e2e/printers/test_data/test_external_calls/IERC20.sol | 2 +- tests/e2e/printers/test_printers.py | 7 +++++-- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/tests/e2e/printers/test_data/test_external_calls/A.sol b/tests/e2e/printers/test_data/test_external_calls/A.sol index 4917ce7ccd..a580e905a8 100644 --- a/tests/e2e/printers/test_data/test_external_calls/A.sol +++ b/tests/e2e/printers/test_data/test_external_calls/A.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL3 -pragma solidity ^0.8.20; +pragma solidity ^0.8.0; import "./IERC20.sol"; diff --git a/tests/e2e/printers/test_data/test_external_calls/IERC20.sol b/tests/e2e/printers/test_data/test_external_calls/IERC20.sol index bcd73a9b10..378d712356 100644 --- a/tests/e2e/printers/test_data/test_external_calls/IERC20.sol +++ b/tests/e2e/printers/test_data/test_external_calls/IERC20.sol @@ -1,7 +1,7 @@ // SPDX-License-Identifier: MIT // OpenZeppelin Contracts (last updated v5.0.0) (token/ERC20/IERC20.sol) -pragma solidity ^0.8.20; +pragma solidity ^0.8.0; /** * @dev Interface of the ERC-20 standard as defined in the ERC. diff --git a/tests/e2e/printers/test_printers.py b/tests/e2e/printers/test_printers.py index 3a6ec772b7..f7efab0c58 100644 --- a/tests/e2e/printers/test_printers.py +++ b/tests/e2e/printers/test_printers.py @@ -37,8 +37,11 @@ def test_inheritance_printer(solc_binary_path) -> None: assert counter["C -> A"] == 1 -def test_external_call_printers() -> None: - compilation = compile_all((TEST_DATA_DIR / "test_external_calls" / "A.sol").as_posix()).pop() +def test_external_call_printers(solc_binary_path) -> None: + solc_path = solc_binary_path("0.8.0") + compilation = compile_all( + (TEST_DATA_DIR / "test_external_calls" / "A.sol").as_posix(), solc=solc_path + ).pop() slither = Slither(compilation) printer = ExternalCallPrinter(slither, None)