Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Deprecated safeApprove detector #1552

Closed
wants to merge 6 commits into from
Closed

Deprecated safeApprove detector #1552

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
045a5c8
Add incorrect safeApprove detector
aviggiano Jan 5, 2023
903ef3f
Include test
aviggiano Jan 5, 2023
16742f4
Generate test artifact
aviggiano Jan 5, 2023
a95ed5c
Update incorrect safeApprove to deprecated
aviggiano Jan 5, 2023
e5696a0
Update module docs
aviggiano Jan 5, 2023
3d128ce
Fix wording
aviggiano Jan 5, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions slither/detectors/all_detectors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.deprecated_safe_approve import DeprecatedSafeApprove
from .statements.deprecated_calls import DeprecatedStandards
from .source.rtlo import RightToLeftOverride
from .statements.too_many_digits import TooManyDigits
Expand Down
69 changes: 69 additions & 0 deletions slither/detectors/erc/deprecated_safe_approve.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
"""
Module detecting deprecated 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 `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
22 changes: 22 additions & 0 deletions tests/detectors/erc20-deprecated-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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);
}
}
126 changes: 126 additions & 0 deletions ...ted-safe-approve/0.7.6/erc20_deprecated_safe_approve.sol.0.7.6.DeprecatedSafeApprove.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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"
}
]
]
5 changes: 5 additions & 0 deletions tests/test_detectors.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,6 +236,11 @@ def id_test(test_item: Test):
"incorrect_erc20_interface.sol",
"0.7.6",
),
Test(
all_detectors.DeprecatedSafeApprove,
"erc20_deprecated_safe_approve.sol",
"0.7.6",
),
Test(
all_detectors.IncorrectERC721InterfaceDetection,
"incorrect_erc721_interface.sol",
Expand Down