From e4e4a186ac3f4d895fd79293c1d5f8e2ebb492bb Mon Sep 17 00:00:00 2001 From: kyzooghost <73516204+kyzooghost@users.noreply.github.com> Date: Thu, 27 Feb 2025 17:38:34 +1100 Subject: [PATCH] [Fix] OZ-Upgrade-Plugin + Solhint warnings (#661) * fix tokenbridge init ordering given oz plugin warnings * adjust solhint linting * fix scripts/tokenBridge/gasEstimation/gasEstimation.ts * added /// @custom:oz-upgrades-unsafe-allow incorrect-initializer-order * fix for L2MessageService * remove warning silencer for demo purpose * Revert "remove warning silencer for demo purpose" This reverts commit 4eccc3d0fc32501ab0e99f737e7b25daa8214601. * add incorrect-initializer-flag to upgrades constructor function --- contracts/.solhintignore | 5 +-- .../gasEstimation/gasEstimation.ts | 2 +- .../src/_testing/mocks/tokens/TestERC20.sol | 4 +- contracts/src/bridging/token/TokenBridge.sol | 4 +- .../src/messaging/l2/L2MessageService.sol | 2 +- contracts/test/hardhat/L1MessageService.ts | 37 +++++++++---------- contracts/test/hardhat/rollup/LineaRollup.ts | 17 +++++---- 7 files changed, 35 insertions(+), 36 deletions(-) diff --git a/contracts/.solhintignore b/contracts/.solhintignore index e60ed8fa..99644921 100644 --- a/contracts/.solhintignore +++ b/contracts/.solhintignore @@ -1,6 +1,5 @@ node_modules lib/forge-std -contracts/test-contracts test/foundry -/contracts/proxies -/contracts/tokenBridge/mocks \ No newline at end of file +src/_testing +src/proxies \ No newline at end of file diff --git a/contracts/scripts/tokenBridge/gasEstimation/gasEstimation.ts b/contracts/scripts/tokenBridge/gasEstimation/gasEstimation.ts index 1fdc42d3..0900d581 100644 --- a/contracts/scripts/tokenBridge/gasEstimation/gasEstimation.ts +++ b/contracts/scripts/tokenBridge/gasEstimation/gasEstimation.ts @@ -1,5 +1,5 @@ import { ethers, upgrades } from "hardhat"; -import { getPermitData } from "../../../test/tokenBridge/utils/permitHelper"; +import { getPermitData } from "../../../test/hardhat/bridging/token/utils/permitHelper"; import { BridgedToken, MockTokenBridge } from "../../../typechain-types"; import { deployBridgedTokenBeacon } from "../test/deployBridgedTokenBeacon"; import { deployTokens } from "../test/deployTokens"; diff --git a/contracts/src/_testing/mocks/tokens/TestERC20.sol b/contracts/src/_testing/mocks/tokens/TestERC20.sol index 8d99a333..74c7315f 100644 --- a/contracts/src/_testing/mocks/tokens/TestERC20.sol +++ b/contracts/src/_testing/mocks/tokens/TestERC20.sol @@ -2,8 +2,8 @@ pragma solidity 0.8.19; -import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; -import "@openzeppelin/contracts/access/Ownable.sol"; +import { ERC20 } from "@openzeppelin/contracts/token/ERC20/ERC20.sol"; +import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; /** * @title TestERC20 diff --git a/contracts/src/bridging/token/TokenBridge.sol b/contracts/src/bridging/token/TokenBridge.sol index b6010cd8..8720cb24 100644 --- a/contracts/src/bridging/token/TokenBridge.sol +++ b/contracts/src/bridging/token/TokenBridge.sol @@ -153,9 +153,9 @@ contract TokenBridge is nonZeroChainId(_initializationData.targetChainId) initializer { - __PauseManager_init(_initializationData.pauseTypeRoles, _initializationData.unpauseTypeRoles); - __MessageServiceBase_init(_initializationData.messageService); __ReentrancyGuard_init(); + __MessageServiceBase_init(_initializationData.messageService); + __PauseManager_init(_initializationData.pauseTypeRoles, _initializationData.unpauseTypeRoles); if (_initializationData.defaultAdmin == address(0)) { revert ZeroAddressNotAllowed(); diff --git a/contracts/src/messaging/l2/L2MessageService.sol b/contracts/src/messaging/l2/L2MessageService.sol index 8a2c99bc..9d11e347 100644 --- a/contracts/src/messaging/l2/L2MessageService.sol +++ b/contracts/src/messaging/l2/L2MessageService.sol @@ -46,8 +46,8 @@ contract L2MessageService is AccessControlUpgradeable, L2MessageServiceV1, L2Mes __AccessControl_init(); __RateLimiter_init(_rateLimitPeriod, _rateLimitAmount); - __ReentrancyGuard_init(); __PauseManager_init(_pauseTypeRoles, _unpauseTypeRoles); + __ReentrancyGuard_init(); if (_defaultAdmin == address(0)) { revert ZeroAddressNotAllowed(); diff --git a/contracts/test/hardhat/L1MessageService.ts b/contracts/test/hardhat/L1MessageService.ts index 410cbb1a..eb4a30bd 100644 --- a/contracts/test/hardhat/L1MessageService.ts +++ b/contracts/test/hardhat/L1MessageService.ts @@ -61,21 +61,19 @@ describe("L1MessageService", () => { let l2Sender: SignerWithAddress; async function deployTestL1MessageServiceFixture(): Promise { - return deployUpgradableFromFactory("TestL1MessageService", [ - ONE_DAY_IN_SECONDS, - INITIAL_WITHDRAW_LIMIT, - pauseTypeRoles, - unpauseTypeRoles, - ]) as unknown as Promise; + return deployUpgradableFromFactory( + "TestL1MessageService", + [ONE_DAY_IN_SECONDS, INITIAL_WITHDRAW_LIMIT, pauseTypeRoles, unpauseTypeRoles], + { unsafeAllow: ["incorrect-initializer-order"] }, + ) as unknown as Promise; } async function deployL1MessageServiceMerkleFixture(): Promise { - return deployUpgradableFromFactory("TestL1MessageServiceMerkleProof", [ - ONE_DAY_IN_SECONDS, - INITIAL_WITHDRAW_LIMIT, - pauseTypeRoles, - unpauseTypeRoles, - ]) as unknown as Promise; + return deployUpgradableFromFactory( + "TestL1MessageServiceMerkleProof", + [ONE_DAY_IN_SECONDS, INITIAL_WITHDRAW_LIMIT, pauseTypeRoles, unpauseTypeRoles], + { unsafeAllow: ["incorrect-initializer-order"] }, + ) as unknown as Promise; } async function deployL1TestRevertFixture(): Promise { @@ -146,7 +144,9 @@ describe("L1MessageService", () => { it("Should fail to deploy missing amount", async () => { await expectRevertWithCustomError( l1MessageService, - deployUpgradableFromFactory("TestL1MessageService", [ONE_DAY_IN_SECONDS, 0, pauseTypeRoles, unpauseTypeRoles]), + deployUpgradableFromFactory("TestL1MessageService", [ONE_DAY_IN_SECONDS, 0, pauseTypeRoles, unpauseTypeRoles], { + unsafeAllow: ["incorrect-initializer-order"], + }), "LimitIsZero", ); }); @@ -154,12 +154,11 @@ describe("L1MessageService", () => { it("Should fail to deploy missing limit period", async () => { await expectRevertWithCustomError( l1MessageService, - deployUpgradableFromFactory("TestL1MessageService", [ - 0, - INITIAL_WITHDRAW_LIMIT, - pauseTypeRoles, - unpauseTypeRoles, - ]), + deployUpgradableFromFactory( + "TestL1MessageService", + [0, INITIAL_WITHDRAW_LIMIT, pauseTypeRoles, unpauseTypeRoles], + { unsafeAllow: ["incorrect-initializer-order"] }, + ), "PeriodIsZero", ); }); diff --git a/contracts/test/hardhat/rollup/LineaRollup.ts b/contracts/test/hardhat/rollup/LineaRollup.ts index c092d872..e6dd7b16 100644 --- a/contracts/test/hardhat/rollup/LineaRollup.ts +++ b/contracts/test/hardhat/rollup/LineaRollup.ts @@ -136,7 +136,7 @@ describe("Linea Rollup contract", () => { const lineaRollup = (await deployUpgradableFromFactory("TestLineaRollup", [initializationData], { initializer: LINEA_ROLLUP_INITIALIZE_SIGNATURE, - unsafeAllow: ["constructor"], + unsafeAllow: ["constructor", "incorrect-initializer-order"], })) as unknown as TestLineaRollup; return lineaRollup; @@ -198,7 +198,7 @@ describe("Linea Rollup contract", () => { const deployCall = deployUpgradableFromFactory("src/rollup/LineaRollup.sol:LineaRollup", [initializationData], { initializer: LINEA_ROLLUP_INITIALIZE_SIGNATURE, - unsafeAllow: ["constructor"], + unsafeAllow: ["constructor", "incorrect-initializer-order"], }); await expectRevertWithCustomError(lineaRollup, deployCall, "ZeroAddressNotAllowed"); @@ -221,7 +221,7 @@ describe("Linea Rollup contract", () => { const deployCall = deployUpgradableFromFactory("TestLineaRollup", [initializationData], { initializer: LINEA_ROLLUP_INITIALIZE_SIGNATURE, - unsafeAllow: ["constructor"], + unsafeAllow: ["constructor", "incorrect-initializer-order"], }); await expectRevertWithCustomError(lineaRollup, deployCall, "ZeroAddressNotAllowed"); @@ -244,7 +244,7 @@ describe("Linea Rollup contract", () => { const deployCall = deployUpgradableFromFactory("TestLineaRollup", [initializationData], { initializer: LINEA_ROLLUP_INITIALIZE_SIGNATURE, - unsafeAllow: ["constructor"], + unsafeAllow: ["constructor", "incorrect-initializer-order"], }); await expectRevertWithCustomError(lineaRollup, deployCall, "ZeroAddressNotAllowed"); @@ -267,7 +267,7 @@ describe("Linea Rollup contract", () => { const deployCall = deployUpgradableFromFactory("TestLineaRollup", [initializationData], { initializer: LINEA_ROLLUP_INITIALIZE_SIGNATURE, - unsafeAllow: ["constructor"], + unsafeAllow: ["constructor", "incorrect-initializer-order"], }); await expectRevertWithCustomError(lineaRollup, deployCall, "ZeroAddressNotAllowed"); @@ -313,7 +313,7 @@ describe("Linea Rollup contract", () => { [initializationData], { initializer: LINEA_ROLLUP_INITIALIZE_SIGNATURE, - unsafeAllow: ["constructor"], + unsafeAllow: ["constructor", "incorrect-initializer-order"], }, ); @@ -340,7 +340,7 @@ describe("Linea Rollup contract", () => { [initializationData], { initializer: LINEA_ROLLUP_INITIALIZE_SIGNATURE, - unsafeAllow: ["constructor"], + unsafeAllow: ["constructor", "incorrect-initializer-order"], }, ); @@ -1560,7 +1560,7 @@ describe("Linea Rollup contract", () => { const betaV1LineaRollup = (await deployUpgradableFromFactory("TestLineaRollup", [initializationData], { initializer: LINEA_ROLLUP_INITIALIZE_SIGNATURE, - unsafeAllow: ["constructor"], + unsafeAllow: ["constructor", "incorrect-initializer-order"], })) as unknown as TestLineaRollup; await betaV1LineaRollup.setupParentShnarf(betaV1FinalizationData.parentAggregationFinalShnarf); @@ -2268,6 +2268,7 @@ describe("Linea Rollup contract", () => { ); const newLineaRollup = await upgrades.upgradeProxy(lineaRollup, newLineaRollupFactory, { unsafeAllowRenames: true, + unsafeAllow: ["incorrect-initializer-order"], }); const upgradedContract = await newLineaRollup.waitForDeployment();