diff --git a/.circleci/config.yml b/.circleci/config.yml index b1feb16c5..68e6f8de6 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -168,8 +168,7 @@ jobs: - utils/checkout-with-mise - run: name: just new recipe tests - command: | - (cd src/ && bash script/test-just-new.sh) + command: (cd src/ && bash ../test/script/test-just-new.sh) print_versions: docker: @@ -202,6 +201,18 @@ jobs: - run: name: check task names command: (cd src/script && ./check-task-names.sh) + # Check that structs used with JSON/TOML parseRaw have alphabetically ordered fields. + # This prevents a subtle bug where Foundry's parseRaw orders keys alphabetically, + # causing abi.decode to assign values to wrong fields if struct fields aren't sorted. See: + # https://getfoundry.sh/reference/cheatcodes/parse-toml/#decoding-toml-tables-into-solidity-structs + check_struct_order: + docker: + - image: <> + steps: + - utils/checkout-with-mise + - run: + name: check struct field ordering + command: (cd src/script && ./check-struct-order.sh) close-issue: machine: image: ubuntu-2204:2024.08.1 @@ -256,6 +267,7 @@ workflows: context: - circleci-repo-readonly-authenticated-github-token - check_task_names + - check_struct_order # Verify that all tasks have a valid status in their README. - check_task_statuses: context: diff --git a/src/script/check-struct-order.sh b/src/script/check-struct-order.sh new file mode 100755 index 000000000..d44146495 --- /dev/null +++ b/src/script/check-struct-order.sh @@ -0,0 +1,209 @@ +#!/bin/bash +set -eo pipefail + +# --------------------------------------------- +# Script to detect structs used with TOML/JSON parseRaw that have +# non-alphabetically ordered fields. +# +# When Foundry's parseRaw() converts TOML/JSON to ABI-encoded bytes, +# it orders struct fields ALPHABETICALLY by key name. If the Solidity +# struct fields are not in alphabetical order, abi.decode() will +# assign values to the wrong fields. See: +# https://getfoundry.sh/reference/cheatcodes/parse-toml/#decoding-toml-tables-into-solidity-structs +# +# This script: +# 1. Finds all abi.decode() calls with parseRaw/parseToml/parseJson +# 2. Extracts the struct type being decoded into +# 3. Finds the struct definition +# 4. Verifies fields are in alphabetical order +# +# Usage: Run from any directory inside the git repo. +# --------------------------------------------- + +# Get the root directory of the git repo +root_dir=$(git rev-parse --show-toplevel) +src_dir="$root_dir/src" +exit_code=0 + +# Colors for output (disabled if not a terminal) +if [ -t 1 ]; then + RED='\033[0;31m' + GREEN='\033[0;32m' + YELLOW='\033[0;33m' + NC='\033[0m' # No Color +else + RED='' + GREEN='' + YELLOW='' + NC='' +fi + +# Primitive types to skip - these don't have named fields, so alphabetical +# ordering doesn't apply. Only structs with named fields can have the bug. +# Example: abi.decode(..., (address[])) is safe, but abi.decode(..., (MyStruct[])) needs checking. +# Pattern matches: address, bool, string, bytes, uint8-uint256, int8-int256, bytes1-bytes32 +PRIMITIVE_TYPES="address|bool|string|bytes|uint[0-9]+|int[0-9]+|bytes[0-9]+" + +# Function to extract struct field names from a file +# Args: $1 = file path, $2 = struct name +# Returns: newline-separated field names +extract_struct_fields() { + local file=$1 + local struct_name=$2 + + # Use awk to extract the struct body (handles indented closing brace) + # Then grep for field declarations: ; or [] ; + awk "/struct ${struct_name} \\{/,/^[[:space:]]*\\}/" "$file" 2>/dev/null | \ + grep -E '^[[:space:]]+[A-Za-z][A-Za-z0-9_]*(\[\])?[[:space:]]+[a-z][A-Za-z0-9_]*;' | \ + awk '{print $2}' | \ + sed 's/;//g' +} + +# Function to find struct definition file +# Args: $1 = struct name, $2 = starting file (to check first) +# Returns: file path containing the struct, or empty if not found +find_struct_file() { + local struct_name=$1 + local start_file=$2 + + # First check the starting file + if grep -q "struct ${struct_name} {" "$start_file" 2>/dev/null; then + echo "$start_file" + return + fi + + # Search in common locations + local found_file + found_file=$(grep -rl "struct ${struct_name} {" "$src_dir" 2>/dev/null | head -1) + + if [ -n "$found_file" ]; then + echo "$found_file" + fi +} + +# Function to check if fields are alphabetically ordered +# Args: field names via stdin (newline-separated) +# Returns: 0 if ordered, 1 if not +check_alphabetical_order() { + local fields + fields=$(cat) + + if [ -z "$fields" ]; then + return 0 # Empty is considered ordered + fi + + local sorted + sorted=$(echo "$fields" | sort) + + if [ "$fields" = "$sorted" ]; then + return 0 + else + return 1 + fi +} + +# Function to extract struct type from abi.decode line +# Args: $1 = line containing abi.decode +# Returns: struct type name or empty if primitive/not found +extract_struct_type() { + local line=$1 + + # Extract the type from patterns like: + # abi.decode(..., (StructName[])) + # abi.decode(..., (StructName)) + local type_match + + # Try to match (TypeName[]) or (TypeName) at the end + type_match=$(echo "$line" | sed -E 's/.*\(([A-Z][A-Za-z0-9_]+)(\[\])?\)\s*\)\s*;.*/\1/' 2>/dev/null) + + # Check if we got a valid type (starts with uppercase, not a primitive) + if [ -n "$type_match" ] && [ "$type_match" != "$line" ]; then + # Skip primitive types + if echo "$type_match" | grep -qE "^(${PRIMITIVE_TYPES})$"; then + return + fi + echo "$type_match" + fi +} + +echo "Checking TOML/JSON struct field ordering..." +echo "" + +# Find all files with abi.decode + parseRaw/parseToml/parseJson patterns +# Using a temp file for portability (process substitution behavior varies) +temp_file=$(mktemp) +checked_file=$(mktemp) +trap 'rm -f "$temp_file" "$checked_file"' EXIT + +grep -rn "abi\.decode.*\(parseRaw\|\.parseRaw\|vm\.parseToml\|vm\.parseJson\)" \ + --include="*.sol" "$src_dir" 2>/dev/null > "$temp_file" || true + +if [ ! -s "$temp_file" ]; then + echo "No TOML/JSON struct parsing patterns found." + exit 0 +fi + +while IFS= read -r match; do + # Parse the grep output: file:line:content + file=$(echo "$match" | cut -d: -f1) + line_num=$(echo "$match" | cut -d: -f2) + line_content=$(echo "$match" | cut -d: -f3-) + + # Extract struct type + struct_type=$(extract_struct_type "$line_content") + + if [ -z "$struct_type" ]; then + continue # Skip primitives or unparseable lines + fi + + # Find the struct definition file + struct_file=$(find_struct_file "$struct_type" "$file") + + if [ -z "$struct_file" ]; then + echo -e "${YELLOW}Warning: Could not find struct '$struct_type' (used in $file:$line_num)${NC}" + continue + fi + + # Create a unique key for this struct+file combination + check_key="${struct_type}:${struct_file}" + + # Skip if we've already checked this combination (use grep on temp file) + if grep -qF "$check_key" "$checked_file" 2>/dev/null; then + continue + fi + echo "$check_key" >> "$checked_file" + + # Extract and check field ordering + fields=$(extract_struct_fields "$struct_file" "$struct_type") + + if [ -z "$fields" ]; then + echo -e "${YELLOW}Warning: Could not extract fields from struct '$struct_type' in $struct_file${NC}" + continue + fi + + if echo "$fields" | check_alphabetical_order; then + echo -e "${GREEN}OK${NC} $struct_type (in ${struct_file#"$root_dir"/})" + else + echo -e "${RED}FAIL${NC} $struct_type (in ${struct_file#"$root_dir"/})" + echo " Fields are not in alphabetical order!" + echo " Current order: $(echo "$fields" | tr '\n' ' ')" + echo " Expected order: $(echo "$fields" | sort | tr '\n' ' ')" + echo "" + exit_code=1 + fi +done < "$temp_file" + +echo "" +if [ $exit_code -eq 0 ]; then + echo -e "${GREEN}All TOML/JSON-parsed structs have correctly ordered fields.${NC}" +else + echo -e "${RED}[FAIL] Some structs have incorrectly ordered fields! See above for details.${NC}" + echo "" + echo "To fix: Reorder the struct fields to be in alphabetical order." + echo "This is required because Foundry's parseRaw()/parseToml()/parseJson()" + echo "orders keys alphabetically when converting to ABI-encoded bytes." + echo "" + echo "See: https://getfoundry.sh/reference/cheatcodes/parse-toml/#decoding-toml-tables-into-solidity-structs" +fi + +exit $exit_code diff --git a/src/template/OPCMUpgradeV600.sol b/src/template/OPCMUpgradeV600.sol index 4feaef597..e0715b43f 100644 --- a/src/template/OPCMUpgradeV600.sol +++ b/src/template/OPCMUpgradeV600.sol @@ -18,8 +18,8 @@ contract OPCMUpgradeV600 is OPCMTaskBase { /// @notice Struct to store inputs data for each L2 chain. struct OPCMUpgrade { - Claim cannonPrestate; Claim cannonKonaPrestate; + Claim cannonPrestate; uint256 chainId; string expectedValidationErrors; } diff --git a/test/script/test-check-struct-order.sh b/test/script/test-check-struct-order.sh new file mode 100755 index 000000000..7f4c1ed1f --- /dev/null +++ b/test/script/test-check-struct-order.sh @@ -0,0 +1,296 @@ +#!/bin/bash +set -eo pipefail + +# --------------------------------------------- +# Test script for check-struct-order.sh +# +# Creates temporary Solidity files with various struct patterns +# and verifies the check script correctly identifies: +# - Structs with alphabetically ordered fields (should pass) +# - Structs with non-alphabetically ordered fields (should fail) +# +# Usage: Run from any directory inside the git repo. +# --------------------------------------------- + +# Get the repo root and script directories +repo_root="$(git rev-parse --show-toplevel)" +src_script_dir="$repo_root/src/script" + +echo "Running check-struct-order.sh tests..." +echo "" + +# Track test results +tests_passed=0 +tests_failed=0 + +# Track temp directories for cleanup +temp_dirs="" + +# shellcheck disable=SC2317,SC2329 # cleanup is called via trap +cleanup() { + for dir in $temp_dirs; do + [ -d "$dir" ] && rm -rf "$dir" + done +} +trap cleanup EXIT + +# Helper function to run a test case +# Args: $1 = test name, $2 = expected result (pass|fail), $3 = file content +run_test() { + local test_name=$1 + local expected=$2 + local content=$3 + + # Create a fresh temp directory for this test + local test_dir + test_dir=$(mktemp -d) + temp_dirs="$temp_dirs $test_dir" + + # Create test file + local test_file="$test_dir/src/template/Test${test_name}.sol" + mkdir -p "$(dirname "$test_file")" + echo "$content" > "$test_file" + + # Initialize git repo + (cd "$test_dir" && git init -q) > /dev/null 2>&1 + + # Run the check script from the test directory + local result + if (cd "$test_dir" && "$src_script_dir/check-struct-order.sh") > /dev/null 2>&1; then + result="pass" + else + result="fail" + fi + + # Check result + if [ "$result" = "$expected" ]; then + echo " PASS: $test_name (expected $expected, got $result)" + tests_passed=$((tests_passed + 1)) + else + echo " FAIL: $test_name (expected $expected, got $result)" + tests_failed=$((tests_failed + 1)) + fi +} + +echo "=== Test Cases ===" +echo "" + +# Test 1: Alphabetically ordered struct (should pass) +run_test "AlphabeticalOrder" "pass" ' +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import {stdToml} from "forge-std/StdToml.sol"; + +contract TestAlphabeticalOrder { + using stdToml for string; + + struct Config { + address addr; + uint256 chainId; + string name; + } + + function setup(string memory toml) internal { + Config[] memory configs = abi.decode(toml.parseRaw(".configs"), (Config[])); + } +} +' + +# Test 2: Non-alphabetically ordered struct (should fail) +run_test "NonAlphabeticalOrder" "fail" ' +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import {stdToml} from "forge-std/StdToml.sol"; + +contract TestNonAlphabeticalOrder { + using stdToml for string; + + struct Config { + string name; + address addr; + uint256 chainId; + } + + function setup(string memory toml) internal { + Config[] memory configs = abi.decode(toml.parseRaw(".configs"), (Config[])); + } +} +' + +# Test 3: Primitive array (should pass - no struct to check) +run_test "PrimitiveArray" "pass" ' +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import {stdToml} from "forge-std/StdToml.sol"; + +contract TestPrimitiveArray { + using stdToml for string; + + function setup(string memory toml) internal { + address[] memory addrs = abi.decode(toml.parseRaw(".addresses"), (address[])); + uint256[] memory ids = abi.decode(toml.parseRaw(".ids"), (uint256[])); + } +} +' + +# Test 4: Single struct (not array) - alphabetical (should pass) +run_test "SingleStructAlphabetical" "pass" ' +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import {VmSafe} from "forge-std/Vm.sol"; + +contract TestSingleStructAlphabetical { + VmSafe internal constant vm = VmSafe(address(uint160(uint256(keccak256("hevm cheat code"))))); + + struct Settings { + bool enabled; + uint256 value; + } + + function setup(string memory toml) internal { + Settings memory s = abi.decode(vm.parseToml(toml, ".settings"), (Settings)); + } +} +' + +# Test 5: Similar field name prefixes - wrong order (should fail) +# This is the exact bug pattern from OPCMUpgradeV600 +run_test "SimilarPrefixes" "fail" ' +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import {stdToml} from "forge-std/StdToml.sol"; + +contract TestSimilarPrefixes { + using stdToml for string; + + struct Prestate { + bytes32 cannonPrestate; + bytes32 cannonKonaPrestate; + } + + function setup(string memory toml) internal { + Prestate[] memory p = abi.decode(toml.parseRaw(".prestates"), (Prestate[])); + } +} +' + +# Test 6: Similar prefixes but correct order (should pass) +run_test "SimilarPrefixesCorrect" "pass" ' +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import {stdToml} from "forge-std/StdToml.sol"; + +contract TestSimilarPrefixesCorrect { + using stdToml for string; + + struct Prestate { + bytes32 cannonKonaPrestate; + bytes32 cannonPrestate; + } + + function setup(string memory toml) internal { + Prestate[] memory p = abi.decode(toml.parseRaw(".prestates"), (Prestate[])); + } +} +' + +# Test 7: No parseRaw usage (should pass - nothing to check) +run_test "NoParseRaw" "pass" ' +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +contract TestNoParseRaw { + struct Config { + string name; + address addr; + } + + function doSomething() internal pure returns (uint256) { + return 42; + } +} +' + +# Test 8: Complex types in struct (should pass if alphabetical) +run_test "ComplexTypes" "pass" ' +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import {stdToml} from "forge-std/StdToml.sol"; + +contract TestComplexTypes { + using stdToml for string; + + struct GameConfig { + address[] addresses; + bytes32 gameType; + uint256[] values; + } + + function setup(string memory toml) internal { + GameConfig[] memory configs = abi.decode(toml.parseRaw(".games"), (GameConfig[])); + } +} +' + +# Test 9: JSON with stdJson.parseRaw - wrong order (should fail) +run_test "JsonWrongOrder" "fail" ' +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import {stdJson} from "forge-std/StdJson.sol"; + +contract TestJsonWrongOrder { + using stdJson for string; + + struct JsonConfig { + string name; + address addr; + } + + function setup(string memory json) internal { + JsonConfig[] memory configs = abi.decode(json.parseRaw(".configs"), (JsonConfig[])); + } +} +' + +# Test 10: JSON with vm.parseJson - correct order (should pass) +run_test "JsonCorrectOrder" "pass" ' +// SPDX-License-Identifier: MIT +pragma solidity 0.8.15; + +import {VmSafe} from "forge-std/Vm.sol"; + +contract TestJsonCorrectOrder { + VmSafe internal constant vm = VmSafe(address(uint160(uint256(keccak256("hevm cheat code"))))); + + struct JsonConfig { + address addr; + string name; + } + + function setup(string memory json) internal { + JsonConfig memory config = abi.decode(vm.parseJson(json, ".config"), (JsonConfig)); + } +} +' + +echo "" +echo "=== Summary ===" +echo "Passed: $tests_passed" +echo "Failed: $tests_failed" +echo "" + +if [ $tests_failed -gt 0 ]; then + echo "Some tests failed!" + exit 1 +else + echo "All tests passed!" + exit 0 +fi diff --git a/src/script/test-just-new.sh b/test/script/test-just-new.sh similarity index 100% rename from src/script/test-just-new.sh rename to test/script/test-just-new.sh diff --git a/test/tasks/Regression.t.sol b/test/tasks/Regression.t.sol index 4a5535760..fd3831416 100644 --- a/test/tasks/Regression.t.sol +++ b/test/tasks/Regression.t.sol @@ -878,7 +878,7 @@ contract RegressionTest is Test { function testRegressionCallDataMatches_OPCMUpgradeV600Template() public { string memory taskConfigFilePath = "test/tasks/example/sep/033-opcm-upgrade-v600/config.toml"; string memory expectedCallData = - "0x82ad56cb000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000020000000000000000000000000f0a2e224519e876979ea6b2cd15ef5cc3d6703bd0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000a4cbeda5a700000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000001000000000000000000000000034edd2a225f7f429a63e0f1d2084b9e0a93b53803845751c66672c0b09e68ba7c3024a7543a1a22edaa90d7c2c90ebc8cecee6803f833cc2a644a9f7bba9718c13f622b867c513f3c43f3eb5a0cad17784bd40800000000000000000000000000000000000000000000000000000000"; + "0x82ad56cb000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000010000000000000000000000000000000000000000000000000000000000000020000000000000000000000000f0a2e224519e876979ea6b2cd15ef5cc3d6703bd0000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000a4cbeda5a700000000000000000000000000000000000000000000000000000000000000200000000000000000000000000000000000000000000000000000000000000001000000000000000000000000034edd2a225f7f429a63e0f1d2084b9e0a93b53803f833cc2a644a9f7bba9718c13f622b867c513f3c43f3eb5a0cad17784bd40803845751c66672c0b09e68ba7c3024a7543a1a22edaa90d7c2c90ebc8cecee6800000000000000000000000000000000000000000000000000000000"; MultisigTask multisigTask = new OPCMUpgradeV600(); address rootSafe = address(0x1Eb2fFc903729a0F03966B917003800b145F56E2); address nestedSafe = address(0xDEe57160aAfCF04c34C887B5962D0a69676d3C8B); // sepolia @@ -891,10 +891,10 @@ contract RegressionTest is Test { string[] memory expectedDataToSign = new string[](2); // Foundation expectedDataToSign[0] = - "0x190137e1f5dd3b92a004a23589b741196c8a214629d4ea3a690ec8e41ae45c689cbbdebb61a012c61cc79de4251db4172889f9c2bea3593fdb3cf79b7016d90b88d8"; + "0x190137e1f5dd3b92a004a23589b741196c8a214629d4ea3a690ec8e41ae45c689cbb0e99c9006e46153badc324f7b1c50824fa5afe34e8b2a22b33ce602d2d3d5d8a"; // Security Council expectedDataToSign[1] = - "0x1901be081970e9fc104bd1ea27e375cd21ec7bb1eec56bfe43347c3e36c5d27b85338af46bfe0ef8d4c4dcd740b727f664c01b5d0ee612f67383466fdebaff9ae1fb"; + "0x1901be081970e9fc104bd1ea27e375cd21ec7bb1eec56bfe43347c3e36c5d27b8533f37689cf5b2227f8ba807d4618c41f8dac247be9429d1c5339241e2a9938c16e"; _assertDataToSignNestedMultisig(multisigTask, actions, expectedDataToSign, MULTICALL3_ADDRESS, rootSafe); }