feat(contracts): add more require revert error message (#12702)

* ct/scripts/ChainAssertions: add require revert message Signed-off-by: jsvisa <[email protected]> * ct/scripts/deploy/Deploy: add require revert message Signed-off-by: jsvisa <[email protected]> * ct/scripts/deploy/DeployOwnership: add require revert message Signed-off-by: jsvisa <[email protected]> * ct/scripts/deploy/DeployPeriphery: add require revert message Signed-off-by: jsvisa <[email protected]> * feat(semgrep): enable for all solidity files, except WETH9 Signed-off-by: jsvisa <[email protected]> * fix require issues Signed-off-by: jsvisa <[email protected]> * fix(semgrep): run just semgrep to fix the require rules Signed-off-by: jsvisa <[email protected]> --------- Signed-off-by: jsvisa <[email protected]>
ethereum-optimism · Jan 2, 2025 · 7562e4c · 7562e4c
1 parent d543909
commit 7562e4c
Show file tree

Hide file tree

Showing 9 changed files with 40 additions and 23 deletions.
diff --git a/.semgrep/rules/sol-rules.yaml b/.semgrep/rules/sol-rules.yaml
@@ -178,7 +178,7 @@ rules:
       - pattern-not: require($ERR, $MSG);
     paths:
       exclude:
-        - packages/contracts-bedrock/
+        - packages/contracts-bedrock/src/universal/WETH98.sol
 
   - id: sol-style-no-bare-imports
     languages: [solidity]

diff --git a/packages/contracts-bedrock/scripts/deploy/ChainAssertions.sol b/packages/contracts-bedrock/scripts/deploy/ChainAssertions.sol
@@ -44,7 +44,7 @@ library ChainAssertions {
         console.log("Running post-deploy assertions");
         IResourceMetering.ResourceConfig memory rcfg = ISystemConfig(_prox.SystemConfig).resourceConfig();
         IResourceMetering.ResourceConfig memory dflt = Constants.DEFAULT_RESOURCE_CONFIG();
-        require(keccak256(abi.encode(rcfg)) == keccak256(abi.encode(dflt)));
+        require(keccak256(abi.encode(rcfg)) == keccak256(abi.encode(dflt)), "CHECK-RCFG-10");
 
         checkSystemConfig({ _contracts: _prox, _cfg: _cfg, _isProxy: true });
         checkL1CrossDomainMessenger({ _contracts: _prox, _vm: _vm, _isProxy: true });

diff --git a/packages/contracts-bedrock/scripts/deploy/Deploy.s.sol b/packages/contracts-bedrock/scripts/deploy/Deploy.s.sol
@@ -450,7 +450,7 @@ contract Deploy is Deployer {
                 _args: DeployUtils.encodeConstructor(abi.encodeCall(IProxy.__constructor__, (_proxyOwner)))
             })
         );
-        require(EIP1967Helper.getAdmin(address(proxy)) == _proxyOwner);
+        require(EIP1967Helper.getAdmin(address(proxy)) == _proxyOwner, "Deploy: EIP1967Proxy admin not set");
         addr_ = address(proxy);
     }
 
@@ -466,7 +466,9 @@ contract Deploy is Deployer {
                 _args: DeployUtils.encodeConstructor(abi.encodeCall(IProxy.__constructor__, (proxyAdmin)))
             })
         );
-        require(EIP1967Helper.getAdmin(address(proxy)) == proxyAdmin);
+        require(
+            EIP1967Helper.getAdmin(address(proxy)) == proxyAdmin, "Deploy: DataAvailabilityChallengeProxy admin not set"
+        );
         addr_ = address(proxy);
     }
 
@@ -565,11 +567,16 @@ contract Deploy is Deployer {
         string memory version = dac.version();
         console.log("DataAvailabilityChallenge version: %s", version);
 
-        require(dac.owner() == finalSystemOwner);
-        require(dac.challengeWindow() == daChallengeWindow);
-        require(dac.resolveWindow() == daResolveWindow);
-        require(dac.bondSize() == daBondSize);
-        require(dac.resolverRefundPercentage() == daResolverRefundPercentage);
+        require(dac.owner() == finalSystemOwner, "Deploy: DataAvailabilityChallenge owner not set");
+        require(
+            dac.challengeWindow() == daChallengeWindow, "Deploy: DataAvailabilityChallenge challenge window not set"
+        );
+        require(dac.resolveWindow() == daResolveWindow, "Deploy: DataAvailabilityChallenge resolve window not set");
+        require(dac.bondSize() == daBondSize, "Deploy: DataAvailabilityChallenge bond size not set");
+        require(
+            dac.resolverRefundPercentage() == daResolverRefundPercentage,
+            "Deploy: DataAvailabilityChallenge resolver refund percentage not set"
+        );
     }
 
     ////////////////////////////////////////////////////////////////

diff --git a/packages/contracts-bedrock/scripts/deploy/DeployOPCM.s.sol b/packages/contracts-bedrock/scripts/deploy/DeployOPCM.s.sol
@@ -241,7 +241,7 @@ contract DeployOPCM is Script {
         OPContractsManager impl = OPContractsManager(address(_doo.opcm()));
         require(address(impl.superchainConfig()) == address(_doi.superchainConfig()), "OPCMI-10");
         require(address(impl.protocolVersions()) == address(_doi.protocolVersions()), "OPCMI-20");
-        require(LibString.eq(impl.l1ContractsRelease(), _doi.l1ContractsRelease()));
+        require(LibString.eq(impl.l1ContractsRelease(), _doi.l1ContractsRelease()), "OPCMI-30");
 
         OPContractsManager.Blueprints memory blueprints = impl.blueprints();
         require(blueprints.addressManager == _doi.addressManagerBlueprint(), "OPCMI-40");

diff --git a/packages/contracts-bedrock/scripts/deploy/DeployOPChain.s.sol b/packages/contracts-bedrock/scripts/deploy/DeployOPChain.s.sol
@@ -631,7 +631,7 @@ contract DeployOPChain is Script {
 
         // This slot is the custom gas token _balance and this check ensures
         // that it stays unset for forwards compatibility with custom gas token.
-        require(vm.load(address(portal), bytes32(uint256(61))) == bytes32(0));
+        require(vm.load(address(portal), bytes32(uint256(61))) == bytes32(0), "PORTAL-70");
     }
 
     function assertValidDisputeGameFactory(DeployOPChainInput _doi, DeployOPChainOutput _doo) internal {

diff --git a/packages/contracts-bedrock/scripts/deploy/DeployOwnership.s.sol b/packages/contracts-bedrock/scripts/deploy/DeployOwnership.s.sol
@@ -327,9 +327,9 @@ contract DeployOwnership is Deploy {
             })
         );
 
-        require(superchainConfig.guardian() == address(0));
+        require(superchainConfig.guardian() == address(0), "SuperchainConfig: guardian must be address(0)");
         bytes32 initialized = vm.load(address(superchainConfig), bytes32(0));
-        require(initialized != 0);
+        require(initialized != 0, "SuperchainConfig: must be initialized");
     }
 
     /// @notice Configure the Guardian Safe with the DeputyGuardianModule.

diff --git a/packages/contracts-bedrock/scripts/periphery/deploy/DeployPeriphery.s.sol b/packages/contracts-bedrock/scripts/periphery/deploy/DeployPeriphery.s.sol
@@ -86,7 +86,7 @@ contract DeployPeriphery is Script, Artifacts {
         });
 
         ProxyAdmin admin = ProxyAdmin(addr_);
-        require(admin.owner() == msg.sender);
+        require(admin.owner() == msg.sender, "DeployPeriphery: ProxyAdmin owner mismatch");
     }
 
     /// @notice Deploy FaucetProxy.
@@ -98,7 +98,10 @@ contract DeployPeriphery is Script, Artifacts {
         });
 
         Proxy proxy = Proxy(payable(addr_));
-        require(EIP1967Helper.getAdmin(address(proxy)) == mustGetAddress("ProxyAdmin"));
+        require(
+            EIP1967Helper.getAdmin(address(proxy)) == mustGetAddress("ProxyAdmin"),
+            "DeployPeriphery: FaucetProxy admin mismatch"
+        );
     }
 
     /// @notice Deploy the Faucet contract.
@@ -110,7 +113,7 @@ contract DeployPeriphery is Script, Artifacts {
         });
 
         Faucet faucet = Faucet(payable(addr_));
-        require(faucet.ADMIN() == cfg.faucetAdmin());
+        require(faucet.ADMIN() == cfg.faucetAdmin(), "DeployPeriphery: Faucet admin mismatch");
     }
 
     /// @notice Deploy the Drippie contract.
@@ -122,7 +125,7 @@ contract DeployPeriphery is Script, Artifacts {
         });
 
         Drippie drippie = Drippie(payable(addr_));
-        require(drippie.owner() == cfg.faucetDrippieOwner());
+        require(drippie.owner() == cfg.faucetDrippieOwner(), "DeployPeriphery: FaucetDrippie owner mismatch");
     }
 
     /// @notice Deploy the Drippie contract for standard operations.
@@ -134,7 +137,7 @@ contract DeployPeriphery is Script, Artifacts {
         });
 
         Drippie drippie = Drippie(payable(addr_));
-        require(drippie.owner() == cfg.operationsDrippieOwner());
+        require(drippie.owner() == cfg.operationsDrippieOwner(), "DeployPeriphery: OperationsDrippie owner mismatch");
     }
 
     /// @notice Deploy On-Chain Authentication Module.
@@ -146,7 +149,9 @@ contract DeployPeriphery is Script, Artifacts {
         });
 
         AdminFaucetAuthModule module = AdminFaucetAuthModule(addr_);
-        require(module.ADMIN() == cfg.faucetOnchainAuthModuleAdmin());
+        require(
+            module.ADMIN() == cfg.faucetOnchainAuthModuleAdmin(), "DeployPeriphery: OnChainAuthModule admin mismatch"
+        );
     }
 
     /// @notice Deploy Off-Chain Authentication Module.
@@ -158,7 +163,9 @@ contract DeployPeriphery is Script, Artifacts {
         });
 
         AdminFaucetAuthModule module = AdminFaucetAuthModule(addr_);
-        require(module.ADMIN() == cfg.faucetOffchainAuthModuleAdmin());
+        require(
+            module.ADMIN() == cfg.faucetOffchainAuthModuleAdmin(), "DeployPeriphery: OffChainAuthModule admin mismatch"
+        );
     }
 
     /// @notice Deploy CheckTrue contract.
@@ -209,7 +216,10 @@ contract DeployPeriphery is Script, Artifacts {
             proxyAdmin.upgrade({ _proxy: payable(faucetProxy), _implementation: faucet });
         }
 
-        require(Faucet(payable(faucetProxy)).ADMIN() == Faucet(payable(faucet)).ADMIN());
+        require(
+            Faucet(payable(faucetProxy)).ADMIN() == Faucet(payable(faucet)).ADMIN(),
+            "DeployPeriphery: Faucet admin mismatch"
+        );
     }
 
     /// @notice Installs the OnChain AuthModule on the Faucet contract.

diff --git a/packages/contracts-bedrock/test/safe/LivenessModule.t.sol b/packages/contracts-bedrock/test/safe/LivenessModule.t.sol
@@ -146,7 +146,7 @@ contract LivenessModule_GetRequiredThreshold_Test is LivenessModule_TestInit {
         pure
         returns (uint256)
     {
-        require(_percentage > 0 && _percentage <= 100);
+        require(_percentage > 0 && _percentage <= 100, "LivenessModule: _percentage must be between 1 and 100");
         uint256 toAdd;
 
         // If the total multiplied by the percentage is not divisible by 100, we need to add 1 to the result to

diff --git a/packages/contracts-bedrock/test/setup/CommonTest.sol b/packages/contracts-bedrock/test/setup/CommonTest.sol
@@ -197,7 +197,7 @@ contract CommonTest is Test, Setup, Events {
         if (!(alice == address(0) && bob == address(0))) {
             revert("CommonTest: Cannot enable custom gas token after deployment. Consider overriding `setUp`.");
         }
-        require(_token != Constants.ETHER);
+        require(_token != Constants.ETHER, "CommonTest: Cannot set gas token to ETHER");
 
         customGasToken = _token;
     }