contracts: implement onlyDelegateCall and add tests for audit fixes

packages/contracts-bedrock/interfaces/L1/opcm/IOPContractsManagerV2.sol

@@ -80,6 +80,8 @@ interface IOPContractsManagerV2 {
     error OPContractsManagerV2_InvalidUpgradeInput();
     error OPContractsManagerV2_SuperchainConfigNeedsUpgrade();
     error OPContractsManagerV2_InvalidUpgradeInstruction(string _key);
+    error OPContractsManagerV2_DuplicateUpgradeInstruction(string _key);
+    error OPContractsManagerV2_OnlyDelegateCall();
     error OPContractsManagerV2_CannotUpgradeToCustomGasToken();
     error OPContractsManagerV2_InvalidUpgradeSequence(string _lastVersion, string _thisVersion);
     error IdentityPrecompileCallFailed();

packages/contracts-bedrock/scripts/deploy/VerifyOPCM.s.sol

@@ -98,9 +98,6 @@ contract VerifyOPCM is Script {
     /// @notice Thrown when a staticcall to a validator getter fails.
     error VerifyOPCM_ValidatorCallFailed(string sig);
 
-    /// @notice Thrown when _findChar is called with a multi-character string.
-    error VerifyOPCM_MustBeSingleChar();
-
     /// @notice Preamble used for blueprint contracts.
     bytes constant BLUEPRINT_PREAMBLE = hex"FE7100";
 
@@ -290,6 +287,11 @@ contract VerifyOPCM is Script {
     /// @param _addr Address of the contract to verify.
     /// @param _skipConstructorVerification Whether to skip constructor verification.
     function runSingle(string memory _name, address _addr, bool _skipConstructorVerification) public {
+        // Make sure the setup function has been called.
+        if (!ready) {
+            setUp();
+        }
+
         // This function is used as part of the release checklist to verify new contracts.
         // Rather than requiring an opcm input parameter, just pass in an empty reference
         // as we really only need this for features that are in development.
@@ -1604,21 +1606,4 @@ contract VerifyOPCM is Script {
         if (!ok) revert VerifyOPCM_ValidatorCallFailed(_sig);
         return abi.decode(data, (bytes32));
     }
-
-    /// @notice Finds the position of a character in a string.
-    /// @param _str The string to search.
-    /// @param _char The character to find (as a single-char string).
-    /// @return The index of the first occurrence, or string length if not found.
-    function _findChar(string memory _str, string memory _char) internal pure returns (uint256) {
-        bytes memory strBytes = bytes(_str);
-        bytes memory charBytes = bytes(_char);
-        if (charBytes.length != 1) revert VerifyOPCM_MustBeSingleChar();
-        bytes1 target = charBytes[0];
-        for (uint256 i = 0; i < strBytes.length; i++) {
-            if (strBytes[i] == target) {
-                return i;
-            }
-        }
-        return strBytes.length;
-    }
 }

packages/contracts-bedrock/snapshots/abi/OPContractsManagerV2.json

@@ -804,6 +804,17 @@
     "name": "OPContractsManagerV2_CannotUpgradeToCustomGasToken",
     "type": "error"
   },
+  {
+    "inputs": [
+      {
+        "internalType": "string",
+        "name": "_key",
+        "type": "string"
+      }
+    ],
+    "name": "OPContractsManagerV2_DuplicateUpgradeInstruction",
+    "type": "error"
+  },
   {
     "inputs": [],
     "name": "OPContractsManagerV2_InvalidGameConfigs",
@@ -841,6 +852,11 @@
     "name": "OPContractsManagerV2_InvalidUpgradeSequence",
     "type": "error"
   },
+  {
+    "inputs": [],
+    "name": "OPContractsManagerV2_OnlyDelegateCall",
+    "type": "error"
+  },
   {
     "inputs": [],
     "name": "OPContractsManagerV2_SuperchainConfigNeedsUpgrade",

packages/contracts-bedrock/snapshots/semver-lock.json

@@ -52,8 +52,8 @@
     "sourceCodeHash": "0xb3184aa5d95a82109e7134d1f61941b30e25f655b9849a0e303d04bbce0cde0b"
   },
   "src/L1/opcm/OPContractsManagerV2.sol:OPContractsManagerV2": {
-    "initCodeHash": "0x88ada0dfefb77eea33baaf11d9b5a5ad51cb8c6476611d0f2376897413074619",
-    "sourceCodeHash": "0x1cc9dbcd4c7652f482c43e2630b324d088e825d12532711a41c636e8392636b3"
+    "initCodeHash": "0xca9edfa050a5583f063194fd8d098124d6f3c1367eec8875c0c8acf5d971657f",
+    "sourceCodeHash": "0x0238b990636aab82f93450b1ee2ff7a1f69d55a0b197265e696b70d285c85992"
   },
   "src/L2/BaseFeeVault.sol:BaseFeeVault": {
     "initCodeHash": "0x838bbd7f381e84e21887f72bd1da605bfc4588b3c39aed96cbce67c09335b3ee",

packages/contracts-bedrock/src/L1/opcm/OPContractsManagerMigrator.sol

@@ -11,7 +11,6 @@ import { Constants } from "src/libraries/Constants.sol";
 import { Features } from "src/libraries/Features.sol";
 
 // Interfaces
-import { IAddressManager } from "interfaces/legacy/IAddressManager.sol";
 import { IDelayedWETH } from "interfaces/dispute/IDelayedWETH.sol";
 import { IAnchorStateRegistry } from "interfaces/dispute/IAnchorStateRegistry.sol";
 import { IDisputeGame } from "interfaces/dispute/IDisputeGame.sol";
@@ -107,7 +106,7 @@ contract OPContractsManagerMigrator is OPContractsManagerUtilsCaller {
         // what we use here.
         IOPContractsManagerUtils.ProxyDeployArgs memory proxyDeployArgs = IOPContractsManagerUtils.ProxyDeployArgs({
             proxyAdmin: _input.chainSystemConfigs[0].proxyAdmin(),
-            addressManager: IAddressManager(address(0)), // AddressManager NOT needed for these proxies.
+            addressManager: _input.chainSystemConfigs[0].proxyAdmin().addressManager(),
             l2ChainId: block.timestamp,
             saltMixer: "interop salt mixer"
         });

packages/contracts-bedrock/src/L1/opcm/OPContractsManagerUtils.sol

@@ -195,6 +195,12 @@ contract OPContractsManagerUtils {
             return overrideInstruction.data;
         }
 
+        // Check that the source contract has code. Calling an EOA returns success with empty
+        // data, which would cause issues when the caller tries to decode the result.
+        if (_source.code.length == 0) {
+            revert OPContractsManagerUtils_ConfigLoadFailed(_name);
+        }
+
         // Otherwise, load the data from the source contract.
         (bool success, bytes memory result) = address(_source).staticcall(abi.encodePacked(_selector));
         if (!success) {

packages/contracts-bedrock/src/L1/opcm/OPContractsManagerV2.sol

@@ -126,6 +126,12 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
     /// @notice Thrown when an invalid upgrade instruction is provided.
     error OPContractsManagerV2_InvalidUpgradeInstruction(string _key);
 
+    /// @notice Thrown when duplicate upgrade instruction keys are provided.
+    error OPContractsManagerV2_DuplicateUpgradeInstruction(string _key);
+
+    /// @notice Thrown when a function that must be delegatecalled is called directly.
+    error OPContractsManagerV2_OnlyDelegateCall();
+
     /// @notice Thrown when a chain attempts to upgrade to custom gas token after initial deployment.
     error OPContractsManagerV2_CannotUpgradeToCustomGasToken();
 
@@ -147,9 +153,9 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
     ///         - Major bump: New required sequential upgrade
     ///         - Minor bump: Replacement OPCM for same upgrade
     ///         - Patch bump: Development changes (expected for normal dev work)
-    /// @custom:semver 7.0.9
+    /// @custom:semver 7.0.10
     function version() public pure returns (string memory) {
-        return "7.0.9";
+        return "7.0.10";
     }
 
     /// @param _standardValidator The standard validator for this OPCM release.
@@ -176,6 +182,8 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
     ///         Superchain-wide contracts.
     /// @param _inp The input for the Superchain upgrade.
     function upgradeSuperchain(SuperchainUpgradeInput memory _inp) external returns (SuperchainContracts memory) {
+        _onlyDelegateCall();
+
         // NOTE: Since this function is very minimal and only upgrades the SuperchainConfig
         // contract, not bothering to fully follow the pattern of the normal chain upgrade flow.
         // If we expand the scope of this function to add other Superchain-wide contracts, we'll
@@ -197,6 +205,9 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
     /// @param _cfg The full chain deployment configuration.
     /// @return The chain contracts.
     function deploy(FullConfig memory _cfg) external returns (ChainContracts memory) {
+        // Include msg.sender in the salt mixer to prevent cross-caller CREATE2 collisions.
+        string memory saltMixer = string(bytes.concat(bytes20(msg.sender), bytes(_cfg.saltMixer)));
+
         // Deploy is the ONLY place where we allow the "ALL" permission for proxy deployment.
         IOPContractsManagerUtils.ExtraInstruction[] memory instructions =
             new IOPContractsManagerUtils.ExtraInstruction[](1);
@@ -207,7 +218,7 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
 
         // Load the chain contracts.
         ChainContracts memory cts =
-            _loadChainContracts(ISystemConfig(address(0)), _cfg.l2ChainId, _cfg.saltMixer, instructions);
+            _loadChainContracts(ISystemConfig(address(0)), _cfg.l2ChainId, saltMixer, instructions);
 
         // Execute the deployment.
         return _apply(_cfg, cts, true);
@@ -217,6 +228,8 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
     /// @param _inp The chain upgrade input.
     /// @return The upgraded chain contracts.
     function upgrade(UpgradeInput memory _inp) external returns (ChainContracts memory) {
+        _onlyDelegateCall();
+
         // Sanity check that the SystemConfig isn't address(0). We use address(0) as a special
         // value to indicate that this is an initial deployment, so we definitely don't want to
         // allow it here.
@@ -264,6 +277,8 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
     ///      look or function like all of the other functions in OPCMv2.
     /// @param _input The input parameters for the migration.
     function migrate(IOPContractsManagerMigrator.MigrateInput calldata _input) public {
+        _onlyDelegateCall();
+
         // Delegatecall to the migrator contract.
         (bool success, bytes memory result) =
             address(opcmMigrator).delegatecall(abi.encodeCall(IOPContractsManagerMigrator.migrate, (_input)));
@@ -286,6 +301,17 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
         view
     {
         for (uint256 i = 0; i < _extraInstructions.length; i++) {
+            // Check for duplicate instruction keys. PermittedProxyDeployment is exempt because
+            // multiple proxy deployments may need to be permitted in a single upgrade.
+            if (!_isMatchingInstructionByKey(_extraInstructions[i], Constants.PERMITTED_PROXY_DEPLOYMENT_KEY)) {
+                for (uint256 j = i + 1; j < _extraInstructions.length; j++) {
+                    if (keccak256(bytes(_extraInstructions[i].key)) == keccak256(bytes(_extraInstructions[j].key))) {
+                        revert OPContractsManagerV2_DuplicateUpgradeInstruction(_extraInstructions[i].key);
+                    }
+                }
+            }
+
+            // Check that the instruction is permitted.
             if (!_isPermittedInstruction(_extraInstructions[i])) {
                 revert OPContractsManagerV2_InvalidUpgradeInstruction(_extraInstructions[i].key);
             }
@@ -316,6 +342,13 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
             }
         }
 
+        // Allow overriding the starting respected game type during upgrades. This is needed when
+        // disabling the currently-respected game type, since the validation requires the starting
+        // respected game type to correspond to an enabled game config.
+        if (_isMatchingInstructionByKey(_instruction, "overrides.cfg.startingRespectedGameType")) {
+            return true;
+        }
+
         // Always return false by default.
         return false;
     }
@@ -684,6 +717,21 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
         if (!_cfg.disputeGameConfigs[1].enabled) {
             revert OPContractsManagerV2_InvalidGameConfigs();
         }
+
+        // Validate that the starting respected game type corresponds to an enabled game config.
+        bool startingGameTypeFound = false;
+        for (uint256 i = 0; i < _cfg.disputeGameConfigs.length; i++) {
+            if (
+                _cfg.disputeGameConfigs[i].gameType.raw() == _cfg.startingRespectedGameType.raw()
+                    && _cfg.disputeGameConfigs[i].enabled
+            ) {
+                startingGameTypeFound = true;
+                break;
+            }
+        }
+        if (!startingGameTypeFound) {
+            revert OPContractsManagerV2_InvalidGameConfigs();
+        }
     }
 
     /// @notice Executes the deployment/upgrade action.
@@ -1003,6 +1051,13 @@ contract OPContractsManagerV2 is ISemver, OPContractsManagerUtilsCaller {
     //                       INTERNAL UTILITY FUNCTIONS                      //
     ///////////////////////////////////////////////////////////////////////////
 
+    /// @notice Reverts if the function is being called directly rather than via delegatecall.
+    function _onlyDelegateCall() internal view {
+        if (address(this) == address(opcmV2)) {
+            revert OPContractsManagerV2_OnlyDelegateCall();
+        }
+    }
+
     /// @notice Helper for retrieving the version of the OPCM contract.
     /// @dev We use opcmV2.version() because it allows us to properly mock the version function
     ///      in tests without running into issues because this contract is being DELEGATECALLed.

packages/contracts-bedrock/test/L1/opcm/OPContractsManagerUtils.t.sol

@@ -331,6 +331,18 @@ contract OPContractsManagerUtils_LoadBytes_Test is OPContractsManagerUtils_TestI
         assertEq(result, _overrideData, "Should return override data");
     }
 
+    /// @notice Tests that loadBytes reverts when the source address has no code.
+    function test_loadBytes_sourceNoCode_reverts() public {
+        address eoa = makeAddr("eoa");
+
+        vm.expectRevert(
+            abi.encodeWithSelector(
+                IOPContractsManagerUtils.OPContractsManagerUtils_ConfigLoadFailed.selector, "testField"
+            )
+        );
+        utils.loadBytes(eoa, MOCK_SELECTOR, "testField", _emptyInstructions());
+    }
+
     /// @notice Tests that loadBytes reverts when the source call fails.
     function test_loadBytes_sourceCallFails_reverts() public {
         // Mock the source to revert.