Skip to content

Commit

Permalink
Merge pull request #1510 from crytic/local-shadowing-return-vars
Browse files Browse the repository at this point in the history 
detect local shadowing of return vars
  • Loading branch information
@montyly
montyly authored Mar 10, 2023
2 parents e11ae90 + 841f460 commit d4a7254
Show file tree
Hide file tree
Showing 9 changed files with 1,039 additions and 0 deletions.
10 changes: 10 additions & 0 deletions slither/detectors/shadowing/local.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class LocalShadowing(AbstractDetector):
OVERSHADOWED_MODIFIER = "modifier"
OVERSHADOWED_STATE_VARIABLE = "state variable"
OVERSHADOWED_EVENT = "event"
OVERSHADOWED_RETURN_VARIABLE = "return variable"

# pylint: disable=too-many-branches
def detect_shadowing_definitions(
Expand Down Expand Up @@ -111,6 +112,15 @@ def detect_shadowing_definitions(
overshadowed.append(
(self.OVERSHADOWED_STATE_VARIABLE, scope_state_variable)
)
# Check named return variables
for named_return in function.returns:
# Shadowed local delcarations in the same function will have "_scope_" in their name.
# See `FunctionSolc._add_local_variable`
if (
"_scope_" in variable.name
and variable.name.split("_scope_")[0] == named_return.name
):
overshadowed.append((self.OVERSHADOWED_RETURN_VARIABLE, named_return))

# If we have found any overshadowed objects, we'll want to add it to our result list.
if overshadowed:
Expand Down
10 changes: 10 additions & 0 deletions tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,13 @@ contract FurtherExtendedContract is ExtendedContract {

function shadowingParent(uint x) public pure { int y; uint z; uint w; uint v; }
}

contract LocalReturnVariables {
uint state;
function shadowedState() external view returns(uint state) {
return state;
}
function good() external view returns(uint val1) {
return val1;
}
}
123 changes: 123 additions & 0 deletions .../detectors/shadowing-local/0.4.25/shadowing_local_variable.sol.0.4.25.LocalShadowing.json
View file
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,129 @@
"impact": "Low",
"confidence": "High"
},
{
"elements": [
{
"type": "variable",
"name": "state",
"source_mapping": {
"start": 533,
"length": 10,
"filename_relative": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol",
"is_dependency": false,
"lines": [
30
],
"starting_column": 52,
"ending_column": 62
},
"type_specific_fields": {
"parent": {
"type": "function",
"name": "shadowedState",
"source_mapping": {
"start": 486,
"length": 88,
"filename_relative": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol",
"is_dependency": false,
"lines": [
30,
31,
32
],
"starting_column": 5,
"ending_column": 6
},
"type_specific_fields": {
"parent": {
"type": "contract",
"name": "LocalReturnVariables",
"source_mapping": {
"start": 434,
"length": 225,
"filename_relative": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol",
"is_dependency": false,
"lines": [
28,
29,
30,
31,
32,
33,
34,
35,
36,
37
],
"starting_column": 1,
"ending_column": 0
}
},
"signature": "shadowedState()"
}
}
}
},
{
"type": "variable",
"name": "state",
"source_mapping": {
"start": 470,
"length": 10,
"filename_relative": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol",
"is_dependency": false,
"lines": [
29
],
"starting_column": 5,
"ending_column": 15
},
"type_specific_fields": {
"parent": {
"type": "contract",
"name": "LocalReturnVariables",
"source_mapping": {
"start": 434,
"length": 225,
"filename_relative": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol",
"filename_absolute": "/GENERIC_PATH",
"filename_short": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol",
"is_dependency": false,
"lines": [
28,
29,
30,
31,
32,
33,
34,
35,
36,
37
],
"starting_column": 1,
"ending_column": 0
}
}
}
}
],
"description": "LocalReturnVariables.shadowedState().state (tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol#30) shadows:\n\t- LocalReturnVariables.state (tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol#29) (state variable)\n",
"markdown": "[LocalReturnVariables.shadowedState().state](tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol#L30) shadows:\n\t- [LocalReturnVariables.state](tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol#L29) (state variable)\n",
"first_markdown_element": "tests/detectors/shadowing-local/0.4.25/shadowing_local_variable.sol#L30",
"id": "1b0030affabcff703e57e4f388b86dbda0f412e51ba8d15248bcae9e4748a012",
"check": "shadowing-local",
"impact": "Low",
"confidence": "High"
},
{
"elements": [
{
Expand Down
14 changes: 14 additions & 0 deletions tests/detectors/shadowing-local/0.5.16/shadowing_local_variable.sol
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,17 @@ contract FurtherExtendedContract is ExtendedContract {

function shadowingParent(uint x) public pure { int y; uint z; uint w; uint v; }
}

contract LocalReturnVariables {
uint state;
function shadowedState() external view returns(uint state) {
return state;
}
function shadowedReturn() external view returns(uint local) {
uint local = 1;
return local;
}
function good() external view returns(uint val1) {
return val1;
}
}
Loading

0 comments on commit d4a7254

Please sign in to comment.