# NM-0131 WorldCoin - INTERNAL NOTES

**Repo** https://github.com/worldcoin/world-id-state-bridge/tree/9edc22a94d38870e175c14d0058c4b15bf09bc35

**Commit**: 9edc22a94d38870e175c14d0058c4b15bf09bc35

---

### [Medium] Wrong state variable is set in the function `setGasLimitSendRoot(...)`

**File(s)**: [`OpStateBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/OpStateBridge.sol#L171)

**Description**: The `OpStateBridge` contract allows the owner to customize the gas limit for different methods. The value of `_gasLimitSendRoot` can be set by calling the function `setGasLimitSendRoot(...)`. However, in the current codebase, it sets the value for the wrong variable `_gasLimitSetRootHistoryExpiry` instead.

function setGasLimitSendRoot(uint32 _opGasLimit) external onlyOwner {
    // @audit Should set _gasLimitSendRoot instead of _gasLimitSetRootHistoryExpiry
    _gasLimitSetRootHistoryExpiry = _opGasLimit; 

    emit SetGasLimitSendRoot(_opGasLimit);
}

**Recommendation(s)**: In the function `setGasLimitSendRoot(...)`, set the value for `_gasLimitSendRoot` instead of `_gasLimitSetRootHistoryExpiry`.

**Status**: Fixed

**Update from the client**: Fixed, [current state](https://github.com/worldcoin/world-id-state-bridge/blob/fd56d7b1096143f82e1f5e356422e12c63517b1a/src/OpStateBridge.sol#L192-L200).

---

### [Best Practices] Avoid using magic numbers

**File(s)**: [`OpStateBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/OpStateBridge.sol#L98), [`PolygonWorldID.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/PolygonWorldID.sol#L80)

**Description**: Enhancing code readability can be achieved by replacing magic numbers with meaningful constants. The code snippet below shows an example:

constructor(
    address _worldIDIdentityManager,
    address _opWorldIDAddress,
    address _crossDomainMessenger
) {
    opWorldIDAddress = _opWorldIDAddress;
    worldIDAddress = _worldIDIdentityManager;
    crossDomainMessengerAddress = _crossDomainMessenger;
    // @audit Avoid using magic number
    _gasLimitSendRoot = 100000; 
    _gasLimitSetRootHistoryExpiry = 100000;
    _gasLimitTransferOwnership = 100000;
}

**Recommendation(s)**: Consider replacing these magic numbers with named constants or variables that provide meaningful names and context for these values.

**Status**: Fixed

**Update from the client**: Fixed in commits [364826209](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/36482620947101eb2fef37995f962075f2c27f9a), [33df6b53](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/33df6b53fde9f542b81716dd87ce9557ab7ceb63)

---

### [Best Practices] Lack of event emission when updating state variables

**File(s)**: [`PolygonStateBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/PolygonStateBridge.sol#L104), [`PolygonWorldID.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/PolygonWorldID.sol#L115)

**Description**: It is best practice to emit an event when changing important state variables. The code snippet below shows an example where the function sets the address for `fxRootTunnel`, but does not emit an event.

function setFxRootTunnel(address _fxRootTunnel) external virtual override onlyOwner {
    require(fxRootTunnel == address(0x0), "FxBaseChildTunnel: ROOT_TUNNEL_ALREADY_SET");
    // @audit Should emit event
    fxRootTunnel = _fxRootTunnel; 
}

**Recommendation(s)**: Consider emitting a suitable event when updating state variables.

**Status**: Fixed

**Update from the client**: Fixed in commits [33df6b53](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/33df6b53fde9f542b81716dd87ce9557ab7ceb63), [c3c25679](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/c3c256793ec086a748765370c81574228cc65d5f)

---

### [Best Practices] Inconsistent usage of custom errors

**File(s)**: [`PolygonStateBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/PolygonStateBridge.sol), [`PolygonWorldID.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/PolygonWorldID.sol)

**Description**: Within these contract, both `require()/revert()` and custom errors are employed. It is recommended to adopt a consistent approach and use only one error-handling method within each file.

**Recommendation(s)**: Consider adopting a uniform error-handling method throughout the codebase.

**Status**: Fixed

**Update from the client**: Fixed in commit [52f2c77](https://github.com/worldcoin/world-id-state-bridge/pull/108/commits/52f2c776ee18c23595ad205aec693b0b986b2438)

---

### [Best Practices] Missing input validation in some functions

**File(s)**: [`OpStateBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/OpStateBridge.sol), [`PolygonStateBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/PolygonStateBridge.sol), [`PolygonWorldID.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/PolygonWorldID.sol)

**Description**: Some functions do not check for the validity of their input before utilizing them:

- `OpStateBridge.transferOwnershipOp(...)` does not check if `_owner` is different than `address(0)`. Note that a check is [implemented](https://github.com/ethereum-optimism/optimism/blob/2f745cb53cfeb94e4fa0d9760232415c2ebacd80/packages/contracts-bedrock/src/L2/CrossDomainOwnable3.sol#L29) in `transferOwnership` function of `CrossDomainOwnable3` contract, but it can be endorsed at the protocol contract level.
- gas limit setters (`setGasLimitSendRoot(...)`,`setGasLimitSetRootHistoryExpiry(...)`,`setGasLimitTransferOwnershipOp(...)` .. ) does not check for the validity of the gas limit (i.e., whether it differs from 0).
- `PolygonStateBridge` constructor function lacks validation for inputs `_checkpointManager` `_fxRoot` and `_worldIDIdentityManager`, allowing them to be set as `address(0)`.
- `PolygonWorldID` contract constructor lacks validation of `_fxChild` address.

It is a good practice to include input validation in order to enhance code robustness and prevent unexpected errors.

**Recommendation(s)**: We recommend implementing a check for inputs in different functions.

**Status**: Fixed

**Update from the client**: Fixed in commits [c3c25679](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/c3c256793ec086a748765370c81574228cc65d5f), [0c08228](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/0c08228457efbd652d01635da3bef73cd656b5aa)

---

### [Best Practices] Remove the testing library from the code base

**File(s)**: [`WorldIDBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/abstract/WorldIDBridge.sol#L9)

**Description**: The `WorldIdBridge` abstract contract imports the `forge-std` console library.

import "forge-std/console.sol";

**Recommendation(s)**: Consider removing the testing library in the production code base.

**Status**: Fixed

**Update from the client**: Fixed in commit [88beb2e](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/88beb2ed4b8a2a424abb25b9f0597056d765de43)

---

### [Best Practices] Incorrect NatSpec description in `_receiveRoot(...)` function

**File(s)**: [`WorldIDBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/abstract/WorldIDBridge.sol#L109)

**Description**: The `_receiveRoot(...)` function contains incorrect custom NatSpec. It specifies that the function may revert if the caller is not the owner. However, the function does not have this logic.

/// @custom:reverts string If the caller is not the owner.
function _receiveRoot(uint256 newRoot) internal {
[...]
}

**Recommendation(s)**: Consider correcting the NatSpec documentation to reflect the function's implementation.

**Status**: Fixed

**Update from the client**: Fixed in commit [844da90](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/844da908d748f2bff4d37aba5ea9f9a5689560eb)

---

### [Best Practices] Typos in the code base

**File(s)**: [`WorldIDBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/abstract/WorldIDBridge.sol#L161), [`OpStateBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/OpStateBridge.sol#L66-L67)

**Description**: The code base contains the following typos:
- The NatSpec documentation from the `verifyProof(...)` function of the `WorldIDBridge` contract.

/// @param root The of the Merkle tree

- The NatSpec documentation from the `OpStateBridge` contract:

/// @notice Emmitted when the the StateBridge sets the gas limit for SetRootHistoryExpiryt
/// @param _opGasLimit The new opGasLimit for SetRootHistoryExpirytimism

**Recommendation(s)**: Consider correcting the typos to improve code quality.

**Status**: Fixed

**Update from the client**: Fixed in commits [d89e337](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/d89e337b5c75188fc2bd69999ea5738eb5f80736), [399dec2](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/399dec2d8048cc84333fdfa855ca41371c6ee0b2), [e986574](https://github.com/worldcoin/world-id-state-bridge/pull/107/commits/e986574f41d10f132723e682cdb6e7535ea1c03c)

---

### [Best Practices] Optimization of `WorldIdBridge`

**File(s)**: [`WorldIDBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/abstract/WorldIDBridge.sol)

**Description**: The `treeDepth` state variable from the `WorldIDBridge` abstract contract is never updated. This value is only set within the `constructor`.

/// @notice Constructs a new instance of the state bridge.
///
/// @param _treeDepth The depth of the identities merkle tree.
constructor(uint8 _treeDepth) {
    if (!SemaphoreTreeDepthValidator.validate(_treeDepth)) {
        revert UnsupportedTreeDepth(_treeDepth);
    }

    treeDepth = _treeDepth;
}

**Recommendation(s)**: Gas optimization can be done following these steps:
- change `treedepth` to an immutable variable (initialization in constructor)
- change type of `getTreeDepth()` function to `pure`

**Status**: Fixed

**Update from the client**: `getTreeDepth()` needs to remain a `view` function for the contract to compile. The solidity compiler considers immutable variables to be non-pure because it is possible to initialize them with some state [link](https://docs.soliditylang.org/en/latest/contracts.html#pure-functions). Fixed in commits [12d09df](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/12d09dfea213785435dcbb3b1b267f7818fdf489), [8e0e5b4](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/8e0e5b4b4273abebf2974a1aefce7831ef4337f4)

---

### [Best Practices] Remove unused imports

**File(s)**: [`PolygonWorldID.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/PolygonWorldID.sol#L7)

**Description**: `PolygonWorldID` contract has the following imports that are not being used.

import {SemaphoreTreeDepthValidator} from "./utils/SemaphoreTreeDepthValidator.sol";
import {SemaphoreVerifier} from "semaphore/base/SemaphoreVerifier.sol";

**Recommendation(s)**: Consider removing unused imports from the contract.

**Status**: Fixed

**Update from the client**: Fixed in commit [33df6b5](https://github.com/worldcoin/world-id-state-bridge/pull/106/commits/33df6b53fde9f542b81716dd87ce9557ab7ceb63)

---

### [Info] Transaction ordering in L2 may leave `_latestRoot` in the wrong state after root hash expiration

**File(s)**: [`WorldIDBridge.sol`](https://github.com/worldcoin/world-id-state-bridge/blob/9edc22a94d38870e175c14d0058c4b15bf09bc35/src/abstract/WorldIDBridge.sol#L135)

**Description**: The propagation of `latestRoot` between L1 and L2 is done via messaging. The message data contains the new root hash but does not include the timestamp indicating the time of emission from L1.

Depending on the different L1-L2 messaging mechanisms, the ordering of transactions cannot be ensured, and it could be possible that two messages sent from L1 to L2 to propagate root get validated in the reverse order on L2.

This scenario has a limited impact since the user on L2 would just need to specify the root hash that contains its identity.

However, there is an edge case when the root hash is not updated and expiration time (`ROOT_HISTORY_EXPIRY`) is reached for all root hashes. The only valid root hash is the latest one (`_latestRoot`) which could be the second to last one sent from L1.

function requireValidRoot(uint256 root) internal view {
    // The latest root is always valid.
    if (root == _latestRoot) {
        return;
    }

All users registered between these two root hash propagations would not be able to create valid proofs on L2 until a new root hash is propagated.

**Recommendation(s)**: Consider sending the timestamp from L1 to ensure the latest root hash is correctly updated.

**Status**: Acknowledged

**Update from the client**: Acknowledged, ordering of transactions doesn't impact the correct functioning of bridged World IDs. The [`signup-sequencer`](https://github.com/worldcoin/signup-sequencer/) is the one that takes care providing roots against the latest root on the L2, which solves the edge case mentioned above.