diff --git a/tests/check-upgradeability/test_1.txt b/tests/check-upgradeability/test_1.txt index 2bb466c54a..82a9bedbb0 100644 --- a/tests/check-upgradeability/test_1.txt +++ b/tests/check-upgradeability/test_1.txt @@ -2,6 +2,6 @@ INFO:CheckInitialization:Run initialization checks... (see https://github.c INFO:CheckInitialization:Initializable contract not found, the contract does not follow a standard initalization schema. INFO:CompareFunctions:Run function ids checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#functions-ids-checks) INFO:CompareFunctions:No function ids collision found -INFO:VariablesOrder:Run variables order checks between the implementation and the proxy... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) -INFO:VariablesOrder:Variable in the proxy: destination address -INFO:VariablesOrder:No variables ordering error found between implementation and the proxy +INFO:VariablesOrder:Run variable ordering checks between the proxy and implementation... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +INFO:VariablesOrder:state variable defined in proxy Proxy: address destination +INFO:VariablesOrder:No variable ordering issues found between proxy and implementation diff --git a/tests/check-upgradeability/test_2.txt b/tests/check-upgradeability/test_2.txt index 611c9db4c3..41252e5b24 100644 --- a/tests/check-upgradeability/test_2.txt +++ b/tests/check-upgradeability/test_2.txt @@ -4,8 +4,8 @@ INFO:CheckInitialization:Run initialization checks... (see https://github.c INFO:CheckInitialization:Initializable contract not found, the contract does not follow a standard initalization schema. INFO:CompareFunctions:Run function ids checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#functions-ids-checks) INFO:CompareFunctions:No function ids collision found -INFO:VariablesOrder:Run variables order checks between the implementation and the proxy... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) -INFO:VariablesOrder:Variable in the proxy: destination address -INFO:VariablesOrder:No variables ordering error found between implementation and the proxy -INFO:VariablesOrder:Run variables order checks between implementations... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) -INFO:VariablesOrder:No variables ordering error found between implementations +INFO:VariablesOrder:Run variable ordering checks between the proxy and implementation... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +INFO:VariablesOrder:state variable defined in proxy Proxy: address destination +INFO:VariablesOrder:No variable ordering issues found between proxy and implementation +INFO:VariablesOrder:Run variable ordering checks between the implementations... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +INFO:VariablesOrder:No variable ordering issues found between implementations diff --git a/tests/check-upgradeability/test_3.txt b/tests/check-upgradeability/test_3.txt index 2954e66c9a..3e657b0c94 100644 --- a/tests/check-upgradeability/test_3.txt +++ b/tests/check-upgradeability/test_3.txt @@ -4,8 +4,12 @@ INFO:CheckInitialization:Run initialization checks... (see https://github.c INFO:CheckInitialization:Initializable contract not found, the contract does not follow a standard initalization schema. INFO:CompareFunctions:Run function ids checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#functions-ids-checks) INFO:CompareFunctions:Shadowing between proxy and implementation found myFunc() -INFO:VariablesOrder:Run variables order checks between the implementation and the proxy... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) -INFO:VariablesOrder:Different variables between proxy and implem: destination address -> destination uint256 -INFO:VariablesOrder:Run variables order checks between implementations... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) -INFO:VariablesOrder:Different variables between v1 and v2: destination address -> destination uint256 -INFO:VariablesOrder:New variable: myFunc uint256 +INFO:VariablesOrder:Run variable ordering checks between the proxy and implementation... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +INFO:VariablesOrder:state variable defined in proxy Proxy: address destination +INFO:VariablesOrder:Extra state variable in proxy Proxy: address destination +INFO:VariablesOrder:Order of state variables in Proxy does not match ContractV2. Expected declaration #1 in Proxy to be address destination, found uint256 destination instead. +INFO:VariablesOrder:Run variable ordering checks between the implementations... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +INFO:VariablesOrder:New state variable in ContractV2: uint256 destination +INFO:VariablesOrder:New state variable in ContractV2: uint256 myFunc +INFO:VariablesOrder:Missing state variable in ContractV2: address destination +INFO:VariablesOrder:Order of state variables in ContractV2 does not match ContractV1. Expected declaration #1 in ContractV2 to be address destination, found uint256 destination instead. diff --git a/tests/check-upgradeability/test_4.txt b/tests/check-upgradeability/test_4.txt index 5f9dbb0c7d..75bbd63c66 100644 --- a/tests/check-upgradeability/test_4.txt +++ b/tests/check-upgradeability/test_4.txt @@ -4,8 +4,9 @@ INFO:CheckInitialization:Run initialization checks... (see https://github.c INFO:CheckInitialization:Initializable contract not found, the contract does not follow a standard initalization schema. INFO:CompareFunctions:Run function ids checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#functions-ids-checks) INFO:CompareFunctions:No function ids collision found -INFO:VariablesOrder:Run variables order checks between the implementation and the proxy... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) -INFO:VariablesOrder:Different variables between proxy and implem: destination address -> val uint256 -INFO:VariablesOrder:Run variables order checks between implementations... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) -INFO:VariablesOrder:Different variables between v1 and v2: destination address -> val uint256 -INFO:VariablesOrder:New variable: destination address +INFO:VariablesOrder:Run variable ordering checks between the proxy and implementation... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +INFO:VariablesOrder:state variable defined in proxy Proxy: address destination +INFO:VariablesOrder:Order of state variables in Proxy does not match ContractV2. Expected declaration #1 in Proxy to be address destination, found uint256 val instead. +INFO:VariablesOrder:Run variable ordering checks between the implementations... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +INFO:VariablesOrder:New state variable in ContractV2: uint256 val +INFO:VariablesOrder:Order of state variables in ContractV2 does not match ContractV1. Expected declaration #1 in ContractV2 to be address destination, found uint256 val instead. diff --git a/tests/check-upgradeability/test_5.txt b/tests/check-upgradeability/test_5.txt index fc7bf6e2c7..ec3fa5500e 100644 --- a/tests/check-upgradeability/test_5.txt +++ b/tests/check-upgradeability/test_5.txt @@ -11,6 +11,6 @@ Contract_double_call needs to be initialized by initialize()  INFO:CompareFunctions:Run function ids checks... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#functions-ids-checks) INFO:CompareFunctions:No function ids collision found -INFO:VariablesOrder:Run variables order checks between the implementation and the proxy... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) -INFO:VariablesOrder:Variable in the proxy: destination address -INFO:VariablesOrder:No variables ordering error found between implementation and the proxy +INFO:VariablesOrder:Run variable ordering checks between the proxy and implementation... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks) +INFO:VariablesOrder:state variable defined in proxy Proxy: address destination +INFO:VariablesOrder:No variable ordering issues found between proxy and implementation diff --git a/utils/upgradeability/compare_variables_order.py b/utils/upgradeability/compare_variables_order.py index 59a3b2f5af..f27313e702 100644 --- a/utils/upgradeability/compare_variables_order.py +++ b/utils/upgradeability/compare_variables_order.py @@ -10,91 +10,113 @@ logger.setLevel(logging.INFO) def compare_variables_order_implementation(v1, contract_name1, v2, contract_name2): - - logger.info(green('Run variables order checks between implementations... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks)')) - - contract_v1 = v1.get_contract_from_name(contract_name1) - if contract_v1 is None: - logger.info(red('Contract {} not found in {}'.format(contract_name1, v1.filename))) - exit(-1) - - contract_v2 = v2.get_contract_from_name(contract_name2) - if contract_v2 is None: - logger.info(red('Contract {} not found in {}'.format(contract_name2, v2.filename))) - exit(-1) - - - order_v1 = [(variable.name, variable.type) for variable in contract_v1.state_variables if not variable.is_constant] - order_v2 = [(variable.name, variable.type) for variable in contract_v2.state_variables if not variable.is_constant] - - - found = False - for idx in range(0, len(order_v1)): - (v1_name, v1_type) = order_v1[idx] - if len(order_v2) < idx: - logger.info(red('Missing variable in the new version: {} {}'.format(v1_name, v1_type))) - continue - (v2_name, v2_type) = order_v2[idx] - - if (v1_name != v2_name) or (v1_type != v2_type): - found = True - logger.info(red('Different variables between v1 and v2: {} {} -> {} {}'.format(v1_name, - v1_type, - v2_name, - v2_type))) - - if len(order_v2) > len(order_v1): - new_variables = order_v2[len(order_v1):] - for (name, t) in new_variables: - logger.info(green('New variable: {} {}'.format(name, t))) - - if not found: - logger.info(green('No variables ordering error found between implementations')) + logger.info(green('Run variable ordering checks between the ' + + 'implementations... (see https://github.com/crytic/slither/wiki/' + + 'Upgradeability-Checks#variables-order-checks)' + )) + + contract_v1 = __get_and_check_contracts_existence(v1, contract_name1) + contract_v2 = __get_and_check_contracts_existence(v2, contract_name2) + vars_v1 = __contract_state_variables(contract_v1) + vars_v2 = __contract_state_variables(contract_v2) + issues_found = False + + new_vars = [var for var in vars_v2 if not var in vars_v1] + for (var_type, var_name) in new_vars: + logger.info(green('New state variable in {}: {} {}'.format( + contract_name2, + var_type, + var_name + ))) + + missing_vars = [var for var in vars_v1 if not var in vars_v2] + for (var_type, var_name) in missing_vars: + issues_found = True + logger.info(red('Missing state variable in {}: {} {}'.format( + contract_name2, + var_type, + var_name + ))) + + if __check_vars_order(contract_name1, contract_name2, vars_v1, vars_v2): + issues_found = True + + if not issues_found: + logger.info(green('No variable ordering issues found between implementations')) def compare_variables_order_proxy(implem, implem_name, proxy, proxy_name): - - logger.info(green('Run variables order checks between the implementation and the proxy... (see https://github.com/crytic/slither/wiki/Upgradeability-Checks#variables-order-checks)')) - - contract_implem = implem.get_contract_from_name(implem_name) - if contract_implem is None: - logger.info(red('Contract {} not found in {}'.format(implem_name, implem.filename))) - exit(-1) - - contract_proxy = proxy.get_contract_from_name(proxy_name) - if contract_proxy is None: - logger.info(red('Contract {} not found in {}'.format(proxy_name, proxy.filename))) + logger.info(green('Run variable ordering checks between the proxy and ' + + 'implementation... (see https://github.com/crytic/slither/wiki/' + + 'Upgradeability-Checks#variables-order-checks)' + )) + + contract_implem = __get_and_check_contracts_existence(implem, implem_name) + contract_proxy = __get_and_check_contracts_existence(proxy, proxy_name) + vars_implem = __contract_state_variables(contract_implem) + vars_proxy = __contract_state_variables(contract_proxy) + issues_found = False + + for (var_type, var_name) in vars_proxy: + logger.info(yellow('state variable defined in proxy {}: {} {}'.format( + proxy_name, + var_type, + var_name + ))) + + extra_vars = [var for var in vars_proxy if not var in vars_implem] + for (var_type, var_name) in extra_vars: + issues_found = True + logger.info(red('Extra state variable in proxy {}: {} {}'.format( + proxy_name, + var_type, + var_name + ))) + + if __check_vars_order(implem_name, proxy_name, vars_proxy, vars_implem): + issues_found = True + + if not issues_found: + logger.info(green('No variable ordering issues found between proxy and implementation')) + +def __get_and_check_contracts_existence(source, name): + contract = source.get_contract_from_name(name) + if contract is None: + logger.info(red('Contract {} not found in {}'.format( + name, + source.filename + ))) exit(-1) - - - order_implem = [(variable.name, variable.type) for variable in contract_implem.state_variables if not variable.is_constant] - order_proxy = [(variable.name, variable.type) for variable in contract_proxy.state_variables if not variable.is_constant] - - - found = False - for idx in range(0, len(order_proxy)): - (proxy_name, proxy_type) = order_proxy[idx] - if len(order_implem) <= idx: - logger.info(red('Extra variable in the proxy: {} {}'.format(proxy_name, proxy_type))) - continue - (implem_name, implem_type) = order_implem[idx] - - if (proxy_name != implem_name) or (proxy_type != implem_type): - found = True - logger.info(red('Different variables between proxy and implem: {} {} -> {} {}'.format(proxy_name, - proxy_type, - implem_name, - implem_type))) - else: - logger.info(yellow('Variable in the proxy: {} {}'.format(proxy_name, - proxy_type))) - - - #if len(order_implem) > len(order_proxy): - # new_variables = order_implem[len(order_proxy):] - # for (name, t) in new_variables: - # logger.info(green('Variable only in implem: {} {}'.format(name, t))) - - if not found: - logger.info(green('No variables ordering error found between implementation and the proxy')) - - + return contract + +def __check_vars_order(old_name, new_name, vars_v1, vars_v2): + issues_found = False + for idx in range(len(vars_v1)): + if idx >= len(vars_v2) or vars_v2[idx] != vars_v1[idx]: + issues_found = True + (v1_var_type, v1_var_name) = vars_v1[idx] + + msg = 'Order of state variables in {} does not match {}. ' + msg += 'Expected declaration #{} in {} to be {} {}' + msg = msg.format( + new_name, + old_name, + idx+1, + new_name, + v1_var_type, + v1_var_name + ) + + if idx < len(vars_v2): + v2_var_type, v2_var_name = vars_v2[idx] + msg += ', found {} {} instead.'.format( + v2_var_type, + v2_var_name + ) + else: + msg += '.' + + logger.info(red(msg)) + return issues_found + +def __contract_state_variables(contract): + return [(v.type, v.name) for v in contract.state_variables if not v.is_constant]