General improvements for utils/upgradeability/compare_variables_order.py

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

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

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.

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.

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

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]