From d22abb1f45e2c8e60df4439e1c1c065a5b2e5487 Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Tue, 6 Dec 2022 13:04:23 +0100 Subject: [PATCH 1/2] Improve protected variable detector Have the detector look through all the state variables written --- .../detectors/functions/protected_variable.py | 2 +- .../protected-vars/0.8.2/comment.sol | 19 ++ .../comment.sol.0.8.2.ProtectedVariables.json | 217 ------------------ 3 files changed, 20 insertions(+), 218 deletions(-) delete mode 100644 tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json diff --git a/slither/detectors/functions/protected_variable.py b/slither/detectors/functions/protected_variable.py index 0c1341d3c..cbd640e18 100644 --- a/slither/detectors/functions/protected_variable.py +++ b/slither/detectors/functions/protected_variable.py @@ -48,7 +48,7 @@ contract Buggy{ def _analyze_function(self, function: Function, contract: Contract) -> List[Output]: results = [] - for state_variable_written in function.state_variables_written: + for state_variable_written in function.all_state_variables_written(): if state_variable_written.write_protection: for function_sig in state_variable_written.write_protection: function_protection = contract.get_function_from_signature(function_sig) diff --git a/tests/detectors/protected-vars/0.8.2/comment.sol b/tests/detectors/protected-vars/0.8.2/comment.sol index e06ff3902..a49558b61 100644 --- a/tests/detectors/protected-vars/0.8.2/comment.sol +++ b/tests/detectors/protected-vars/0.8.2/comment.sol @@ -33,3 +33,22 @@ contract ReentrancyAndWrite{ } } +contract Internal { + /// @custom:security write-protection="onlyOwner()" + address owner; + + + + modifier onlyOwner(){ + // lets assume there is an access control + _; + } + + function buggy() public { + internal_write(); + } + + function internal_write() internal { + owner = msg.sender; + } +} \ No newline at end of file diff --git a/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json b/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json deleted file mode 100644 index cb1ead621..000000000 --- a/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json +++ /dev/null @@ -1,217 +0,0 @@ -[ - [ - { - "elements": [ - { - "type": "function", - "name": "set_not_protected", - "source_mapping": { - "start": 653, - "length": 85, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 31, - 32, - 33 - ], - "starting_column": 5, - "ending_column": 6 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "ReentrancyAndWrite", - "source_mapping": { - "start": 55, - "length": 685, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "set_not_protected()" - } - }, - { - "type": "function", - "name": "onlyOwner", - "source_mapping": { - "start": 210, - "length": 88, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 11, - 12, - 13, - 14 - ], - "starting_column": 5, - "ending_column": 6 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "ReentrancyAndWrite", - "source_mapping": { - "start": 55, - "length": 685, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34 - ], - "starting_column": 1, - "ending_column": 2 - } - }, - "signature": "onlyOwner()" - } - }, - { - "type": "variable", - "name": "external_contract", - "source_mapping": { - "start": 184, - "length": 19, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 9 - ], - "starting_column": 5, - "ending_column": 24 - }, - "type_specific_fields": { - "parent": { - "type": "contract", - "name": "ReentrancyAndWrite", - "source_mapping": { - "start": 55, - "length": 685, - "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", - "filename_absolute": "/GENERIC_PATH", - "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", - "is_dependency": false, - "lines": [ - 5, - 6, - 7, - 8, - 9, - 10, - 11, - 12, - 13, - 14, - 15, - 16, - 17, - 18, - 19, - 20, - 21, - 22, - 23, - 24, - 25, - 26, - 27, - 28, - 29, - 30, - 31, - 32, - 33, - 34 - ], - "starting_column": 1, - "ending_column": 2 - } - } - } - } - ], - "description": "ReentrancyAndWrite.set_not_protected() (tests/detectors/protected-vars/0.8.2/comment.sol#31-33) should have ReentrancyAndWrite.onlyOwner() (tests/detectors/protected-vars/0.8.2/comment.sol#11-14) to protect ReentrancyAndWrite.external_contract (tests/detectors/protected-vars/0.8.2/comment.sol#9)\n", - "markdown": "[ReentrancyAndWrite.set_not_protected()](tests/detectors/protected-vars/0.8.2/comment.sol#L31-L33) should have [ReentrancyAndWrite.onlyOwner()](tests/detectors/protected-vars/0.8.2/comment.sol#L11-L14) to protect [ReentrancyAndWrite.external_contract](tests/detectors/protected-vars/0.8.2/comment.sol#L9)\n", - "first_markdown_element": "tests/detectors/protected-vars/0.8.2/comment.sol#L31-L33", - "id": "3f3bc8c8a9b3e23482f47f1133aceaed81c2c781c6aaf25656a8e578c9f6cb0e", - "check": "protected-vars", - "impact": "High", - "confidence": "High" - } - ] -] \ No newline at end of file From 3825b1ac496cdcffac4d1a0b5f2c12f95d02ccbd Mon Sep 17 00:00:00 2001 From: Josselin Feist Date: Tue, 6 Dec 2022 13:07:59 +0100 Subject: [PATCH 2/2] Add missing json --- .../comment.sol.0.8.2.ProtectedVariables.json | 400 ++++++++++++++++++ 1 file changed, 400 insertions(+) create mode 100644 tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json diff --git a/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json b/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json new file mode 100644 index 000000000..2c1a11fa4 --- /dev/null +++ b/tests/detectors/protected-vars/0.8.2/comment.sol.0.8.2.ProtectedVariables.json @@ -0,0 +1,400 @@ +[ + [ + { + "elements": [ + { + "type": "function", + "name": "buggy", + "source_mapping": { + "start": 938, + "length": 57, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 47, + 48, + 49 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "Internal", + "source_mapping": { + "start": 742, + "length": 331, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44, + 45, + 46, + 47, + 48, + 49, + 50, + 51, + 52, + 53, + 54, + 55 + ], + "starting_column": 1, + "ending_column": 0 + } + }, + "signature": "buggy()" + } + }, + { + "type": "function", + "name": "onlyOwner", + "source_mapping": { + "start": 844, + "length": 88, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 42, + 43, + 44, + 45 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "Internal", + "source_mapping": { + "start": 742, + "length": 331, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44, + 45, + 46, + 47, + 48, + 49, + 50, + 51, + 52, + 53, + 54, + 55 + ], + "starting_column": 1, + "ending_column": 0 + } + }, + "signature": "onlyOwner()" + } + }, + { + "type": "variable", + "name": "owner", + "source_mapping": { + "start": 822, + "length": 13, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 38 + ], + "starting_column": 5, + "ending_column": 18 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "Internal", + "source_mapping": { + "start": 742, + "length": 331, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 36, + 37, + 38, + 39, + 40, + 41, + 42, + 43, + 44, + 45, + 46, + 47, + 48, + 49, + 50, + 51, + 52, + 53, + 54, + 55 + ], + "starting_column": 1, + "ending_column": 0 + } + } + } + } + ], + "description": "Internal.buggy() (tests/detectors/protected-vars/0.8.2/comment.sol#47-49) should have Internal.onlyOwner() (tests/detectors/protected-vars/0.8.2/comment.sol#42-45) to protect Internal.owner (tests/detectors/protected-vars/0.8.2/comment.sol#38)\n", + "markdown": "[Internal.buggy()](tests/detectors/protected-vars/0.8.2/comment.sol#L47-L49) should have [Internal.onlyOwner()](tests/detectors/protected-vars/0.8.2/comment.sol#L42-L45) to protect [Internal.owner](tests/detectors/protected-vars/0.8.2/comment.sol#L38)\n", + "first_markdown_element": "tests/detectors/protected-vars/0.8.2/comment.sol#L47-L49", + "id": "347d5dbdb03710066bc29d7772156fe5ff3d3371fa4eee4839ee221a1d0de0a4", + "check": "protected-vars", + "impact": "High", + "confidence": "High" + }, + { + "elements": [ + { + "type": "function", + "name": "set_not_protected", + "source_mapping": { + "start": 653, + "length": 85, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 31, + 32, + 33 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "ReentrancyAndWrite", + "source_mapping": { + "start": 55, + "length": 685, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "set_not_protected()" + } + }, + { + "type": "function", + "name": "onlyOwner", + "source_mapping": { + "start": 210, + "length": 88, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 11, + 12, + 13, + 14 + ], + "starting_column": 5, + "ending_column": 6 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "ReentrancyAndWrite", + "source_mapping": { + "start": 55, + "length": 685, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34 + ], + "starting_column": 1, + "ending_column": 2 + } + }, + "signature": "onlyOwner()" + } + }, + { + "type": "variable", + "name": "external_contract", + "source_mapping": { + "start": 184, + "length": 19, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 9 + ], + "starting_column": 5, + "ending_column": 24 + }, + "type_specific_fields": { + "parent": { + "type": "contract", + "name": "ReentrancyAndWrite", + "source_mapping": { + "start": 55, + "length": 685, + "filename_relative": "tests/detectors/protected-vars/0.8.2/comment.sol", + "filename_absolute": "/GENERIC_PATH", + "filename_short": "tests/detectors/protected-vars/0.8.2/comment.sol", + "is_dependency": false, + "lines": [ + 5, + 6, + 7, + 8, + 9, + 10, + 11, + 12, + 13, + 14, + 15, + 16, + 17, + 18, + 19, + 20, + 21, + 22, + 23, + 24, + 25, + 26, + 27, + 28, + 29, + 30, + 31, + 32, + 33, + 34 + ], + "starting_column": 1, + "ending_column": 2 + } + } + } + } + ], + "description": "ReentrancyAndWrite.set_not_protected() (tests/detectors/protected-vars/0.8.2/comment.sol#31-33) should have ReentrancyAndWrite.onlyOwner() (tests/detectors/protected-vars/0.8.2/comment.sol#11-14) to protect ReentrancyAndWrite.external_contract (tests/detectors/protected-vars/0.8.2/comment.sol#9)\n", + "markdown": "[ReentrancyAndWrite.set_not_protected()](tests/detectors/protected-vars/0.8.2/comment.sol#L31-L33) should have [ReentrancyAndWrite.onlyOwner()](tests/detectors/protected-vars/0.8.2/comment.sol#L11-L14) to protect [ReentrancyAndWrite.external_contract](tests/detectors/protected-vars/0.8.2/comment.sol#L9)\n", + "first_markdown_element": "tests/detectors/protected-vars/0.8.2/comment.sol#L31-L33", + "id": "3f3bc8c8a9b3e23482f47f1133aceaed81c2c781c6aaf25656a8e578c9f6cb0e", + "check": "protected-vars", + "impact": "High", + "confidence": "High" + } + ] +] \ No newline at end of file