From 045a5c8adbb5de4dacafb9999df9e46aa95b81b8 Mon Sep 17 00:00:00 2001 From: Antonio Guilherme Ferreira Viggiano Date: Thu, 5 Jan 2023 07:40:57 -0300 Subject: [PATCH 1/6] Add incorrect safeApprove detector --- slither/detectors/all_detectors.py | 1 + .../erc/erc20/incorrect_safe_approve.py | 87 +++++++++++++++++++ .../0.7.6/erc20_incorrect_safe_approve.sol | 44 ++++++++++ 3 files changed, 132 insertions(+) create mode 100644 slither/detectors/erc/erc20/incorrect_safe_approve.py create mode 100644 tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index fb2e3c731c..2f89133acc 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -87,3 +87,4 @@ from .functions.protected_variable import ProtectedVariables from .functions.permit_domain_signature_collision import DomainSeparatorCollision from .functions.codex import Codex +from .erc.erc20.incorrect_safe_approve import IncorrectSafeApprove diff --git a/slither/detectors/erc/erc20/incorrect_safe_approve.py b/slither/detectors/erc/erc20/incorrect_safe_approve.py new file mode 100644 index 0000000000..9af96ae691 --- /dev/null +++ b/slither/detectors/erc/erc20/incorrect_safe_approve.py @@ -0,0 +1,87 @@ +""" +Module detecting incorrect safeApprove usage +""" + +from slither.core.declarations.function import Function +from slither.core.variables.variable import Variable +from slither.detectors.abstract_detector import (AbstractDetector, + DetectorClassification) +from slither.slithir.variables.constant import Constant + + +class IncorrectSafeApprove(AbstractDetector): + """ + Incorrect safeApprove usage detector + """ + + ARGUMENT = "incorrect-safe-approve" + HELP = "Detects incorrect safeApprove usage" + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.HIGH + + WIKI = ( + "https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-safe-approve" + ) + + WIKI_TITLE = "Incorrect safeApprove usage" + WIKI_DESCRIPTION = "The `safeApprove` function of the OpenZeppelin `SafeERC20` library prevents changing an allowance between non-zero values to mitigate a possible front-running attack. Instead, the `safeIncreaseAllowance` and `safeDecreaseAllowance` functions should be used. `safeApprove` should only be called when setting an initial allowance, or when resetting it to zero." + + # region wiki_exploit_scenario + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract Contract { + function _deposit() internal (uint256) { + if(token.allowance(address(this), address(vault)) < token.balanceOf(address(this))) { + token.safeApprove(address(vault), type(uint256).max); + } + return vault.deposit(); + } +} +``` +If the existing allowance is non-zero, then `safeApprove` will revert, causing deposits to fail leading to denial-of-service.""" + # endregion wiki_exploit_scenario + + WIKI_RECOMMENDATION = "Use `safeIncreaseAllowance` and `safeDecreaseAllowance` to change an allowance between non-zero values." + + @staticmethod + def is_zero_argument(f): + for node in f.nodes: + for ir in node.irs: + for read in ir.read: + if isinstance(read, Constant): + return read == 0 + elif isinstance(read, Variable) and read.expression is not None: + return str(read.expression) == "0" + + def detect_safe_approve_non_zero(self, contract): + ret = [] + for f in contract.functions_declared: + calls = [ + f_called[1].solidity_signature + for f_called in f.high_level_calls + if isinstance(f_called[1], Function) + ] + if ( + "safeApprove(address,address,uint256)" in calls and + not self.is_zero_argument(f) + ): + ret.append(f) + return ret + + def _detect(self): + """Detects incorrect safeApprove usage""" + results = [] + for c in self.contracts: + functions = self.detect_safe_approve_non_zero(c) + for func in functions: + + info = [ + func, + " calls `safeApprove` with a non-zero value\n", + ] + + res = self.generate_result(info) + + results.append(res) + + return results diff --git a/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol b/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol new file mode 100644 index 0000000000..626f567816 --- /dev/null +++ b/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol @@ -0,0 +1,44 @@ +interface IERC20 {} + +library SafeERC20 { + function safeApprove( + IERC20 token, + address spender, + uint256 value + ) internal {} +} + +contract C { + using SafeERC20 for IERC20; + IERC20 token; + uint256 zero = 0; + + function good1() public { + token.safeApprove(address(this), 0); + } + + function good2() public { + SafeERC20.safeApprove(token, address(this), 0); + } + + function good3() public { + token.safeApprove(address(this), zero); + } + + function good4(uint256 value) public { + token.safeApprove(address(this), 0); + good4(value); + } + + function bad1() public { + token.safeApprove(address(this), type(uint256).max); + } + + function bad2() public { + SafeERC20.safeApprove(token, address(this), type(uint256).max); + } + + function bad3(uint256 value) public { + token.safeApprove(address(this), value); + } +} From 903ef3f0f9ec7b2485a5091e271f4ab114a404fe Mon Sep 17 00:00:00 2001 From: Antonio Guilherme Ferreira Viggiano Date: Thu, 5 Jan 2023 08:14:01 -0300 Subject: [PATCH 2/6] Include test --- slither/detectors/all_detectors.py | 2 +- slither/detectors/erc/{erc20 => }/incorrect_safe_approve.py | 4 ++-- tests/test_detectors.py | 5 +++++ 3 files changed, 8 insertions(+), 3 deletions(-) rename slither/detectors/erc/{erc20 => }/incorrect_safe_approve.py (97%) diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 2f89133acc..0a4dfd8548 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -40,6 +40,7 @@ from .erc.erc20.incorrect_erc20_interface import IncorrectERC20InterfaceDetection from .erc.incorrect_erc721_interface import IncorrectERC721InterfaceDetection from .erc.unindexed_event_parameters import UnindexedERC20EventParameters +from .erc.incorrect_safe_approve import IncorrectSafeApprove from .statements.deprecated_calls import DeprecatedStandards from .source.rtlo import RightToLeftOverride from .statements.too_many_digits import TooManyDigits @@ -87,4 +88,3 @@ from .functions.protected_variable import ProtectedVariables from .functions.permit_domain_signature_collision import DomainSeparatorCollision from .functions.codex import Codex -from .erc.erc20.incorrect_safe_approve import IncorrectSafeApprove diff --git a/slither/detectors/erc/erc20/incorrect_safe_approve.py b/slither/detectors/erc/incorrect_safe_approve.py similarity index 97% rename from slither/detectors/erc/erc20/incorrect_safe_approve.py rename to slither/detectors/erc/incorrect_safe_approve.py index 9af96ae691..e7ab5e0ee8 100644 --- a/slither/detectors/erc/erc20/incorrect_safe_approve.py +++ b/slither/detectors/erc/incorrect_safe_approve.py @@ -14,13 +14,13 @@ class IncorrectSafeApprove(AbstractDetector): Incorrect safeApprove usage detector """ - ARGUMENT = "incorrect-safe-approve" + ARGUMENT = "erc20-incorrect-safe-approve" HELP = "Detects incorrect safeApprove usage" IMPACT = DetectorClassification.MEDIUM CONFIDENCE = DetectorClassification.HIGH WIKI = ( - "https://github.com/crytic/slither/wiki/Detector-Documentation#incorrect-safe-approve" + "https://github.com/crytic/slither/wiki/Detector-Documentation#erc20-incorrect-safe-approve" ) WIKI_TITLE = "Incorrect safeApprove usage" diff --git a/tests/test_detectors.py b/tests/test_detectors.py index a5ecaae2aa..3f021dd88d 100644 --- a/tests/test_detectors.py +++ b/tests/test_detectors.py @@ -236,6 +236,11 @@ def id_test(test_item: Test): "incorrect_erc20_interface.sol", "0.7.6", ), + Test( + all_detectors.IncorrectSafeApprove, + "erc20_incorrect_safe_approve.sol", + "0.7.6", + ), Test( all_detectors.IncorrectERC721InterfaceDetection, "incorrect_erc721_interface.sol", From 16742f4eb60136422202768100c3edc97fdb3a4c Mon Sep 17 00:00:00 2001 From: Antonio Guilherme Ferreira Viggiano Date: Thu, 5 Jan 2023 08:21:26 -0300 Subject: [PATCH 3/6] Generate test artifact --- ...pprove.sol.0.7.6.IncorrectSafeApprove.json | 253 ++++++++++++++++++ 1 file changed, 253 insertions(+) create mode 100644 tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol.0.7.6.IncorrectSafeApprove.json diff --git a/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol.0.7.6.IncorrectSafeApprove.json b/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol.0.7.6.IncorrectSafeApprove.json new file mode 100644 index 0000000000..afb201a00c --- /dev/null +++ b/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol.0.7.6.IncorrectSafeApprove.json @@ -0,0 +1,253 @@ +[ + [ + { + "elements": [ + { + "type": "function", + "name": "bad3", + "source_mapping": { + "start": 829, + "length": 92, + "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "is_dependency": false, + "lines": [ + 41, + 42, + 43 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "C", + "source_mapping": { + "start": 157, + "length": 766, + "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "is_dependency": false, + "lines": [ + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34, + 35, + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "bad3(uint256)" + } + } + ], + "description": "C.bad3(uint256) (tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#41-43) calls `safeApprove` with a non-zero value\n", + "markdown": "[C.bad3(uint256)](tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L41-L43) calls `safeApprove` with a non-zero value\n", + "first_markdown_element": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L41-L43", + "id": "0e2bcb981fa9de58958bca199e6d7f294d6edaadb1fcc7e4f0235c761eee6bc5", + "check": "erc20-incorrect-safe-approve", + "impact": "Medium", + "confidence": "High" + }, + { + "elements": [ + { + "type": "function", + "name": "bad1", + "source_mapping": { + "start": 624, + "length": 91, + "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "is_dependency": false, + "lines": [ + 33, + 34, + 35 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "C", + "source_mapping": { + "start": 157, + "length": 766, + "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "is_dependency": false, + "lines": [ + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34, + 35, + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "bad1()" + } + } + ], + "description": "C.bad1() (tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#33-35) calls `safeApprove` with a non-zero value\n", + "markdown": "[C.bad1()](tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L33-L35) calls `safeApprove` with a non-zero value\n", + "first_markdown_element": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L33-L35", + "id": "5a61daaa853ba7b6502b414b31c0ea71b1e8ba1a7ecf2c492fdedabfead8fa2e", + "check": "erc20-incorrect-safe-approve", + "impact": "Medium", + "confidence": "High" + }, + { + "elements": [ + { + "type": "function", + "name": "bad2", + "source_mapping": { + "start": 721, + "length": 102, + "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "is_dependency": false, + "lines": [ + 37, + 38, + 39 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "C", + "source_mapping": { + "start": 157, + "length": 766, + "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", + "is_dependency": false, + "lines": [ + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34, + 35, + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "bad2()" + } + } + ], + "description": "C.bad2() (tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#37-39) calls `safeApprove` with a non-zero value\n", + "markdown": "[C.bad2()](tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L37-L39) calls `safeApprove` with a non-zero value\n", + "first_markdown_element": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L37-L39", + "id": "b28a6f139c8dba51776faf3b44e993507e497bb34d42b2b3892b1c9685d933e8", + "check": "erc20-incorrect-safe-approve", + "impact": "Medium", + "confidence": "High" + } + ] +] \ No newline at end of file From a95ed5c1dab985b8f3af7b0433e512ccb27ee1d9 Mon Sep 17 00:00:00 2001 From: Antonio Guilherme Ferreira Viggiano Date: Thu, 5 Jan 2023 08:40:56 -0300 Subject: [PATCH 4/6] Update incorrect safeApprove to deprecated --- slither/detectors/all_detectors.py | 2 +- .../detectors/erc/deprecated_safe_approve.py | 69 +++++ .../detectors/erc/incorrect_safe_approve.py | 87 ------ .../0.7.6/erc20_deprecated_safe_approve.sol | 22 ++ ...prove.sol.0.7.6.DeprecatedSafeApprove.json | 126 +++++++++ .../0.7.6/erc20_incorrect_safe_approve.sol | 44 --- ...pprove.sol.0.7.6.IncorrectSafeApprove.json | 253 ------------------ tests/test_detectors.py | 4 +- 8 files changed, 220 insertions(+), 387 deletions(-) create mode 100644 slither/detectors/erc/deprecated_safe_approve.py delete mode 100644 slither/detectors/erc/incorrect_safe_approve.py create mode 100644 tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol create mode 100644 tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol.0.7.6.DeprecatedSafeApprove.json delete mode 100644 tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol delete mode 100644 tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol.0.7.6.IncorrectSafeApprove.json diff --git a/slither/detectors/all_detectors.py b/slither/detectors/all_detectors.py index 0a4dfd8548..00d4c45e7d 100644 --- a/slither/detectors/all_detectors.py +++ b/slither/detectors/all_detectors.py @@ -40,7 +40,7 @@ from .erc.erc20.incorrect_erc20_interface import IncorrectERC20InterfaceDetection from .erc.incorrect_erc721_interface import IncorrectERC721InterfaceDetection from .erc.unindexed_event_parameters import UnindexedERC20EventParameters -from .erc.incorrect_safe_approve import IncorrectSafeApprove +from .erc.deprecated_safe_approve import DeprecatedSafeApprove from .statements.deprecated_calls import DeprecatedStandards from .source.rtlo import RightToLeftOverride from .statements.too_many_digits import TooManyDigits diff --git a/slither/detectors/erc/deprecated_safe_approve.py b/slither/detectors/erc/deprecated_safe_approve.py new file mode 100644 index 0000000000..9db81d3102 --- /dev/null +++ b/slither/detectors/erc/deprecated_safe_approve.py @@ -0,0 +1,69 @@ +""" +Module detecting incorrect safeApprove usage +""" + +from slither.core.declarations.function import Function +from slither.detectors.abstract_detector import (AbstractDetector, + DetectorClassification) + + +class DeprecatedSafeApprove(AbstractDetector): + """ + Deprecated safeApprove usage detector + """ + + ARGUMENT = "erc20-deprecated-safe-approve" + HELP = "Detects deprecated safeApprove usage" + IMPACT = DetectorClassification.MEDIUM + CONFIDENCE = DetectorClassification.HIGH + + WIKI = ( + "https://github.com/crytic/slither/wiki/Detector-Documentation#erc20-deprecated-safe-approve" + ) + + WIKI_TITLE = "Deprecated safeApprove usage" + WIKI_DESCRIPTION = "The purpose of this `safeApprove` is to check that a user is either setting their allowance to zero, or setting it from zero. This is so that the user doesn't try to re-set their allowance from one non-zero number to another non-zero number where they can get front-run by the approved address using their allowance in-between those two transactions. However, this function doesn't actually solve the problem as the approval will still pass if the sandwich attack uses the rest of the remaining allowance. Such behavior lends itself to bugs, unnecessary gas usage, and a false sense of security. Because of that, `safeApprove` has been deprecated." + + # region wiki_exploit_scenario + WIKI_EXPLOIT_SCENARIO = """ +```solidity +contract Contract { + function _deposit() internal (uint256) { + token.safeApprove(address(vault), 0); + } +} +``` +`safeApprove` has been deprecated.""" + # endregion wiki_exploit_scenario + + WIKI_RECOMMENDATION = "Use `safeIncreaseAllowance` and `safeDecreaseAllowance` to change an allowance." + + def detect_safe_approve(self, contract): + ret = [] + for f in contract.functions_declared: + calls = [ + f_called[1].solidity_signature + for f_called in f.high_level_calls + if isinstance(f_called[1], Function) + ] + if ("safeApprove(address,address,uint256)" in calls): + ret.append(f) + return ret + + def _detect(self): + """Detects safeApprove usage""" + results = [] + for c in self.contracts: + functions = self.detect_safe_approve(c) + for func in functions: + + info = [ + func, + " calls `safeApprove`, which has been deprecated\n", + ] + + res = self.generate_result(info) + + results.append(res) + + return results diff --git a/slither/detectors/erc/incorrect_safe_approve.py b/slither/detectors/erc/incorrect_safe_approve.py deleted file mode 100644 index e7ab5e0ee8..0000000000 --- a/slither/detectors/erc/incorrect_safe_approve.py +++ /dev/null @@ -1,87 +0,0 @@ -""" -Module detecting incorrect safeApprove usage -""" - -from slither.core.declarations.function import Function -from slither.core.variables.variable import Variable -from slither.detectors.abstract_detector import (AbstractDetector, - DetectorClassification) -from slither.slithir.variables.constant import Constant - - -class IncorrectSafeApprove(AbstractDetector): - """ - Incorrect safeApprove usage detector - """ - - ARGUMENT = "erc20-incorrect-safe-approve" - HELP = "Detects incorrect safeApprove usage" - IMPACT = DetectorClassification.MEDIUM - CONFIDENCE = DetectorClassification.HIGH - - WIKI = ( - "https://github.com/crytic/slither/wiki/Detector-Documentation#erc20-incorrect-safe-approve" - ) - - WIKI_TITLE = "Incorrect safeApprove usage" - WIKI_DESCRIPTION = "The `safeApprove` function of the OpenZeppelin `SafeERC20` library prevents changing an allowance between non-zero values to mitigate a possible front-running attack. Instead, the `safeIncreaseAllowance` and `safeDecreaseAllowance` functions should be used. `safeApprove` should only be called when setting an initial allowance, or when resetting it to zero." - - # region wiki_exploit_scenario - WIKI_EXPLOIT_SCENARIO = """ -```solidity -contract Contract { - function _deposit() internal (uint256) { - if(token.allowance(address(this), address(vault)) < token.balanceOf(address(this))) { - token.safeApprove(address(vault), type(uint256).max); - } - return vault.deposit(); - } -} -``` -If the existing allowance is non-zero, then `safeApprove` will revert, causing deposits to fail leading to denial-of-service.""" - # endregion wiki_exploit_scenario - - WIKI_RECOMMENDATION = "Use `safeIncreaseAllowance` and `safeDecreaseAllowance` to change an allowance between non-zero values." - - @staticmethod - def is_zero_argument(f): - for node in f.nodes: - for ir in node.irs: - for read in ir.read: - if isinstance(read, Constant): - return read == 0 - elif isinstance(read, Variable) and read.expression is not None: - return str(read.expression) == "0" - - def detect_safe_approve_non_zero(self, contract): - ret = [] - for f in contract.functions_declared: - calls = [ - f_called[1].solidity_signature - for f_called in f.high_level_calls - if isinstance(f_called[1], Function) - ] - if ( - "safeApprove(address,address,uint256)" in calls and - not self.is_zero_argument(f) - ): - ret.append(f) - return ret - - def _detect(self): - """Detects incorrect safeApprove usage""" - results = [] - for c in self.contracts: - functions = self.detect_safe_approve_non_zero(c) - for func in functions: - - info = [ - func, - " calls `safeApprove` with a non-zero value\n", - ] - - res = self.generate_result(info) - - results.append(res) - - return results diff --git a/tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol b/tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol new file mode 100644 index 0000000000..00455aaba5 --- /dev/null +++ b/tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol @@ -0,0 +1,22 @@ +interface IERC20 {} + +library SafeERC20 { + function safeApprove( + IERC20 token, + address spender, + uint256 value + ) internal {} +} + +contract C { + using SafeERC20 for IERC20; + IERC20 token; + + function bad1() public { + token.safeApprove(address(this), 0); + } + + function bad2() public { + SafeERC20.safeApprove(token, address(this), 0); + } +} diff --git a/tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol.0.7.6.DeprecatedSafeApprove.json b/tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol.0.7.6.DeprecatedSafeApprove.json new file mode 100644 index 0000000000..f5b7e3281e --- /dev/null +++ b/tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol.0.7.6.DeprecatedSafeApprove.json @@ -0,0 +1,126 @@ +[ + [ + { + "elements": [ + { + "type": "function", + "name": "bad2", + "source_mapping": { + "start": 306, + "length": 86, + "filename_relative": "tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol", + "is_dependency": false, + "lines": [ + 19, + 20, + 21 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "C", + "source_mapping": { + "start": 157, + "length": 237, + "filename_relative": "tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol", + "is_dependency": false, + "lines": [ + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "bad2()" + } + } + ], + "description": "C.bad2() (tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol#19-21) calls `safeApprove`, which has been deprecated\n", + "markdown": "[C.bad2()](tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol#L19-L21) calls `safeApprove`, which has been deprecated\n", + "first_markdown_element": "tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol#L19-L21", + "id": "c005c70720e5e33036bffe946533c62fc6248465826af62df2c2ed3f2de31470", + "check": "erc20-deprecated-safe-approve", + "impact": "Medium", + "confidence": "High" + }, + { + "elements": [ + { + "type": "function", + "name": "bad1", + "source_mapping": { + "start": 225, + "length": 75, + "filename_relative": "tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol", + "is_dependency": false, + "lines": [ + 15, + 16, + 17 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "C", + "source_mapping": { + "start": 157, + "length": 237, + "filename_relative": "tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol", + "is_dependency": false, + "lines": [ + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "bad1()" + } + } + ], + "description": "C.bad1() (tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol#15-17) calls `safeApprove`, which has been deprecated\n", + "markdown": "[C.bad1()](tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol#L15-L17) calls `safeApprove`, which has been deprecated\n", + "first_markdown_element": "tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol#L15-L17", + "id": "e03bc1e0b71bac015bd267f09eb5ade627b580bb0e1666d43b54bb8fda21e292", + "check": "erc20-deprecated-safe-approve", + "impact": "Medium", + "confidence": "High" + } + ] +] \ No newline at end of file diff --git a/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol b/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol deleted file mode 100644 index 626f567816..0000000000 --- a/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol +++ /dev/null @@ -1,44 +0,0 @@ -interface IERC20 {} - -library SafeERC20 { - function safeApprove( - IERC20 token, - address spender, - uint256 value - ) internal {} -} - -contract C { - using SafeERC20 for IERC20; - IERC20 token; - uint256 zero = 0; - - function good1() public { - token.safeApprove(address(this), 0); - } - - function good2() public { - SafeERC20.safeApprove(token, address(this), 0); - } - - function good3() public { - token.safeApprove(address(this), zero); - } - - function good4(uint256 value) public { - token.safeApprove(address(this), 0); - good4(value); - } - - function bad1() public { - token.safeApprove(address(this), type(uint256).max); - } - - function bad2() public { - SafeERC20.safeApprove(token, address(this), type(uint256).max); - } - - function bad3(uint256 value) public { - token.safeApprove(address(this), value); - } -} diff --git a/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol.0.7.6.IncorrectSafeApprove.json b/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol.0.7.6.IncorrectSafeApprove.json deleted file mode 100644 index afb201a00c..0000000000 --- a/tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol.0.7.6.IncorrectSafeApprove.json +++ /dev/null @@ -1,253 +0,0 @@ -[ - [ - { - "elements": [ - { - "type": "function", - "name": "bad3", - "source_mapping": { - "start": 829, - "length": 92, - "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "is_dependency": false, - "lines": [ - 41, - 42, - 43 - ], - "starting_column": 5, - "ending_column": 6 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "C", - "source_mapping": { - "start": 157, - "length": 766, - "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "is_dependency": false, - "lines": [ - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34, - 35, - 36, - 37, - 38, - 39, - 40, - 41, - 42, - 43, - 44 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "bad3(uint256)" - } - } - ], - "description": "C.bad3(uint256) (tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#41-43) calls `safeApprove` with a non-zero value\n", - "markdown": "[C.bad3(uint256)](tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L41-L43) calls `safeApprove` with a non-zero value\n", - "first_markdown_element": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L41-L43", - "id": "0e2bcb981fa9de58958bca199e6d7f294d6edaadb1fcc7e4f0235c761eee6bc5", - "check": "erc20-incorrect-safe-approve", - "impact": "Medium", - "confidence": "High" - }, - { - "elements": [ - { - "type": "function", - "name": "bad1", - "source_mapping": { - "start": 624, - "length": 91, - "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "is_dependency": false, - "lines": [ - 33, - 34, - 35 - ], - "starting_column": 5, - "ending_column": 6 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "C", - "source_mapping": { - "start": 157, - "length": 766, - "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "is_dependency": false, - "lines": [ - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34, - 35, - 36, - 37, - 38, - 39, - 40, - 41, - 42, - 43, - 44 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "bad1()" - } - } - ], - "description": "C.bad1() (tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#33-35) calls `safeApprove` with a non-zero value\n", - "markdown": "[C.bad1()](tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L33-L35) calls `safeApprove` with a non-zero value\n", - "first_markdown_element": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L33-L35", - "id": "5a61daaa853ba7b6502b414b31c0ea71b1e8ba1a7ecf2c492fdedabfead8fa2e", - "check": "erc20-incorrect-safe-approve", - "impact": "Medium", - "confidence": "High" - }, - { - "elements": [ - { - "type": "function", - "name": "bad2", - "source_mapping": { - "start": 721, - "length": 102, - "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "is_dependency": false, - "lines": [ - 37, - 38, - 39 - ], - "starting_column": 5, - "ending_column": 6 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "C", - "source_mapping": { - "start": 157, - "length": 766, - "filename_relative": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol", - "is_dependency": false, - "lines": [ - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34, - 35, - 36, - 37, - 38, - 39, - 40, - 41, - 42, - 43, - 44 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "bad2()" - } - } - ], - "description": "C.bad2() (tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#37-39) calls `safeApprove` with a non-zero value\n", - "markdown": "[C.bad2()](tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L37-L39) calls `safeApprove` with a non-zero value\n", - "first_markdown_element": "tests/detectors/erc20-incorrect-safe-approve/0.7.6/erc20_incorrect_safe_approve.sol#L37-L39", - "id": "b28a6f139c8dba51776faf3b44e993507e497bb34d42b2b3892b1c9685d933e8", - "check": "erc20-incorrect-safe-approve", - "impact": "Medium", - "confidence": "High" - } - ] -] \ No newline at end of file diff --git a/tests/test_detectors.py b/tests/test_detectors.py index 3f021dd88d..89ef7bcc33 100644 --- a/tests/test_detectors.py +++ b/tests/test_detectors.py @@ -237,8 +237,8 @@ def id_test(test_item: Test): "0.7.6", ), Test( - all_detectors.IncorrectSafeApprove, - "erc20_incorrect_safe_approve.sol", + all_detectors.DeprecatedSafeApprove, + "erc20_deprecated_safe_approve.sol", "0.7.6", ), Test( From e5696a0a651778a18ad8e14ef4c33367b5413b35 Mon Sep 17 00:00:00 2001 From: Antonio Guilherme Ferreira Viggiano Date: Thu, 5 Jan 2023 08:45:06 -0300 Subject: [PATCH 5/6] Update module docs --- slither/detectors/erc/deprecated_safe_approve.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slither/detectors/erc/deprecated_safe_approve.py b/slither/detectors/erc/deprecated_safe_approve.py index 9db81d3102..e0ee6016e2 100644 --- a/slither/detectors/erc/deprecated_safe_approve.py +++ b/slither/detectors/erc/deprecated_safe_approve.py @@ -1,5 +1,5 @@ """ -Module detecting incorrect safeApprove usage +Module detecting deprecated safeApprove usage """ from slither.core.declarations.function import Function From 3d128ce355209faed99dc8d27d34649755b83ee5 Mon Sep 17 00:00:00 2001 From: Antonio Guilherme Ferreira Viggiano Date: Thu, 5 Jan 2023 08:48:23 -0300 Subject: [PATCH 6/6] Fix wording --- slither/detectors/erc/deprecated_safe_approve.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/slither/detectors/erc/deprecated_safe_approve.py b/slither/detectors/erc/deprecated_safe_approve.py index e0ee6016e2..d14bfc74af 100644 --- a/slither/detectors/erc/deprecated_safe_approve.py +++ b/slither/detectors/erc/deprecated_safe_approve.py @@ -22,7 +22,7 @@ class DeprecatedSafeApprove(AbstractDetector): ) WIKI_TITLE = "Deprecated safeApprove usage" - WIKI_DESCRIPTION = "The purpose of this `safeApprove` is to check that a user is either setting their allowance to zero, or setting it from zero. This is so that the user doesn't try to re-set their allowance from one non-zero number to another non-zero number where they can get front-run by the approved address using their allowance in-between those two transactions. However, this function doesn't actually solve the problem as the approval will still pass if the sandwich attack uses the rest of the remaining allowance. Such behavior lends itself to bugs, unnecessary gas usage, and a false sense of security. Because of that, `safeApprove` has been deprecated." + WIKI_DESCRIPTION = "The purpose of `safeApprove` is to check that a user is either setting their allowance to zero, or setting it from zero. This is so that the user doesn't try to re-set their allowance from one non-zero number to another non-zero number where they can get front-run by the approved address using their allowance in-between those two transactions. However, this function doesn't actually solve the problem as the approval will still pass if the sandwich attack uses the rest of the remaining allowance. Such behavior lends itself to bugs, unnecessary gas usage, and a false sense of security. Because of that, `safeApprove` has been deprecated." # region wiki_exploit_scenario WIKI_EXPLOIT_SCENARIO = """