fix: OPCM additional safety checks (#12107)

ethereum-optimism · Sep 25, 2024 · e81c50d · e81c50d
1 parent 06f1406
commit e81c50d
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 10 deletions.
diff --git a/packages/contracts-bedrock/scripts/DeployOPChain.s.sol b/packages/contracts-bedrock/scripts/DeployOPChain.s.sol
@@ -139,9 +139,10 @@ contract DeployOPChainInput is BaseDeployIO {
         return abi.encode(defaultStartingAnchorRoots);
     }
 
-    // TODO: Check that opcm is proxied and it has an implementation.
-    function opcmProxy() public view returns (OPContractsManager) {
+    function opcmProxy() public returns (OPContractsManager) {
         require(address(_opcmProxy) != address(0), "DeployOPChainInput: not set");
+        DeployUtils.assertValidContractAddress(address(_opcmProxy));
+        DeployUtils.assertImplementationSet(address(_opcmProxy));
         return _opcmProxy;
     }
 }
@@ -303,7 +304,7 @@ contract DeployOPChainOutput is BaseDeployIO {
         assertValidSystemConfig(_doi);
     }
 
-    function assertValidPermissionedDisputeGame(DeployOPChainInput _doi) internal view {
+    function assertValidPermissionedDisputeGame(DeployOPChainInput _doi) internal {
         PermissionedDisputeGame game = permissionedDisputeGame();
 
         require(GameType.unwrap(game.gameType()) == GameType.unwrap(GameTypes.PERMISSIONED_CANNON), "DPG-10");
@@ -344,7 +345,7 @@ contract DeployOPChainOutput is BaseDeployIO {
         require(address(registry.disputeGameFactory()) == address(disputeGameFactoryProxy()), "ANCHORI-10");
     }
 
-    function assertValidSystemConfig(DeployOPChainInput _doi) internal view {
+    function assertValidSystemConfig(DeployOPChainInput _doi) internal {
         SystemConfig systemConfig = systemConfigProxy();
 
         DeployUtils.assertInitialized({ _contractAddress: address(systemConfig), _slot: 0, _offset: 0 });
@@ -383,7 +384,7 @@ contract DeployOPChainOutput is BaseDeployIO {
         require(gasPayingToken == Constants.ETHER, "SYSCON-220");
     }
 
-    function assertValidL1CrossDomainMessenger(DeployOPChainInput _doi) internal view {
+    function assertValidL1CrossDomainMessenger(DeployOPChainInput _doi) internal {
         L1CrossDomainMessenger messenger = l1CrossDomainMessengerProxy();
 
         DeployUtils.assertInitialized({ _contractAddress: address(messenger), _slot: 0, _offset: 20 });
@@ -399,7 +400,7 @@ contract DeployOPChainOutput is BaseDeployIO {
         require(address(uint160(uint256(xdmSenderSlot))) == Constants.DEFAULT_L2_SENDER, "L1xDM-60");
     }
 
-    function assertValidL1StandardBridge(DeployOPChainInput _doi) internal view {
+    function assertValidL1StandardBridge(DeployOPChainInput _doi) internal {
         L1StandardBridge bridge = l1StandardBridgeProxy();
         L1CrossDomainMessenger messenger = l1CrossDomainMessengerProxy();
 
@@ -421,7 +422,7 @@ contract DeployOPChainOutput is BaseDeployIO {
         require(factory.bridge() == address(l1StandardBridgeProxy()), "MERC20F-20");
     }
 
-    function assertValidL1ERC721Bridge(DeployOPChainInput _doi) internal view {
+    function assertValidL1ERC721Bridge(DeployOPChainInput _doi) internal {
         L1ERC721Bridge bridge = l1ERC721BridgeProxy();
 
         DeployUtils.assertInitialized({ _contractAddress: address(bridge), _slot: 0, _offset: 0 });
@@ -434,7 +435,7 @@ contract DeployOPChainOutput is BaseDeployIO {
         require(address(bridge.superchainConfig()) == address(_doi.opcmProxy().superchainConfig()), "L721B-50");
     }
 
-    function assertValidOptimismPortal(DeployOPChainInput _doi) internal view {
+    function assertValidOptimismPortal(DeployOPChainInput _doi) internal {
         OptimismPortal2 portal = optimismPortalProxy();
         ISuperchainConfig superchainConfig = ISuperchainConfig(address(_doi.opcmProxy().superchainConfig()));
 

diff --git a/packages/contracts-bedrock/test/DeployOPChain.t.sol b/packages/contracts-bedrock/test/DeployOPChain.t.sol
@@ -30,6 +30,7 @@ import { L1CrossDomainMessenger } from "src/L1/L1CrossDomainMessenger.sol";
 import { L1ERC721Bridge } from "src/L1/L1ERC721Bridge.sol";
 import { L1StandardBridge } from "src/L1/L1StandardBridge.sol";
 import { OptimismMintableERC20Factory } from "src/universal/OptimismMintableERC20Factory.sol";
+import { Proxy } from "src/universal/Proxy.sol";
 
 import { GameType, GameTypes, Hash, OutputRoot } from "src/dispute/lib/Types.sol";
 
@@ -52,6 +53,15 @@ contract DeployOPChainInput_Test is Test {
         doi = new DeployOPChainInput();
     }
 
+    function buildOpcmProxy() public returns (Proxy opcmProxy) {
+        opcmProxy = new Proxy(address(0));
+        OPContractsManager opcmImpl = OPContractsManager(address(makeAddr("opcmImpl")));
+        vm.prank(address(0));
+        opcmProxy.upgradeTo(address(opcmImpl));
+        vm.etch(address(opcmProxy), address(opcmProxy).code);
+        vm.etch(address(opcmImpl), hex"01");
+    }
+
     function test_set_succeeds() public {
         doi.set(doi.opChainProxyAdminOwner.selector, opChainProxyAdminOwner);
         doi.set(doi.systemConfigOwner.selector, systemConfigOwner);
@@ -62,7 +72,10 @@ contract DeployOPChainInput_Test is Test {
         doi.set(doi.basefeeScalar.selector, basefeeScalar);
         doi.set(doi.blobBaseFeeScalar.selector, blobBaseFeeScalar);
         doi.set(doi.l2ChainId.selector, l2ChainId);
-        doi.set(doi.opcmProxy.selector, address(opcm));
+
+        (Proxy opcmProxy) = buildOpcmProxy();
+        doi.set(doi.opcmProxy.selector, address(opcmProxy));
+
         // Compare the default inputs to the getter methods.
         assertEq(opChainProxyAdminOwner, doi.opChainProxyAdminOwner(), "200");
         assertEq(systemConfigOwner, doi.systemConfigOwner(), "300");
@@ -73,7 +86,7 @@ contract DeployOPChainInput_Test is Test {
         assertEq(basefeeScalar, doi.basefeeScalar(), "800");
         assertEq(blobBaseFeeScalar, doi.blobBaseFeeScalar(), "900");
         assertEq(l2ChainId, doi.l2ChainId(), "1000");
-        assertEq(address(opcm), address(doi.opcmProxy()), "1100");
+        assertEq(address(opcmProxy), address(doi.opcmProxy()), "1100");
     }
 
     function test_getters_whenNotSet_revert() public {