From 77bf3e576e9ba80b900f98fbcb0eb4188e6c9e82 Mon Sep 17 00:00:00 2001 From: Feist Josselin Date: Fri, 24 Feb 2023 20:20:49 +0100 Subject: [PATCH] More fixes --- .../statements/delegatecall_in_loop.py | 2 +- slither/printers/functions/dominator.py | 3 +- slither/printers/guidance/echidna.py | 6 +- slither/printers/summary/data_depenency.py | 12 +- slither/printers/summary/declaration.py | 3 +- slither/slithir/convert.py | 122 +++++++----------- slither/slithir/operations/delete.py | 2 +- .../slithir/operations/return_operation.py | 25 ++-- slither/slithir/utils/utils.py | 6 +- slither/utils/myprettytable.py | 4 +- 10 files changed, 85 insertions(+), 100 deletions(-) diff --git a/slither/detectors/statements/delegatecall_in_loop.py b/slither/detectors/statements/delegatecall_in_loop.py index d97466edf..bdcf5dcae 100644 --- a/slither/detectors/statements/delegatecall_in_loop.py +++ b/slither/detectors/statements/delegatecall_in_loop.py @@ -42,7 +42,7 @@ def delegatecall_in_loop( and ir.function_name == "delegatecall" ): results.append(ir.node) - if isinstance(ir, (InternalCall)): + if isinstance(ir, (InternalCall)) and ir.function: delegatecall_in_loop(ir.function.entry_point, in_loop_counter, visited, results) for son in node.sons: diff --git a/slither/printers/functions/dominator.py b/slither/printers/functions/dominator.py index f618fd5db..1b32498f9 100644 --- a/slither/printers/functions/dominator.py +++ b/slither/printers/functions/dominator.py @@ -1,4 +1,5 @@ from slither.printers.abstract_printer import AbstractPrinter +from slither.utils.output import Output class Dominator(AbstractPrinter): @@ -8,7 +9,7 @@ class Dominator(AbstractPrinter): WIKI = "https://github.com/trailofbits/slither/wiki/Printer-documentation#dominator" - def output(self, filename): + def output(self, filename: str) -> Output: """ _filename is not used Args: diff --git a/slither/printers/guidance/echidna.py b/slither/printers/guidance/echidna.py index 166fa48f5..3a555562f 100644 --- a/slither/printers/guidance/echidna.py +++ b/slither/printers/guidance/echidna.py @@ -79,7 +79,7 @@ def _is_constant(f: Function) -> bool: # pylint: disable=too-many-branches :return: """ if f.view or f.pure: - if not f.contract.compilation_unit.solc_version.startswith("0.4"): + if not f.compilation_unit.solc_version.startswith("0.4"): return True if f.payable: return False @@ -102,11 +102,11 @@ def _is_constant(f: Function) -> bool: # pylint: disable=too-many-branches if isinstance(ir, HighLevelCall): if isinstance(ir.function, Variable) or ir.function.view or ir.function.pure: # External call to constant functions are ensured to be constant only for solidity >= 0.5 - if f.contract.compilation_unit.solc_version.startswith("0.4"): + if f.compilation_unit.solc_version.startswith("0.4"): return False else: return False - if isinstance(ir, InternalCall): + if isinstance(ir, InternalCall) and ir.function: # Storage write are not properly handled by all_state_variables_written if any(parameter.is_storage for parameter in ir.function.parameters): return False diff --git a/slither/printers/summary/data_depenency.py b/slither/printers/summary/data_depenency.py index 41659a299..f1c0dc8d5 100644 --- a/slither/printers/summary/data_depenency.py +++ b/slither/printers/summary/data_depenency.py @@ -1,19 +1,22 @@ """ Module printing summary of the contract """ +from typing import List +from slither.core.declarations import Contract from slither.printers.abstract_printer import AbstractPrinter -from slither.analyses.data_dependency.data_dependency import get_dependencies +from slither.analyses.data_dependency.data_dependency import get_dependencies, SUPPORTED_TYPES from slither.slithir.variables import TemporaryVariable, ReferenceVariable from slither.utils.myprettytable import MyPrettyTable +from slither.utils.output import Output -def _get(v, c): +def _get(v: SUPPORTED_TYPES, c: Contract) -> List[str]: return list( { d.name for d in get_dependencies(v, c) - if not isinstance(d, (TemporaryVariable, ReferenceVariable)) + if not isinstance(d, (TemporaryVariable, ReferenceVariable)) and d.name } ) @@ -25,7 +28,7 @@ class DataDependency(AbstractPrinter): WIKI = "https://github.com/trailofbits/slither/wiki/Printer-documentation#data-dependencies" - def output(self, _filename): + def output(self, _filename: str) -> Output: """ _filename is not used Args: @@ -42,6 +45,7 @@ class DataDependency(AbstractPrinter): txt += f"\nContract {c.name}\n" table = MyPrettyTable(["Variable", "Dependencies"]) for v in c.state_variables: + assert v.name table.add_row([v.name, sorted(_get(v, c))]) txt += str(table) diff --git a/slither/printers/summary/declaration.py b/slither/printers/summary/declaration.py index 5888a1f00..9266d5580 100644 --- a/slither/printers/summary/declaration.py +++ b/slither/printers/summary/declaration.py @@ -1,4 +1,5 @@ from slither.printers.abstract_printer import AbstractPrinter +from slither.utils.output import Output from slither.utils.source_mapping import get_definition, get_implementation, get_references @@ -8,7 +9,7 @@ class Declaration(AbstractPrinter): WIKI = "TODO" - def output(self, _filename): + def output(self, _filename: str) -> Output: """ _filename is not used Args: diff --git a/slither/slithir/convert.py b/slither/slithir/convert.py index cc47ea913..e05526d86 100644 --- a/slither/slithir/convert.py +++ b/slither/slithir/convert.py @@ -1,6 +1,6 @@ import logging from pathlib import Path -from typing import Any, List, TYPE_CHECKING, Union, Optional +from typing import Any, List, TYPE_CHECKING, Union, Optional, Dict # pylint: disable= too-many-lines,import-outside-toplevel,too-many-branches,too-many-statements,too-many-nested-blocks from slither.core.declarations import ( @@ -13,12 +13,14 @@ from slither.core.declarations import ( SolidityVariableComposed, Structure, ) +from slither.core.declarations.contract import USING_FOR_KEY, USING_FOR_ITEM from slither.core.declarations.custom_error import CustomError from slither.core.declarations.function_contract import FunctionContract from slither.core.declarations.function_top_level import FunctionTopLevel from slither.core.declarations.solidity_import_placeholder import SolidityImportPlaceHolder from slither.core.declarations.solidity_variables import SolidityCustomRevert from slither.core.expressions import Identifier, Literal +from slither.core.expressions.expression import Expression from slither.core.solidity_types import ( ArrayType, ElementaryType, @@ -83,28 +85,6 @@ from slither.slithir.variables import TupleVariable from slither.utils.function import get_function_id from slither.utils.type import export_nested_types_from_variable from slither.visitors.slithir.expression_to_slithir import ExpressionToSlithIR -import slither.core.declarations.contract -import slither.core.declarations.function -import slither.core.solidity_types.elementary_type -import slither.core.solidity_types.function_type -import slither.core.solidity_types.user_defined_type -import slither.slithir.operations.assignment -import slither.slithir.operations.binary -import slither.slithir.operations.call -import slither.slithir.operations.high_level_call -import slither.slithir.operations.index -import slither.slithir.operations.init_array -import slither.slithir.operations.internal_call -import slither.slithir.operations.length -import slither.slithir.operations.library_call -import slither.slithir.operations.low_level_call -import slither.slithir.operations.member -import slither.slithir.operations.operation -import slither.slithir.operations.send -import slither.slithir.operations.solidity_call -import slither.slithir.operations.transfer -import slither.slithir.variables.temporary -from slither.core.expressions.expression import Expression if TYPE_CHECKING: from slither.core.cfg.node import Node @@ -112,7 +92,7 @@ if TYPE_CHECKING: logger = logging.getLogger("ConvertToIR") -def convert_expression(expression: Expression, node: "Node") -> List[Any]: +def convert_expression(expression: Expression, node: "Node") -> List[Operation]: # handle standlone expression # such as return true; from slither.core.cfg.node import NodeType @@ -122,8 +102,7 @@ def convert_expression(expression: Expression, node: "Node") -> List[Any]: cond = Condition(cst) cond.set_expression(expression) cond.set_node(node) - result = [cond] - return result + return [cond] if isinstance(expression, Identifier) and node.type in [ NodeType.IF, NodeType.IFLOOP, @@ -131,8 +110,7 @@ def convert_expression(expression: Expression, node: "Node") -> List[Any]: cond = Condition(expression.value) cond.set_expression(expression) cond.set_node(node) - result = [cond] - return result + return [cond] visitor = ExpressionToSlithIR(expression, node) result = visitor.result() @@ -141,15 +119,17 @@ def convert_expression(expression: Expression, node: "Node") -> List[Any]: if result: if node.type in [NodeType.IF, NodeType.IFLOOP]: - assert isinstance(result[-1], (OperationWithLValue)) - cond = Condition(result[-1].lvalue) + prev = result[-1] + assert isinstance(prev, (OperationWithLValue)) and prev.lvalue + cond = Condition(prev.lvalue) cond.set_expression(expression) cond.set_node(node) result.append(cond) elif node.type == NodeType.RETURN: # May return None - if isinstance(result[-1], (OperationWithLValue)): - r = Return(result[-1].lvalue) + prev = result[-1] + if isinstance(prev, (OperationWithLValue)): + r = Return(prev.lvalue) r.set_expression(expression) r.set_node(node) result.append(r) @@ -273,7 +253,7 @@ def _find_function_from_parameter( type_args += ["string"] not_found = True - candidates_kept = [] + candidates_kept: List[Function] = [] for type_arg in type_args: if not not_found: break @@ -336,7 +316,7 @@ def integrate_value_gas(result: List[Operation]) -> List[Operation]: # Find all the assignments assigments = {} for i in result: - if isinstance(i, OperationWithLValue): + if isinstance(i, OperationWithLValue) and i.lvalue: assigments[i.lvalue.name] = i if isinstance(i, TmpCall): if isinstance(i.called, Variable) and i.called.name in assigments: @@ -350,20 +330,25 @@ def integrate_value_gas(result: List[Operation]) -> List[Operation]: for idx, ins in enumerate(result): # value can be shadowed, so we check that the prev ins # is an Argument - if is_value(ins) and isinstance(result[idx - 1], Argument): + if idx == 0: + continue + prev_ins = result[idx - 1] + if is_value(ins) and isinstance(prev_ins, Argument): was_changed = True - result[idx - 1].set_type(ArgumentType.VALUE) - result[idx - 1].call_id = ins.ori.variable_left.name - calls.append(ins.ori.variable_left) + prev_ins.set_type(ArgumentType.VALUE) + # Types checked by is_value + prev_ins.call_id = ins.ori.variable_left.name # type: ignore + calls.append(ins.ori.variable_left) # type: ignore to_remove.append(ins) - variable_to_replace[ins.lvalue.name] = ins.ori.variable_left - elif is_gas(ins) and isinstance(result[idx - 1], Argument): + variable_to_replace[ins.lvalue.name] = ins.ori.variable_left # type: ignore + elif is_gas(ins) and isinstance(prev_ins, Argument): was_changed = True - result[idx - 1].set_type(ArgumentType.GAS) - result[idx - 1].call_id = ins.ori.variable_left.name - calls.append(ins.ori.variable_left) + prev_ins.set_type(ArgumentType.GAS) + # Types checked by is_gas + prev_ins.call_id = ins.ori.variable_left.name # type: ignore + calls.append(ins.ori.variable_left) # type: ignore to_remove.append(ins) - variable_to_replace[ins.lvalue.name] = ins.ori.variable_left + variable_to_replace[ins.lvalue.name] = ins.ori.variable_left # type: ignore # Remove the call to value/gas instruction result = [i for i in result if not i in to_remove] @@ -446,7 +431,7 @@ def propagate_type_and_convert_call(result: List[Operation], node: "Node") -> Li if isinstance(ins, (HighLevelCall, NewContract, InternalDynamicCall)): if ins.call_id in calls_value: ins.call_value = calls_value[ins.call_id] - if ins.call_id in calls_gas: + if ins.call_id in calls_gas and isinstance(ins, (HighLevelCall, InternalDynamicCall)): ins.call_gas = calls_gas[ins.call_id] if isinstance(ins, (Call, NewContract, NewStructure)): @@ -525,17 +510,15 @@ def _convert_type_contract(ir: Member) -> Assignment: raise SlithIRError(f"type({contract.name}).{ir.variable_right} is unknown") -def propagate_types( - ir: slither.slithir.operations.operation.Operation, node: "Node" -): # pylint: disable=too-many-locals +def propagate_types(ir: Operation, node: "Node"): # pylint: disable=too-many-locals # propagate the type node_function = node.function - using_for = ( + using_for: Dict[USING_FOR_KEY, USING_FOR_ITEM] = ( node_function.contract.using_for_complete if isinstance(node_function, FunctionContract) else {} ) - if isinstance(ir, OperationWithLValue): + if isinstance(ir, OperationWithLValue) and ir.lvalue: # Force assignment in case of missing previous correct type if not ir.lvalue.type: if isinstance(ir, Assignment): @@ -646,11 +629,12 @@ def propagate_types( and not isinstance(ir.variable_left, Contract) and isinstance(ir.variable_left.type, (ElementaryType, ArrayType)) ): - length = Length(ir.variable_left, ir.lvalue) - length.set_expression(ir.expression) - length.lvalue.points_to = ir.variable_left - length.set_node(ir.node) - return length + new_length = Length(ir.variable_left, ir.lvalue) + assert ir.expression + new_length.set_expression(ir.expression) + new_length.lvalue.points_to = ir.variable_left + new_length.set_node(ir.node) + return new_length # This only happen for .balance/code/codehash access on a variable for which we dont know at # early parsing time the type # Like @@ -794,6 +778,7 @@ def propagate_types( ir.lvalue.set_type(ir.array_type) elif isinstance(ir, NewContract): contract = node.file_scope.get_contract_from_name(ir.contract_name) + assert contract ir.lvalue.set_type(UserDefinedType(contract)) elif isinstance(ir, NewElementaryType): ir.lvalue.set_type(ir.type) @@ -837,9 +822,7 @@ def propagate_types( # pylint: disable=too-many-locals -def extract_tmp_call( - ins: TmpCall, contract: Optional[Contract] -) -> slither.slithir.operations.call.Call: +def extract_tmp_call(ins: TmpCall, contract: Optional[Contract]) -> Union[Call, Nop]: assert isinstance(ins, TmpCall) if isinstance(ins.called, Variable) and isinstance(ins.called.type, FunctionType): # If the call is made to a variable member, where the member is this @@ -1328,16 +1311,8 @@ def convert_to_push_set_val( def convert_to_push( - ir: slither.slithir.operations.high_level_call.HighLevelCall, node: "Node" -) -> List[ - Union[ - slither.slithir.operations.length.Length, - slither.slithir.operations.assignment.Assignment, - slither.slithir.operations.binary.Binary, - slither.slithir.operations.index.Index, - slither.slithir.operations.init_array.InitArray, - ] -]: + ir: HighLevelCall, node: "Node" +) -> List[Union[Length, Assignment, Binary, Index, InitArray,]]: """ Convert a call to a series of operations to push a new value onto the array @@ -1357,22 +1332,23 @@ def convert_to_push( return ret -def convert_to_pop(ir, node): +def convert_to_pop(ir: HighLevelCall, node: "Node") -> List[Operation]: """ Convert pop operators Return a list of 6 operations """ - ret = [] + ret: List[Operation] = [] arr = ir.destination length = ReferenceVariable(node) length.set_type(ElementaryType("uint256")) ir_length = Length(arr, length) + assert ir.expression ir_length.set_expression(ir.expression) ir_length.set_node(ir.node) - ir_length.lvalue.points_to = arr + length.points_to = arr ret.append(ir_length) val = TemporaryVariable(node) @@ -1384,6 +1360,8 @@ def convert_to_pop(ir, node): element_to_delete = ReferenceVariable(node) ir_assign_element_to_delete = Index(element_to_delete, arr, val) + # TODO the following is equivalent to length.points_to = arr + # Should it be removed? ir_length.lvalue.points_to = arr element_to_delete.set_type(ElementaryType("uint256")) ir_assign_element_to_delete.set_expression(ir.expression) @@ -1399,7 +1377,7 @@ def convert_to_pop(ir, node): length_to_assign.set_type(ElementaryType("uint256")) ir_length = Length(arr, length_to_assign) ir_length.set_expression(ir.expression) - ir_length.lvalue.points_to = arr + length_to_assign.points_to = arr ir_length.set_node(ir.node) ret.append(ir_length) diff --git a/slither/slithir/operations/delete.py b/slither/slithir/operations/delete.py index 496d170ad..d241033c5 100644 --- a/slither/slithir/operations/delete.py +++ b/slither/slithir/operations/delete.py @@ -36,5 +36,5 @@ class Delete(OperationWithLValue): ) -> Union[StateIRVariable, StateVariable, ReferenceVariable, ReferenceVariableSSA]: return self._variable - def __str__(self): + def __str__(self) -> str: return f"{self.lvalue} = delete {self.variable} " diff --git a/slither/slithir/operations/return_operation.py b/slither/slithir/operations/return_operation.py index c21579763..290572ebf 100644 --- a/slither/slithir/operations/return_operation.py +++ b/slither/slithir/operations/return_operation.py @@ -1,11 +1,10 @@ -from typing import List +from typing import List, Optional, Union, Any from slither.core.declarations import Function +from slither.core.variables.variable import Variable from slither.slithir.operations.operation import Operation - +from slither.slithir.utils.utils import is_valid_rvalue, RVALUE from slither.slithir.variables.tuple import TupleVariable -from slither.slithir.utils.utils import is_valid_rvalue -from slither.core.variables.variable import Variable class Return(Operation): @@ -14,10 +13,13 @@ class Return(Operation): Only present as last operation in RETURN node """ - def __init__(self, values) -> None: + def __init__( + self, values: Optional[Union[RVALUE, TupleVariable, Function, List[RVALUE]]] + ) -> None: # Note: Can return None # ex: return call() # where call() dont return + self._values: List[Union[RVALUE, TupleVariable, Function]] if not isinstance(values, list): assert ( is_valid_rvalue(values) @@ -25,20 +27,19 @@ class Return(Operation): or values is None ) if values is None: - values = [] + self._values = [] else: - values = [values] + self._values = [values] else: # Remove None # Prior Solidity 0.5 # return (0,) # was valid for returns(uint) - values = [v for v in values if not v is None] - self._valid_value(values) + self._values = [v for v in values if not v is None] + self._valid_value(self._values) super().__init__() - self._values = values - def _valid_value(self, value) -> bool: + def _valid_value(self, value: Any) -> bool: if isinstance(value, list): assert all(self._valid_value(v) for v in value) else: @@ -53,5 +54,5 @@ class Return(Operation): def values(self) -> List[Variable]: return self._unroll(self._values) - def __str__(self): + def __str__(self) -> str: return f"RETURN {','.join([f'{x}' for x in self.values])}" diff --git a/slither/slithir/utils/utils.py b/slither/slithir/utils/utils.py index 4619c08bc..49b1a879c 100644 --- a/slither/slithir/utils/utils.py +++ b/slither/slithir/utils/utils.py @@ -1,4 +1,4 @@ -from typing import Union +from typing import Union, Optional from slither.core.variables.local_variable import LocalVariable from slither.core.variables.state_variable import StateVariable @@ -31,7 +31,7 @@ LVALUE = Union[ ] -def is_valid_rvalue(v: SourceMapping) -> bool: +def is_valid_rvalue(v: Optional[SourceMapping]) -> bool: return isinstance( v, ( @@ -46,7 +46,7 @@ def is_valid_rvalue(v: SourceMapping) -> bool: ) -def is_valid_lvalue(v: SourceMapping) -> bool: +def is_valid_lvalue(v: Optional[SourceMapping]) -> bool: return isinstance( v, ( diff --git a/slither/utils/myprettytable.py b/slither/utils/myprettytable.py index a1dfd7ac0..af10a6ff2 100644 --- a/slither/utils/myprettytable.py +++ b/slither/utils/myprettytable.py @@ -1,4 +1,4 @@ -from typing import List, Dict +from typing import List, Dict, Union from prettytable import PrettyTable @@ -8,7 +8,7 @@ class MyPrettyTable: self._field_names = field_names self._rows: List = [] - def add_row(self, row: List[str]) -> None: + def add_row(self, row: List[Union[str, List[str]]]) -> None: self._rows.append(row) def to_pretty_table(self) -> PrettyTable: