From c10b68a1259a7d432a33bbe495e0775ca9789646 Mon Sep 17 00:00:00 2001 From: ggrieco-tob Date: Thu, 19 Nov 2020 08:53:14 -0300 Subject: [PATCH 01/15] improved ERC20 property descriptions --- .../properties/ercs/erc20/properties/transfer.py | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/slither/tools/properties/properties/ercs/erc20/properties/transfer.py b/slither/tools/properties/properties/ercs/erc20/properties/transfer.py index bea208f02..5e199a49c 100644 --- a/slither/tools/properties/properties/ercs/erc20/properties/transfer.py +++ b/slither/tools/properties/properties/ercs/erc20/properties/transfer.py @@ -57,7 +57,7 @@ ERC20_Transferable = [ ), Property( name="crytic_revert_transfer_to_zero_ERC20PropertiesTransferable()", - description="No one should be able to send tokens to the address 0x0 (transfer).", + description="Using transfer to send tokens to the address 0x0 will revert.", content=""" \t\tif (this.balanceOf(msg.sender) == 0){ \t\t\trevert(); @@ -71,7 +71,7 @@ ERC20_Transferable = [ ), Property( name="crytic_revert_transferFrom_to_zero_ERC20PropertiesTransferable()", - description="No one should be able to send tokens to the address 0x0 (transferFrom).", + description="Using transferFrom to send tokens to the address 0x0 will revert.", content=""" \t\tuint balance = this.balanceOf(msg.sender); \t\tif (balance == 0){ @@ -87,7 +87,7 @@ ERC20_Transferable = [ ), Property( name="crytic_self_transferFrom_ERC20PropertiesTransferable()", - description="Self transferFrom works.", + description="Self transfering tokens using transferFrom works as expected.", content=""" \t\tuint balance = this.balanceOf(msg.sender); \t\tbool approve_return = approve(msg.sender, balance); @@ -101,7 +101,7 @@ ERC20_Transferable = [ ), Property( name="crytic_self_transferFrom_to_other_ERC20PropertiesTransferable()", - description="transferFrom works.", + description="Transfering tokens to other address using transferFrom works as expected.", content=""" \t\tuint balance = this.balanceOf(msg.sender); \t\tbool approve_return = approve(msg.sender, balance); @@ -119,7 +119,7 @@ ERC20_Transferable = [ ), Property( name="crytic_self_transfer_ERC20PropertiesTransferable()", - description="Self transfer works.", + description="Self transfering tokens using transfer works as expected.", content=""" \t\tuint balance = this.balanceOf(msg.sender); \t\tbool transfer_return = transfer(msg.sender, balance); @@ -132,7 +132,7 @@ ERC20_Transferable = [ ), Property( name="crytic_transfer_to_other_ERC20PropertiesTransferable()", - description="transfer works.", + description="Transfering tokens to other address using transfer works as expected.", content=""" \t\tuint balance = this.balanceOf(msg.sender); \t\taddress other = crytic_user; @@ -152,7 +152,7 @@ ERC20_Transferable = [ ), Property( name="crytic_revert_transfer_to_user_ERC20PropertiesTransferable()", - description="Cannot transfer more than the balance.", + description="Transfering more tokens than the balance will revert.", content=""" \t\tuint balance = this.balanceOf(msg.sender); \t\tif (balance == (2 ** 256 - 1)) From d3ee35900b61843198f5c65595a95b4016bc12dd Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 24 Nov 2020 11:16:39 +0100 Subject: [PATCH 02/15] Add --perf flag --- slither/__main__.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/slither/__main__.py b/slither/__main__.py index b1539aa76..1dc26c940 100644 --- a/slither/__main__.py +++ b/slither/__main__.py @@ -1,13 +1,16 @@ #!/usr/bin/env python3 import argparse +import cProfile import glob import inspect import json import logging import os +import pstats import sys import traceback +from typing import Optional from pkg_resources import iter_entry_points, require @@ -489,6 +492,13 @@ def parse_args(detector_classes, printer_classes): default=defaults_flag_in_config["ignore_return_value"], ) + parser.add_argument( + "--perf", + help=argparse.SUPPRESS, + action="store_true", + default=False, + ) + # if the json is splitted in different files parser.add_argument("--splitted", help=argparse.SUPPRESS, action="store_true", default=False) @@ -598,6 +608,11 @@ def main_impl(all_detector_classes, all_printer_classes): logger.setLevel(logging.INFO) args = parse_args(all_detector_classes, all_printer_classes) + cp: Optional[cProfile.Profile] = None + if args.perf: + cp = cProfile.Profile() + cp.enable() + # Set colorization option set_colorization_enabled(not args.disable_color) @@ -773,6 +788,11 @@ def main_impl(all_detector_classes, all_printer_classes): if outputting_zip: output_to_zip(args.zip, output_error, json_results, args.zip_type) + if args.perf: + cp.disable() + stats = pstats.Stats(cp).sort_stats('cumtime') + stats.print_stats() + # Exit with the appropriate status code if output_error: sys.exit(-1) From 76f240cec36354bf53ea3a4d2cac2c60c675513f Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 24 Nov 2020 11:17:51 +0100 Subject: [PATCH 03/15] black --- slither/__main__.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/slither/__main__.py b/slither/__main__.py index 1dc26c940..78dc153bb 100644 --- a/slither/__main__.py +++ b/slither/__main__.py @@ -493,10 +493,7 @@ def parse_args(detector_classes, printer_classes): ) parser.add_argument( - "--perf", - help=argparse.SUPPRESS, - action="store_true", - default=False, + "--perf", help=argparse.SUPPRESS, action="store_true", default=False, ) # if the json is splitted in different files @@ -790,7 +787,7 @@ def main_impl(all_detector_classes, all_printer_classes): if args.perf: cp.disable() - stats = pstats.Stats(cp).sort_stats('cumtime') + stats = pstats.Stats(cp).sort_stats("cumtime") stats.print_stats() # Exit with the appropriate status code From f4e9cd5fbf241f40da6abf694df5dc264347b51b Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 24 Nov 2020 13:38:45 +0100 Subject: [PATCH 04/15] Use get_line_from_offset from crytic_compile (a75e2e816972c50a9e94d47a33394dfceda31c0a) --- slither/core/source_mapping/source_mapping.py | 66 ++++++------------- 1 file changed, 20 insertions(+), 46 deletions(-) diff --git a/slither/core/source_mapping/source_mapping.py b/slither/core/source_mapping/source_mapping.py index 9cafc8643..80156220a 100644 --- a/slither/core/source_mapping/source_mapping.py +++ b/slither/core/source_mapping/source_mapping.py @@ -1,64 +1,44 @@ import re -from typing import Dict, Union, Optional +from typing import Dict, Union, Optional, List, Tuple from slither.core.context.context import Context + + class SourceMapping(Context): def __init__(self): super().__init__() # TODO create a namedtuple for the source mapping rather than a dict self._source_mapping: Optional[Dict] = None + self._start: Optional[int] = None + self._length: Optional[int] = None + self._filename_used: Optional[str] = None + self._filename_relative: Optional[str] = None + self._filename_absolute: Optional[str] = None + self._filename_short: Optional[str] = None + self._is_dependency: Optional[bool] = None + self._lines: Optional[List[int]] = None + self._starting_column: Optional[int] = None + self._ending_column: Optional[int] = None @property def source_mapping(self) -> Optional[Dict]: return self._source_mapping @staticmethod - def _compute_line(source_code, start, length): + def _compute_line(slither, filename, start: int, length: int) -> Tuple[List[int], int, int]: """ Compute line(s) numbers and starting/ending columns from a start/end offset. All numbers start from 1. Not done in an efficient way """ - source_code = source_code.encode("utf-8") - total_length = len(source_code) - source_code = source_code.splitlines(True) - counter = 0 - i = 0 - lines = [] - starting_column = None - ending_column = None - while counter < total_length: - # Determine the length of the line, and advance the line number - line_content = source_code[i] - line_length = len(line_content) - i = i + 1 - - # Determine our column numbers. - if starting_column is None and counter + line_length > start: - starting_column = (start - counter) + 1 - if ( - starting_column is not None - and ending_column is None - and counter + line_length > start + length - ): - ending_column = ((start + length) - counter) + 1 - - # Advance the current position counter, and determine line numbers. - counter += line_length - if counter > start: - lines.append(i) + start_line, starting_column = slither.crytic_compile.get_line_from_offset(filename, start) + end_line, ending_column = slither.crytic_compile.get_line_from_offset(filename, start+length) + return list(range(start_line, end_line+1)), starting_column, ending_column - # If our advanced position for the next line is out of range, stop. - if counter > start + length: - break - - return lines, starting_column, ending_column - - @staticmethod - def _convert_source_mapping(offset: str, slither): # pylint: disable=too-many-locals + def _convert_source_mapping(self, offset: str, slither): # pylint: disable=too-many-locals """ Convert a text offset to a real offset see https://solidity.readthedocs.io/en/develop/miscellaneous.html#source-mappings @@ -85,8 +65,6 @@ class SourceMapping(Context): is_dependency = False - lines = [] - # If possible, convert the filename to its absolute/relative version if slither.crytic_compile: filenames = slither.crytic_compile.filename_lookup(filename_used) @@ -110,12 +88,8 @@ class SourceMapping(Context): else: filename = filename_used - if slither.crytic_compile and filename in slither.crytic_compile.src_content: - source_code = slither.crytic_compile.src_content[filename] - (lines, starting_column, ending_column) = SourceMapping._compute_line(source_code, s, l) - elif filename in slither.source_code: - source_code = slither.source_code[filename] - (lines, starting_column, ending_column) = SourceMapping._compute_line(source_code, s, l) + if slither.crytic_compile: + (lines, starting_column, ending_column) = self._compute_line(slither, filename, s, l) else: (lines, starting_column, ending_column) = ([], None, None) From 448374ee3095d939533cffbe84ab22f54af82ec0 Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 24 Nov 2020 14:17:40 +0100 Subject: [PATCH 05/15] black --- slither/core/source_mapping/source_mapping.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/slither/core/source_mapping/source_mapping.py b/slither/core/source_mapping/source_mapping.py index 80156220a..72bc5c0ff 100644 --- a/slither/core/source_mapping/source_mapping.py +++ b/slither/core/source_mapping/source_mapping.py @@ -4,8 +4,6 @@ from typing import Dict, Union, Optional, List, Tuple from slither.core.context.context import Context - - class SourceMapping(Context): def __init__(self): super().__init__() @@ -35,8 +33,10 @@ class SourceMapping(Context): Not done in an efficient way """ start_line, starting_column = slither.crytic_compile.get_line_from_offset(filename, start) - end_line, ending_column = slither.crytic_compile.get_line_from_offset(filename, start+length) - return list(range(start_line, end_line+1)), starting_column, ending_column + end_line, ending_column = slither.crytic_compile.get_line_from_offset( + filename, start + length + ) + return list(range(start_line, end_line + 1)), starting_column, ending_column def _convert_source_mapping(self, offset: str, slither): # pylint: disable=too-many-locals """ From eabcc49b6a765cdb253c4c017376696e16d129d3 Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 24 Nov 2020 14:57:55 +0100 Subject: [PATCH 06/15] Memoize core.function/contract properties --- slither/core/declarations/contract.py | 25 +++++++----- slither/core/declarations/function.py | 56 ++++++++++++++++++--------- 2 files changed, 54 insertions(+), 27 deletions(-) diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index aba7ffdf8..e94e2a185 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -77,6 +77,9 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public- self._is_incorrectly_parsed: bool = False + self._available_functions_as_dict: Optional[Dict[str, "Function"]] = None + self._all_functions_called: Optional[List["InternalCallType"]] = None + ################################################################################### ################################################################################### # region General's properties @@ -392,7 +395,9 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public- return list(self._functions.values()) def available_functions_as_dict(self) -> Dict[str, "Function"]: - return {f.full_name: f for f in self._functions.values() if not f.is_shadowed} + if self._available_functions_as_dict is None: + self._available_functions_as_dict = {f.full_name: f for f in self._functions.values() if not f.is_shadowed} + return self._available_functions_as_dict def add_function(self, func: "Function"): self._functions[func.canonical_name] = func @@ -731,17 +736,19 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public- list(Function): List of functions reachable from the contract Includes super, and private/internal functions not shadowed """ - all_functions = [f for f in self.functions + self.modifiers if not f.is_shadowed] # type: ignore - all_callss = [f.all_internal_calls() for f in all_functions] + [list(all_functions)] - all_calls = [item for sublist in all_callss for item in sublist] - all_calls = list(set(all_calls)) + if self._all_functions_called is None: + all_functions = [f for f in self.functions + self.modifiers if not f.is_shadowed] # type: ignore + all_callss = [f.all_internal_calls() for f in all_functions] + [list(all_functions)] + all_calls = [item for sublist in all_callss for item in sublist] + all_calls = list(set(all_calls)) - all_constructors = [c.constructor for c in self.inheritance if c.constructor] - all_constructors = list(set(all_constructors)) + all_constructors = [c.constructor for c in self.inheritance if c.constructor] + all_constructors = list(set(all_constructors)) - set_all_calls = set(all_calls + list(all_constructors)) + set_all_calls = set(all_calls + list(all_constructors)) - return [c for c in set_all_calls if isinstance(c, Function)] + self._all_functions_called = [c for c in set_all_calls if isinstance(c, Function)] + return self._all_functions_called @property def all_state_variables_written(self) -> List["StateVariable"]: diff --git a/slither/core/declarations/function.py b/slither/core/declarations/function.py index 6b6fce056..d876eb77e 100644 --- a/slither/core/declarations/function.py +++ b/slither/core/declarations/function.py @@ -197,6 +197,14 @@ class Function( self._counter_nodes = 0 + # Memoize parameters: + # TODO: identify all the memoize parameters and add a way to undo the memoization + self._full_name: Optional[str] = None + self._signature: Optional[Tuple[str, List[str], List[str]]] = None + self._solidity_signature: Optional[str] = None + self._signature_str: Optional[str] = None + self._canonical_name: Optional[str] = None + ################################################################################### ################################################################################### # region General properties @@ -244,8 +252,11 @@ class Function( str: func_name(type1,type2) Return the function signature without the return values """ - name, parameters, _ = self.signature - return ".".join(self._scope + [name]) + "(" + ",".join(parameters) + ")" + if self._full_name is None: + name, parameters, _ = self.signature + full_name = ".".join(self._scope + [name]) + "(" + ",".join(parameters) + ")" + self._full_name = full_name + return self._full_name @property def canonical_name(self) -> str: @@ -253,13 +264,15 @@ class Function( str: contract.func_name(type1,type2) Return the function signature without the return values """ - name, parameters, _ = self.signature - return ( - ".".join([self.contract_declarer.name] + self._scope + [name]) - + "(" - + ",".join(parameters) - + ")" - ) + if self._canonical_name is None: + name, parameters, _ = self.signature + self._canonical_name = ( + ".".join([self.contract_declarer.name] + self._scope + [name]) + + "(" + + ",".join(parameters) + + ")" + ) + return self._canonical_name @property def contains_assembly(self) -> bool: @@ -921,8 +934,10 @@ class Function( Contract and converted into address :return: the solidity signature """ - parameters = [self._convert_type_for_solidity_signature(x.type) for x in self.parameters] - return self.name + "(" + ",".join(parameters) + ")" + if self._solidity_signature is None: + parameters = [self._convert_type_for_solidity_signature(x.type) for x in self.parameters] + self._solidity_signature = self.name + "(" + ",".join(parameters) + ")" + return self._solidity_signature @property def signature(self) -> Tuple[str, List[str], List[str]]: @@ -930,11 +945,14 @@ class Function( (str, list(str), list(str)): Function signature as (name, list parameters type, list return values type) """ - return ( - self.name, - [str(x.type) for x in self.parameters], - [str(x.type) for x in self.returns], - ) + if self._signature is None: + signature = ( + self.name, + [str(x.type) for x in self.parameters], + [str(x.type) for x in self.returns], + ) + self._signature = signature + return self._signature @property def signature_str(self) -> str: @@ -942,8 +960,10 @@ class Function( str: func_name(type1,type2) returns (type3) Return the function signature as a str (contains the return values) """ - name, parameters, returnVars = self.signature - return name + "(" + ",".join(parameters) + ") returns(" + ",".join(returnVars) + ")" + if self._signature_str is None: + name, parameters, returnVars = self.signature + self._signature_str = name + "(" + ",".join(parameters) + ") returns(" + ",".join(returnVars) + ")" + return self._signature_str # endregion ################################################################################### From 3368b52a243504dcff84e50fb257ee937674c03f Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 24 Nov 2020 14:59:00 +0100 Subject: [PATCH 07/15] Minor --- slither/core/source_mapping/source_mapping.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/slither/core/source_mapping/source_mapping.py b/slither/core/source_mapping/source_mapping.py index 72bc5c0ff..943b82c83 100644 --- a/slither/core/source_mapping/source_mapping.py +++ b/slither/core/source_mapping/source_mapping.py @@ -9,16 +9,16 @@ class SourceMapping(Context): super().__init__() # TODO create a namedtuple for the source mapping rather than a dict self._source_mapping: Optional[Dict] = None - self._start: Optional[int] = None - self._length: Optional[int] = None - self._filename_used: Optional[str] = None - self._filename_relative: Optional[str] = None - self._filename_absolute: Optional[str] = None - self._filename_short: Optional[str] = None - self._is_dependency: Optional[bool] = None - self._lines: Optional[List[int]] = None - self._starting_column: Optional[int] = None - self._ending_column: Optional[int] = None + # self._start: Optional[int] = None + # self._length: Optional[int] = None + # self._filename_used: Optional[str] = None + # self._filename_relative: Optional[str] = None + # self._filename_absolute: Optional[str] = None + # self._filename_short: Optional[str] = None + # self._is_dependency: Optional[bool] = None + # self._lines: Optional[List[int]] = None + # self._starting_column: Optional[int] = None + # self._ending_column: Optional[int] = None @property def source_mapping(self) -> Optional[Dict]: From 72c0a1b8d4061f17d721b1acd667f6850717a506 Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 24 Nov 2020 14:59:00 +0100 Subject: [PATCH 08/15] Minor --- slither/core/source_mapping/source_mapping.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/slither/core/source_mapping/source_mapping.py b/slither/core/source_mapping/source_mapping.py index 72bc5c0ff..943b82c83 100644 --- a/slither/core/source_mapping/source_mapping.py +++ b/slither/core/source_mapping/source_mapping.py @@ -9,16 +9,16 @@ class SourceMapping(Context): super().__init__() # TODO create a namedtuple for the source mapping rather than a dict self._source_mapping: Optional[Dict] = None - self._start: Optional[int] = None - self._length: Optional[int] = None - self._filename_used: Optional[str] = None - self._filename_relative: Optional[str] = None - self._filename_absolute: Optional[str] = None - self._filename_short: Optional[str] = None - self._is_dependency: Optional[bool] = None - self._lines: Optional[List[int]] = None - self._starting_column: Optional[int] = None - self._ending_column: Optional[int] = None + # self._start: Optional[int] = None + # self._length: Optional[int] = None + # self._filename_used: Optional[str] = None + # self._filename_relative: Optional[str] = None + # self._filename_absolute: Optional[str] = None + # self._filename_short: Optional[str] = None + # self._is_dependency: Optional[bool] = None + # self._lines: Optional[List[int]] = None + # self._starting_column: Optional[int] = None + # self._ending_column: Optional[int] = None @property def source_mapping(self) -> Optional[Dict]: From 19ad2e94508fc0846771a789f061ae831383567f Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 24 Nov 2020 15:25:22 +0100 Subject: [PATCH 09/15] Additional memoization --- slither/core/declarations/contract.py | 14 +------------- slither/core/declarations/function.py | 14 +++++++++----- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index e94e2a185..f3ec9ffdf 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -71,7 +71,7 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public- self._is_upgradeable: Optional[bool] = None self._is_upgradeable_proxy: Optional[bool] = None - self._is_top_level = False + self.is_top_level = False # heavily used, so no @property self._initial_state_variables: List["StateVariable"] = [] # ssa @@ -1207,18 +1207,6 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public- for func in self.functions + self.modifiers: func.fix_phi(last_state_variables_instances, initial_state_variables_instances) - @property - def is_top_level(self) -> bool: - """ - The "TopLevel" contract is used to hold structures and enums defined at the top level - ie. structures and enums that are represented outside of any contract - :return: - """ - return self._is_top_level - - @is_top_level.setter - def is_top_level(self, t: bool): - self._is_top_level = t # endregion ################################################################################### diff --git a/slither/core/declarations/function.py b/slither/core/declarations/function.py index d876eb77e..e871d60f4 100644 --- a/slither/core/declarations/function.py +++ b/slither/core/declarations/function.py @@ -204,6 +204,7 @@ class Function( self._solidity_signature: Optional[str] = None self._signature_str: Optional[str] = None self._canonical_name: Optional[str] = None + self._is_protected: Optional[bool] = None ################################################################################### ################################################################################### @@ -1427,11 +1428,14 @@ class Function( (bool) """ - if self.is_constructor: - return True - conditional_vars = self.all_conditional_solidity_variables_read(include_loop=False) - args_vars = self.all_solidity_variables_used_as_args() - return SolidityVariableComposed("msg.sender") in conditional_vars + args_vars + if self._is_protected is None: + if self.is_constructor: + self._is_protected = True + return True + conditional_vars = self.all_conditional_solidity_variables_read(include_loop=False) + args_vars = self.all_solidity_variables_used_as_args() + self._is_protected = SolidityVariableComposed("msg.sender") in conditional_vars + args_vars + return self._is_protected # endregion ################################################################################### From 8d943dc764bba9ad91e0657b08e973701d583869 Mon Sep 17 00:00:00 2001 From: Josselin Date: Tue, 24 Nov 2020 16:16:54 +0100 Subject: [PATCH 10/15] black --- slither/core/declarations/contract.py | 7 ++++--- slither/core/declarations/function.py | 16 +++++++++++----- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/slither/core/declarations/contract.py b/slither/core/declarations/contract.py index f3ec9ffdf..2b15e29f5 100644 --- a/slither/core/declarations/contract.py +++ b/slither/core/declarations/contract.py @@ -71,7 +71,7 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public- self._is_upgradeable: Optional[bool] = None self._is_upgradeable_proxy: Optional[bool] = None - self.is_top_level = False # heavily used, so no @property + self.is_top_level = False # heavily used, so no @property self._initial_state_variables: List["StateVariable"] = [] # ssa @@ -396,7 +396,9 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public- def available_functions_as_dict(self) -> Dict[str, "Function"]: if self._available_functions_as_dict is None: - self._available_functions_as_dict = {f.full_name: f for f in self._functions.values() if not f.is_shadowed} + self._available_functions_as_dict = { + f.full_name: f for f in self._functions.values() if not f.is_shadowed + } return self._available_functions_as_dict def add_function(self, func: "Function"): @@ -1207,7 +1209,6 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public- for func in self.functions + self.modifiers: func.fix_phi(last_state_variables_instances, initial_state_variables_instances) - # endregion ################################################################################### ################################################################################### diff --git a/slither/core/declarations/function.py b/slither/core/declarations/function.py index e871d60f4..f71899a2b 100644 --- a/slither/core/declarations/function.py +++ b/slither/core/declarations/function.py @@ -936,8 +936,10 @@ class Function( :return: the solidity signature """ if self._solidity_signature is None: - parameters = [self._convert_type_for_solidity_signature(x.type) for x in self.parameters] - self._solidity_signature = self.name + "(" + ",".join(parameters) + ")" + parameters = [ + self._convert_type_for_solidity_signature(x.type) for x in self.parameters + ] + self._solidity_signature = self.name + "(" + ",".join(parameters) + ")" return self._solidity_signature @property @@ -947,7 +949,7 @@ class Function( (name, list parameters type, list return values type) """ if self._signature is None: - signature = ( + signature = ( self.name, [str(x.type) for x in self.parameters], [str(x.type) for x in self.returns], @@ -963,7 +965,9 @@ class Function( """ if self._signature_str is None: name, parameters, returnVars = self.signature - self._signature_str = name + "(" + ",".join(parameters) + ") returns(" + ",".join(returnVars) + ")" + self._signature_str = ( + name + "(" + ",".join(parameters) + ") returns(" + ",".join(returnVars) + ")" + ) return self._signature_str # endregion @@ -1434,7 +1438,9 @@ class Function( return True conditional_vars = self.all_conditional_solidity_variables_read(include_loop=False) args_vars = self.all_solidity_variables_used_as_args() - self._is_protected = SolidityVariableComposed("msg.sender") in conditional_vars + args_vars + self._is_protected = ( + SolidityVariableComposed("msg.sender") in conditional_vars + args_vars + ) return self._is_protected # endregion From 08514c753d795d033db663e88a98c59134fb1d2c Mon Sep 17 00:00:00 2001 From: Feist Josselin Date: Tue, 24 Nov 2020 19:35:32 +0100 Subject: [PATCH 11/15] Update test_ast_parsing.py --- tests/test_ast_parsing.py | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/tests/test_ast_parsing.py b/tests/test_ast_parsing.py index 47f7424e0..316f1614d 100644 --- a/tests/test_ast_parsing.py +++ b/tests/test_ast_parsing.py @@ -84,6 +84,8 @@ XFAIL = [ "function_0.7.3_compact", "function_0.7.4_legacy", "function_0.7.4_compact", + "function_0.7.5_legacy", + "function_0.7.5_compact", "import_0.4.0_legacy", "import_0.4.1_legacy", "import_0.4.2_legacy", @@ -198,6 +200,8 @@ XFAIL = [ "import_0.7.3_compact", "import_0.7.4_legacy", "import_0.7.4_compact", + "import_0.7.5_legacy", + "import_0.7.5_compact", "indexrangeaccess_0.6.1_legacy", "indexrangeaccess_0.6.2_legacy", "indexrangeaccess_0.6.3_legacy", @@ -215,6 +219,7 @@ XFAIL = [ "indexrangeaccess_0.7.2_legacy", "indexrangeaccess_0.7.3_legacy", "indexrangeaccess_0.7.4_legacy", + "indexrangeaccess_0.7.5_legacy", "literal_0.7.0_legacy", "literal_0.7.0_compact", "literal_0.7.1_legacy", @@ -225,6 +230,8 @@ XFAIL = [ "literal_0.7.3_compact", "literal_0.7.4_legacy", "literal_0.7.4_compact", + "literal_0.7.5_legacy", + "literal_0.7.5_compact", "memberaccess_0.6.8_legacy", "memberaccess_0.6.9_legacy", "memberaccess_0.6.10_legacy", @@ -251,6 +258,7 @@ XFAIL = [ "struct_0.7.2_legacy", "struct_0.7.3_legacy", "struct_0.7.4_legacy", + "struct_0.7.5_legacy", "trycatch_0.6.0_legacy", "trycatch_0.6.1_legacy", "trycatch_0.6.2_legacy", @@ -269,6 +277,7 @@ XFAIL = [ "trycatch_0.7.2_legacy", "trycatch_0.7.3_legacy", "trycatch_0.7.4_legacy", + "trycatch_0.7.5_legacy", "variable_0.6.5_legacy", "variable_0.6.5_compact", "variable_0.6.6_legacy", @@ -367,6 +376,7 @@ XFAIL = [ "variabledeclaration_0.7.2_legacy", "variabledeclaration_0.7.3_legacy", "variabledeclaration_0.7.4_legacy", + "variabledeclaration_0.7.5_legacy", "yul_0.6.0_compact", "yul_0.6.1_compact", "yul_0.6.2_compact", @@ -385,6 +395,7 @@ XFAIL = [ "yul_0.7.2_compact", "yul_0.7.3_compact", "yul_0.7.4_compact", + "yul_0.7.5_compact", ] From 0442afe79be0e8cbac34b268224e0057bc70cbc7 Mon Sep 17 00:00:00 2001 From: Josselin Date: Wed, 25 Nov 2020 12:32:56 +0100 Subject: [PATCH 12/15] Optimize immediate dominator computation --- slither/core/dominators/utils.py | 57 +++++++++++++++++++++----------- 1 file changed, 37 insertions(+), 20 deletions(-) diff --git a/slither/core/dominators/utils.py b/slither/core/dominators/utils.py index 2279118e7..837fe46ea 100644 --- a/slither/core/dominators/utils.py +++ b/slither/core/dominators/utils.py @@ -15,18 +15,9 @@ def intersection_predecessor(node: "Node"): return ret -def compute_dominators(nodes: List["Node"]): - """ - Naive implementation of Cooper, Harvey, Kennedy algo - See 'A Simple,Fast Dominance Algorithm' - - Compute strict domniators - """ +def _compute_dominators(nodes: List["Node"]): changed = True - for n in nodes: - n.dominators = set(nodes) - while changed: changed = False @@ -36,20 +27,30 @@ def compute_dominators(nodes: List["Node"]): node.dominators = new_set changed = True - # compute immediate dominator + +def _compute_immediate_dominators(nodes: List["Node"]): for node in nodes: idom_candidates = set(node.dominators) idom_candidates.remove(node) - for dominator in node.dominators: - if dominator != node: - # pylint: disable=expression-not-assigned - [ - idom_candidates.remove(d) - for d in dominator.dominators - if d in idom_candidates and d != dominator - ] - + if len(idom_candidates) == 1: + idom = idom_candidates.pop() + node.immediate_dominator = idom + idom.dominator_successors.add(node) + continue + + # all_dominators contain all the dominators of all the node's dominators + # But self inclusion is removed + # The idom is then the only node that in idom_candidate that is not in all_dominators + all_dominators = set() + for d in idom_candidates: + # optimization: if a node is already in all_dominators, then + # its dominators are already in too + if d in all_dominators: + continue + all_dominators |= d.dominators - {d} + + idom_candidates = all_dominators.symmetric_difference(idom_candidates) assert len(idom_candidates) <= 1 if idom_candidates: idom = idom_candidates.pop() @@ -57,6 +58,22 @@ def compute_dominators(nodes: List["Node"]): idom.dominator_successors.add(node) +def compute_dominators(nodes: List["Node"]): + """ + Naive implementation of Cooper, Harvey, Kennedy algo + See 'A Simple,Fast Dominance Algorithm' + + Compute strict domniators + """ + + for n in nodes: + n.dominators = set(nodes) + + _compute_dominators(nodes) + + _compute_immediate_dominators(nodes) + + def compute_dominance_frontier(nodes: List["Node"]): """ Naive implementation of Cooper, Harvey, Kennedy algo From d2bf6ce06438af9af1fdf1c0182dd64e8b4b3b46 Mon Sep 17 00:00:00 2001 From: Josselin Date: Wed, 25 Nov 2020 12:53:29 +0100 Subject: [PATCH 13/15] Remove recursion in divide-before-multiplity --- .../statements/divide_before_multiply.py | 124 +++++++++--------- 1 file changed, 64 insertions(+), 60 deletions(-) diff --git a/slither/detectors/statements/divide_before_multiply.py b/slither/detectors/statements/divide_before_multiply.py index f1afbcd36..b8fdd12e6 100644 --- a/slither/detectors/statements/divide_before_multiply.py +++ b/slither/detectors/statements/divide_before_multiply.py @@ -50,37 +50,13 @@ def is_assert(node): return False -class DivideBeforeMultiply(AbstractDetector): - """ - Divide before multiply - """ - - ARGUMENT = "divide-before-multiply" - HELP = "Imprecise arithmetic operations order" - IMPACT = DetectorClassification.MEDIUM - CONFIDENCE = DetectorClassification.MEDIUM - - WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply" - - WIKI_TITLE = "Divide before multiply" - WIKI_DESCRIPTION = """Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision.""" - WIKI_EXPLOIT_SCENARIO = """ -```solidity -contract A { - function f(uint n) public { - coins = (oldSupply / n) * interest; - } -} -``` -If `n` is greater than `oldSupply`, `coins` will be zero. For example, with `oldSupply = 5; n = 10, interest = 2`, coins will be zero. -If `(oldSupply * interest / n)` was used, `coins` would have been `1`. -In general, it's usually a good idea to re-arrange arithmetic to perform multiplication before division, unless the limit of a smaller type makes this dangerous.""" - - WIKI_RECOMMENDATION = """Consider ordering multiplication before division.""" +def _explore(to_explore, f_results, divisions): # pylint: disable=too-many-branches + explored = set() + while to_explore: # pylint: disable=too-many-nested-blocks + node = to_explore.pop() - def _explore(self, node, explored, f_results, divisions): # pylint: disable=too-many-branches if node in explored: - return + continue explored.add(node) equality_found = False @@ -121,47 +97,75 @@ In general, it's usually a good idea to re-arrange arithmetic to perform multipl # We do not track the case where the multiplication is done in a require() or assert() # Which also contains a ==, to prevent FP due to the form # assert(a == b * c + a % b) - if is_assert(node) and equality_found: - pass - else: + if not (is_assert(node) and equality_found): f_results.append(node_results) for son in node.sons: - self._explore(son, explored, f_results, divisions) + to_explore.add(son) - def detect_divide_before_multiply(self, contract): - """ - Detects and returns all nodes with multiplications of division results. - :param contract: Contract to detect assignment within. - :return: A list of nodes with multiplications of divisions. - """ - # Create our result set. - # List of tuple (function -> list(list(nodes))) - # Each list(nodes) of the list is one bug instances - # Each node in the list(nodes) is involved in the bug - results = [] +def detect_divide_before_multiply(contract): + """ + Detects and returns all nodes with multiplications of division results. + :param contract: Contract to detect assignment within. + :return: A list of nodes with multiplications of divisions. + """ - # Loop for each function and modifier. - for function in contract.functions_declared: - if not function.entry_point: - continue + # Create our result set. + # List of tuple (function -> list(list(nodes))) + # Each list(nodes) of the list is one bug instances + # Each node in the list(nodes) is involved in the bug + results = [] - # List of list(nodes) - # Each list(nodes) is one bug instances - f_results = [] + # Loop for each function and modifier. + for function in contract.functions_declared: + if not function.entry_point: + continue - # lvalue -> node - # track all the division results (and the assignment of the division results) - divisions = defaultdict(list) + # List of list(nodes) + # Each list(nodes) is one bug instances + f_results = [] - self._explore(function.entry_point, set(), f_results, divisions) + # lvalue -> node + # track all the division results (and the assignment of the division results) + divisions = defaultdict(list) - for f_result in f_results: - results.append((function, f_result)) + _explore({function.entry_point}, f_results, divisions) - # Return the resulting set of nodes with divisions before multiplications - return results + for f_result in f_results: + results.append((function, f_result)) + + # Return the resulting set of nodes with divisions before multiplications + return results + + +class DivideBeforeMultiply(AbstractDetector): + """ + Divide before multiply + """ + + ARGUMENT = "divide-before-multiply" + HELP = "Imprecise arithmetic operations order" + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#divide-before-multiply" + + WIKI_TITLE = "Divide before multiply" + WIKI_DESCRIPTION = """Solidity integer division might truncate. As a result, performing multiplication before division can sometimes avoid loss of precision.""" + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract A { + function f(uint n) public { + coins = (oldSupply / n) * interest; + } +} +``` +If `n` is greater than `oldSupply`, `coins` will be zero. For example, with `oldSupply = 5; n = 10, interest = 2`, coins will be zero. +If `(oldSupply * interest / n)` was used, `coins` would have been `1`. +In general, it's usually a good idea to re-arrange arithmetic to perform multiplication before division, unless the limit of a smaller type makes this dangerous.""" + + WIKI_RECOMMENDATION = """Consider ordering multiplication before division.""" def _detect(self): """ @@ -169,7 +173,7 @@ In general, it's usually a good idea to re-arrange arithmetic to perform multipl """ results = [] for contract in self.contracts: - divisions_before_multiplications = self.detect_divide_before_multiply(contract) + divisions_before_multiplications = detect_divide_before_multiply(contract) if divisions_before_multiplications: for (func, nodes) in divisions_before_multiplications: From 068d871dfec0c6e4419c18a84fbb8187bec6a9df Mon Sep 17 00:00:00 2001 From: Josselin Date: Wed, 25 Nov 2020 16:39:59 +0100 Subject: [PATCH 14/15] Optimize transitive_close_dependencies --- .../data_dependency/data_dependency.py | 28 +++++++++++-------- 1 file changed, 17 insertions(+), 11 deletions(-) diff --git a/slither/analyses/data_dependency/data_dependency.py b/slither/analyses/data_dependency/data_dependency.py index af6a860d3..9ddcd9c4d 100644 --- a/slither/analyses/data_dependency/data_dependency.py +++ b/slither/analyses/data_dependency/data_dependency.py @@ -1,6 +1,7 @@ """ Compute the data depenency between all the SSA variables """ +from collections import defaultdict from typing import Union, Set, Dict from slither.core.declarations import ( @@ -297,18 +298,23 @@ def propagate_function(contract, function, context_key, context_key_non_ssa): def transitive_close_dependencies(context, context_key, context_key_non_ssa): # transitive closure changed = True - while changed: # pylint: disable=too-many-nested-blocks + keys = context.context[context_key].keys() + while changed: changed = False - # Need to create new set() as its changed during iteration - data_depencencies = {k: set(values) for k, values in context.context[context_key].items()} - for key, items in data_depencencies.items(): - for item in items: - if item in data_depencencies: - additional_items = context.context[context_key][item] - for additional_item in additional_items: - if not additional_item in items and additional_item != key: - changed = True - context.context[context_key][key].add(additional_item) + to_add = defaultdict(set) + [ # pylint: disable=expression-not-assigned + [ + to_add[key].update(context.context[context_key][item] - {key} - items) + for item in items & keys + ] + for key, items in context.context[context_key].items() + ] + for k, v in to_add.items(): + # Because we dont have any check on the update operation + # We might update an empty set with an empty set + if v: + changed = True + context.context[context_key][k] |= v context.context[context_key_non_ssa] = convert_to_non_ssa(context.context[context_key]) From 999fd2994c9e00e950b5d628708b922396a8f1cb Mon Sep 17 00:00:00 2001 From: Josselin Date: Fri, 4 Dec 2020 11:20:51 +0100 Subject: [PATCH 15/15] Fix deterministic output for multiple detectors (fix #486) --- slither/detectors/functions/arbitrary_send.py | 4 ++++ slither/detectors/functions/external_function.py | 3 +++ slither/detectors/operations/block_timestamp.py | 4 ++++ slither/detectors/operations/low_level_calls.py | 3 +++ slither/detectors/statements/assembly.py | 3 +++ slither/detectors/statements/divide_before_multiply.py | 3 +++ 6 files changed, 20 insertions(+) diff --git a/slither/detectors/functions/arbitrary_send.py b/slither/detectors/functions/arbitrary_send.py index 15e0f4254..ac5422743 100644 --- a/slither/detectors/functions/arbitrary_send.py +++ b/slither/detectors/functions/arbitrary_send.py @@ -117,6 +117,10 @@ Bob calls `setDestination` and `withdraw`. As a result he withdraws the contract info = [func, " sends eth to arbitrary user\n"] info += ["\tDangerous calls:\n"] + + # sort the nodes to get deterministic results + nodes.sort(key=lambda x: x.node_id) + for node in nodes: info += ["\t- ", node, "\n"] diff --git a/slither/detectors/functions/external_function.py b/slither/detectors/functions/external_function.py index a6d11af20..d68b0c31f 100644 --- a/slither/detectors/functions/external_function.py +++ b/slither/detectors/functions/external_function.py @@ -195,6 +195,9 @@ class ExternalFunction(AbstractDetector): if f.visibility == "public" and f.contract == f.contract_declarer ] if all_function_definitions: + all_function_definitions = sorted( + all_function_definitions, key=lambda x: x.canonical_name + ) function_definition = all_function_definitions[0] all_function_definitions = all_function_definitions[1:] diff --git a/slither/detectors/operations/block_timestamp.py b/slither/detectors/operations/block_timestamp.py index e733a4b40..54d091d0d 100644 --- a/slither/detectors/operations/block_timestamp.py +++ b/slither/detectors/operations/block_timestamp.py @@ -78,6 +78,10 @@ class Timestamp(AbstractDetector): info = [func, " uses timestamp for comparisons\n"] info += ["\tDangerous comparisons:\n"] + + # sort the nodes to get deterministic results + nodes.sort(key=lambda x: x.node_id) + for node in nodes: info += ["\t- ", node, "\n"] diff --git a/slither/detectors/operations/low_level_calls.py b/slither/detectors/operations/low_level_calls.py index ed00fc48b..7e0f45e34 100644 --- a/slither/detectors/operations/low_level_calls.py +++ b/slither/detectors/operations/low_level_calls.py @@ -48,6 +48,9 @@ class LowLevelCalls(AbstractDetector): for func, nodes in values: info = ["Low level call in ", func, ":\n"] + # sort the nodes to get deterministic results + nodes.sort(key=lambda x: x.node_id) + for node in nodes: info += ["\t- ", node, "\n"] diff --git a/slither/detectors/statements/assembly.py b/slither/detectors/statements/assembly.py index be1f2fa2d..3a554e380 100644 --- a/slither/detectors/statements/assembly.py +++ b/slither/detectors/statements/assembly.py @@ -50,6 +50,9 @@ class Assembly(AbstractDetector): for func, nodes in values: info = [func, " uses assembly\n"] + # sort the nodes to get deterministic results + nodes.sort(key=lambda x: x.node_id) + for node in nodes: info += ["\t- ", node, "\n"] diff --git a/slither/detectors/statements/divide_before_multiply.py b/slither/detectors/statements/divide_before_multiply.py index b8fdd12e6..f504497ed 100644 --- a/slither/detectors/statements/divide_before_multiply.py +++ b/slither/detectors/statements/divide_before_multiply.py @@ -182,6 +182,9 @@ In general, it's usually a good idea to re-arrange arithmetic to perform multipl " performs a multiplication on the result of a division:\n", ] + # sort the nodes to get deterministic results + nodes.sort(key=lambda x: x.node_id) + for node in nodes: info += ["\t-", node, "\n"]