From cfb53e8200e6d598ab4e7b8447697f3ef29a1fa5 Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Sat, 3 Dec 2022 12:06:37 -0600 Subject: [PATCH 01/15] support ternary in call value --- slither/utils/expression_manipulations.py | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/slither/utils/expression_manipulations.py b/slither/utils/expression_manipulations.py index 1a300c39b..3e12ae8c1 100644 --- a/slither/utils/expression_manipulations.py +++ b/slither/utils/expression_manipulations.py @@ -28,15 +28,18 @@ def f_expressions( e._expressions.append(x) -def f_call(e, x): +def f_call(e: CallExpression, x): e._arguments.append(x) +def f_call_value(e: CallExpression, x): + e._value = x + def f_expression(e, x): e._expression = x -def f_called(e, x): +def f_called(e: CallExpression, x): e._called = x @@ -123,6 +126,15 @@ class SplitTernaryExpression: if self.apply_copy(next_expr, true_expression, false_expression, f_called): self.copy_expression(next_expr, true_expression.called, false_expression.called) + next_expr = expression.call_value + # case of (..).func{value: .. ? .. : ..}() + if self.apply_copy(next_expr, true_expression, false_expression, f_call_value): + self.copy_expression( + next_expr, + true_expression.call_value, + false_expression.call_value, + ) + true_expression._arguments = [] false_expression._arguments = [] From eb49e396fd0d60c10e4db1771b443319e1faf55b Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Sat, 10 Dec 2022 15:41:39 -0600 Subject: [PATCH 02/15] support ternaries in both call options, refactor index access --- slither/solc_parsing/declarations/function.py | 4 +- slither/utils/expression_manipulations.py | 116 ++++++++++-------- tests/slithir/ternary_expressions.sol | 11 ++ 3 files changed, 75 insertions(+), 56 deletions(-) diff --git a/slither/solc_parsing/declarations/function.py b/slither/solc_parsing/declarations/function.py index 130375211..269ca580f 100644 --- a/slither/solc_parsing/declarations/function.py +++ b/slither/solc_parsing/declarations/function.py @@ -308,7 +308,7 @@ class FunctionSolc(CallerContextExpression): for node_parser in self._node_to_yulobject.values(): node_parser.analyze_expressions() - self._filter_ternary() + self._rewrite_ternary_as_if_else() self._remove_alone_endif() @@ -1336,7 +1336,7 @@ class FunctionSolc(CallerContextExpression): ################################################################################### ################################################################################### - def _filter_ternary(self) -> bool: + def _rewrite_ternary_as_if_else(self) -> bool: ternary_found = True updated = False while ternary_found: diff --git a/slither/utils/expression_manipulations.py b/slither/utils/expression_manipulations.py index 3e12ae8c1..777c0c2a1 100644 --- a/slither/utils/expression_manipulations.py +++ b/slither/utils/expression_manipulations.py @@ -3,7 +3,7 @@ as they should be immutable """ import copy -from typing import Union, Callable +from typing import Union, Callable, Tuple, Optional from slither.core.expressions import UnaryOperation from slither.core.expressions.assignment_operation import AssignmentOperation from slither.core.expressions.binary_operation import BinaryOperation @@ -35,6 +35,11 @@ def f_call(e: CallExpression, x): def f_call_value(e: CallExpression, x): e._value = x + +def f_call_gas(e: CallExpression, x): + e._gas = x + + def f_expression(e, x): e._expression = x @@ -56,7 +61,7 @@ class SplitTernaryExpression: self.condition = None self.copy_expression(expression, self.true_expression, self.false_expression) - def apply_copy( + def conditional_not_ahead( self, next_expr: Expression, true_expression: Union[AssignmentOperation, MemberAccess], @@ -94,7 +99,9 @@ class SplitTernaryExpression: # (.. ? .. : ..).add if isinstance(expression, MemberAccess): next_expr = expression.expression - if self.apply_copy(next_expr, true_expression, false_expression, f_expression): + if self.conditional_not_ahead( + next_expr, true_expression, false_expression, f_expression + ): self.copy_expression( next_expr, true_expression.expression, false_expression.expression ) @@ -102,44 +109,75 @@ class SplitTernaryExpression: elif isinstance(expression, (AssignmentOperation, BinaryOperation, TupleExpression)): true_expression._expressions = [] false_expression._expressions = [] - for next_expr in expression.expressions: - if isinstance(next_expr, IndexAccess): - # create an index access for each branch - if isinstance(next_expr.expression_right, ConditionalExpression): - next_expr = _handle_ternary_access( - next_expr, true_expression, false_expression + # TODO: can we get rid of `NoneType` expressions in `TupleExpression`? + if next_expr: + if isinstance(next_expr, IndexAccess): + # create an index access for each branch + # x[if cond ? 1 : 2] -> if cond { x[1] } else { x[2] } + for expr in next_expr.expressions: + if self.conditional_not_ahead( + expr, true_expression, false_expression, f_expressions + ): + self.copy_expression( + expr, + true_expression.expressions[-1], + false_expression.expressions[-1], + ) + + if self.conditional_not_ahead( + next_expr, true_expression, false_expression, f_expressions + ): + # always on last arguments added + self.copy_expression( + next_expr, + true_expression.expressions[-1], + false_expression.expressions[-1], ) - if self.apply_copy(next_expr, true_expression, false_expression, f_expressions): - # always on last arguments added - self.copy_expression( - next_expr, - true_expression.expressions[-1], - false_expression.expressions[-1], - ) elif isinstance(expression, CallExpression): next_expr = expression.called # case of lib # (.. ? .. : ..).add - if self.apply_copy(next_expr, true_expression, false_expression, f_called): + if self.conditional_not_ahead(next_expr, true_expression, false_expression, f_called): self.copy_expression(next_expr, true_expression.called, false_expression.called) - next_expr = expression.call_value - # case of (..).func{value: .. ? .. : ..}() - if self.apply_copy(next_expr, true_expression, false_expression, f_call_value): + # In order to handle ternaries in both call options, gas and value, we return early if the + # conditional is not ahead to rewrite both ternaries (see `_rewrite_ternary_as_if_else`). + if expression.call_gas: + # case of (..).func{gas: .. ? .. : ..}() + next_expr = expression.call_gas + if self.conditional_not_ahead( + next_expr, true_expression, false_expression, f_call_gas + ): + self.copy_expression( + next_expr, + true_expression.call_gas, + false_expression.call_gas, + ) + else: + return + + if expression.call_value: + # case of (..).func{value: .. ? .. : ..}() + next_expr = expression.call_value + if self.conditional_not_ahead( + next_expr, true_expression, false_expression, f_call_value + ): self.copy_expression( next_expr, true_expression.call_value, false_expression.call_value, ) + else: + return true_expression._arguments = [] false_expression._arguments = [] for next_expr in expression.arguments: - if self.apply_copy(next_expr, true_expression, false_expression, f_call): + if self.conditional_not_ahead(next_expr, true_expression, false_expression, f_call): # always on last arguments added self.copy_expression( next_expr, @@ -149,7 +187,9 @@ class SplitTernaryExpression: elif isinstance(expression, (TypeConversion, UnaryOperation)): next_expr = expression.expression - if self.apply_copy(next_expr, true_expression, false_expression, f_expression): + if self.conditional_not_ahead( + next_expr, true_expression, false_expression, f_expression + ): self.copy_expression( expression.expression, true_expression.expression, @@ -160,35 +200,3 @@ class SplitTernaryExpression: raise SlitherException( f"Ternary operation not handled {expression}({type(expression)})" ) - - -def _handle_ternary_access( - next_expr: IndexAccess, - true_expression: AssignmentOperation, - false_expression: AssignmentOperation, -): - """ - Conditional ternary accesses are split into two accesses, one true and one false - E.g. x[if cond ? 1 : 2] -> if cond { x[1] } else { x[2] } - """ - true_index_access = IndexAccess( - next_expr.expression_left, - next_expr.expression_right.then_expression, - next_expr.type, - ) - false_index_access = IndexAccess( - next_expr.expression_left, - next_expr.expression_right.else_expression, - next_expr.type, - ) - - f_expressions( - true_expression, - true_index_access, - ) - f_expressions( - false_expression, - false_index_access, - ) - - return next_expr.expression_right diff --git a/tests/slithir/ternary_expressions.sol b/tests/slithir/ternary_expressions.sol index c2e50b719..7fcc675c1 100644 --- a/tests/slithir/ternary_expressions.sol +++ b/tests/slithir/ternary_expressions.sol @@ -1,3 +1,6 @@ +interface NameReg { + function addressOf() external payable; +} contract C { // TODO // 1) support variable declarations @@ -21,4 +24,12 @@ contract C { function d(bool cond, bytes calldata x) external { bytes1 a = x[cond ? 1 : 2]; } + + function e(address one, address two) public { + return NameReg(one).addressOf{value: msg.sender == two ? 1 : 2, gas: true ? 2 : gasleft()}(); + } + // TODO: nested ternary + // function f(address one, address two) public { + // return NameReg(one).addressOf{value: msg.sender == two ? 1 : 2, gas: true ? (1 == 1 ? 1 : 2) : gasleft()}(); + // } } From a1a0abe17dbc0bc805f5b038fe673376f0d5f14d Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Mon, 12 Dec 2022 12:45:31 -0600 Subject: [PATCH 03/15] support parenthetical ternary expr and update tests --- slither/utils/expression_manipulations.py | 7 +++++++ tests/slithir/ternary_expressions.sol | 21 ++++++++++++++------- tests/slithir/test_ternary_expressions.py | 6 +++--- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/slither/utils/expression_manipulations.py b/slither/utils/expression_manipulations.py index 777c0c2a1..9ae01fde7 100644 --- a/slither/utils/expression_manipulations.py +++ b/slither/utils/expression_manipulations.py @@ -68,6 +68,13 @@ class SplitTernaryExpression: false_expression: Union[AssignmentOperation, MemberAccess], f: Callable, ) -> bool: + # parentetical expression (.. ? .. : ..) + if ( + isinstance(next_expr, TupleExpression) + and len(next_expr.expressions) == 1 + and isinstance(next_expr.expressions[0], ConditionalExpression) + ): + next_expr = next_expr.expressions[0] if isinstance(next_expr, ConditionalExpression): f(true_expression, copy.copy(next_expr.then_expression)) diff --git a/tests/slithir/ternary_expressions.sol b/tests/slithir/ternary_expressions.sol index 7fcc675c1..c6a6d4643 100644 --- a/tests/slithir/ternary_expressions.sol +++ b/tests/slithir/ternary_expressions.sol @@ -1,5 +1,6 @@ -interface NameReg { - function addressOf() external payable; +interface Test { + function test() external payable returns (uint); + function testTuple() external payable returns (uint, uint); } contract C { // TODO @@ -26,10 +27,16 @@ contract C { } function e(address one, address two) public { - return NameReg(one).addressOf{value: msg.sender == two ? 1 : 2, gas: true ? 2 : gasleft()}(); + uint x = Test(one).test{value: msg.sender == two ? 1 : 2, gas: true ? 2 : gasleft()}(); + } + + // Parenthteical expression + function f(address one, address two) public { + uint x = Test(one).test{value: msg.sender == two ? 1 : 2, gas: true ? (1 == 1 ? 1 : 2) : gasleft()}(); + } + + // Unused tuple variable + function g(address one) public { + (, uint x) = Test(one).testTuple(); } - // TODO: nested ternary - // function f(address one, address two) public { - // return NameReg(one).addressOf{value: msg.sender == two ? 1 : 2, gas: true ? (1 == 1 ? 1 : 2) : gasleft()}(); - // } } diff --git a/tests/slithir/test_ternary_expressions.py b/tests/slithir/test_ternary_expressions.py index db5658787..17cac6b2f 100644 --- a/tests/slithir/test_ternary_expressions.py +++ b/tests/slithir/test_ternary_expressions.py @@ -9,10 +9,10 @@ def test_ternary_conversions() -> None: slither = Slither("./tests/slithir/ternary_expressions.sol") for contract in slither.contracts: for function in contract.functions: + vars_declared = 0 + vars_assigned = 0 for node in function.nodes: if node.type in [NodeType.IF, NodeType.IFLOOP]: - vars_declared = 0 - vars_assigned = 0 # Iterate over true and false son for inner_node in node.sons: @@ -31,7 +31,7 @@ def test_ternary_conversions() -> None: if isinstance(ir, Assignment): vars_assigned += 1 - assert vars_declared == vars_assigned + assert vars_declared == vars_assigned if __name__ == "__main__": From a1343a8df596746999e766f8f7c51ed6a8ed93b4 Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Mon, 12 Dec 2022 12:47:27 -0600 Subject: [PATCH 04/15] update function name --- slither/solc_parsing/declarations/modifier.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slither/solc_parsing/declarations/modifier.py b/slither/solc_parsing/declarations/modifier.py index a3f07da7f..e55487612 100644 --- a/slither/solc_parsing/declarations/modifier.py +++ b/slither/solc_parsing/declarations/modifier.py @@ -87,7 +87,7 @@ class ModifierSolc(FunctionSolc): for node in self._node_to_nodesolc.values(): node.analyze_expressions(self) - self._filter_ternary() + self._rewrite_ternary_as_if_else() self._remove_alone_endif() # self._analyze_read_write() From ca252f147293e648d748593c891f1742a9b07db5 Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Mon, 12 Dec 2022 13:10:25 -0600 Subject: [PATCH 05/15] spelling and linting --- slither/utils/expression_manipulations.py | 6 +++--- tests/slithir/ternary_expressions.sol | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/slither/utils/expression_manipulations.py b/slither/utils/expression_manipulations.py index 9ae01fde7..591fab0ef 100644 --- a/slither/utils/expression_manipulations.py +++ b/slither/utils/expression_manipulations.py @@ -3,7 +3,7 @@ as they should be immutable """ import copy -from typing import Union, Callable, Tuple, Optional +from typing import Union, Callable from slither.core.expressions import UnaryOperation from slither.core.expressions.assignment_operation import AssignmentOperation from slither.core.expressions.binary_operation import BinaryOperation @@ -68,7 +68,7 @@ class SplitTernaryExpression: false_expression: Union[AssignmentOperation, MemberAccess], f: Callable, ) -> bool: - # parentetical expression (.. ? .. : ..) + # look ahead for parenthetical expression (.. ? .. : ..) if ( isinstance(next_expr, TupleExpression) and len(next_expr.expressions) == 1 @@ -112,7 +112,7 @@ class SplitTernaryExpression: self.copy_expression( next_expr, true_expression.expression, false_expression.expression ) - + # pylint: disable=too-many-nested-blocks elif isinstance(expression, (AssignmentOperation, BinaryOperation, TupleExpression)): true_expression._expressions = [] false_expression._expressions = [] diff --git a/tests/slithir/ternary_expressions.sol b/tests/slithir/ternary_expressions.sol index c6a6d4643..89fdc59d1 100644 --- a/tests/slithir/ternary_expressions.sol +++ b/tests/slithir/ternary_expressions.sol @@ -30,7 +30,7 @@ contract C { uint x = Test(one).test{value: msg.sender == two ? 1 : 2, gas: true ? 2 : gasleft()}(); } - // Parenthteical expression + // Parenthetical expression function f(address one, address two) public { uint x = Test(one).test{value: msg.sender == two ? 1 : 2, gas: true ? (1 == 1 ? 1 : 2) : gasleft()}(); } From 9c339a69299bbcb4becadf8fb84f27f3c275e1f8 Mon Sep 17 00:00:00 2001 From: webthethird Date: Fri, 23 Dec 2022 13:28:40 -0600 Subject: [PATCH 06/15] Implement `--detect` and `--exclude` for slither-check-upgradeability --- slither/tools/upgradeability/__main__.py | 61 ++++++++++++++++++++---- 1 file changed, 52 insertions(+), 9 deletions(-) diff --git a/slither/tools/upgradeability/__main__.py b/slither/tools/upgradeability/__main__.py index 414a4c175..bafe3fa9e 100644 --- a/slither/tools/upgradeability/__main__.py +++ b/slither/tools/upgradeability/__main__.py @@ -27,7 +27,9 @@ logger: logging.Logger = logging.getLogger("Slither") logger.setLevel(logging.INFO) -def parse_args() -> argparse.Namespace: +def parse_args( + check_classes: List[Type[AbstractCheck]] +) -> argparse.Namespace: parser = argparse.ArgumentParser( description="Slither Upgradeability Checks. For usage information see https://github.com/crytic/slither/wiki/Upgradeability-Checks.", usage="slither-check-upgradeability contract.sol ContractName", @@ -51,6 +53,23 @@ def parse_args() -> argparse.Namespace: default=False, ) + parser.add_argument( + "--detect", + help="Comma-separated list of detectors, defaults to all, " + f"available detectors: {', '.join(d.ARGUMENT for d in check_classes)}", + action="store", + dest="detectors_to_run", + default="all", + ) + + parser.add_argument( + "--exclude", + help="Comma-separated list of detectors that should be excluded", + action="store", + dest="detectors_to_exclude", + default=None, + ) + parser.add_argument( "--list-detectors", help="List available detectors", @@ -104,6 +123,30 @@ def _get_checks() -> List[Type[AbstractCheck]]: return detectors +def choose_checks( + args: argparse.Namespace, all_check_classes: List[Type[AbstractCheck]] +) -> List[Type[AbstractCheck]]: + detectors_to_run = [] + detectors = {d.ARGUMENT: d for d in all_check_classes} + + if args.detectors_to_run == "all": + detectors_to_run = all_check_classes + if args.detectors_to_exclude: + detectors_excluded = args.detectors_to_exclude.split(",") + for detector in detectors: + if detector in detectors_excluded: + detectors_to_run.remove(detectors[detector]) + else: + for detector in args.detectors_to_run.split(","): + if detector in detectors: + detectors_to_run.append(detectors[detector]) + else: + raise Exception(f"Error: {detector} is not a detector") + detectors_to_run = sorted(detectors_to_run, key=lambda x: x.IMPACT) + return detectors_to_run + return detectors_to_run + + class ListDetectors(argparse.Action): # pylint: disable=too-few-public-methods def __call__( self, parser: Any, *args: Any, **kwargs: Any @@ -200,11 +243,11 @@ def main() -> None: "detectors": [], } - args = parse_args() - + detectors = _get_checks() + args = parse_args(detectors) + detectors_to_run = choose_checks(args, detectors) v1_filename = vars(args)["contract.sol"] number_detectors_run = 0 - detectors = _get_checks() try: variable1 = Slither(v1_filename, **vars(args)) @@ -219,7 +262,7 @@ def main() -> None: return v1_contract = v1_contracts[0] - detectors_results, number_detectors = _checks_on_contract(detectors, v1_contract) + detectors_results, number_detectors = _checks_on_contract(detectors_to_run, v1_contract) json_results["detectors"] += detectors_results number_detectors_run += number_detectors @@ -242,7 +285,7 @@ def main() -> None: json_results["proxy-present"] = True detectors_results, number_detectors = _checks_on_contract_and_proxy( - detectors, v1_contract, proxy_contract + detectors_to_run, v1_contract, proxy_contract ) json_results["detectors"] += detectors_results number_detectors_run += number_detectors @@ -267,19 +310,19 @@ def main() -> None: if proxy_contract: detectors_results, _ = _checks_on_contract_and_proxy( - detectors, v2_contract, proxy_contract + detectors_to_run, v2_contract, proxy_contract ) json_results["detectors"] += detectors_results detectors_results, number_detectors = _checks_on_contract_update( - detectors, v1_contract, v2_contract + detectors_to_run, v1_contract, v2_contract ) json_results["detectors"] += detectors_results number_detectors_run += number_detectors # If there is a V2, we run the contract-only check on the V2 - detectors_results, number_detectors = _checks_on_contract(detectors, v2_contract) + detectors_results, number_detectors = _checks_on_contract(detectors_to_run, v2_contract) json_results["detectors"] += detectors_results number_detectors_run += number_detectors From 194b1bd90563092dbf97d254dda4ad253cda91fe Mon Sep 17 00:00:00 2001 From: webthethird Date: Fri, 23 Dec 2022 13:41:37 -0600 Subject: [PATCH 07/15] Implement `--exclude-` for slither-check-upgradeability --- slither/tools/upgradeability/__main__.py | 63 +++++++++++++++++++++--- 1 file changed, 56 insertions(+), 7 deletions(-) diff --git a/slither/tools/upgradeability/__main__.py b/slither/tools/upgradeability/__main__.py index bafe3fa9e..f71ed1e5b 100644 --- a/slither/tools/upgradeability/__main__.py +++ b/slither/tools/upgradeability/__main__.py @@ -35,6 +35,8 @@ def parse_args( usage="slither-check-upgradeability contract.sol ContractName", ) + group_checks = parser.add_argument_group("Checks") + parser.add_argument("contract.sol", help="Codebase to analyze") parser.add_argument("ContractName", help="Contract name (logic contract)") @@ -53,7 +55,7 @@ def parse_args( default=False, ) - parser.add_argument( + group_checks.add_argument( "--detect", help="Comma-separated list of detectors, defaults to all, " f"available detectors: {', '.join(d.ARGUMENT for d in check_classes)}", @@ -62,7 +64,15 @@ def parse_args( default="all", ) - parser.add_argument( + group_checks.add_argument( + "--list-detectors", + help="List available detectors", + action=ListDetectors, + nargs=0, + default=False, + ) + + group_checks.add_argument( "--exclude", help="Comma-separated list of detectors that should be excluded", action="store", @@ -70,11 +80,31 @@ def parse_args( default=None, ) - parser.add_argument( - "--list-detectors", - help="List available detectors", - action=ListDetectors, - nargs=0, + group_checks.add_argument( + "--exclude-informational", + help="Exclude informational impact analyses", + action="store_true", + default=False, + ) + + group_checks.add_argument( + "--exclude-low", + help="Exclude low impact analyses", + action="store_true", + default=False, + ) + + group_checks.add_argument( + "--exclude-medium", + help="Exclude medium impact analyses", + action="store_true", + default=False, + ) + + group_checks.add_argument( + "--exclude-high", + help="Exclude high impact analyses", + action="store_true", default=False, ) @@ -144,6 +174,25 @@ def choose_checks( raise Exception(f"Error: {detector} is not a detector") detectors_to_run = sorted(detectors_to_run, key=lambda x: x.IMPACT) return detectors_to_run + + if args.exclude_informational: + detectors_to_run = [ + d for d in detectors_to_run if d.IMPACT != DetectorClassification.INFORMATIONAL + ] + if args.exclude_low: + detectors_to_run = [ + d for d in detectors_to_run if d.IMPACT != DetectorClassification.LOW + ] + if args.exclude_medium: + detectors_to_run = [ + d for d in detectors_to_run if d.IMPACT != DetectorClassification.MEDIUM + ] + if args.exclude_high: + detectors_to_run = [ + d for d in detectors_to_run if d.IMPACT != DetectorClassification.HIGH + ] + + detectors_to_run = sorted(detectors_to_run, key=lambda x: x.IMPACT) return detectors_to_run From c66f2dca4069b5e83b5e8e6f6820cf35db96f56e Mon Sep 17 00:00:00 2001 From: webthethird Date: Fri, 23 Dec 2022 13:51:04 -0600 Subject: [PATCH 08/15] Import `CheckClassification` and fix references --- slither/tools/upgradeability/__main__.py | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/slither/tools/upgradeability/__main__.py b/slither/tools/upgradeability/__main__.py index f71ed1e5b..7a5beacf5 100644 --- a/slither/tools/upgradeability/__main__.py +++ b/slither/tools/upgradeability/__main__.py @@ -14,7 +14,10 @@ from slither.exceptions import SlitherException from slither.utils.colors import red from slither.utils.output import output_to_json from slither.tools.upgradeability.checks import all_checks -from slither.tools.upgradeability.checks.abstract_checks import AbstractCheck +from slither.tools.upgradeability.checks.abstract_checks import ( + AbstractCheck, + CheckClassification, +) from slither.tools.upgradeability.utils.command_line import ( output_detectors_json, output_wiki, @@ -177,19 +180,19 @@ def choose_checks( if args.exclude_informational: detectors_to_run = [ - d for d in detectors_to_run if d.IMPACT != DetectorClassification.INFORMATIONAL + d for d in detectors_to_run if d.IMPACT != CheckClassification.INFORMATIONAL ] if args.exclude_low: detectors_to_run = [ - d for d in detectors_to_run if d.IMPACT != DetectorClassification.LOW + d for d in detectors_to_run if d.IMPACT != CheckClassification.LOW ] if args.exclude_medium: detectors_to_run = [ - d for d in detectors_to_run if d.IMPACT != DetectorClassification.MEDIUM + d for d in detectors_to_run if d.IMPACT != CheckClassification.MEDIUM ] if args.exclude_high: detectors_to_run = [ - d for d in detectors_to_run if d.IMPACT != DetectorClassification.HIGH + d for d in detectors_to_run if d.IMPACT != CheckClassification.HIGH ] detectors_to_run = sorted(detectors_to_run, key=lambda x: x.IMPACT) From 1965d262b1f9255a9bb17e4546d74b0af19d572b Mon Sep 17 00:00:00 2001 From: webthethird Date: Fri, 23 Dec 2022 13:52:22 -0600 Subject: [PATCH 09/15] Black --- slither/tools/upgradeability/__main__.py | 16 ++++------------ 1 file changed, 4 insertions(+), 12 deletions(-) diff --git a/slither/tools/upgradeability/__main__.py b/slither/tools/upgradeability/__main__.py index 7a5beacf5..ceb9ce421 100644 --- a/slither/tools/upgradeability/__main__.py +++ b/slither/tools/upgradeability/__main__.py @@ -30,9 +30,7 @@ logger: logging.Logger = logging.getLogger("Slither") logger.setLevel(logging.INFO) -def parse_args( - check_classes: List[Type[AbstractCheck]] -) -> argparse.Namespace: +def parse_args(check_classes: List[Type[AbstractCheck]]) -> argparse.Namespace: parser = argparse.ArgumentParser( description="Slither Upgradeability Checks. For usage information see https://github.com/crytic/slither/wiki/Upgradeability-Checks.", usage="slither-check-upgradeability contract.sol ContractName", @@ -183,17 +181,11 @@ def choose_checks( d for d in detectors_to_run if d.IMPACT != CheckClassification.INFORMATIONAL ] if args.exclude_low: - detectors_to_run = [ - d for d in detectors_to_run if d.IMPACT != CheckClassification.LOW - ] + detectors_to_run = [d for d in detectors_to_run if d.IMPACT != CheckClassification.LOW] if args.exclude_medium: - detectors_to_run = [ - d for d in detectors_to_run if d.IMPACT != CheckClassification.MEDIUM - ] + detectors_to_run = [d for d in detectors_to_run if d.IMPACT != CheckClassification.MEDIUM] if args.exclude_high: - detectors_to_run = [ - d for d in detectors_to_run if d.IMPACT != CheckClassification.HIGH - ] + detectors_to_run = [d for d in detectors_to_run if d.IMPACT != CheckClassification.HIGH] detectors_to_run = sorted(detectors_to_run, key=lambda x: x.IMPACT) return detectors_to_run From cd8c6388e7e2e1865a590caae9707f39ec9b0eae Mon Sep 17 00:00:00 2001 From: webthethird Date: Fri, 23 Dec 2022 13:58:55 -0600 Subject: [PATCH 10/15] Don't sort checks by impact which caused CI test to fail --- slither/tools/upgradeability/__main__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slither/tools/upgradeability/__main__.py b/slither/tools/upgradeability/__main__.py index ceb9ce421..56b838b9c 100644 --- a/slither/tools/upgradeability/__main__.py +++ b/slither/tools/upgradeability/__main__.py @@ -187,7 +187,7 @@ def choose_checks( if args.exclude_high: detectors_to_run = [d for d in detectors_to_run if d.IMPACT != CheckClassification.HIGH] - detectors_to_run = sorted(detectors_to_run, key=lambda x: x.IMPACT) + # detectors_to_run = sorted(detectors_to_run, key=lambda x: x.IMPACT) return detectors_to_run From 45e90dc4c10120ac0f52b1490e62167870277c97 Mon Sep 17 00:00:00 2001 From: Simone Date: Tue, 3 Jan 2023 15:13:14 +0100 Subject: [PATCH 11/15] Fix top level struct parsing --- slither/solc_parsing/slither_compilation_unit_solc.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/slither/solc_parsing/slither_compilation_unit_solc.py b/slither/solc_parsing/slither_compilation_unit_solc.py index d12bda1b4..6dce9b005 100644 --- a/slither/solc_parsing/slither_compilation_unit_solc.py +++ b/slither/solc_parsing/slither_compilation_unit_solc.py @@ -24,6 +24,7 @@ from slither.solc_parsing.declarations.function import FunctionSolc from slither.solc_parsing.declarations.structure_top_level import StructureTopLevelSolc from slither.solc_parsing.exceptions import VariableNotFound from slither.solc_parsing.variables.top_level_variable import TopLevelVariableSolc +from slither.solc_parsing.declarations.caller_context import CallerContextExpression logging.basicConfig() logger = logging.getLogger("SlitherSolcParsing") @@ -57,7 +58,7 @@ def _handle_import_aliases( scope.renaming[local_name] = original_name -class SlitherCompilationUnitSolc: +class SlitherCompilationUnitSolc(CallerContextExpression): # pylint: disable=no-self-use,too-many-instance-attributes def __init__(self, compilation_unit: SlitherCompilationUnit): super().__init__() @@ -95,6 +96,10 @@ class SlitherCompilationUnitSolc: def underlying_contract_to_parser(self) -> Dict[Contract, ContractSolc]: return self._underlying_contract_to_parser + @property + def slither_parser(self) -> "SlitherCompilationUnitSolc": + return self + ################################################################################### ################################################################################### # region AST From 680c914ded73e5b221f9df903da8050ed4756ce7 Mon Sep 17 00:00:00 2001 From: Simone Date: Tue, 3 Jan 2023 15:29:20 +0100 Subject: [PATCH 12/15] Add test --- ...p-level-struct-0.8.0.sol-0.8.0-compact.zip | Bin 0 -> 2027 bytes ...-level-struct-0.8.0.sol-0.8.0-compact.json | 3 +++ tests/ast-parsing/top-level-struct-0.8.0.sol | 18 ++++++++++++++++++ tests/test_ast_parsing.py | 1 + 4 files changed, 22 insertions(+) create mode 100644 tests/ast-parsing/compile/top-level-struct-0.8.0.sol-0.8.0-compact.zip create mode 100644 tests/ast-parsing/expected/top-level-struct-0.8.0.sol-0.8.0-compact.json create mode 100644 tests/ast-parsing/top-level-struct-0.8.0.sol diff --git a/tests/ast-parsing/compile/top-level-struct-0.8.0.sol-0.8.0-compact.zip b/tests/ast-parsing/compile/top-level-struct-0.8.0.sol-0.8.0-compact.zip new file mode 100644 index 0000000000000000000000000000000000000000..7cb2fb659609b654a99a51480f172e7555306ef0 GIT binary patch literal 2027 zcmb8wX&}>$0|)TmOq&+TP087uZRT+;(#n)GH1bq)#GH%Sm>IciEFO|0w~0i}BM~i+ z5|X2sBg7OUDrb(c9RHsG`~Qpo_xs}e>ihcRU;`G!00O{4AR~ z005{#WJG{B%{u@QLJbZjP!T938i_)N1O>Qz)5$@>RCkYHf}cOl8%YcaBJCI42e<-2 zEC84?nZA14dX#(pJ@qCc1#KjMc#{Gn1$X$A;&8|dY29_~sZ`z(53#8)oRtI4MYq>2 z53g41;1+YTEz1v$yr)tMNkWb9u$51%tmmZPmi(CbvDy*U;=X`4g>g*HFQ!P>F$C}} z4wvS9({^#fNLRHNdF?mr(!ztp2Su!Sz3vlPj-3-%Z^9%z0X7U{vU)iQ^1BD6Kt;yaqh?RTYzFae;%GnYvxX8eOb6W0#j^oXNfb?OX7+8d|ed1N-B z-0BW|icHqLExr(z{k5EzB`|%Yz+CL+OX#y%gT3)rT=Brq+;6%fJqm-FgYhHFL-Fw0 zM_@U$|A*uUWA)8#ABNkLpktOys1l=IZvIol2{#0aWSbPFt?_pgyXKU?QGU zw|ciCJNKd6^eVG0Hu(xY?e{A9Z$j&Ck-BbP&q3y#AZ zEHuA#oeJA4Dr(iWwOq@m9q%W-wIwvB59Z4WuSy&R*LQ09*yD~R!gxD%^9#LHRA~3v zptY+*q(4EC+7Se|zs}z=tzSf^JzQ({yuE)S^@)KOpA}s=G5O%AYTEe|Nnh6-&m z#c1Y9eDix&4K2#U9`@bQ(*ZX0;9IaoJFHxIZK{Cv`}Q>*TZ}2~amOmm6fuBZsN(MG z+zQ|0@?m3nfl69}_BglaYNy*xvL?2FIbkKT-tbSoEL)B-Fd%B|kL{l_R$BfQ`{{cr zUuIg|r}to3bqIX)*WJ1YVSzd!DvMcvKZtm-yf-{p-jYR%+JpoiAmWdWnDe&bX9z}7 zT#V}VC>}@`5+k*|+F$Jyu1*^t>YDLGy@WC$yZ8M>jIOMu0K02sC-0XGA)xk9J^F+I zcsR18=RD1EtN99=(=CI`(2Ne>8~u6Gjh^|~Mr^UmA50sU=`0O>A^t3Z4>;HI@{ z1*A)IuB0oIXPsGY(=cRr^l6wdEV*@1CtDbSkqX_=jNvD1!EseMyrpa!MBA8wt^OW;F45L#Q|HJ zT{5%NC|#p$_twRpPuY`$c))Coclw-KSsBeQj76kaKB0Z<>;3m3FJnQL?iTFj@7#3szYSzmtA#%^x1R1!rC`7jzNRW<$;#+sL*Q z9*%iT&j?gK7qQlE$bY`nW-gBh*b_ZdJ)VKAVAn4By{qw;>CCReKa=fw=o*#0kI^Ci zN#!F9Llw3e!JW@1xVN3tOI!zU<3~l~L<2nqEtDwgUa#!QGqJi;=})b93`F9fO~NI;X5&ELI4UToDpZjmk0;Ol^%KN}}U}5i`2|qb6Px;g2tLD%bRZ zdKZdwyd74DshduessvR9PV5dvs(j9feS8Tna>EJ%@0!2JZGVA)0JEabjL|&&-j7Ev pYYsLb0Sx$m)B9WB|0Wjn@BDudJJ{@l{QCm^uFLNY|4ljo{09_{xwZfR literal 0 HcmV?d00001 diff --git a/tests/ast-parsing/expected/top-level-struct-0.8.0.sol-0.8.0-compact.json b/tests/ast-parsing/expected/top-level-struct-0.8.0.sol-0.8.0-compact.json new file mode 100644 index 000000000..65dfd3b51 --- /dev/null +++ b/tests/ast-parsing/expected/top-level-struct-0.8.0.sol-0.8.0-compact.json @@ -0,0 +1,3 @@ +{ + "BaseContract": {} +} \ No newline at end of file diff --git a/tests/ast-parsing/top-level-struct-0.8.0.sol b/tests/ast-parsing/top-level-struct-0.8.0.sol new file mode 100644 index 000000000..8a335680c --- /dev/null +++ b/tests/ast-parsing/top-level-struct-0.8.0.sol @@ -0,0 +1,18 @@ +struct my_struct { + uint[][] a; // works fine + uint[][3] b; // works fine + uint[3][] c; // fails + uint[3][3] d; // fails + uint[2**20] e; // works fine +} +contract BaseContract{ + struct my_struct_2 { + uint[][] f; // works fine + uint[][3] g; // works fine + uint[3][] h; // works fine + uint[3][3] i; // works fine + uint[2**20] j; // works fine + } + + uint[3][] k; // works fine +} diff --git a/tests/test_ast_parsing.py b/tests/test_ast_parsing.py index e96a129b8..47f230534 100644 --- a/tests/test_ast_parsing.py +++ b/tests/test_ast_parsing.py @@ -425,6 +425,7 @@ ALL_TESTS = [ Test("free_functions/library_constant_function_collision.sol", ["0.8.12"]), Test("ternary-with-max.sol", ["0.8.15"]), Test("library_event-0.8.16.sol", ["0.8.16"]), + Test("top-level-struct-0.8.0.sol", ["0.8.0"]), ] # create the output folder if needed try: From baf4143345059339719a12960283eed6e6490f6c Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Tue, 3 Jan 2023 12:03:29 -0600 Subject: [PATCH 13/15] move nested logic into functions --- slither/utils/expression_manipulations.py | 178 +++++++++++----------- 1 file changed, 92 insertions(+), 86 deletions(-) diff --git a/slither/utils/expression_manipulations.py b/slither/utils/expression_manipulations.py index 591fab0ef..974c6f68b 100644 --- a/slither/utils/expression_manipulations.py +++ b/slither/utils/expression_manipulations.py @@ -23,7 +23,8 @@ from slither.all_exceptions import SlitherException # pylint: disable=protected-access def f_expressions( - e: AssignmentOperation, x: Union[Identifier, Literal, MemberAccess, IndexAccess] + e: Union[AssignmentOperation, BinaryOperation, TupleExpression], + x: Union[Identifier, Literal, MemberAccess, IndexAccess], ) -> None: e._expressions.append(x) @@ -40,7 +41,7 @@ def f_call_gas(e: CallExpression, x): e._gas = x -def f_expression(e, x): +def f_expression(e: Union[TypeConversion, UnaryOperation, MemberAccess], x): e._expression = x @@ -86,7 +87,6 @@ class SplitTernaryExpression: f(false_expression, copy.copy(next_expr)) return True - # pylint: disable=too-many-branches def copy_expression( self, expression: Expression, true_expression: Expression, false_expression: Expression ) -> None: @@ -102,108 +102,114 @@ class SplitTernaryExpression: ): return - # case of lib - # (.. ? .. : ..).add - if isinstance(expression, MemberAccess): - next_expr = expression.expression - if self.conditional_not_ahead( - next_expr, true_expression, false_expression, f_expression - ): - self.copy_expression( - next_expr, true_expression.expression, false_expression.expression - ) - # pylint: disable=too-many-nested-blocks elif isinstance(expression, (AssignmentOperation, BinaryOperation, TupleExpression)): true_expression._expressions = [] false_expression._expressions = [] - for next_expr in expression.expressions: - # TODO: can we get rid of `NoneType` expressions in `TupleExpression`? - if next_expr: - if isinstance(next_expr, IndexAccess): - # create an index access for each branch - # x[if cond ? 1 : 2] -> if cond { x[1] } else { x[2] } - for expr in next_expr.expressions: - if self.conditional_not_ahead( - expr, true_expression, false_expression, f_expressions - ): - self.copy_expression( - expr, - true_expression.expressions[-1], - false_expression.expressions[-1], - ) - - if self.conditional_not_ahead( - next_expr, true_expression, false_expression, f_expressions - ): - # always on last arguments added - self.copy_expression( - next_expr, - true_expression.expressions[-1], - false_expression.expressions[-1], - ) + self.convert_expressions(expression, true_expression, false_expression) elif isinstance(expression, CallExpression): next_expr = expression.called + self.convert_call_expression(expression, next_expr, true_expression, false_expression) + + elif isinstance(expression, (TypeConversion, UnaryOperation, MemberAccess)): + next_expr = expression.expression + if self.conditional_not_ahead( + next_expr, true_expression, false_expression, f_expression + ): + self.copy_expression( + expression.expression, + true_expression.expression, + false_expression.expression, + ) - # case of lib - # (.. ? .. : ..).add - if self.conditional_not_ahead(next_expr, true_expression, false_expression, f_called): - self.copy_expression(next_expr, true_expression.called, false_expression.called) + else: + raise SlitherException( + f"Ternary operation not handled {expression}({type(expression)})" + ) - # In order to handle ternaries in both call options, gas and value, we return early if the - # conditional is not ahead to rewrite both ternaries (see `_rewrite_ternary_as_if_else`). - if expression.call_gas: - # case of (..).func{gas: .. ? .. : ..}() - next_expr = expression.call_gas - if self.conditional_not_ahead( - next_expr, true_expression, false_expression, f_call_gas - ): - self.copy_expression( - next_expr, - true_expression.call_gas, - false_expression.call_gas, - ) - else: - return + def convert_expressions( + self, + expression: Union[AssignmentOperation, BinaryOperation, TupleExpression], + true_expression: Expression, + false_expression: Expression, + ) -> None: + for next_expr in expression.expressions: + # TODO: can we get rid of `NoneType` expressions in `TupleExpression`? + if next_expr: + if isinstance(next_expr, IndexAccess): + self.convert_index_access(next_expr, true_expression, false_expression) - if expression.call_value: - # case of (..).func{value: .. ? .. : ..}() - next_expr = expression.call_value if self.conditional_not_ahead( - next_expr, true_expression, false_expression, f_call_value + next_expr, true_expression, false_expression, f_expressions ): + # always on last arguments added self.copy_expression( next_expr, - true_expression.call_value, - false_expression.call_value, + true_expression.expressions[-1], + false_expression.expressions[-1], ) - else: - return - true_expression._arguments = [] - false_expression._arguments = [] + def convert_index_access( + self, next_expr: IndexAccess, true_expression: Expression, false_expression: Expression + ) -> None: + # create an index access for each branch + # x[if cond ? 1 : 2] -> if cond { x[1] } else { x[2] } + for expr in next_expr.expressions: + if self.conditional_not_ahead(expr, true_expression, false_expression, f_expressions): + self.copy_expression( + expr, + true_expression.expressions[-1], + false_expression.expressions[-1], + ) - for next_expr in expression.arguments: - if self.conditional_not_ahead(next_expr, true_expression, false_expression, f_call): - # always on last arguments added - self.copy_expression( - next_expr, - true_expression.arguments[-1], - false_expression.arguments[-1], - ) + def convert_call_expression( + self, + expression: CallExpression, + next_expr: Expression, + true_expression: Expression, + false_expression: Expression, + ) -> None: + # case of lib + # (.. ? .. : ..).add + if self.conditional_not_ahead(next_expr, true_expression, false_expression, f_called): + self.copy_expression(next_expr, true_expression.called, false_expression.called) + + # In order to handle ternaries in both call options, gas and value, we return early if the + # conditional is not ahead to rewrite both ternaries (see `_rewrite_ternary_as_if_else`). + if expression.call_gas: + # case of (..).func{gas: .. ? .. : ..}() + next_expr = expression.call_gas + if self.conditional_not_ahead(next_expr, true_expression, false_expression, f_call_gas): + self.copy_expression( + next_expr, + true_expression.call_gas, + false_expression.call_gas, + ) + else: + return - elif isinstance(expression, (TypeConversion, UnaryOperation)): - next_expr = expression.expression + if expression.call_value: + # case of (..).func{value: .. ? .. : ..}() + next_expr = expression.call_value if self.conditional_not_ahead( - next_expr, true_expression, false_expression, f_expression + next_expr, true_expression, false_expression, f_call_value ): self.copy_expression( - expression.expression, - true_expression.expression, - false_expression.expression, + next_expr, + true_expression.call_value, + false_expression.call_value, ) + else: + return - else: - raise SlitherException( - f"Ternary operation not handled {expression}({type(expression)})" - ) + true_expression._arguments = [] + false_expression._arguments = [] + + for expr in expression.arguments: + if self.conditional_not_ahead(expr, true_expression, false_expression, f_call): + # always on last arguments added + self.copy_expression( + expr, + true_expression.arguments[-1], + false_expression.arguments[-1], + ) From b68f4c17a7aedfe0382828b85e2ecd95dca4fb38 Mon Sep 17 00:00:00 2001 From: alpharush <0xalpharush@protonmail.com> Date: Tue, 3 Jan 2023 12:31:47 -0600 Subject: [PATCH 14/15] pylint --- slither/utils/expression_manipulations.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slither/utils/expression_manipulations.py b/slither/utils/expression_manipulations.py index 974c6f68b..bc2a1556e 100644 --- a/slither/utils/expression_manipulations.py +++ b/slither/utils/expression_manipulations.py @@ -102,7 +102,7 @@ class SplitTernaryExpression: ): return - elif isinstance(expression, (AssignmentOperation, BinaryOperation, TupleExpression)): + if isinstance(expression, (AssignmentOperation, BinaryOperation, TupleExpression)): true_expression._expressions = [] false_expression._expressions = [] self.convert_expressions(expression, true_expression, false_expression) From 254f02b374d906df619edebe099af8f61eaef6e0 Mon Sep 17 00:00:00 2001 From: Feist Josselin Date: Thu, 5 Jan 2023 11:02:43 +0100 Subject: [PATCH 15/15] Update expression_manipulations.py --- slither/utils/expression_manipulations.py | 1 + 1 file changed, 1 insertion(+) diff --git a/slither/utils/expression_manipulations.py b/slither/utils/expression_manipulations.py index bc2a1556e..a63db9829 100644 --- a/slither/utils/expression_manipulations.py +++ b/slither/utils/expression_manipulations.py @@ -135,6 +135,7 @@ class SplitTernaryExpression: ) -> None: for next_expr in expression.expressions: # TODO: can we get rid of `NoneType` expressions in `TupleExpression`? + # montyly: this might happen with unnamed tuple (ex: (,,,) = f()), but it needs to be checked if next_expr: if isinstance(next_expr, IndexAccess): self.convert_index_access(next_expr, true_expression, false_expression)