do not allow non-contract address as new implementations (#120)

* do not allow non-contract address as new implementations

* linting

* lint fixes

contracts/ForkingManager.sol

@@ -3,6 +3,7 @@ pragma solidity ^0.8.20;
 
 import {ERC20PresetMinterPauser} from "@openzeppelin/contracts/token/ERC20/presets/ERC20PresetMinterPauser.sol";
 import {Initializable} from "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol";
+import {AddressUpgradeable} from "@openzeppelin/contracts-upgradeable/utils/AddressUpgradeable.sol";
 import {SafeERC20} from "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
 import {IERC20} from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
 import {IERC20Metadata} from "@openzeppelin/contracts/token/ERC20/extensions/IERC20Metadata.sol";
@@ -97,11 +98,69 @@ contract ForkingManager is IForkingManager, ForkableStructure {
         );
 
         disputeData = _disputeData;
+        require(
+            verifyNewImplementations(_newImplementations),
+            "Invalid new implementations"
+        );
         proposedImplementations = _newImplementations;
         // solhint-disable-next-line not-rely-on-time
         executionTimeForProposal = (block.timestamp + forkPreparationTime);
     }
 
+    /**
+     * @notice function to check that the new implementations are properly set and are actual contracts
+     * @param _newImplementations the addresses of the new implementations that will
+     * be used in the fork
+     * @return true if the new implementations are properly formed
+     */
+    function verifyNewImplementations(
+        NewImplementations calldata _newImplementations
+    ) internal view returns (bool) {
+        // address zero is placeholder for no change and hence allowed. CreateChildren library will then read out the parent address and use it
+        require(
+            _newImplementations.bridgeImplementation == address(0) ||
+                AddressUpgradeable.isContract(
+                    _newImplementations.bridgeImplementation
+                ),
+            "bridge not contract"
+        );
+        require(
+            _newImplementations.zkEVMImplementation == address(0) ||
+                AddressUpgradeable.isContract(
+                    _newImplementations.zkEVMImplementation
+                ),
+            "zkEVM not contract"
+        );
+        require(
+            _newImplementations.forkonomicTokenImplementation == address(0) ||
+                AddressUpgradeable.isContract(
+                    _newImplementations.forkonomicTokenImplementation
+                ),
+            "forkonomic token not contract"
+        );
+        require(
+            _newImplementations.forkingManagerImplementation == address(0) ||
+                AddressUpgradeable.isContract(
+                    _newImplementations.forkingManagerImplementation
+                ),
+            "forking manager not contract"
+        );
+
+        require(
+            _newImplementations.zkEVMImplementation == address(0) ||
+                AddressUpgradeable.isContract(
+                    _newImplementations.globalExitRootImplementation
+                ),
+            "global exit root not contract"
+        );
+        require(
+            _newImplementations.verifier == address(0) ||
+                AddressUpgradeable.isContract(_newImplementations.verifier),
+            "verifier not contract"
+        );
+        return true;
+    }
+
     /**
      * @dev function that executes a fork proposal
      */

test/ForkingManager.t.sol

@@ -3,6 +3,7 @@ pragma solidity ^0.8.20;
 /* solhint-disable not-rely-on-time */
 
 import {Test} from "forge-std/Test.sol";
+import {VerifierRollupHelperMock} from "@RealityETH/zkevm-contracts/contracts/mocks/VerifierRollupHelperMock.sol";
 import {ForkingManager} from "../contracts/ForkingManager.sol";
 import {ForkableBridge} from "../contracts/ForkableBridge.sol";
 import {ForkableZkEVM} from "../contracts/ForkableZkEVM.sol";
@@ -69,7 +70,7 @@ contract ForkingManagerTest is Test {
     address public newForkmanagerImplementation = address(new ForkingManager());
     address public newZkevmImplementation = address(new ForkableZkEVM());
     address public newVerifierImplementation =
-        address(0x1234567890123456789012345678901234567894);
+        address(new VerifierRollupHelperMock());
     address public newGlobalExitRootImplementation =
         address(new ForkableGlobalExitRoot());
     address public newForkonomicTokenImplementation =
@@ -246,6 +247,48 @@ contract ForkingManagerTest is Test {
         assertFalse(forkmanager.canFork());
     }
 
+    function testVerifyNewImplementationsRejectsNonContractBridgeImplementation()
+        public
+    {
+        forkonomicToken.approve(address(forkmanager), arbitrationFee);
+        vm.prank(address(this));
+        forkonomicToken.mint(address(this), arbitrationFee);
+        IForkingManager.NewImplementations
+            memory implementations = IForkingManager.NewImplementations({
+                bridgeImplementation: address(0x1), // non contract
+                zkEVMImplementation: newZkevmImplementation,
+                forkonomicTokenImplementation: newForkonomicTokenImplementation,
+                forkingManagerImplementation: newForkmanagerImplementation,
+                globalExitRootImplementation: newGlobalExitRootImplementation,
+                verifier: newVerifierImplementation,
+                forkID: newForkID
+            });
+
+        vm.expectRevert("bridge not contract");
+        forkmanager.initiateFork(disputeData, implementations);
+    }
+
+    function testVerifyNewImplementationsRejectsNonContractZkEVMImplementation()
+        public
+    {
+        forkonomicToken.approve(address(forkmanager), arbitrationFee);
+        vm.prank(address(this));
+        forkonomicToken.mint(address(this), arbitrationFee);
+        IForkingManager.NewImplementations
+            memory implementations = IForkingManager.NewImplementations({
+                bridgeImplementation: newBridgeImplementation, // non contract
+                zkEVMImplementation: address(0x234),
+                forkonomicTokenImplementation: newForkonomicTokenImplementation,
+                forkingManagerImplementation: newForkmanagerImplementation,
+                globalExitRootImplementation: newGlobalExitRootImplementation,
+                verifier: newVerifierImplementation,
+                forkID: newForkID
+            });
+
+        vm.expectRevert("zkEVM not contract");
+        forkmanager.initiateFork(disputeData, implementations);
+    }
+
     function testInitiateForkChargesFees() public {
         // Call the initiateFork function to create a new fork
         vm.expectRevert(bytes("ERC20: insufficient allowance"));

test/L1GlobalChainInfoPublisher.t.sol

@@ -12,6 +12,7 @@ import {Arbitrator} from "../contracts/lib/reality-eth/Arbitrator.sol";
 // TODO: Replace this with whatever zkEVM or whatever platform we're on uses
 import {IAMB} from "../contracts/interfaces/IAMB.sol";
 
+import {VerifierRollupHelperMock} from "@RealityETH/zkevm-contracts/contracts/mocks/VerifierRollupHelperMock.sol";
 import {IRealityETH} from "../contracts/interfaces/IRealityETH.sol";
 import {IERC20} from "../contracts/interfaces/IERC20.sol";
 import {ForkableRealityETH_ERC20} from "../contracts/ForkableRealityETH_ERC20.sol";
@@ -93,8 +94,9 @@ contract L1GlobalChainInfoPublisherTest is Test {
     address public newBridgeImplementation = address(new ForkableBridge());
     address public newForkmanagerImplementation = address(new ForkingManager());
     address public newZkevmImplementation = address(new ForkableZkEVM());
+
     address public newVerifierImplementation =
-        address(0x1234567890123456789012345678901234567894);
+        address(new VerifierRollupHelperMock());
     address public newGlobalExitRootImplementation =
         address(new ForkableGlobalExitRoot());
     address public newForkonomicTokenImplementation =

test/L1GlobalForkRequester.t.sol

@@ -3,6 +3,7 @@ pragma solidity ^0.8.20;
 /* solhint-disable not-rely-on-time */
 
 import {Test} from "forge-std/Test.sol";
+import {VerifierRollupHelperMock} from "@RealityETH/zkevm-contracts/contracts/mocks/VerifierRollupHelperMock.sol";
 import {ForkingManager} from "../contracts/ForkingManager.sol";
 import {ForkableBridge} from "../contracts/ForkableBridge.sol";
 import {ForkableZkEVM} from "../contracts/ForkableZkEVM.sol";
@@ -76,7 +77,7 @@ contract L1GlobalForkRequesterTest is Test {
     address public newForkmanagerImplementation = address(new ForkingManager());
     address public newZkevmImplementation = address(new ForkableZkEVM());
     address public newVerifierImplementation =
-        address(0x1234567890123456789012345678901234567894);
+        address(new VerifierRollupHelperMock());
     address public newGlobalExitRootImplementation =
         address(new ForkableGlobalExitRoot());
     address public newForkonomicTokenImplementation =
@@ -322,7 +323,16 @@ contract L1GlobalForkRequesterTest is Test {
             vm.prank(address(this));
             forkonomicToken.approve(address(forkmanager), fee);
             // Assume the data contains the questionId and pass it directly to the forkmanager in the fork request
-            IForkingManager.NewImplementations memory newImplementations;
+            IForkingManager.NewImplementations
+                memory newImplementations = IForkingManager.NewImplementations(
+                    newBridgeImplementation,
+                    newZkevmImplementation,
+                    newForkonomicTokenImplementation,
+                    newForkmanagerImplementation,
+                    newGlobalExitRootImplementation,
+                    newVerifierImplementation,
+                    uint64(0x7)
+                );
             IForkingManager.DisputeData memory disputeData = IForkingManager
                 .DisputeData(false, address(this), requestId);
             forkmanager.initiateFork(disputeData, newImplementations);