[Fix] Add more checks to TokenBridge.initialize() (#380)

* added new tests covering TokenBridge.initialise() changes

* ran npx hardhat docgen

* update TokenBridge dynamic artifact

* test verified commit

* reverse verified commit test

* added nonZeroChainId modifier

* Update contracts/contracts/tokenBridge/interfaces/ITokenBridge.sol

Co-authored-by: The Dark Jester <thedarkjester@users.noreply.github.com>
Signed-off-by: kyzooghost <73516204+kyzooghost@users.noreply.github.com>

* update ITokenBridge doc

* add initialization check for default admin

---------

Signed-off-by: kyzooghost <73516204+kyzooghost@users.noreply.github.com>
Co-authored-by: The Dark Jester <thedarkjester@users.noreply.github.com>
Co-authored-by: thedarkjester <grant.southey@consensys.net>

contracts/contracts/lib/TokenBridgePauseManager.sol

@@ -9,8 +9,15 @@ import { PauseManager } from "./PauseManager.sol";
  * @custom:security-contact security-report@linea.build
  */
 abstract contract TokenBridgePauseManager is PauseManager {
+  /// @notice This is used to pause token bridging initiation.
   bytes32 public constant PAUSE_INITIATE_TOKEN_BRIDGING_ROLE = keccak256("PAUSE_INITIATE_TOKEN_BRIDGING_ROLE");
+
+  /// @notice This is used to unpause token bridging initiation.
   bytes32 public constant UNPAUSE_INITIATE_TOKEN_BRIDGING_ROLE = keccak256("UNPAUSE_INITIATE_TOKEN_BRIDGING_ROLE");
+
+  /// @notice This is used to pause token bridging completion.
   bytes32 public constant PAUSE_COMPLETE_TOKEN_BRIDGING_ROLE = keccak256("PAUSE_COMPLETE_TOKEN_BRIDGING_ROLE");
+
+  /// @notice This is used to unpause token bridging completion.
   bytes32 public constant UNPAUSE_COMPLETE_TOKEN_BRIDGING_ROLE = keccak256("UNPAUSE_COMPLETE_TOKEN_BRIDGING_ROLE");
 }

contracts/contracts/tokenBridge/TokenBridge.sol

@@ -113,6 +113,7 @@ contract TokenBridge is
     if (_addr == EMPTY) revert ZeroAddressNotAllowed();
     _;
   }
+
   /**
    * @dev Ensures the amount is not 0.
    * @param _amount amount to check.
@@ -122,6 +123,15 @@ contract TokenBridge is
     _;
   }
 
+  /**
+   * @dev Ensures the chainId is not 0.
+   * @param _chainId chainId to check.
+   */
+  modifier nonZeroChainId(uint256 _chainId) {
+    if (_chainId == 0) revert ZeroChainIdNotAllowed();
+    _;
+  }
+
   /// @dev Disable constructor for safety
   /// @custom:oz-upgrades-unsafe-allow constructor
   constructor() {
@@ -139,12 +149,18 @@ contract TokenBridge is
     external
     nonZeroAddress(_initializationData.messageService)
     nonZeroAddress(_initializationData.tokenBeacon)
+    nonZeroChainId(_initializationData.sourceChainId)
+    nonZeroChainId(_initializationData.targetChainId)
     initializer
   {
     __PauseManager_init(_initializationData.pauseTypeRoles, _initializationData.unpauseTypeRoles);
     __MessageServiceBase_init(_initializationData.messageService);
     __ReentrancyGuard_init();
 
+    if (_initializationData.defaultAdmin == address(0)) {
+      revert ZeroAddressNotAllowed();
+    }
+
     /**
      * @dev DEFAULT_ADMIN_ROLE is set for the security council explicitly,
      * as the permissions init purposefully does not allow DEFAULT_ADMIN_ROLE to be set.
@@ -154,6 +170,7 @@ contract TokenBridge is
     __Permissions_init(_initializationData.roleAddresses);
 
     tokenBeacon = _initializationData.tokenBeacon;
+    if (_initializationData.sourceChainId == _initializationData.targetChainId) revert SourceChainSameAsTargetChain();
     sourceChainId = _initializationData.sourceChainId;
     targetChainId = _initializationData.targetChainId;
 

contracts/contracts/tokenBridge/interfaces/ITokenBridge.sol

@@ -219,6 +219,16 @@ interface ITokenBridge {
    */
   error TokenListEmpty();
 
+  /**
+   * @dev Thrown when a chainId provided during initialization is zero.
+   */
+  error ZeroChainIdNotAllowed();
+
+  /**
+   * @dev Thrown when sourceChainId is the same as targetChainId during initialization.
+   */
+  error SourceChainSameAsTargetChain();
+
   /**
    * @notice Similar to `bridgeToken` function but allows to pass additional
    *   permit data to do the ERC20 approval in a single transaction.

contracts/docs/api/lib/CallForwardingProxy.md

@@ -0,0 +1,26 @@
+# Solidity API
+
+## CallForwardingProxy
+
+### target
+
+```solidity
+address target
+```
+
+The underlying target address that is called.
+
+### constructor
+
+```solidity
+constructor(address _target) public
+```
+
+### fallback
+
+```solidity
+fallback() external payable
+```
+
+Defaults to, and forwards all calls to the target address.
+

contracts/docs/api/lib/TokenBridgePauseManager.mdx

@@ -6,21 +6,29 @@
 bytes32 PAUSE_INITIATE_TOKEN_BRIDGING_ROLE
 ```
 
+This is used to pause token bridging initiation.
+
 ### UNPAUSE_INITIATE_TOKEN_BRIDGING_ROLE
 
 ```solidity
 bytes32 UNPAUSE_INITIATE_TOKEN_BRIDGING_ROLE
 ```
 
+This is used to unpause token bridging initiation.
+
 ### PAUSE_COMPLETE_TOKEN_BRIDGING_ROLE
 
 ```solidity
 bytes32 PAUSE_COMPLETE_TOKEN_BRIDGING_ROLE
 ```
 
+This is used to pause token bridging completion.
+
 ### UNPAUSE_COMPLETE_TOKEN_BRIDGING_ROLE
 
 ```solidity
 bytes32 UNPAUSE_COMPLETE_TOKEN_BRIDGING_ROLE
 ```
 
+This is used to unpause token bridging completion.
+

contracts/docs/api/tokenBridge/TokenBridge.mdx

@@ -166,6 +166,20 @@ _Ensures the amount is not 0._
 | ---- | ---- | ----------- |
 | _amount | uint256 | amount to check. |
 
+### nonZeroChainId
+
+```solidity
+modifier nonZeroChainId(uint256 _chainId)
+```
+
+_Ensures the chainId is not 0._
+
+#### Parameters
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| _chainId | uint256 | chainId to check. |
+
 ### constructor
 
 ```solidity

contracts/docs/api/tokenBridge/interfaces/ITokenBridge.mdx

@@ -337,6 +337,22 @@ error TokenListEmpty()
 
 _Thrown when the token list is empty._
 
+### ZeroChainIdNotAllowed
+
+```solidity
+error ZeroChainIdNotAllowed()
+```
+
+_Thrown when a chainId provided during initialization is zero._
+
+### SourceChainSameAsTargetChain
+
+```solidity
+error SourceChainSameAsTargetChain()
+```
+
+_Thrown when sourceChainId is the same as targetChainId during initialization._
+
 ### bridgeTokenWithPermit
 
 ```solidity

contracts/test/tokenBridge/TokenBridge.ts

@@ -175,6 +175,94 @@ describe("TokenBridge", function () {
         "ZeroAddressNotAllowed",
       );
     });
+
+    it("Should revert if one of the initializing parameters is chainId 0", async function () {
+      const { chainIds } = await loadFixture(deployContractsFixture);
+      const TokenBridge = await ethers.getContractFactory("TokenBridge");
+
+      await expectRevertWithCustomError(
+        TokenBridge,
+        upgrades.deployProxy(TokenBridge, [
+          {
+            defaultAdmin: PLACEHOLDER_ADDRESS,
+            messageService: PLACEHOLDER_ADDRESS,
+            tokenBeacon: PLACEHOLDER_ADDRESS,
+            sourceChainId: 0,
+            targetChainId: chainIds[1],
+            reservedTokens: [],
+            roleAddresses: [],
+            pauseTypeRoles: [],
+            unpauseTypeRoles: [],
+          },
+        ]),
+        "ZeroChainIdNotAllowed",
+      );
+
+      await expectRevertWithCustomError(
+        TokenBridge,
+        upgrades.deployProxy(TokenBridge, [
+          {
+            defaultAdmin: PLACEHOLDER_ADDRESS,
+            messageService: PLACEHOLDER_ADDRESS,
+            tokenBeacon: PLACEHOLDER_ADDRESS,
+            sourceChainId: chainIds[0],
+            targetChainId: 0,
+            reservedTokens: [],
+            roleAddresses: [],
+            pauseTypeRoles: [],
+            unpauseTypeRoles: [],
+          },
+        ]),
+        "ZeroChainIdNotAllowed",
+      );
+    });
+
+    it("Should revert if the sourceChainId is the same as the targetChainId", async function () {
+      const { chainIds } = await loadFixture(deployContractsFixture);
+      const TokenBridge = await ethers.getContractFactory("TokenBridge");
+
+      await expectRevertWithCustomError(
+        TokenBridge,
+        upgrades.deployProxy(TokenBridge, [
+          {
+            defaultAdmin: PLACEHOLDER_ADDRESS,
+            messageService: PLACEHOLDER_ADDRESS,
+            tokenBeacon: PLACEHOLDER_ADDRESS,
+            sourceChainId: chainIds[0],
+            targetChainId: chainIds[0],
+            reservedTokens: [],
+            roleAddresses: [],
+            pauseTypeRoles: [],
+            unpauseTypeRoles: [],
+          },
+        ]),
+        "SourceChainSameAsTargetChain",
+      );
+    });
+
+    it("Should revert if the default admin is empty", async function () {
+      const { chainIds } = await loadFixture(deployContractsFixture);
+      const TokenBridge = await ethers.getContractFactory("TokenBridge");
+
+      await expectRevertWithCustomError(
+        TokenBridge,
+        upgrades.deployProxy(TokenBridge, [
+          {
+            defaultAdmin: ADDRESS_ZERO,
+            messageService: PLACEHOLDER_ADDRESS,
+            tokenBeacon: PLACEHOLDER_ADDRESS,
+            sourceChainId: chainIds[0],
+            targetChainId: chainIds[1],
+            reservedTokens: [],
+            roleAddresses: [],
+            pauseTypeRoles: [],
+            unpauseTypeRoles: [],
+          },
+        ]),
+        "ZeroAddressNotAllowed",
+      );
+    });
+
     it("Should return 'NOT_VALID_ENCODING' for invalid data in _returnDataToString", async function () {
       const TestTokenBridgeFactory = await ethers.getContractFactory("TestTokenBridge");