From da2847bbbfb4c27688c2eaa9525ad4a221ded259 Mon Sep 17 00:00:00 2001 From: Ahmad Kaouk <56095276+ahmadkaouk@users.noreply.github.com> Date: Thu, 5 Feb 2026 12:08:35 +0100 Subject: [PATCH] =?UTF-8?q?test:=20=E2=9C=A8=20Add=20storage=20layout=20ch?= =?UTF-8?q?ecks=20for=20upgradeable=20contracts=20(#420)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 --- .github/workflows/CI.yml | 2 + .github/workflows/task-storage-layout.yml | 51 +++++ .../DataHavenServiceManagerBadLayout.sol | 31 ++++ .../scripts/check-storage-layout-negative.sh | 32 ++++ contracts/scripts/check-storage-layout.sh | 62 +++++++ .../DataHavenServiceManager.storage.json | 143 ++++++++++++++ contracts/storage-snapshots/README.md | 75 ++++++++ contracts/test/storage/StorageLayout.t.sol | 175 ++++++++++++++++++ 8 files changed, 571 insertions(+) create mode 100644 .github/workflows/task-storage-layout.yml create mode 100644 contracts/script/fixtures/DataHavenServiceManagerBadLayout.sol create mode 100755 contracts/scripts/check-storage-layout-negative.sh create mode 100755 contracts/scripts/check-storage-layout.sh create mode 100644 contracts/storage-snapshots/DataHavenServiceManager.storage.json create mode 100644 contracts/storage-snapshots/README.md create mode 100644 contracts/test/storage/StorageLayout.t.sol diff --git a/.github/workflows/CI.yml b/.github/workflows/CI.yml index d8d2ba06..63221122 100644 --- 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 diff --git a/.github/workflows/task-storage-layout.yml b/.github/workflows/task-storage-layout.yml new file mode 100644 index 00000000..2a1a78a5 --- /dev/null +++ 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 diff --git a/contracts/script/fixtures/DataHavenServiceManagerBadLayout.sol b/contracts/script/fixtures/DataHavenServiceManagerBadLayout.sol new file mode 100644 index 00000000..919cc346 --- /dev/null +++ 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(); + } +} + diff --git a/contracts/scripts/check-storage-layout-negative.sh b/contracts/scripts/check-storage-layout-negative.sh new file mode 100755 index 00000000..18d842cf --- /dev/null +++ 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." + diff --git a/contracts/scripts/check-storage-layout.sh b/contracts/scripts/check-storage-layout.sh new file mode 100755 index 00000000..8e2aeb09 --- /dev/null +++ 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" diff --git a/contracts/storage-snapshots/DataHavenServiceManager.storage.json b/contracts/storage-snapshots/DataHavenServiceManager.storage.json new file mode 100644 index 00000000..37f6fa6d --- /dev/null +++ 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" + } + } +} diff --git a/contracts/storage-snapshots/README.md b/contracts/storage-snapshots/README.md new file mode 100644 index 00000000..bf708a11 --- /dev/null +++ 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. diff --git a/contracts/test/storage/StorageLayout.t.sol b/contracts/test/storage/StorageLayout.t.sol new file mode 100644 index 00000000..de81e0b9 --- /dev/null +++ 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" + ); + } +}