Added storage layout safety check on deployment and GHA

.github/workflows/test.yml

@@ -42,11 +42,16 @@ jobs:
           version: nightly
 
       # first, build contracts excluding the tests and scripts. Check contract sizes in this step.
-      # then, build contracts including the tests and scripts. Don't check contract sizes.
-      - name: Run Forge build
+      - name: Run Contract Size check
         run: |
           forge --version
           forge build --force --sizes --skip test --skip script
+
+      # This step requires full build to be run first
+      - name: Upgrade Safety test
+        run: |
+          forge clean && forge build
+          npx @openzeppelin/upgrades-core validate out/build-info
 
       - name: Run Forge tests
         run: |

.gitignore

@@ -33,5 +33,3 @@ typechain
 # converage
 lcov.info
 
-# forge lib folder
-/lib/

remappings.txt

@@ -1,5 +1,6 @@
 @ethereum-waffle/=node_modules/@ethereum-waffle/
 @openzeppelin/=node_modules/@openzeppelin/
+@openzeppelin-foundry-upgrades/=lib/openzeppelin-foundry-upgrades
 forge-std/=node_modules/forge-std/src/
 ds-test/=node_modules/ds-test/src/
 base64-sol/=node_modules/base64-sol/

script/foundry/deployment/Main.s.sol

@@ -47,13 +47,14 @@ import { IHookModule } from "contracts/interfaces/modules/base/IHookModule.sol";
 import { StringUtil } from "../../../script/foundry/utils/StringUtil.sol";
 import { BroadcastManager } from "../../../script/foundry/utils/BroadcastManager.s.sol";
 import { JsonDeploymentHandler } from "../../../script/foundry/utils/JsonDeploymentHandler.s.sol";
+import { StorageLayoutChecker } from "../../../script/foundry/utils/upgrades/StorageLayoutCheck.s.sol";
 
 // test
 import { MockERC20 } from "test/foundry/mocks/token/MockERC20.sol";
 import { MockERC721 } from "test/foundry/mocks/token/MockERC721.sol";
 import { MockTokenGatedHook } from "test/foundry/mocks/MockTokenGatedHook.sol";
 
-contract Main is Script, BroadcastManager, JsonDeploymentHandler {
+contract Main is Script, BroadcastManager, JsonDeploymentHandler, StorageLayoutChecker {
     using StringUtil for uint256;
     using stdJson for string;
 
@@ -112,7 +113,9 @@ contract Main is Script, BroadcastManager, JsonDeploymentHandler {
     /// @dev To use, run the following command (e.g. for Sepolia):
     /// forge script script/foundry/deployment/Main.s.sol:Main --rpc-url $RPC_URL --broadcast --verify -vvvv
 
-    function run() public {
+    function run() virtual override public {
+        // This will run OZ storage layout check for all contracts. Requires --ffi flag.
+        super.run(); 
         _beginBroadcast(); // BroadcastManager.s.sol
 
         bool configByMultisig = vm.envBool("DEPLOYMENT_CONFIG_BY_MULTISIG");

script/foundry/utils/StringUtil.sol

@@ -22,4 +22,8 @@ library StringUtil {
         }
         return string(buffer);
     }
+
+    function concat(string memory a, string memory b) internal pure returns (string memory) {
+        return string(abi.encodePacked(a, b));
+    }
 }

script/foundry/utils/upgrades/StorageLayoutCheck.s.sol

@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: MIT
+pragma solidity ^0.8.23;
+
+import { Script } from "forge-std/Script.sol";
+import { Vm } from "forge-std/Vm.sol";
+import { console } from "forge-std/console.sol";
+import { strings } from "solidity-stringutils/src/strings.sol";
+
+import { Utils } from "@openzeppelin-foundry-upgrades/src/internal/Utils.sol";
+import { StringUtil } from "../StringUtil.sol";
+
+/// @title StorageLayoutChecker
+/// @notice Helper contract to check if the storage layout of the contracts is compatible with upgrades
+/// @dev Picked relevant functionality from OpenZeppelin's `@openzeppelin/upgrades-core` package.
+/// NOTE: Replace this contract for their library, if these issues are resolved
+
+/// MUST be called in scripts that deploy or upgrade contracts
+/// MUST be called with `--ffi` flag
+contract StorageLayoutChecker is Script {
+
+    using strings for *;
+
+    function run() virtual public {
+        _validate();
+    }
+
+    /// @notice Runs the storage layout check
+    /// @dev For simplicity and efficiency, we check all the upgradeablecontracts in the project
+    /// instead of going 1 by 1 using ffi.
+    function _validate() private {
+        string[] memory inputs = _buildValidateCommand();
+        Vm.FfiResult memory result = Utils.runAsBashCommand(inputs);
+        string memory stdout = string(result.stdout);
+
+        // CLI validate command uses exit code to indicate if the validation passed or failed.
+        // As an extra precaution, we also check stdout for "SUCCESS" to ensure it actually ran.
+        if (result.exitCode == 0 && stdout.toSlice().contains("SUCCESS".toSlice())) {
+            return;
+        } else if (result.stderr.length > 0) {
+            // Validations failed to run
+            revert(StringUtil.concat("Failed to run upgrade safety validation: ", string(result.stderr)));
+        } else {
+            // Validations ran but some contracts were not upgrade safe
+            revert(StringUtil.concat("Upgrade safety validation failed:\n", stdout));
+        }
+    }
+
+    function _buildValidateCommand() private view returns (string[] memory) {
+        string memory outDir = "out";
+
+        string[] memory inputBuilder = new string[](255);
+
+        uint8 i = 0;
+        // npx @openzeppelin/upgrades-core validate <build-info-dir> --requireReference
+        inputBuilder[i++] = "npx";
+        inputBuilder[i++] = string.concat("@openzeppelin/upgrades-core");
+        inputBuilder[i++] = "validate";
+        inputBuilder[i++] = string.concat(outDir, "/build-info");
+
+        // Create a copy of inputs but with the correct length
+        string[] memory inputs = new string[](i);
+        for (uint8 j = 0; j < i; j++) {
+            inputs[j] = inputBuilder[j];
+        }
+
+        return inputs;
+    }
+}

test/foundry/utils/DeployHelper.t.sol

@@ -44,7 +44,6 @@ import { MockERC20 } from "../mocks/token/MockERC20.sol";
 import { MockERC721 } from "../mocks/token/MockERC721.sol";
 import { TestProxyHelper } from "./TestProxyHelper.sol";
 
-
 contract DeployHelper {
     // TODO: three options, auto/mock/real in deploy condition, so that we don't need to manually
     //       call getXXX to get mock contract (if there's no real contract deployed).