-
Notifications
You must be signed in to change notification settings - Fork 970
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds a new printer for external calls. #2420
Open
DarkaMaul
wants to merge
7
commits into
dev
Choose a base branch
from
dev-external-calls
base: dev
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
b9d68be
Adds a new printer for external calls.
DarkaMaul 227aec5
Add a simple test for the printer
DarkaMaul b0d525b
Fix code that was breaking the test.
DarkaMaul 6c31020
Improve output generation
DarkaMaul fc6c4ca
Force solc version for tests.
DarkaMaul 56baa06
Merge branch 'dev' into dev-external-calls
DarkaMaul 5459ee6
Merge branch 'dev' into dev-external-calls
DarkaMaul File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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"{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.canonical_name} -> {function.canonical_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 |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
// SPDX-License-Identifier: GPL3 | ||
pragma solidity ^0.8.0; | ||
|
||
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(); | ||
} | ||
} |
79 changes: 79 additions & 0 deletions
79
tests/e2e/printers/test_data/test_external_calls/IERC20.sol
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
// SPDX-License-Identifier: MIT | ||
// OpenZeppelin Contracts (last updated v5.0.0) (token/ERC20/IERC20.sol) | ||
|
||
pragma solidity ^0.8.0; | ||
|
||
/** | ||
* @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); | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be able to be
isinstance(element.called.value, SolidityVariable)
and not require a try/exceptThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was my initial try but the test failed on the CI :
https://github.com/crytic/slither/actions/runs/8649176961/job/23714782903