From cb2c6073ec2ed6a8b5630cabf0450baeff8bd8d1 Mon Sep 17 00:00:00 2001 From: Josselin Date: Sat, 26 Oct 2019 10:48:01 +0200 Subject: [PATCH] Upgradeability checks: - add missing file - Test constant conformance - Test variable initilization - Minor change in output --- scripts/travis_test_upgradability.sh | 28 ++++++++++++++++++- slither/tools/upgradeability/__main__.py | 8 ++++-- .../tools/upgradeability/constant_checks.py | 4 +-- .../contract_v1_var_init.sol | 3 ++ .../contract_v2_constant.sol | 3 ++ tests/check-upgradeability/test_10.txt | 12 ++++++++ tests/check-upgradeability/test_11.txt | 6 ++++ tests/check-upgradeability/test_6.txt | 18 ++++++++++++ tests/check-upgradeability/test_7.txt | 18 ++++++++++++ tests/check-upgradeability/test_8.txt | 18 ++++++++++++ tests/check-upgradeability/test_9.txt | 18 ++++++++++++ 11 files changed, 130 insertions(+), 6 deletions(-) create mode 100644 tests/check-upgradeability/contract_v1_var_init.sol create mode 100644 tests/check-upgradeability/contract_v2_constant.sol create mode 100644 tests/check-upgradeability/test_10.txt create mode 100644 tests/check-upgradeability/test_11.txt create mode 100644 tests/check-upgradeability/test_6.txt create mode 100644 tests/check-upgradeability/test_7.txt create mode 100644 tests/check-upgradeability/test_8.txt create mode 100644 tests/check-upgradeability/test_9.txt diff --git a/scripts/travis_test_upgradability.sh b/scripts/travis_test_upgradability.sh index 1e1dad3c9..610356c92 100755 --- a/scripts/travis_test_upgradability.sh +++ b/scripts/travis_test_upgradability.sh @@ -128,6 +128,31 @@ then exit -1 fi +slither-check-upgradeability "$DIR_TESTS/contractV1.sol" ContractV1 --solc solc-0.5.0 --new-contract-filename "$DIR_TESTS/contract_v2_constant.sol" --new-contract-name ContractV2 > test_10.txt 2>&1 +DIFF=$(diff test_10.txt "$DIR_TESTS/test_10.txt") +if [ "$DIFF" != "" ] +then + echo "slither-check-upgradeability 10 failed" + cat test_10.txt + echo "" + cat "$DIR_TESTS/test_10.txt" + echo "" + echo "$DIFF" + exit -1 +fi + +slither-check-upgradeability "$DIR_TESTS/contract_v1_var_init.sol" ContractV1 --solc solc-0.5.0 > test_11.txt 2>&1 +DIFF=$(diff test_11.txt "$DIR_TESTS/test_11.txt") +if [ "$DIFF" != "" ] +then + echo "slither-check-upgradeability 11 failed" + cat test_11.txt + echo "" + cat "$DIR_TESTS/test_11.txt" + echo "" + echo "$DIFF" + exit -1 +fi rm test_1.txt rm test_2.txt @@ -138,4 +163,5 @@ rm test_6.txt rm test_7.txt rm test_8.txt rm test_9.txt - +rm test_10.txt +rm test_11.txt diff --git a/slither/tools/upgradeability/__main__.py b/slither/tools/upgradeability/__main__.py index 069d0f437..305a43dae 100644 --- a/slither/tools/upgradeability/__main__.py +++ b/slither/tools/upgradeability/__main__.py @@ -6,6 +6,7 @@ from collections import defaultdict from slither import Slither from crytic_compile import cryticparser from slither.exceptions import SlitherException +from slither.utils.colors import red from slither.utils.json_utils import output_json from .compare_variables_order import compare_variables_order @@ -79,6 +80,7 @@ def _checks_on_contract_update(contract_v1, contract_v2, json_results): :param json_results: :return: """ + ret = compare_variables_order(contract_v1, contract_v2) json_results['compare-variables-order-implementation'][contract_v1.name][contract_v2.name] = ret @@ -131,7 +133,7 @@ def main(): v1_contract = v1.get_contract_from_name(v1_name) if v1_contract is None: info = 'Contract {} not found in {}'.format(v1_name, v1.filename) - logger.error(info) + logger.error(red(info)) if args.json: output_json(args.json, str(info), {"upgradeability-check": json_results}) return @@ -149,7 +151,7 @@ def main(): proxy_contract = proxy.get_contract_from_name(args.proxy_name) if proxy_contract is None: info = 'Proxy {} not found in {}'.format(args.proxy_name, proxy.filename) - logger.error(info) + logger.error(red(info)) if args.json: output_json(args.json, str(info), {"upgradeability-check": json_results}) return @@ -166,7 +168,7 @@ def main(): v2_contract = v2.get_contract_from_name(args.new_contract_name) if v2_contract is None: info = 'New logic contract {} not found in {}'.format(args.new_contract_name, v2.filename) - logger.error(info) + logger.error(red(info)) if args.json: output_json(args.json, str(info), {"upgradeability-check": json_results}) return diff --git a/slither/tools/upgradeability/constant_checks.py b/slither/tools/upgradeability/constant_checks.py index 98847ffc3..653842f8d 100644 --- a/slither/tools/upgradeability/constant_checks.py +++ b/slither/tools/upgradeability/constant_checks.py @@ -30,7 +30,7 @@ def constant_conformance_check(contract_v1, contract_v2): if state_v2: if state_v1.is_constant: if not state_v2.is_constant: - info = f'{state_v1.canonical_name} was constant and {contract_v2.name} is not' + info = f'{state_v1.canonical_name} ({state_v1.source_mapping_str}) was constant and {state_v2.canonical_name} is not ({state_v2.source_mapping_str})' logger.info(red(info)) results['were_constants'].append({ 'description': info, @@ -41,7 +41,7 @@ def constant_conformance_check(contract_v1, contract_v2): }) error_found = True elif state_v2.is_constant: - info = f'{state_v1.canonical_name} was not constant and {contract_v2.name} is' + info = f'{state_v1.canonical_name} ({state_v1.source_mapping_str}) was not constant but {state_v2.canonical_name} is ({state_v2.source_mapping_str})' logger.info(red(info)) results['became_constants'].append({ 'description': info, diff --git a/tests/check-upgradeability/contract_v1_var_init.sol b/tests/check-upgradeability/contract_v1_var_init.sol new file mode 100644 index 000000000..df9bb3fa1 --- /dev/null +++ b/tests/check-upgradeability/contract_v1_var_init.sol @@ -0,0 +1,3 @@ +contract ContractV1{ + address destination = address(0x41); +} diff --git a/tests/check-upgradeability/contract_v2_constant.sol b/tests/check-upgradeability/contract_v2_constant.sol new file mode 100644 index 000000000..c985e3f98 --- /dev/null +++ b/tests/check-upgradeability/contract_v2_constant.sol @@ -0,0 +1,3 @@ +contract ContractV2{ + address constant destination = address(0x41); +} diff --git a/tests/check-upgradeability/test_10.txt b/tests/check-upgradeability/test_10.txt new file mode 100644 index 000000000..89589b364 --- /dev/null +++ b/tests/check-upgradeability/test_10.txt @@ -0,0 +1,12 @@ + +## Run initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#initialization-checks) +Initializable contract not found, the contract does not follow a standard initalization schema. + +## Run variable initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks) +No error found + +## Run variables ordering checks between ContractV1 and ContractV2... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +Variable only in ContractV1: destination (tests/check-upgradeability/contractV1.sol#2) + +## Run variable constants conformance check... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks) +ContractV1.destination (tests/check-upgradeability/contractV1.sol#2) was not constant but ContractV2.destination is (tests/check-upgradeability/contract_v2_constant.sol#2) diff --git a/tests/check-upgradeability/test_11.txt b/tests/check-upgradeability/test_11.txt new file mode 100644 index 000000000..24764c8c5 --- /dev/null +++ b/tests/check-upgradeability/test_11.txt @@ -0,0 +1,6 @@ + +## Run initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#initialization-checks) +Initializable contract not found, the contract does not follow a standard initalization schema. + +## Run variable initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks) +ContractV1.destination has an initial value (tests/check-upgradeability/contract_v1_var_init.sol#2) diff --git a/tests/check-upgradeability/test_6.txt b/tests/check-upgradeability/test_6.txt new file mode 100644 index 000000000..85da69ae4 --- /dev/null +++ b/tests/check-upgradeability/test_6.txt @@ -0,0 +1,18 @@ + +## Run initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#initialization-checks) +Contract_lack_to_call_modifier.initialize() does not call the initializer modifier +No missing call to an init function found +No double call to init functions found +Check the deployement script to ensure that these functions are called: +Contract_lack_to_call_modifier needs to be initialized by initialize() + +No error found + +## Run variable initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks) +No error found + +## Run function ids checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#functions-ids-checks) +No error found + +## Run variables ordering checks between Contract_lack_to_call_modifier and Proxy... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +No error found diff --git a/tests/check-upgradeability/test_7.txt b/tests/check-upgradeability/test_7.txt new file mode 100644 index 000000000..d2110df9c --- /dev/null +++ b/tests/check-upgradeability/test_7.txt @@ -0,0 +1,18 @@ + +## Run initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#initialization-checks) +All the init functions have the initializer modifier +Missing call to Contract_no_bug.initialize() in Contract_not_called_super_init.initialize() +No double call to init functions found +Check the deployement script to ensure that these functions are called: +Contract_not_called_super_init needs to be initialized by initialize() + +No error found + +## Run variable initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks) +No error found + +## Run function ids checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#functions-ids-checks) +No error found + +## Run variables ordering checks between Contract_not_called_super_init and Proxy... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +No error found diff --git a/tests/check-upgradeability/test_8.txt b/tests/check-upgradeability/test_8.txt new file mode 100644 index 000000000..69ff9ebec --- /dev/null +++ b/tests/check-upgradeability/test_8.txt @@ -0,0 +1,18 @@ + +## Run initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#initialization-checks) +All the init functions have the initializer modifier +No missing call to an init function found +No double call to init functions found +Check the deployement script to ensure that these functions are called: +Contract_no_bug_inherits needs to be initialized by initialize() + +No error found + +## Run variable initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks) +No error found + +## Run function ids checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#functions-ids-checks) +No error found + +## Run variables ordering checks between Contract_no_bug_inherits and Proxy... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +No error found diff --git a/tests/check-upgradeability/test_9.txt b/tests/check-upgradeability/test_9.txt new file mode 100644 index 000000000..3e12a4525 --- /dev/null +++ b/tests/check-upgradeability/test_9.txt @@ -0,0 +1,18 @@ + +## Run initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#initialization-checks) +All the init functions have the initializer modifier +No missing call to an init function found +Contract_no_bug.initialize() is called multiple times in initialize() +Check the deployement script to ensure that these functions are called: +Contract_double_call needs to be initialized by initialize() + +No error found + +## Run variable initialization checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks) +No error found + +## Run function ids checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#functions-ids-checks) +No error found + +## Run variables ordering checks between Contract_double_call and Proxy... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +No error found