From b319c1f88e317b7b2c55600614f4a363912c2ee5 Mon Sep 17 00:00:00 2001 From: olegggatttor Date: Wed, 7 Feb 2024 12:56:18 +0200 Subject: [PATCH 1/3] handle if with reverts --- slitherin/detectors/unprotected_initialize.py | 11 ++++++++++- tests/unprotected_initialize_test.sol | 8 ++++++++ 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/slitherin/detectors/unprotected_initialize.py b/slitherin/detectors/unprotected_initialize.py index 6e4d55f..4adcd69 100644 --- a/slitherin/detectors/unprotected_initialize.py +++ b/slitherin/detectors/unprotected_initialize.py @@ -43,6 +43,14 @@ def _has_require(self, fun: Function) -> bool: if str(variable.type) == "address": return True return False + + def _has_if_with_reverts(self, fun: Function) -> bool: + for node in fun.nodes: + if node.contains_if(): + for child in node.sons: + if "revert()" in str(child) or "revert(string)" in str(child): + return True + return False def _detect(self) -> List[Output]: """Main function""" @@ -55,7 +63,8 @@ def _detect(self) -> List[Output]: if x: is_safe = self._has_modifiers(f) is_safe2 = self._has_require(f) - if not is_safe and not is_safe2: + is_safe3 = self._has_if_with_reverts(f) + if not is_safe and not is_safe2 and not is_safe3: res.append( self.generate_result( [ diff --git a/tests/unprotected_initialize_test.sol b/tests/unprotected_initialize_test.sol index edb753f..662f235 100644 --- a/tests/unprotected_initialize_test.sol +++ b/tests/unprotected_initialize_test.sol @@ -1,3 +1,4 @@ +//SPDX-License-Identifier: UNLICENSED pragma solidity ^0.8.0; interface I { @@ -43,4 +44,11 @@ contract unprotected_initialize { function init_vuln(uint256 setter) external { toSet = setter; } + + function init_ok3(uint256 setter) external { + if (msg.sender != owner) { + revert("Not owner"); + } + toSet = setter; + } } From 71768cce1a399de9f4bf9ada97020c9c88993664 Mon Sep 17 00:00:00 2001 From: olegggatttor Date: Thu, 8 Feb 2024 15:41:48 +0200 Subject: [PATCH 2/3] add arithmetic overflow detector --- docs/potential_arith_overflow.md | 21 +++ slitherin/__init__.py | 2 + .../detectors/potential_arith_overflow.py | 122 +++++++++++++++++ tests/potential_arith_overflow.sol | 129 ++++++++++++++++++ 4 files changed, 274 insertions(+) create mode 100644 docs/potential_arith_overflow.md create mode 100644 slitherin/detectors/potential_arith_overflow.py create mode 100644 tests/potential_arith_overflow.sol diff --git a/docs/potential_arith_overflow.md b/docs/potential_arith_overflow.md new file mode 100644 index 0000000..d96c648 --- /dev/null +++ b/docs/potential_arith_overflow.md @@ -0,0 +1,21 @@ +# Potential arithmetic overflow + +## Configuration +* Check: `pess-potential-arithmetic-overflow` +* Severity: `Medium` +* Confidence: `Medium` + +## Description +The detector sees if there are assignments/returns that calculate some arithmetic expressions and if some intermediate calculations +contain a type that is lower than the expected result. Such behavior may lead to unexpected overflow/underflow, e.g., trying to assign the multiplication of two `uint48` variables to `uint256` would look like `uint48 * uint48` and it may overflow (however, the final type would fit such multiplication). + +### Potential Improvement +- Handle return statements that return tuples of arithmetic expressions; +- Handle signed integer underflow for subtraction operation e.g. `int256 = int64 - int64` should produce error (`int64` should be cast to `int256`). +- Improve the output of the detector (unroll complex expressions in something readable, not just `...`) + +## Vulnerable Scenario +[test scenario](../tests/potential_arith_overflow.sol) + +## Recommendation +Use explicit type casting in sub expressions when the assigment to a larger type is performed. \ No newline at end of file diff --git a/slitherin/__init__.py b/slitherin/__init__.py index f39a225..35c1218 100644 --- a/slitherin/__init__.py +++ b/slitherin/__init__.py @@ -30,6 +30,7 @@ from slitherin.detectors.arbitrum.block_number_timestamp import ( ArbitrumBlockNumberTimestamp, ) +from slitherin.detectors.potential_arith_overflow import PotentialArithmOverflow artbitrum_detectors = [ ArbitrumPrevrandaoDifficulty, @@ -61,6 +62,7 @@ Ecrecover, PublicVsExternal, AAVEFlashloanCallbackDetector, + PotentialArithmOverflow ] plugin_printers = [] diff --git a/slitherin/detectors/potential_arith_overflow.py b/slitherin/detectors/potential_arith_overflow.py new file mode 100644 index 0000000..2f9d66a --- /dev/null +++ b/slitherin/detectors/potential_arith_overflow.py @@ -0,0 +1,122 @@ +from typing import List +from slither.utils.output import Output +from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification +from slither.core.declarations import Function +from slither.core.solidity_types.elementary_type import Int, Uint +from slither.core.variables.local_variable import LocalVariable +import slither.slithir.operations as ops +import slither.slithir.variables as vrs +from slither.core.cfg.node import NodeType +import typing as tp + +INT_TYPES = [*Int, *Uint] + +class PotentialArithmOverflow(AbstractDetector): + """ + Detects expressions where overflow may occur, but no overflow is expected + """ + + ARGUMENT = "pess-potential-arithmetic-overflow" # slither will launch the detector with slither.py --detect mydetector + HELP = "Add explicit type casting in temporary expressions during the evaluation of arithmetic expressions in case the final type is larger than the intermediate one." + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.MEDIUM + + WIKI = ( + "https://github.com/pessimistic-io/slitherin/blob/master/docs/potential_arith_overflow.md" + ) + WIKI_TITLE = "Potential Arithmetic Overflow" + WIKI_DESCRIPTION = "The detector sees if there are assignments/returns that calculate some arithmetic expressions and if some intermediate calculations contain a type that is lower than the expected result. Such behavior may lead to unexpected overflow/underflow, e.g., trying to assign the multiplication of two `uint48` variables to `uint256` would look like `uint48 * uint48` and it may overflow (however, the final type would fit such multiplication)." + WIKI_EXPLOIT_SCENARIO = "A transaction will revert with overflow, but the expected behavior that it should not." + WIKI_RECOMMENDATION = "Use explicit type casting in sub expressions when the assigment to a larger type is performed." + + def _has_op_overflowing_sub_expression(self, op: ops.Operation, high_level_bits_needed: tp.Optional[int]) -> (bool, list, str): + if isinstance(op, ops.Unary): + (is_err, nodes, sub_expr) = self._has_op_overflowing_sub_expression(op.rvalue, high_level_bits_needed) # ! and ~ do not affect result + return (is_err, nodes, f"{op.type_str} {sub_expr}") + elif isinstance(op, ops.Binary): + result_type = str(op.lvalue.type) + [l_check_res, l_check_list, l_sub_expr] = self._has_op_overflowing_sub_expression(op.variable_left, high_level_bits_needed) + [r_check_res, r_check_list, r_sub_expr] = self._has_op_overflowing_sub_expression(op.variable_right, high_level_bits_needed) + + + if l_check_res or r_check_res: # overflow possible in sub expression (left or right part) + return (True, []) + + cur_expr = f"{l_sub_expr} {op._type.value} {r_sub_expr}" + if op._type is ops.BinaryType.ADDITION or op._type is ops.BinaryType.MULTIPLICATION or op._type is ops.BinaryType.POWER: + result_bits_cut = result_type.removeprefix("uint").removeprefix("int") + result_bits = int(256 if not result_bits_cut else result_bits_cut) + if high_level_bits_needed and high_level_bits_needed > result_bits: # result expects bigger type, but expression returns lower and overflow may occur here + return (True, [(cur_expr, result_type), *l_check_list, *r_check_list], cur_expr) + else: + return (l_check_res or r_check_res, [*l_check_list, *r_check_list], cur_expr) + else: # @todo currently we do not check other operations, return as everything is ok + return (l_check_res or r_check_res, [*l_check_list, *r_check_list], cur_expr) + elif isinstance(op, ops.OperationWithLValue): + return (False, [], f"{op.lvalue}") + elif isinstance(op, vrs.Constant): + return (False, [], f"{op}") + elif isinstance(op, LocalVariable): + return (False, [], f"{op}") + elif isinstance(op, vrs.state_variable.StateVariable): + return (False, [], f"{op}") + elif isinstance(op, vrs.TemporaryVariable): + return (False, [], f"...") + elif isinstance(op, ops.Return): + return (False, [], f"{op}") + elif isinstance(op, vrs.ReferenceVariable): + return (False, [], f"{op}") + else: + return (False, [], f"{op}") + + def _find_vulnerable_expressions(self, fun: Function) -> list: + final_results = [] + is_active_assembly = False + for node in fun.nodes: + if node.type is NodeType.ASSEMBLY: + is_active_assembly = True + elif node.type is NodeType.ENDASSEMBLY: + is_active_assembly = False + if (node.type is NodeType.VARIABLE or node.type is NodeType.EXPRESSION or node.type is NodeType.RETURN) and not is_active_assembly: + irs = node.irs + if len(irs) > 0 and isinstance(irs[-1], ops.Assignment) and str(irs[-1].lvalue._type) in INT_TYPES: + expected_bits_cut = str(irs[-1].lvalue._type).removeprefix("uint").removeprefix("int") + expected_bits = int(256 if not expected_bits_cut else expected_bits_cut) + has_problems = False + errors = [] + for x in irs[:-1]: + (response_res, response_errors, _) = self._has_op_overflowing_sub_expression(x, expected_bits) + has_problems |= response_res + errors.extend(response_errors) + if has_problems: + final_results.append((node, str(irs[-1].lvalue._type), errors)) + if len(irs) > 0 and isinstance(irs[-1], ops.Return) and len(fun.return_type) == 1 and str(fun.return_type[0]) in INT_TYPES: # @todo currently works only with single returns + expected_bits_cut = str(fun.return_type[0]).removeprefix("uint").removeprefix("int") + expected_bits = int(256 if not expected_bits_cut else expected_bits_cut) + has_problems = False + errors = [] + for x in irs[:-1]: + (response_res, response_errors, _) = self._has_op_overflowing_sub_expression(x, expected_bits) + has_problems |= response_res + errors.extend(response_errors) + if has_problems: + final_results.append((node, str(fun.return_type[0]), errors)) + + return final_results + + def _detect(self) -> List[Output]: + """Main function""" + res = [] + for contract in self.compilation_unit.contracts_derived: + if contract.is_interface: + continue + for f in contract.functions_and_modifiers_declared: + vulnerable_expressions = self._find_vulnerable_expressions(f) + if vulnerable_expressions: + info = [f, f" contains integer variables whose type is larger than the type of one of its intermediate expressions. Consider casting sub expressions explicitly as they might lead to unexpected overflow:\n"] + for (node, node_final_type, op_with_ret_type) in vulnerable_expressions: + info += ["\tIn `", node, "` intermidiate expressions returns type of lower order:", "\n"] + for (op, op_ret_type) in op_with_ret_type: + info += ["\t\t`", str(op), f"` returns {op_ret_type}, but the type of the resulting expression is {node_final_type}.", "\n"] + res.append(self.generate_result(info)) + return res diff --git a/tests/potential_arith_overflow.sol b/tests/potential_arith_overflow.sol new file mode 100644 index 0000000..af4b8c4 --- /dev/null +++ b/tests/potential_arith_overflow.sol @@ -0,0 +1,129 @@ +// SPDX-License-Identifier: SEE LICENSE IN LICENSE +pragma solidity ^0.8.0; + +contract Test { + uint16 t; + + struct TStruct { + uint160 xx; + } + + function decimals16() public returns (uint16) { + return 10 ** 4; + } + + /* + `a` has type `uint64` + `decimals16()` returns `uint16` + `x` has type `uint256` + => Printing warning because mul of uint64 and uint16 may overflow (but casting `a` or the result of `decimals16()` safes from it). + */ + function test_vuln1(uint64 a) external { + uint256 x = a * decimals16(); + } + + function test_vuln2() external returns (uint256 result) { + uint48 x = 10; + uint48 y = type(uint48).max; + uint k = 1; + result = x * y / k; + } + + function test_vuln2_wrong_fix() external returns (uint256 result) { + uint48 x = 10; + uint48 y = type(uint48).max; + uint k = 1; + result = uint256(x * y) / k; + } + + function test_vuln3() external returns (uint256 result) { + uint48 x; + uint32 y; + result = x + y; + } + + function test_vuln4() external returns (uint256 result) { + uint8 x = 10; + uint8 y = 20; + result = x ** y; + } + + function test_vuln5() external returns (uint256) { // @todo handle expression in return statements + uint8 x = 10; + uint8 y = 20; + return x ** y; + } + + function test_vuln6() external returns (uint256 result) { + uint8 x = 10; + result = t * x; + } + + function test_vuln7() external returns (int64 result) { + int8 x = 10; + int32 y; + result = y * x + 1; + } + + function test_vuln8() external returns (int64 result) { + int8 x = 10; + int32 y; + result = y * x + 1; + + result = x * y - 10 + 100; + + result = x * y * x * y; + } + + function test_vuln9() external returns (uint64, uint128) { // @todo currently we do not handle tuple returns + uint32 a; + uint32 b; + return (a + b, uint128(a) + b); + } + + function test_vuln10() external returns (uint64) { + uint32 a; + uint32 b; + return a + b; + } + + function test_vuln11() external { + TStruct memory s = TStruct({xx: 120}); + uint16 a; + uint32 b; + s.xx = a + b; + } + + function test_ok1() external returns (uint128 result) { + uint128 a; + uint64 b; + result = a * b; + } + + function test_ok2() external returns (uint128) { // @todo handle expression in return statements + uint128 a; + uint64 b; + return a * b; + } + + function test_ok3() external returns (uint128 result) { + uint128 a; + uint64 b; + uint32 c; + uint16 d; + result = (a * b) / c + d; + } + + function test_fix_vuln1_ok4(uint64 a) external { + uint256 x = uint256(a) * decimals16(); + } + + function test_fix_vuln1_slither_bug(uint64 a) external { // @note for some reason slither invokes type incorrectly in this case. So fix need to be applied on the left side of binary expression + uint256 x = a * uint256(decimals16()); + } + + function test_should_not_trigger(uint64 a) external { // @note for some reason slither + bytes32 a = bytes32(uint256(1)); + address x = address(uint160(1) + uint160(2)); + } +} \ No newline at end of file From 9a0e3a5fa6e54a3cbace8fdfc18849ec591a4465 Mon Sep 17 00:00:00 2001 From: olegggatttor Date: Thu, 8 Feb 2024 15:55:09 +0200 Subject: [PATCH 3/3] small fix --- slitherin/detectors/potential_arith_overflow.py | 4 ---- 1 file changed, 4 deletions(-) diff --git a/slitherin/detectors/potential_arith_overflow.py b/slitherin/detectors/potential_arith_overflow.py index 2f9d66a..61ce44c 100644 --- a/slitherin/detectors/potential_arith_overflow.py +++ b/slitherin/detectors/potential_arith_overflow.py @@ -37,10 +37,6 @@ def _has_op_overflowing_sub_expression(self, op: ops.Operation, high_level_bits_ result_type = str(op.lvalue.type) [l_check_res, l_check_list, l_sub_expr] = self._has_op_overflowing_sub_expression(op.variable_left, high_level_bits_needed) [r_check_res, r_check_list, r_sub_expr] = self._has_op_overflowing_sub_expression(op.variable_right, high_level_bits_needed) - - - if l_check_res or r_check_res: # overflow possible in sub expression (left or right part) - return (True, []) cur_expr = f"{l_sub_expr} {op._type.value} {r_sub_expr}" if op._type is ops.BinaryType.ADDITION or op._type is ops.BinaryType.MULTIPLICATION or op._type is ops.BinaryType.POWER: