Audit review fixes

docs/deployments.md

@@ -130,7 +130,7 @@ each of the deployment commands do and which forge scripts are executed:
   - asks for the `OpStateBridge.sol` (Base) contract address
   - asks for the new gas limits for different actions
   - sets the OP-stack `CrossDomainMessenger` gas limits for the `OpStateBridge.sol` contract on Ethereum mainnet/testnet
-    using the `setGasLimitSendRoot()`, `setGasLimitSetRootHistoryExpiry()`, and`setGasLimitTransferOwnershipOp()`
+    using the `setGasLimitPropagateRoot()`, `setGasLimitSetRootHistoryExpiry()`, and`setGasLimitTransferOwnershipOp()`
     methods.
 
 ## Addresses of the production and staging deployments

src/OpStateBridge.sol

@@ -28,15 +28,18 @@ contract OpStateBridge is Ownable2Step {
     /// @notice Ethereum mainnet worldID Address
     address public immutable worldIDAddress;
 
-    /// @notice Amount of gas purchased on the OP Stack chain for _propagateRootOptimism
-    uint32 internal _gasLimitSendRoot;
+    /// @notice Amount of gas purchased on the OP Stack chain for propagateRoot
+    uint32 internal _gasLimitPropagateRoot;
 
-    /// @notice Amount of gas purchased on the OP Stack chain for SetRootHistoryExpirytimism
+    /// @notice Amount of gas purchased on the OP Stack chain for SetRootHistoryExpiry
     uint32 internal _gasLimitSetRootHistoryExpiry;
 
-    /// @notice Amount of gas purchased on the OP Stack chain for transferOwnershipOptimism
+    /// @notice Amount of gas purchased on the OP Stack chain for transferOwnershipOp
     uint32 internal _gasLimitTransferOwnership;
 
+    /// @notice The default gas limit amount to buy on an OP stack chain to do simple transactions
+    uint32 public constant DEFAULT_OP_GAS_LIMIT = 1000000;
+
     ///////////////////////////////////////////////////////////////////
     ///                            EVENTS                           ///
     ///////////////////////////////////////////////////////////////////
@@ -61,10 +64,10 @@ contract OpStateBridge is Ownable2Step {
 
     /// @notice Emmitted when the the StateBridge sets the gas limit for sendRootOp
     /// @param _opGasLimit The new opGasLimit for sendRootOp
-    event SetGasLimitSendRoot(uint32 _opGasLimit);
+    event SetGasLimitPropagateRoot(uint32 _opGasLimit);
 
-    /// @notice Emmitted when the the StateBridge sets the gas limit for SetRootHistoryExpiryt
-    /// @param _opGasLimit The new opGasLimit for SetRootHistoryExpirytimism
+    /// @notice Emmitted when the the StateBridge sets the gas limit for SetRootHistoryExpiry
+    /// @param _opGasLimit The new opGasLimit for SetRootHistoryExpiryt
     event SetGasLimitSetRootHistoryExpiry(uint32 _opGasLimit);
 
     /// @notice Emmitted when the the StateBridge sets the gas limit for transferOwnershipOp
@@ -95,9 +98,9 @@ contract OpStateBridge is Ownable2Step {
         opWorldIDAddress = _opWorldIDAddress;
         worldIDAddress = _worldIDIdentityManager;
         crossDomainMessengerAddress = _crossDomainMessenger;
-        _gasLimitSendRoot = 100000;
-        _gasLimitSetRootHistoryExpiry = 100000;
-        _gasLimitTransferOwnership = 100000;
+        _gasLimitPropagateRoot = DEFAULT_OP_GAS_LIMIT;
+        _gasLimitSetRootHistoryExpiry = DEFAULT_OP_GAS_LIMIT;
+        _gasLimitTransferOwnership = DEFAULT_OP_GAS_LIMIT;
     }
 
     ///////////////////////////////////////////////////////////////////
@@ -117,7 +120,7 @@ contract OpStateBridge is Ownable2Step {
             // Contract address on the OP Stack Chain
             opWorldIDAddress,
             message,
-            _gasLimitSendRoot
+            _gasLimitPropagateRoot
         );
 
         emit RootPropagated(latestRoot);
@@ -128,6 +131,8 @@ contract OpStateBridge is Ownable2Step {
     /// @param _owner new owner (EOA or contract)
     /// @param _isLocal true if new owner is on Optimism, false if it is a cross-domain owner
     function transferOwnershipOp(address _owner, bool _isLocal) external onlyOwner {
+        require(_owner != address(0), "New owner can't be the zero address");
+
         // The `encodeCall` function is strongly typed, so this checks that we are passing the
         // correct data to the OP Stack chain bridge.
         bytes memory message =
@@ -165,17 +170,21 @@ contract OpStateBridge is Ownable2Step {
     ///                         OP GAS LIMIT                        ///
     ///////////////////////////////////////////////////////////////////
 
-    /// @notice Sets the gas limit for the sendRootOp method
-    /// @param _opGasLimit The new gas limit for the sendRootOp method
-    function setGasLimitSendRoot(uint32 _opGasLimit) external onlyOwner {
-        _gasLimitSetRootHistoryExpiry = _opGasLimit;
+    /// @notice Sets the gas limit for the propagateRoot method
+    /// @param _opGasLimit The new gas limit for the propagateRoot method
+    function setGasLimitPropagateRoot(uint32 _opGasLimit) external onlyOwner {
+        require(_opGasLimit > 0, "Gas limit must be greater than 0");
 
-        emit SetGasLimitSendRoot(_opGasLimit);
+        _gasLimitPropagateRoot = _opGasLimit;
+
+        emit SetGasLimitPropagateRoot(_opGasLimit);
     }
 
     /// @notice Sets the gas limit for the SetRootHistoryExpiry method
     /// @param _opGasLimit The new gas limit for the SetRootHistoryExpiry method
     function setGasLimitSetRootHistoryExpiry(uint32 _opGasLimit) external onlyOwner {
+        require(_opGasLimit > 0, "Gas limit must be greater than 0");
+
         _gasLimitSetRootHistoryExpiry = _opGasLimit;
 
         emit SetGasLimitSetRootHistoryExpiry(_opGasLimit);
@@ -184,6 +193,8 @@ contract OpStateBridge is Ownable2Step {
     /// @notice Sets the gas limit for the transferOwnershipOp method
     /// @param _opGasLimit The new gas limit for the transferOwnershipOp method
     function setGasLimitTransferOwnershipOp(uint32 _opGasLimit) external onlyOwner {
+        require(_opGasLimit > 0, "Gas limit must be greater than 0");
+
         _gasLimitTransferOwnership = _opGasLimit;
 
         emit SetGasLimitTransferOwnershipOp(_opGasLimit);

src/PolygonStateBridge.sol

@@ -29,6 +29,9 @@ contract PolygonStateBridge is FxBaseRootTunnel, Ownable2Step {
     /// @param rootHistoryExpiry The new root history expiry
     event SetRootHistoryExpiry(uint256 rootHistoryExpiry);
 
+    /// @notice Emmitted when the owner calls setFxChildTunnel for the first time
+    event SetFxChildTunnel(address fxChildTunnel);
+
     /// @notice Emmitted when a root is sent to PolygonWorldID
     /// @param root The latest WorldID Identity Manager root.
     event RootPropagated(uint256 root);
@@ -51,6 +54,15 @@ contract PolygonStateBridge is FxBaseRootTunnel, Ownable2Step {
     constructor(address _checkpointManager, address _fxRoot, address _worldIDIdentityManager)
         FxBaseRootTunnel(_checkpointManager, _fxRoot)
     {
+        require(
+            _worldIDIdentityManager != address(0),
+            "WorldIDIdentityManager cannot be the zero address"
+        );
+
+        require(_fxRoot != address(0), "fxRoot cannot be the zero address");
+
+        require(_checkpointManager != address(0), "checkpointManager cannot be the zero address");
+
         worldID = IWorldIDIdentityManager(_worldIDIdentityManager);
     }
 
@@ -102,6 +114,8 @@ contract PolygonStateBridge is FxBaseRootTunnel, Ownable2Step {
     function setFxChildTunnel(address _fxChildTunnel) public virtual override onlyOwner {
         require(fxChildTunnel == address(0x0), "FxBaseRootTunnel: CHILD_TUNNEL_ALREADY_SET");
         fxChildTunnel = _fxChildTunnel;
+
+        emit SetFxChildTunnel(_fxChildTunnel);
     }
 
     ///////////////////////////////////////////////////////////////////

src/PolygonWorldID.sol

@@ -4,8 +4,6 @@ pragma solidity ^0.8.15;
 import {WorldIDBridge} from "./abstract/WorldIDBridge.sol";
 import {FxBaseChildTunnel} from "fx-portal/contracts/tunnel/FxBaseChildTunnel.sol";
 import {Ownable2Step} from "openzeppelin-contracts/access/Ownable2Step.sol";
-import {SemaphoreTreeDepthValidator} from "./utils/SemaphoreTreeDepthValidator.sol";
-import {SemaphoreVerifier} from "semaphore/base/SemaphoreVerifier.sol";
 import {BytesUtils} from "./utils/BytesUtils.sol";
 
 /// @title Polygon WorldID Bridge
@@ -30,6 +28,12 @@ contract PolygonWorldID is WorldIDBridge, FxBaseChildTunnel, Ownable2Step {
     bytes4 private immutable receiveRootHistoryExpirySelector =
         bytes4(keccak256("setRootHistoryExpiry(uint256)"));
 
+    ///////////////////////////////////////////////////////////////////
+    ///                            EVENTS                           ///
+    ///////////////////////////////////////////////////////////////////
+    /// @notice Thrown when setFxRootTunnel is called for the first time
+    event SetFxRootTunnel(address fxRootTunnel);
+
     ///////////////////////////////////////////////////////////////////
     ///                            ERRORS                           ///
     ///////////////////////////////////////////////////////////////////
@@ -63,6 +67,9 @@ contract PolygonWorldID is WorldIDBridge, FxBaseChildTunnel, Ownable2Step {
     ///      FxChildTunnel. Can revert if the message is not valid - decoding fails.
     ///      Can not work if Polygon's StateSync mechanism breaks and FxPortal does not receive the message
     ///      on the other end.
+    /// @dev the message payload has the following format:
+    ///      `bytes4 selector` + `bytes payload`, the first 4 bytes are extracted using BytesUtils, once the
+    ///      selector is extracted, the payload is decoded using abi.decode(payload, (uint256))
     ///
     /// @custom:param uint256 stateId An unused placeholder variable for `stateId`,
     /// required by the signature in fxChild.
@@ -115,6 +122,8 @@ contract PolygonWorldID is WorldIDBridge, FxBaseChildTunnel, Ownable2Step {
     function setFxRootTunnel(address _fxRootTunnel) external virtual override onlyOwner {
         require(fxRootTunnel == address(0x0), "FxBaseChildTunnel: ROOT_TUNNEL_ALREADY_SET");
         fxRootTunnel = _fxRootTunnel;
+
+        emit SetFxRootTunnel(_fxRootTunnel);
     }
 
     ///////////////////////////////////////////////////////////////////

src/abstract/WorldIDBridge.sol

@@ -6,8 +6,6 @@ import {IWorldID} from "../interfaces/IWorldID.sol";
 import {SemaphoreTreeDepthValidator} from "../utils/SemaphoreTreeDepthValidator.sol";
 import {SemaphoreVerifier} from "semaphore/base/SemaphoreVerifier.sol";
 
-import "forge-std/console.sol";
-
 /// @title Bridged World ID
 /// @author Worldcoin
 /// @notice A base contract for the WorldID state bridges that exist on other chains. The state
@@ -23,7 +21,7 @@ abstract contract WorldIDBridge is IWorldID {
     ///////////////////////////////////////////////////////////////////////////////
 
     /// @notice The depth of the merkle tree used to store identities.
-    uint8 internal treeDepth;
+    uint8 internal immutable treeDepth;
 
     /// @notice The amount of time a root is considered as valid on the bridged chain.
     uint256 internal ROOT_HISTORY_EXPIRY = 1 weeks;
@@ -158,7 +156,7 @@ abstract contract WorldIDBridge is IWorldID {
     /// @dev Note that a double-signaling check is not included here, and should be carried by the
     ///      caller.
     ///
-    /// @param root The of the Merkle tree
+    /// @param root The root of the Merkle tree
     /// @param signalHash A keccak256 hash of the Semaphore signal
     /// @param nullifierHash The nullifier hash
     /// @param externalNullifierHash A keccak256 hash of the external nullifier

src/script/deploy.js

@@ -445,7 +445,7 @@ async function deployOptimismOpStateBridgeGoerli(config) {
     console.error(err);
   }
 
-  spinner.succeed("DeployStateBridgeMainnet.s.sol ran successfully!");
+  spinner.succeed("DeployOptimismStateBridgeGoerli.s.sol ran successfully!");
 }
 
 async function deployOptimismOpStateBridgeMainnet(config) {
@@ -460,7 +460,7 @@ async function deployOptimismOpStateBridgeMainnet(config) {
     console.error(err);
   }
 
-  spinner.succeed("DeployStateBridgeMainnet.s.sol ran successfully!");
+  spinner.succeed("DeployOptimismStateBridgeMainnet.s.sol ran successfully!");
 }
 
 async function deployBaseOpStateBridgeGoerli(config) {
@@ -475,7 +475,7 @@ async function deployBaseOpStateBridgeGoerli(config) {
     console.error(err);
   }
 
-  spinner.succeed("DeployBaseStateBridgeMainnet.s.sol ran successfully!");
+  spinner.succeed("DeployBaseStateBridgeGoerli.s.sol ran successfully!");
 }
 
 async function deployBaseOpStateBridgeMainnet(config) {
@@ -505,7 +505,7 @@ async function deployPolygonStateBridgeGoerli(config) {
     console.error(err);
   }
 
-  spinner.succeed("DeployStateBridgeMainnet.s.sol ran successfully!");
+  spinner.succeed("DeployPolygonStateBridgeGoerli.s.sol ran successfully!");
 }
 
 async function deployPolygonStateBridgeMainnet(config) {
@@ -520,7 +520,7 @@ async function deployPolygonStateBridgeMainnet(config) {
     console.error(err);
   }
 
-  spinner.succeed("DeployStateBridgeMainnet.s.sol ran successfully!");
+  spinner.succeed("DeployPolygonStateBridgeMainnet.s.sol ran successfully!");
 }
 
 async function deployPolygonWorldIDMumbai(config) {
@@ -535,7 +535,7 @@ async function deployPolygonWorldIDMumbai(config) {
     console.error(err);
   }
 
-  spinner.succeed("DeployPolygonWorldID.s.sol ran successfully!");
+  spinner.succeed("DeployPolygonWorldIDMumbai.s.sol ran successfully!");
 }
 
 async function deployPolygonWorldIDMainnet(config) {
@@ -550,7 +550,7 @@ async function deployPolygonWorldIDMainnet(config) {
     console.error(err);
   }
 
-  spinner.succeed("DeployPolygonWorldID.s.sol ran successfully!");
+  spinner.succeed("DeployPolygonWorldIDMainnet.s.sol ran successfully!");
 }
 
 async function deployOptimismWorldID(config) {

src/script/initialize/op-stack/base/SetGasLimitBase.s.sol

@@ -42,7 +42,7 @@ contract SetOpGasLimitBase is Script {
     function run() public {
         vm.startBroadcast(privateKey);
 
-        baseStateBridge.setGasLimitSendRoot(gasLimitSendRootBase);
+        baseStateBridge.setGasLimitPropagateRoot(gasLimitSendRootBase);
         baseStateBridge.setGasLimitSetRootHistoryExpiry(gasLimitSetRootHistoryExpiryBase);
         baseStateBridge.setGasLimitTransferOwnershipOp(gasLimitTransferOwnershipBase);
 

src/script/initialize/op-stack/optimism/SetGasLimitOptimism.s.sol

@@ -42,7 +42,7 @@ contract SetOpGasLimitOptimism is Script {
     function run() public {
         vm.startBroadcast(privateKey);
 
-        optimismStateBridge.setGasLimitSendRoot(gasLimitSendRootOptimism);
+        optimismStateBridge.setGasLimitPropagateRoot(gasLimitSendRootOptimism);
         optimismStateBridge.setGasLimitSetRootHistoryExpiry(gasLimitSetRootHistoryExpiryOptimism);
         optimismStateBridge.setGasLimitTransferOwnershipOp(gasLimitTransferOwnershipOptimism);
 

src/script/ownership/base/SetGasLimitBase.s.sol

@@ -42,7 +42,7 @@ contract SetOpGasLimitBase is Script {
     function run() public {
         vm.startBroadcast(privateKey);
 
-        baseStateBridge.setGasLimitSendRoot(gasLimitSendRootBase);
+        baseStateBridge.setGasLimitPropagateRoot(gasLimitSendRootBase);
         baseStateBridge.setGasLimitSetRootHistoryExpiry(gasLimitSetRootHistoryExpiryBase);
         baseStateBridge.setGasLimitTransferOwnershipOp(gasLimitTransferOwnershipBase);
 

src/script/ownership/optimism/SetGasLimitOptimism.s.sol

@@ -42,7 +42,7 @@ contract SetOpGasLimitOptimism is Script {
     function run() public {
         vm.startBroadcast(privateKey);
 
-        optimismStateBridge.setGasLimitSendRoot(gasLimitSendRootOptimism);
+        optimismStateBridge.setGasLimitPropagateRoot(gasLimitSendRootOptimism);
         optimismStateBridge.setGasLimitSetRootHistoryExpiry(gasLimitSetRootHistoryExpiryOptimism);
         optimismStateBridge.setGasLimitTransferOwnershipOp(gasLimitTransferOwnershipOptimism);
 

src/test/OpStateBridge.t.sol

@@ -67,7 +67,7 @@ contract OpStateBridgeTest is PRBTest, StdCheats {
 
     /// @notice Emmitted when the the StateBridge sets the gas limit for sendRootOp
     /// @param _opGasLimit The new opGasLimit for sendRootOp
-    event SetGasLimitSendRoot(uint32 _opGasLimit);
+    event SetGasLimitPropagateRoot(uint32 _opGasLimit);
 
     /// @notice Emmitted when the the StateBridge sets the gas limit for SetRootHistoryExpiryt
     /// @param _opGasLimit The new opGasLimit for SetRootHistoryExpirytimism
@@ -186,15 +186,15 @@ contract OpStateBridgeTest is PRBTest, StdCheats {
 
     /// @notice tests whether the StateBridge contract can set the opGasLimit for sendRootOptimism
     /// @param _opGasLimit The new opGasLimit for sendRootOptimism
-    function test_owner_setGasLimitSendRoot_succeeds(uint32 _opGasLimit) public {
+    function test_owner_setGasLimitPropagateRoot_succeeds(uint32 _opGasLimit) public {
         vm.assume(_opGasLimit != 0);
 
         vm.expectEmit(true, true, true, true);
 
-        emit SetGasLimitSendRoot(_opGasLimit);
+        emit SetGasLimitPropagateRoot(_opGasLimit);
 
         vm.prank(owner);
-        opStateBridge.setGasLimitSendRoot(_opGasLimit);
+        opStateBridge.setGasLimitPropagateRoot(_opGasLimit);
     }
 
     /// @notice tests whether the StateBridge contract can set the opGasLimit for SetRootHistoryExpirytimism
@@ -229,6 +229,7 @@ contract OpStateBridgeTest is PRBTest, StdCheats {
 
     /// @notice tests that the StateBridge contract's ownership can't be changed by a non-owner
     /// @param newOwner The new owner of the StateBridge contract (foundry fuzz)
+    /// @param nonOwner An address that is not the owner of the StateBridge contract
     function test_notOwner_transferOwnership_reverts(address nonOwner, address newOwner) public {
         vm.assume(nonOwner != owner && nonOwner != address(0) && newOwner != address(0));
 
@@ -238,6 +239,14 @@ contract OpStateBridgeTest is PRBTest, StdCheats {
         opStateBridge.transferOwnership(newOwner);
     }
 
+    /// @notice tests that the StateBridge contract's ownership can't be set to be the zero address
+    function test_owner_transferOwnershipOp_toZeroAddress_reverts() public {
+        vm.expectRevert("New owner can't be the zero address");
+
+        vm.prank(owner);
+        opStateBridge.transferOwnershipOp(address(0), true);
+    }
+
     /// @notice tests that the StateBridge contract's ownership can't be changed by a non-owner
     /// @param newOwner The new owner of the StateBridge contract (foundry fuzz)
     function test_notOwner_transferOwnershipOp_reverts(
@@ -292,4 +301,22 @@ contract OpStateBridgeTest is PRBTest, StdCheats {
         vm.prank(owner);
         opStateBridge.renounceOwnership();
     }
+
+    /// @notice Tests that the StateBridge contract can't set the opGasLimit for sendRootOptimism to 0
+    function test_setGasLimitToZero_reverts() public {
+        vm.expectRevert("Gas limit must be greater than 0");
+
+        vm.prank(owner);
+        opStateBridge.setGasLimitPropagateRoot(0);
+
+        vm.expectRevert("Gas limit must be greater than 0");
+
+        vm.prank(owner);
+        opStateBridge.setGasLimitSetRootHistoryExpiry(0);
+
+        vm.expectRevert("Gas limit must be greater than 0");
+
+        vm.prank(owner);
+        opStateBridge.setGasLimitTransferOwnershipOp(0);
+    }
 }