diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index a8a7c38e..ec4fc74a 100644 --- a/.github/workflows/test.yml +++ b/.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: | diff --git a/.gitignore b/.gitignore index 01c3f82e..c2c31fad 100644 --- a/.gitignore +++ b/.gitignore @@ -33,5 +33,3 @@ typechain # converage lcov.info -# forge lib folder -/lib/ diff --git a/lib/openzeppelin-foundry-upgrades b/lib/openzeppelin-foundry-upgrades new file mode 160000 index 00000000..c50a7968 --- /dev/null +++ b/lib/openzeppelin-foundry-upgrades @@ -0,0 +1 @@ +Subproject commit c50a7968d369f852607cb72e653d1ed699d823c5 diff --git a/remappings.txt b/remappings.txt index ada4aef1..f4f34a83 100644 --- a/remappings.txt +++ b/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/ diff --git a/script/foundry/deployment/Main.s.sol b/script/foundry/deployment/Main.s.sol index 8eb180cd..6c83556f 100644 --- a/script/foundry/deployment/Main.s.sol +++ b/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"); diff --git a/script/foundry/utils/StringUtil.sol b/script/foundry/utils/StringUtil.sol index 8da5e011..7c1ce25b 100644 --- a/script/foundry/utils/StringUtil.sol +++ b/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)); + } } diff --git a/script/foundry/utils/upgrades/StorageLayoutCheck.s.sol b/script/foundry/utils/upgrades/StorageLayoutCheck.s.sol new file mode 100644 index 00000000..4b3df4ff --- /dev/null +++ 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 --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; + } +} diff --git a/test/foundry/utils/DeployHelper.t.sol b/test/foundry/utils/DeployHelper.t.sol index e7e7219c..b775181f 100644 --- a/test/foundry/utils/DeployHelper.t.sol +++ b/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).