test: ✨ Add storage layout checks for upgradeable contracts (#420)

## Summary Implements storage layout testing for the upgradeable `DataHavenServiceManager` contract to prevent state corruption during proxy upgrades. ## Changes ### New Files - **`contracts/storage-snapshots/DataHavenServiceManager.storage.json`** - Baseline storage layout snapshot - **`contracts/storage-snapshots/README.md`** - Documentation for updating snapshots and known limitations - **`contracts/scripts/check-storage-layout.sh`** - CI script that compares current layout against snapshot - **`contracts/test/storage/StorageLayout.t.sol`** - Upgrade simulation tests verifying state preservation - **`.github/workflows/task-storage-layout.yml`** - CI workflow for storage layout checks ### Modified Files - **`.github/workflows/CI.yml`** - Added `storage-layout` job to run in parallel with other checks ## How It Works **Two-pronged approach:** 1. **Snapshot Diff** - Compares current storage layout against committed snapshot using `forge inspect`. Catches unintended variable reordering, type changes, or gap modifications. 2. **Upgrade Simulation** - Foundry tests that populate state, perform a proxy upgrade, and verify all values survive: - `test_upgradePreservesState` - Verifies core state variables - `test_upgradePreservesValidatorMappings` - Verifies `validatorEthAddressToSolochainAddress` mapping - `test_upgradePreservesMultipleValidators` - Verifies `validatorsAllowlist` with multiple entries - `test_functionalityAfterUpgrade` - Verifies contract remains functional post-upgrade ## Normalization The snapshot comparison normalizes JSON to avoid false positives: - Removes `astId` (changes with compiler runs) - Removes `contract` (contains full file path) - Removes `.types` section (contains unstable AST IDs embedded in type keys) - Sorts by slot number ## Usage ```bash # Check storage layout against snapshot ./scripts/check-storage-layout.sh # Run upgrade simulation tests forge test --match-contract StorageLayoutTest -vvv # Update snapshot (when intentionally changing storage) forge inspect DataHavenServiceManager storage --json > storage-snapshots/DataHavenServiceManager.storage.json ``` ## Test Plan - ./scripts/check-storage-layout.sh passes - forge test --match-contract StorageLayoutTest -vvv passes (4 tests) - CI workflow runs successfully
2026-05-24 01:38:32 +00:00 · 2026-02-05 12:08:35 +01:00 · 2026-02-05 12:08:35 +01:00 · da2847bbbf
commit da2847bbbf
parent a8df6aae95
8 changed files with 571 additions and 0 deletions
--- a/.github/workflows/CI.yml
+++ b/.github/workflows/CI.yml
@ -44,6 +44,8 @@ jobs:
    uses: ./.github/workflows/task-rust-tests.yml
  contract-tests:
    uses: ./.github/workflows/task-foundry-tests.yml
+  storage-layout:
+    uses: ./.github/workflows/task-storage-layout.yml
  rust-lint:
    needs: [warm-sccache]
    uses: ./.github/workflows/task-rust-lint.yml
--- a/.github/workflows/task-storage-layout.yml
+++ b/.github/workflows/task-storage-layout.yml
@ -0,0 +1,51 @@
+# Storage Layout Check: Validates storage layout for upgradeable contracts
+#
+# Overview:
+# 1. Compares current storage layout against committed snapshot
+# 2. Runs upgrade simulation tests to verify state preservation
+
+name: Storage Layout Check
+
+on:
+  workflow_dispatch:
+  workflow_call:
+
+# Explicit minimal permissions
+permissions:
+  contents: read
+
+env:
+  FOUNDRY_PROFILE: ci
+
+jobs:
+  check:
+    name: Storage Layout
+    runs-on: ubuntu-latest
+    defaults:
+      run:
+        working-directory: contracts
+    steps:
+      - uses: actions/checkout@v4
+        with:
+          submodules: recursive
+
+      - name: Install Foundry
+        uses: foundry-rs/foundry-toolchain@v1
+        with:
+          version: v1.4.3
+
+      - name: Build contracts
+        run: forge build --extra-output storageLayout
+
+      - name: Negative check storage layout (should fail)
+        run: |
+          chmod +x scripts/check-storage-layout-negative.sh
+          ./scripts/check-storage-layout-negative.sh
+
+      - name: Check storage layout
+        run: |
+          chmod +x scripts/check-storage-layout.sh
+          ./scripts/check-storage-layout.sh
+
+      - name: Run upgrade simulation tests
+        run: forge test --match-contract StorageLayoutTest -vvv
--- a/contracts/script/fixtures/DataHavenServiceManagerBadLayout.sol
+++ b/contracts/script/fixtures/DataHavenServiceManagerBadLayout.sol
@ -0,0 +1,31 @@
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity ^0.8.27;
+
+import {OwnableUpgradeable} from "@openzeppelin-upgrades/contracts/access/OwnableUpgradeable.sol";
+
+import {IGatewayV2} from "snowbridge/src/v2/IGateway.sol";
+
+/// @notice Test-only fixture contract with intentionally broken storage layout.
+/// @dev This contract is used to validate the snapshot-diff storage layout check fails as expected.
+contract DataHavenServiceManagerBadLayout is OwnableUpgradeable {
+    // Deliberate layout shift: inserted before all original state vars
+    uint256 public layoutBreaker;
+
+    // Original variables (shifted by one slot)
+    address public rewardsInitiator;
+    mapping(address => bool) public validatorsAllowlist;
+    IGatewayV2 private _snowbridgeGateway;
+    mapping(address => address) public validatorEthAddressToSolochainAddress;
+
+    // Keep the original gap size to mirror shape, despite the shift
+    uint256[46] private __GAP;
+
+    // Keep a compatible constructor signature for upgrade tests.
+    constructor(
+        address,
+        address
+    ) {
+        _disableInitializers();
+    }
+}
+
--- a/contracts/scripts/check-storage-layout-negative.sh
+++ b/contracts/scripts/check-storage-layout-negative.sh
@ -0,0 +1,32 @@
+#!/bin/bash
+set -euo pipefail
+
+# Negative check: ensure the snapshot-diff storage layout script fails when the layout is broken.
+
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+cd "$SCRIPT_DIR/.."
+
+set +e
+OUTPUT="$(
+    CONTRACT="DataHavenServiceManagerBadLayout" \
+        SNAPSHOT="storage-snapshots/DataHavenServiceManager.storage.json" \
+        ./scripts/check-storage-layout.sh 2>&1
+)"
+EXIT_CODE=$?
+set -e
+
+if [ "$EXIT_CODE" -eq 0 ]; then
+    echo "ERROR: Expected storage layout check to fail for DataHavenServiceManagerBadLayout, but it succeeded."
+    exit 1
+fi
+
+if ! printf '%s\n' "$OUTPUT" | grep -q "ERROR: Storage layout has changed!"; then
+    echo "ERROR: Storage layout check failed, but not for the expected reason."
+    echo ""
+    echo "Output:"
+    echo "$OUTPUT"
+    exit 1
+fi
+
+echo "Negative check OK: storage layout check failed as expected for DataHavenServiceManagerBadLayout."
+
--- a/contracts/scripts/check-storage-layout.sh
+++ b/contracts/scripts/check-storage-layout.sh
@ -0,0 +1,62 @@
+#!/bin/bash
+set -e
+
+# Storage Layout Check Script
+# Compares current storage layout against committed snapshot to detect unintended changes.
+
+CONTRACT="${CONTRACT:-DataHavenServiceManager}"
+SNAPSHOT_DIR="${SNAPSHOT_DIR:-storage-snapshots}"
+SNAPSHOT="${SNAPSHOT:-${SNAPSHOT_DIR}/${CONTRACT}.storage.json}"
+
+# Ensure we're in the contracts directory
+SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
+cd "$SCRIPT_DIR/.."
+
+# Check if snapshot exists
+if [ ! -f "$SNAPSHOT" ]; then
+    echo "ERROR: Snapshot file not found: $SNAPSHOT"
+    echo "Generate it with: mkdir -p $SNAPSHOT_DIR && forge inspect $CONTRACT storage --json > $SNAPSHOT"
+    exit 1
+fi
+
+# Generate current layout
+echo "Generating current storage layout for $CONTRACT..."
+forge inspect "$CONTRACT" storage --json > /tmp/current_layout.json
+
+# Normalize both files for comparison:
+# - Remove astId (changes with compiler runs)
+# - Remove contract field (contains full path)
+# - Remove types section (contains unstable AST IDs)
+# - Sort by slot number
+normalize_json() {
+    jq 'del(.types)
+        | .storage
+        | map(
+            del(.astId, .contract)
+            # Remove unstable AST ID suffixes from type strings (e.g., t_contract(IGatewayV2)12345)
+            | .type |= sub("\\)[0-9]+$"; ")")
+          )
+        | sort_by(.slot | tonumber)' "$1"
+}
+
+echo "Comparing storage layouts..."
+normalize_json "$SNAPSHOT" > /tmp/snap_normalized.json
+normalize_json /tmp/current_layout.json > /tmp/curr_normalized.json
+
+if ! diff -q /tmp/snap_normalized.json /tmp/curr_normalized.json > /dev/null 2>&1; then
+    echo ""
+    echo "=========================================="
+    echo "ERROR: Storage layout has changed!"
+    echo "=========================================="
+    echo ""
+    echo "Differences found:"
+    diff /tmp/snap_normalized.json /tmp/curr_normalized.json || true
+    echo ""
+    echo "If this change is intentional, update the snapshot:"
+    echo "  forge inspect $CONTRACT storage --json > $SNAPSHOT"
+    echo ""
+    echo "WARNING: Unintended storage layout changes can corrupt state during upgrades!"
+    exit 1
+fi
+
+echo "Storage layout OK - no changes detected"
--- a/contracts/storage-snapshots/DataHavenServiceManager.storage.json
+++ b/contracts/storage-snapshots/DataHavenServiceManager.storage.json
@ -0,0 +1,143 @@
+{
+  "storage": [
+    {
+      "astId": 138,
+      "contract": "src/DataHavenServiceManager.sol:DataHavenServiceManager",
+      "label": "_initialized",
+      "offset": 0,
+      "slot": "0",
+      "type": "t_uint8"
+    },
+    {
+      "astId": 141,
+      "contract": "src/DataHavenServiceManager.sol:DataHavenServiceManager",
+      "label": "_initializing",
+      "offset": 1,
+      "slot": "0",
+      "type": "t_bool"
+    },
+    {
+      "astId": 671,
+      "contract": "src/DataHavenServiceManager.sol:DataHavenServiceManager",
+      "label": "__gap",
+      "offset": 0,
+      "slot": "1",
+      "type": "t_array(t_uint256)50_storage"
+    },
+    {
+      "astId": 10,
+      "contract": "src/DataHavenServiceManager.sol:DataHavenServiceManager",
+      "label": "_owner",
+      "offset": 0,
+      "slot": "51",
+      "type": "t_address"
+    },
+    {
+      "astId": 130,
+      "contract": "src/DataHavenServiceManager.sol:DataHavenServiceManager",
+      "label": "__gap",
+      "offset": 0,
+      "slot": "52",
+      "type": "t_array(t_uint256)49_storage"
+    },
+    {
+      "astId": 23771,
+      "contract": "src/DataHavenServiceManager.sol:DataHavenServiceManager",
+      "label": "rewardsInitiator",
+      "offset": 0,
+      "slot": "101",
+      "type": "t_address"
+    },
+    {
+      "astId": 23776,
+      "contract": "src/DataHavenServiceManager.sol:DataHavenServiceManager",
+      "label": "validatorsAllowlist",
+      "offset": 0,
+      "slot": "102",
+      "type": "t_mapping(t_address,t_bool)"
+    },
+    {
+      "astId": 23780,
+      "contract": "src/DataHavenServiceManager.sol:DataHavenServiceManager",
+      "label": "_snowbridgeGateway",
+      "offset": 0,
+      "slot": "103",
+      "type": "t_contract(IGatewayV2)23481"
+    },
+    {
+      "astId": 23785,
+      "contract": "src/DataHavenServiceManager.sol:DataHavenServiceManager",
+      "label": "validatorEthAddressToSolochainAddress",
+      "offset": 0,
+      "slot": "104",
+      "type": "t_mapping(t_address,t_address)"
+    },
+    {
+      "astId": 23790,
+      "contract": "src/DataHavenServiceManager.sol:DataHavenServiceManager",
+      "label": "__GAP",
+      "offset": 0,
+      "slot": "105",
+      "type": "t_array(t_uint256)46_storage"
+    }
+  ],
+  "types": {
+    "t_address": {
+      "encoding": "inplace",
+      "label": "address",
+      "numberOfBytes": "20"
+    },
+    "t_array(t_uint256)46_storage": {
+      "encoding": "inplace",
+      "label": "uint256[46]",
+      "numberOfBytes": "1472",
+      "base": "t_uint256"
+    },
+    "t_array(t_uint256)49_storage": {
+      "encoding": "inplace",
+      "label": "uint256[49]",
+      "numberOfBytes": "1568",
+      "base": "t_uint256"
+    },
+    "t_array(t_uint256)50_storage": {
+      "encoding": "inplace",
+      "label": "uint256[50]",
+      "numberOfBytes": "1600",
+      "base": "t_uint256"
+    },
+    "t_bool": {
+      "encoding": "inplace",
+      "label": "bool",
+      "numberOfBytes": "1"
+    },
+    "t_contract(IGatewayV2)23481": {
+      "encoding": "inplace",
+      "label": "contract IGatewayV2",
+      "numberOfBytes": "20"
+    },
+    "t_mapping(t_address,t_address)": {
+      "encoding": "mapping",
+      "key": "t_address",
+      "label": "mapping(address => address)",
+      "numberOfBytes": "32",
+      "value": "t_address"
+    },
+    "t_mapping(t_address,t_bool)": {
+      "encoding": "mapping",
+      "key": "t_address",
+      "label": "mapping(address => bool)",
+      "numberOfBytes": "32",
+      "value": "t_bool"
+    },
+    "t_uint256": {
+      "encoding": "inplace",
+      "label": "uint256",
+      "numberOfBytes": "32"
+    },
+    "t_uint8": {
+      "encoding": "inplace",
+      "label": "uint8",
+      "numberOfBytes": "1"
+    }
+  }
+}
--- a/contracts/storage-snapshots/README.md
+++ b/contracts/storage-snapshots/README.md
@ -0,0 +1,75 @@
+# Storage Layout Snapshots
+
+This directory contains storage layout snapshots for upgradeable contracts. These snapshots are used to detect unintended storage layout changes that could corrupt state during proxy upgrades.
+
+## How It Works
+
+1. **Snapshot Comparison**: CI compares the current storage layout against committed snapshots
+2. **Upgrade Simulation**: Foundry tests verify state preservation across upgrades
+
+## Updating Snapshots
+
+When you intentionally modify the storage layout of a contract (e.g., adding new state variables), you must update the snapshot:
+
+```bash
+cd contracts
+forge inspect DataHavenServiceManager storage --json > storage-snapshots/DataHavenServiceManager.storage.json
+```
+
+## Important Guidelines
+
+- **Never reorder existing variables** - This corrupts existing state
+- **Never change types of existing variables** - This corrupts existing state
+- **Always add new variables before the `__GAP`** - This preserves upgrade safety
+- **Reduce gap size when adding variables** - Keep total slot count constant
+- **Review snapshot diffs carefully** - Ensure changes are intentional
+
+## Current Contracts
+
+| Contract | Gap Size | Gap Slot |
+|----------|----------|----------|
+| DataHavenServiceManager | 46 | 105 |
+
+## Verification Commands
+
+```bash
+# Check storage layout (CI script)
+./scripts/check-storage-layout.sh
+
+# Negative check (proves detector fails on broken layout)
+./scripts/check-storage-layout-negative.sh
+
+# Run upgrade simulation tests
+forge test --match-contract StorageLayoutTest -vvv
+
+# View human-readable layout
+forge inspect DataHavenServiceManager storage --pretty
+```
+
+## How Normalization Works
+
+The snapshot comparison normalizes both files to avoid false positives:
+
+- **Removes `astId`**: Changes with each compiler run
+- **Removes `contract`**: Contains full file path
+- **Removes `.types` section**: Contains unstable AST IDs that cause false diffs
+- **Normalizes type IDs**: Strips unstable numeric suffixes from `type` (e.g., `t_contract(IGatewayV2)12345`)
+- **Sorts by slot**: Ensures deterministic comparison
+
+This approach detects:
+- Variable reordering or slot changes
+- Top-level type changes (primitives, mappings, arrays)
+- Gap size modifications
+
+## Note on Struct Storage
+
+If you add struct-typed storage variables in the future, be aware that **internal struct field changes may not be detected** by the snapshot diff. This is because:
+
+1. The `.types` section (which contains struct field definitions) is dropped to avoid unstable AST IDs
+2. The storage slot assignment for a struct variable doesn't change when its internal fields change
+
+**However, this does not break upgrades** in the traditional sense. Struct field reordering or type changes within a struct would cause data misinterpretation (reading field A as field B), but the slot-level layout remains stable.
+
+**Mitigation**: If adding struct storage, ensure the upgrade simulation tests (`StorageLayoutTest`) explicitly verify struct field values survive upgrades.
+
+**Current status**: DataHavenServiceManager has no struct-typed storage variables, so this limitation does not apply.
--- a/contracts/test/storage/StorageLayout.t.sol
+++ b/contracts/test/storage/StorageLayout.t.sol
@ -0,0 +1,175 @@
+// SPDX-License-Identifier: GPL-3.0
+pragma solidity ^0.8.27;
+
+import {Test} from "forge-std/Test.sol";
+import {
+    ITransparentUpgradeableProxy
+} from "@openzeppelin/contracts/proxy/transparent/TransparentUpgradeableProxy.sol";
+
+import {AVSDeployer} from "../utils/AVSDeployer.sol";
+import {DataHavenServiceManager} from "../../src/DataHavenServiceManager.sol";
+
+/// @title Storage Layout Tests for DataHavenServiceManager
+/// @notice Verifies that proxy upgrades preserve state correctly
+contract StorageLayoutTest is AVSDeployer {
+    function setUp() public {
+        _deployMockEigenLayerAndAVS();
+    }
+
+    /// @notice Proves state is preserved across proxy upgrade
+    function test_upgradePreservesState() public {
+        // 1. Populate state
+        address testValidator = address(0x1234);
+        address newRewardsInitiator = address(0x9999);
+
+        vm.startPrank(avsOwner);
+        serviceManager.addValidatorToAllowlist(testValidator);
+        serviceManager.setRewardsInitiator(newRewardsInitiator);
+        vm.stopPrank();
+
+        // 2. Record state before upgrade
+        bool allowlistBefore = serviceManager.validatorsAllowlist(testValidator);
+        address rewardsInitiatorBefore = serviceManager.rewardsInitiator();
+        address ownerBefore = serviceManager.owner();
+        address gatewayBefore = serviceManager.snowbridgeGateway();
+
+        // 3. Deploy new implementation
+        DataHavenServiceManager newImpl =
+            new DataHavenServiceManager(rewardsCoordinator, allocationManager);
+
+        // 4. Upgrade proxy
+        vm.prank(proxyAdminOwner);
+        proxyAdmin.upgrade(ITransparentUpgradeableProxy(address(serviceManager)), address(newImpl));
+
+        // 5. Verify state preserved
+        assertEq(
+            serviceManager.validatorsAllowlist(testValidator),
+            allowlistBefore,
+            "validatorsAllowlist should be preserved"
+        );
+        assertEq(
+            serviceManager.rewardsInitiator(),
+            rewardsInitiatorBefore,
+            "rewardsInitiator should be preserved"
+        );
+        assertEq(serviceManager.owner(), ownerBefore, "owner should be preserved");
+        assertEq(
+            serviceManager.snowbridgeGateway(),
+            gatewayBefore,
+            "snowbridgeGateway should be preserved"
+        );
+    }
+
+    /// @notice Verifies validatorEthAddressToSolochainAddress mapping is preserved
+    function test_upgradePreservesValidatorMappings() public {
+        address testValidator = address(0xABCD);
+        address testSolochainAddress = address(0xDEF0);
+
+        // Add validator to allowlist first
+        vm.prank(avsOwner);
+        serviceManager.addValidatorToAllowlist(testValidator);
+
+        // Register operator via allocationManager to set the solochain address mapping
+        uint32[] memory operatorSetIds = new uint32[](1);
+        operatorSetIds[0] = 0; // VALIDATORS_SET_ID
+
+        vm.prank(address(allocationManager));
+        serviceManager.registerOperator(
+            testValidator,
+            address(serviceManager),
+            operatorSetIds,
+            abi.encodePacked(testSolochainAddress)
+        );
+
+        // Record state before upgrade
+        bool inAllowlistBefore = serviceManager.validatorsAllowlist(testValidator);
+        address solochainAddressBefore =
+            serviceManager.validatorEthAddressToSolochainAddress(testValidator);
+
+        // Verify the mapping was set correctly before upgrade
+        assertEq(solochainAddressBefore, testSolochainAddress, "Solochain address should be set");
+
+        // Deploy new implementation and upgrade
+        DataHavenServiceManager newImpl =
+            new DataHavenServiceManager(rewardsCoordinator, allocationManager);
+
+        vm.prank(proxyAdminOwner);
+        proxyAdmin.upgrade(ITransparentUpgradeableProxy(address(serviceManager)), address(newImpl));
+
+        // Verify both mappings preserved after upgrade
+        assertEq(
+            serviceManager.validatorsAllowlist(testValidator),
+            inAllowlistBefore,
+            "validatorsAllowlist mapping should be preserved after upgrade"
+        );
+        assertEq(
+            serviceManager.validatorEthAddressToSolochainAddress(testValidator),
+            solochainAddressBefore,
+            "validatorEthAddressToSolochainAddress mapping should be preserved after upgrade"
+        );
+        assertEq(
+            serviceManager.validatorEthAddressToSolochainAddress(testValidator),
+            testSolochainAddress,
+            "validatorEthAddressToSolochainAddress should have correct value after upgrade"
+        );
+    }
+
+    /// @notice Verifies multiple validators in allowlist are preserved
+    function test_upgradePreservesMultipleValidators() public {
+        address[] memory validators = new address[](3);
+        validators[0] = address(0x1111);
+        validators[1] = address(0x2222);
+        validators[2] = address(0x3333);
+
+        // Add multiple validators
+        vm.startPrank(avsOwner);
+        for (uint256 i = 0; i < validators.length; i++) {
+            serviceManager.addValidatorToAllowlist(validators[i]);
+        }
+        vm.stopPrank();
+
+        // Deploy new implementation and upgrade
+        DataHavenServiceManager newImpl =
+            new DataHavenServiceManager(rewardsCoordinator, allocationManager);
+
+        vm.prank(proxyAdminOwner);
+        proxyAdmin.upgrade(ITransparentUpgradeableProxy(address(serviceManager)), address(newImpl));
+
+        // Verify all validators still in allowlist
+        for (uint256 i = 0; i < validators.length; i++) {
+            assertTrue(
+                serviceManager.validatorsAllowlist(validators[i]),
+                "All validators should remain in allowlist after upgrade"
+            );
+        }
+    }
+
+    /// @notice Verifies that upgrade doesn't affect functionality
+    function test_functionalityAfterUpgrade() public {
+        // Deploy new implementation and upgrade
+        DataHavenServiceManager newImpl =
+            new DataHavenServiceManager(rewardsCoordinator, allocationManager);
+
+        vm.prank(proxyAdminOwner);
+        proxyAdmin.upgrade(ITransparentUpgradeableProxy(address(serviceManager)), address(newImpl));
+
+        // Verify functionality still works
+        address newValidator = address(0xBEEF);
+
+        vm.prank(avsOwner);
+        serviceManager.addValidatorToAllowlist(newValidator);
+
+        assertTrue(
+            serviceManager.validatorsAllowlist(newValidator),
+            "Should be able to add validators after upgrade"
+        );
+
+        vm.prank(avsOwner);
+        serviceManager.removeValidatorFromAllowlist(newValidator);
+
+        assertFalse(
+            serviceManager.validatorsAllowlist(newValidator),
+            "Should be able to remove validators after upgrade"
+        );
+    }
+}