From d2bf6ce06438af9af1fdf1c0182dd64e8b4b3b46 Mon Sep 17 00:00:00 2001 From: Josselin Date: Wed, 25 Nov 2020 12:53:29 +0100 Subject: [PATCH] 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: