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,20 +28,23 @@ 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                           ///
     ///////////////////////////////////////////////////////////////////
 
-    /// @notice Emmitted when the the StateBridge gives ownership of the OPWorldID contract
+    /// @notice Emitted when the the StateBridge gives ownership of the OPWorldID contract
     /// to the WorldID Identity Manager contract away
     /// @param previousOwner The previous owner of the OPWorldID contract
     /// @param newOwner The new owner of the OPWorldID contract
@@ -51,23 +54,23 @@ contract OpStateBridge is Ownable2Step {
         address indexed previousOwner, address indexed newOwner, bool isLocal
     );
 
-    /// @notice Emmitted when the the StateBridge sends a root to the OPWorldID contract
+    /// @notice Emitted when the the StateBridge sends a root to the OPWorldID contract
     /// @param root The root sent to the OPWorldID contract on the OP Stack chain
     event RootPropagated(uint256 root);
 
-    /// @notice Emmitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
+    /// @notice Emitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
     /// @param rootHistoryExpiry The new root history expiry
     event SetRootHistoryExpiry(uint256 rootHistoryExpiry);
 
-    /// @notice Emmitted when the the StateBridge sets the gas limit for sendRootOp
+    /// @notice Emitted 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 Emitted when the the StateBridge sets the gas limit for SetRootHistoryExpiry
+    /// @param _opGasLimit The new opGasLimit for SetRootHistoryExpiry
     event SetGasLimitSetRootHistoryExpiry(uint32 _opGasLimit);
 
-    /// @notice Emmitted when the the StateBridge sets the gas limit for transferOwnershipOp
+    /// @notice Emitted when the the StateBridge sets the gas limit for transferOwnershipOp
     /// @param _opGasLimit The new opGasLimit for transferOwnershipOptimism
     event SetGasLimitTransferOwnershipOp(uint32 _opGasLimit);
 
@@ -78,6 +81,12 @@ contract OpStateBridge is Ownable2Step {
     /// @notice Emitted when an attempt is made to renounce ownership.
     error CannotRenounceOwnership();
 
+    /// @notice Emitted when an attempt is made to set the gas limit to zero or less
+    error GasLimitZero();
+
+    /// @notice Emitted when an attempt is made to set an address to zero
+    error AddressZero();
+
     ///////////////////////////////////////////////////////////////////
     ///                         CONSTRUCTOR                         ///
     ///////////////////////////////////////////////////////////////////
@@ -87,17 +96,25 @@ contract OpStateBridge is Ownable2Step {
     /// @param _opWorldIDAddress Address of the Optimism contract that will receive the new root and timestamp
     /// @param _crossDomainMessenger L1CrossDomainMessenger contract used to communicate with the desired OP
     /// Stack network
+    /// @custom:revert if any of the constructor params addresses are zero
     constructor(
         address _worldIDIdentityManager,
         address _opWorldIDAddress,
         address _crossDomainMessenger
     ) {
+        if (
+            _worldIDIdentityManager == address(0) || _opWorldIDAddress == address(0)
+                || _crossDomainMessenger == address(0)
+        ) {
+            revert AddressZero();
+        }
+
         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 +134,7 @@ contract OpStateBridge is Ownable2Step {
             // Contract address on the OP Stack Chain
             opWorldIDAddress,
             message,
-            _gasLimitSendRoot
+            _gasLimitPropagateRoot
         );
 
         emit RootPropagated(latestRoot);
@@ -127,7 +144,12 @@ contract OpStateBridge is Ownable2Step {
     /// of OpWorldID to another contract on L1 or to a local OP Stack chain EOA
     /// @param _owner new owner (EOA or contract)
     /// @param _isLocal true if new owner is on Optimism, false if it is a cross-domain owner
+    /// @custom:revert if _owner is set to the zero address
     function transferOwnershipOp(address _owner, bool _isLocal) external onlyOwner {
+        if (_owner == address(0)) {
+            revert AddressZero();
+        }
+
         // 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 +187,25 @@ 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 {
+        if (_opGasLimit <= 0) {
+            revert GasLimitZero();
+        }
+
+        _gasLimitPropagateRoot = _opGasLimit;
 
-        emit SetGasLimitSendRoot(_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 {
+        if (_opGasLimit <= 0) {
+            revert GasLimitZero();
+        }
+
         _gasLimitSetRootHistoryExpiry = _opGasLimit;
 
         emit SetGasLimitSetRootHistoryExpiry(_opGasLimit);
@@ -184,6 +214,10 @@ 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 {
+        if (_opGasLimit <= 0) {
+            revert GasLimitZero();
+        }
+
         _gasLimitTransferOwnership = _opGasLimit;
 
         emit SetGasLimitTransferOwnershipOp(_opGasLimit);

src/PolygonStateBridge.sol

@@ -25,11 +25,14 @@ contract PolygonStateBridge is FxBaseRootTunnel, Ownable2Step {
     ///                            EVENTS                           ///
     ///////////////////////////////////////////////////////////////////
 
-    /// @notice Emmitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
+    /// @notice Emitted when the the StateBridge sets the root history expiry for OpWorldID and PolygonWorldID
     /// @param rootHistoryExpiry The new root history expiry
     event SetRootHistoryExpiry(uint256 rootHistoryExpiry);
 
-    /// @notice Emmitted when a root is sent to PolygonWorldID
+    /// @notice Emitted when the owner calls setFxChildTunnel for the first time
+    event SetFxChildTunnel(address fxChildTunnel);
+
+    /// @notice Emitted when a root is sent to PolygonWorldID
     /// @param root The latest WorldID Identity Manager root.
     event RootPropagated(uint256 root);
 
@@ -40,6 +43,9 @@ contract PolygonStateBridge is FxBaseRootTunnel, Ownable2Step {
     /// @notice Emitted when an attempt is made to renounce ownership.
     error CannotRenounceOwnership();
 
+    /// @notice Emitted when an attempt is made to set the FxChildTunnel to the zero address.
+    error AddressZero();
+
     ///////////////////////////////////////////////////////////////////
     ///                         CONSTRUCTOR                         ///
     ///////////////////////////////////////////////////////////////////
@@ -48,9 +54,17 @@ contract PolygonStateBridge is FxBaseRootTunnel, Ownable2Step {
     /// @param _checkpointManager address of the checkpoint manager contract
     /// @param _fxRoot address of Polygon's fxRoot contract, part of the FxPortal bridge (Goerli or Mainnet)
     /// @param _worldIDIdentityManager Deployment address of the WorldID Identity Manager contract
+    /// @custom:reverts AddressZero If any of the constructor arguments are the zero address
     constructor(address _checkpointManager, address _fxRoot, address _worldIDIdentityManager)
         FxBaseRootTunnel(_checkpointManager, _fxRoot)
     {
+        if (
+            _checkpointManager == address(0) || _fxRoot == address(0)
+                || _worldIDIdentityManager == address(0)
+        ) {
+            revert AddressZero();
+        }
+
         worldID = IWorldIDIdentityManager(_worldIDIdentityManager);
     }
 
@@ -99,9 +113,17 @@ contract PolygonStateBridge is FxBaseRootTunnel, Ownable2Step {
     /// @param _fxChildTunnel The address of the child (non-L1) tunnel contract.
     ///
     /// @custom:reverts string If the root tunnel has already been set.
+    /// @custom:reverts AddressZero If the `_fxChildTunnel` is the zero address.
     function setFxChildTunnel(address _fxChildTunnel) public virtual override onlyOwner {
         require(fxChildTunnel == address(0x0), "FxBaseRootTunnel: CHILD_TUNNEL_ALREADY_SET");
+
+        if (_fxChildTunnel == address(0x0)) {
+            revert AddressZero();
+        }
+
         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                           ///
     ///////////////////////////////////////////////////////////////////
@@ -40,6 +44,9 @@ contract PolygonWorldID is WorldIDBridge, FxBaseChildTunnel, Ownable2Step {
     /// @notice Emitted when an attempt is made to renounce ownership.
     error CannotRenounceOwnership();
 
+    /// @notice Emitted when an attempt is made to set the FxChildTunnel to the zero address.
+    error AddressZero();
+
     ///////////////////////////////////////////////////////////////////////////////
     ///                                CONSTRUCTION                             ///
     ///////////////////////////////////////////////////////////////////////////////
@@ -52,7 +59,11 @@ contract PolygonWorldID is WorldIDBridge, FxBaseChildTunnel, Ownable2Step {
     constructor(uint8 _treeDepth, address _fxChild)
         WorldIDBridge(_treeDepth)
         FxBaseChildTunnel(_fxChild)
-    {}
+    {
+        if (address(_fxChild) == address(0)) {
+            revert AddressZero();
+        }
+    }
 
     ///////////////////////////////////////////////////////////////////////////////
     ///                               ROOT MIRRORING                            ///
@@ -63,6 +74,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.
@@ -114,7 +128,14 @@ contract PolygonWorldID is WorldIDBridge, FxBaseChildTunnel, Ownable2Step {
     /// @custom:reverts string If the root tunnel has already been set.
     function setFxRootTunnel(address _fxRootTunnel) external virtual override onlyOwner {
         require(fxRootTunnel == address(0x0), "FxBaseChildTunnel: ROOT_TUNNEL_ALREADY_SET");
+
+        if (_fxRootTunnel == address(0x0)) {
+            revert AddressZero();
+        }
+
         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;
@@ -106,7 +104,6 @@ abstract contract WorldIDBridge is IWorldID {
     /// @param newRoot The value of the new root.
     ///
     /// @custom:reverts CannotOverwriteRoot If the root already exists in the root history.
-    /// @custom:reverts string If the caller is not the owner.
     function _receiveRoot(uint256 newRoot) internal {
         uint256 existingTimestamp = rootHistory[newRoot];
 
@@ -158,7 +155,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