Open source unprotected upgrade

pull/725/head
Josselin 4 years ago
parent d2b1695851
commit ac109d42f7
  1. 11
      slither/core/declarations/contract.py
  2. 1
      slither/detectors/all_detectors.py
  3. 92
      slither/detectors/statements/unprotected_upgradeable.py
  4. 14
      tests/detectors/unprotected-upgrade/Buggy.sol
  5. 152
      tests/detectors/unprotected-upgrade/Buggy.sol.0.6.12.UnprotectedUpgradeable.json
  6. 39
      tests/detectors/unprotected-upgrade/Fixed.sol
  7. 3
      tests/detectors/unprotected-upgrade/Fixed.sol.0.6.12.UnprotectedUpgradeable.json
  8. 5
      tests/detectors/unprotected-upgrade/Initializable.sol
  9. 3
      tests/test_detectors.py

@ -1027,6 +1027,8 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public-
def is_upgradeable(self) -> bool:
if self._is_upgradeable is None:
self._is_upgradeable = False
if self.is_upgradeable_proxy:
return False
initializable = self.slither.get_contract_from_name("Initializable")
if initializable:
if initializable in self.inheritance:
@ -1034,7 +1036,11 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public-
else:
for c in self.inheritance + [self]:
# This might lead to false positive
if "upgradeable" in c.name.lower() or "upgradable" in c.name.lower():
lower_name = c.name.lower()
if "upgradeable" in lower_name or "upgradable" in lower_name:
self._is_upgradeable = True
break
if "initializable" in lower_name:
self._is_upgradeable = True
break
return self._is_upgradeable
@ -1046,6 +1052,9 @@ class Contract(ChildSlither, SourceMapping): # pylint: disable=too-many-public-
if self._is_upgradeable_proxy is None:
self._is_upgradeable_proxy = False
if "Proxy" in self.name:
self._is_upgradeable_proxy = True
return True
for f in self.functions:
if f.is_fallback:
for node in f.all_nodes():

@ -46,6 +46,7 @@ from .statements.type_based_tautology import TypeBasedTautology
from .statements.boolean_constant_equality import BooleanEquality
from .statements.boolean_constant_misuse import BooleanConstantMisuse
from .statements.divide_before_multiply import DivideBeforeMultiply
from .statements.unprotected_upgradeable import UnprotectedUpgradeable
from .slither.name_reused import NameReused
#

@ -0,0 +1,92 @@
from typing import List
from slither.analyses.data_dependency.data_dependency import is_tainted
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

@ -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);
}
}

@ -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"
}
]
]

@ -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;
}
}

@ -0,0 +1,5 @@
contract Initializable{
modifier initializer() {
_;
}
}

@ -44,6 +44,7 @@ from slither.detectors.statements.controlled_delegatecall import ControlledDeleg
from slither.detectors.statements.incorrect_strict_equality import IncorrectStrictEquality
from slither.detectors.statements.too_many_digits import TooManyDigits
from slither.detectors.statements.tx_origin import TxOrigin
from slither.detectors.statements.unprotected_upgradeable import UnprotectedUpgradeable
from slither.detectors.variables.possible_const_state_variables import ConstCandidateStateVars
from slither.detectors.variables.uninitialized_local_variables import UninitializedLocalVars
from slither.detectors.variables.uninitialized_state_variables import (
@ -234,6 +235,8 @@ ALL_TESTS = [
"0.5.1",
),
Test(TooManyDigits, "tests/detectors/too-many-digits/too_many_digits.sol", "0.5.1"),
Test(UnprotectedUpgradeable, "tests/detectors/unprotected-upgrade/Buggy.sol", "0.6.12",),
Test(UnprotectedUpgradeable, "tests/detectors/unprotected-upgrade/Fixed.sol", "0.6.12",),
]
GENERIC_PATH = "/GENERIC_PATH"

Loading…
Cancel
Save