-
Notifications
You must be signed in to change notification settings - Fork 982
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge pull request #725 from crytic/dev-unprotected-upgrade
Open source unprotected upgrade
- Loading branch information
Showing
9 changed files
with
316 additions
and
1 deletion.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| from typing import List | ||
|
|
||
| from slither.core.declarations import SolidityFunction, Function | ||
| from slither.detectors.abstract_detector import AbstractDetector, DetectorClassification | ||
| from slither.slithir.operations import LowLevelCall, SolidityCall | ||
|
|
||
|
|
||
| def _can_be_destroyed(contract) -> List[Function]: | ||
| targets = [] | ||
| for f in contract.functions_entry_points: | ||
| for ir in f.all_slithir_operations(): | ||
| if ( | ||
| isinstance(ir, LowLevelCall) and ir.function_name in ["delegatecall", "codecall"] | ||
| ) or ( | ||
| isinstance(ir, SolidityCall) | ||
| and ir.function | ||
| in [SolidityFunction("suicide(address)"), SolidityFunction("selfdestruct(address)")] | ||
| ): | ||
| targets.append(f) | ||
| break | ||
| return targets | ||
|
|
||
|
|
||
| class UnprotectedUpgradeable(AbstractDetector): | ||
|
|
||
| ARGUMENT = "unprotected-upgrade" | ||
| HELP = "Unprotected upgradeable contract" | ||
| IMPACT = DetectorClassification.HIGH | ||
| CONFIDENCE = DetectorClassification.HIGH | ||
|
|
||
| WIKI = "https://github.com/crytic/slither/wiki/Detector-Documentation#unprotected-upgradeable-contract" | ||
|
|
||
| WIKI_TITLE = "Unprotected upgradeable contract" | ||
| WIKI_DESCRIPTION = """Detects logic contract that can be destructed.""" | ||
| WIKI_EXPLOIT_SCENARIO = """ | ||
| ```solidity | ||
| contract Buggy is Initializable{ | ||
| address payable owner; | ||
| function initialize() external initializer{ | ||
| require(owner == address(0)); | ||
| owner = msg.sender; | ||
| } | ||
| function kill() external{ | ||
| require(msg.sender == owner); | ||
| selfdestruct(owner); | ||
| } | ||
| } | ||
| ``` | ||
| Buggy is an upgradeable contract. Anyone can call initialize on the logic contract, and destruct the contract.""" | ||
|
|
||
| WIKI_RECOMMENDATION = ( | ||
| """Add a constructor to ensure `initialize` cannot be called on the logic contract.""" | ||
| ) | ||
|
|
||
| def _detect(self): | ||
| results = [] | ||
|
|
||
| for contract in self.slither.contracts_derived: | ||
| if contract.is_upgradeable: | ||
| functions_that_can_destroy = _can_be_destroyed(contract) | ||
| if functions_that_can_destroy: | ||
| initiliaze_functions = [f for f in contract.functions if f.name == "initialize"] | ||
| vars_init_ = [ | ||
| init.all_state_variables_written() for init in initiliaze_functions | ||
| ] | ||
| vars_init = [item for sublist in vars_init_ for item in sublist] | ||
|
|
||
| vars_init_in_constructors_ = [ | ||
| f.all_state_variables_written() for f in contract.constructors | ||
| ] | ||
| vars_init_in_constructors = [ | ||
| item for sublist in vars_init_in_constructors_ for item in sublist | ||
| ] | ||
| if vars_init and (set(vars_init) - set(vars_init_in_constructors)): | ||
| info = ( | ||
| [ | ||
| contract, | ||
| " is an upgradeable contract that does not protect its initiliaze functions: ", | ||
| ] | ||
| + initiliaze_functions | ||
| + [". Anyone can delete the contract with: ",] | ||
| + functions_that_can_destroy | ||
| ) | ||
|
|
||
| res = self.generate_result(info) | ||
| results.append(res) | ||
|
|
||
| return results |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,14 @@ | ||
| import "./Initializable.sol"; | ||
|
|
||
| contract Buggy is Initializable{ | ||
| address payable owner; | ||
|
|
||
| function initialize() external initializer{ | ||
| require(owner == address(0)); | ||
| owner = msg.sender; | ||
| } | ||
| function kill() external{ | ||
| require(msg.sender == owner); | ||
| selfdestruct(owner); | ||
| } | ||
| } |
152 changes: 152 additions & 0 deletions
152
tests/detectors/unprotected-upgrade/Buggy.sol.0.6.12.UnprotectedUpgradeable.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| [ | ||
| [ | ||
| { | ||
| "elements": [ | ||
| { | ||
| "type": "contract", | ||
| "name": "Buggy", | ||
| "source_mapping": { | ||
| "start": 31, | ||
| "length": 285, | ||
| "filename_used": "/GENERIC_PATH", | ||
| "filename_relative": "tests/detectors/unprotected-upgrade/Buggy.sol", | ||
| "filename_absolute": "/GENERIC_PATH", | ||
| "filename_short": "tests/detectors/unprotected-upgrade/Buggy.sol", | ||
| "is_dependency": false, | ||
| "lines": [ | ||
| 3, | ||
| 4, | ||
| 5, | ||
| 6, | ||
| 7, | ||
| 8, | ||
| 9, | ||
| 10, | ||
| 11, | ||
| 12, | ||
| 13, | ||
| 14, | ||
| 15 | ||
| ], | ||
| "starting_column": 1, | ||
| "ending_column": 0 | ||
| } | ||
| }, | ||
| { | ||
| "type": "function", | ||
| "name": "initialize", | ||
| "source_mapping": { | ||
| "start": 96, | ||
| "length": 115, | ||
| "filename_used": "/GENERIC_PATH", | ||
| "filename_relative": "tests/detectors/unprotected-upgrade/Buggy.sol", | ||
| "filename_absolute": "/GENERIC_PATH", | ||
| "filename_short": "tests/detectors/unprotected-upgrade/Buggy.sol", | ||
| "is_dependency": false, | ||
| "lines": [ | ||
| 6, | ||
| 7, | ||
| 8, | ||
| 9 | ||
| ], | ||
| "starting_column": 5, | ||
| "ending_column": 6 | ||
| }, | ||
| "type_specific_fields": { | ||
| "parent": { | ||
| "type": "contract", | ||
| "name": "Buggy", | ||
| "source_mapping": { | ||
| "start": 31, | ||
| "length": 285, | ||
| "filename_used": "/GENERIC_PATH", | ||
| "filename_relative": "tests/detectors/unprotected-upgrade/Buggy.sol", | ||
| "filename_absolute": "/GENERIC_PATH", | ||
| "filename_short": "tests/detectors/unprotected-upgrade/Buggy.sol", | ||
| "is_dependency": false, | ||
| "lines": [ | ||
| 3, | ||
| 4, | ||
| 5, | ||
| 6, | ||
| 7, | ||
| 8, | ||
| 9, | ||
| 10, | ||
| 11, | ||
| 12, | ||
| 13, | ||
| 14, | ||
| 15 | ||
| ], | ||
| "starting_column": 1, | ||
| "ending_column": 0 | ||
| } | ||
| }, | ||
| "signature": "initialize()" | ||
| } | ||
| }, | ||
| { | ||
| "type": "function", | ||
| "name": "kill", | ||
| "source_mapping": { | ||
| "start": 216, | ||
| "length": 98, | ||
| "filename_used": "/GENERIC_PATH", | ||
| "filename_relative": "tests/detectors/unprotected-upgrade/Buggy.sol", | ||
| "filename_absolute": "/GENERIC_PATH", | ||
| "filename_short": "tests/detectors/unprotected-upgrade/Buggy.sol", | ||
| "is_dependency": false, | ||
| "lines": [ | ||
| 10, | ||
| 11, | ||
| 12, | ||
| 13 | ||
| ], | ||
| "starting_column": 5, | ||
| "ending_column": 6 | ||
| }, | ||
| "type_specific_fields": { | ||
| "parent": { | ||
| "type": "contract", | ||
| "name": "Buggy", | ||
| "source_mapping": { | ||
| "start": 31, | ||
| "length": 285, | ||
| "filename_used": "/GENERIC_PATH", | ||
| "filename_relative": "tests/detectors/unprotected-upgrade/Buggy.sol", | ||
| "filename_absolute": "/GENERIC_PATH", | ||
| "filename_short": "tests/detectors/unprotected-upgrade/Buggy.sol", | ||
| "is_dependency": false, | ||
| "lines": [ | ||
| 3, | ||
| 4, | ||
| 5, | ||
| 6, | ||
| 7, | ||
| 8, | ||
| 9, | ||
| 10, | ||
| 11, | ||
| 12, | ||
| 13, | ||
| 14, | ||
| 15 | ||
| ], | ||
| "starting_column": 1, | ||
| "ending_column": 0 | ||
| } | ||
| }, | ||
| "signature": "kill()" | ||
| } | ||
| } | ||
| ], | ||
| "description": "Buggy (tests/detectors/unprotected-upgrade/Buggy.sol#3-15) is an upgradeable contract that does not protect its initiliaze functions: Buggy.initialize() (tests/detectors/unprotected-upgrade/Buggy.sol#6-9). Anyone can delete the contract with: Buggy.kill() (tests/detectors/unprotected-upgrade/Buggy.sol#10-13)", | ||
| "markdown": "[Buggy](tests/detectors/unprotected-upgrade/Buggy.sol#L3-L15) is an upgradeable contract that does not protect its initiliaze functions: [Buggy.initialize()](tests/detectors/unprotected-upgrade/Buggy.sol#L6-L9). Anyone can delete the contract with: [Buggy.kill()](tests/detectors/unprotected-upgrade/Buggy.sol#L10-L13)", | ||
| "id": "aceca400ce0b482809a70df612af22e24d154c5c89c24d630ec0ee5a366d09fe", | ||
| "check": "unprotected-upgrade", | ||
| "impact": "High", | ||
| "confidence": "High" | ||
| } | ||
| ] | ||
| ] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,39 @@ | ||
| import "./Initializable.sol"; | ||
|
|
||
| contract Fixed is Initializable{ | ||
| address payable owner; | ||
|
|
||
| constructor() public{ | ||
| owner = msg.sender; | ||
| } | ||
|
|
||
| function initialize() external initializer{ | ||
| require(owner == address(0)); | ||
| owner = msg.sender; | ||
| } | ||
| function kill() external{ | ||
| require(msg.sender == owner); | ||
| selfdestruct(owner); | ||
| } | ||
|
|
||
| function other_function() external{ | ||
|
|
||
| } | ||
| } | ||
|
|
||
|
|
||
| contract Not_Upgradeable{ | ||
| } | ||
|
|
||
| contract UpgradeableNoDestruct is Initializable{ | ||
| address payable owner; | ||
|
|
||
| constructor() public{ | ||
| owner = msg.sender; | ||
| } | ||
|
|
||
| function initialize() external initializer{ | ||
| require(owner == address(0)); | ||
| owner = msg.sender; | ||
| } | ||
| } |
3 changes: 3 additions & 0 deletions
3
tests/detectors/unprotected-upgrade/Fixed.sol.0.6.12.UnprotectedUpgradeable.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| [ | ||
| [] | ||
| ] |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| contract Initializable{ | ||
| modifier initializer() { | ||
| _; | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters