Fix reorder argument edge case

crytic · Dec 6, 2024 · 7642299 · 7642299
1 parent bcd0f83
commit 7642299
Show file tree

Hide file tree

Showing 3 changed files with 24 additions and 10 deletions.
diff --git a/slither/slithir/convert.py b/slither/slithir/convert.py
@@ -433,8 +433,11 @@ def reorder_arguments(
         assert len(call_names) == len(names)
 
     args_ret = []
-    index_seen = []
+    index_seen = []  # Will have the correct index order
 
+    # We search for declaration of parameters that is fully compatible with the call arguments
+    # that is why if a arg name is not present in the call name we break and clear index_seen
+    # Known issue if the overridden function reuse the same parameters' name but in different positions
     for names in decl_names:
         if len(index_seen) == len(args):
             break
@@ -445,9 +448,12 @@ def reorder_arguments(
                 if ind in index_seen:
                     continue
             except ValueError:
-                continue
+                index_seen.clear()
+                break
             index_seen.append(ind)
-            args_ret.append(args[ind])
+
+    for ind in index_seen:
+        args_ret.append(args[ind])
 
     return args_ret
 

diff --git a/tests/unit/slithir/test_argument_reorder.py b/tests/unit/slithir/test_argument_reorder.py
@@ -77,11 +77,15 @@ def test_overridden_function_reorder(solc_binary_path) -> None:
 
     assert (
         isinstance(internal_calls[0].arguments[0], Constant)
-        and internal_calls[0].arguments[0].value == 34
+        and internal_calls[0].arguments[0].value == 23
     )
     assert (
         isinstance(internal_calls[0].arguments[1], Constant)
-        and internal_calls[0].arguments[1].value == 23
+        and internal_calls[0].arguments[1].value == 36
+    )
+    assert (
+        isinstance(internal_calls[0].arguments[2], Constant)
+        and internal_calls[0].arguments[2].value == 34
     )
 
     operations = slither.contracts[1].functions[1].slithir_operations
@@ -90,9 +94,13 @@ def test_overridden_function_reorder(solc_binary_path) -> None:
 
     assert (
         isinstance(internal_calls[0].arguments[0], Constant)
-        and internal_calls[0].arguments[0].value == 34
+        and internal_calls[0].arguments[0].value == 23
     )
     assert (
         isinstance(internal_calls[0].arguments[1], Constant)
-        and internal_calls[0].arguments[1].value == 23
+        and internal_calls[0].arguments[1].value == 36
+    )
+    assert (
+        isinstance(internal_calls[0].arguments[2], Constant)
+        and internal_calls[0].arguments[2].value == 34
     )
diff --git a/tests/unit/slithir/test_data/argument_reorder/test_overridden_function.sol b/tests/unit/slithir/test_data/argument_reorder/test_overridden_function.sol
@@ -1,8 +1,8 @@
 contract A {
- function a(uint256 q, uint256 e) internal virtual {}
- function b() public { a({e: 23, q: 34}); }
+ function a(uint256 e, uint256 q, uint256 w) internal virtual {}
+ function b() public { a({e: 23, w: 34, q: 36}); }
 }
 
 contract B is A {
- function a(uint256 w, uint256 q) internal override {}
+ function a(uint256 q, uint256 ww, uint256 e) internal override {}
 }