From 88c0bdccd8e36f6acfb3d617d2d0c4d73de674c7 Mon Sep 17 00:00:00 2001
From: spengrah <code@spengrah.xyz>
Date: Fri, 16 Feb 2024 09:16:12 -0600
Subject: [PATCH] feat(HatsGatekeepers): add zero-address check to
 `setMaciInstance()`

Prevents MACI address from being set to the zero address
---
 .../hatsGatekeepers/HatsGatekeeperBase.sol    | 12 +++++++-----
 contracts/tests/HatsGatekeeper.test.ts        | 19 +++++++++++++++++--
 2 files changed, 24 insertions(+), 7 deletions(-)

diff --git a/contracts/contracts/gatekeepers/hatsGatekeepers/HatsGatekeeperBase.sol b/contracts/contracts/gatekeepers/hatsGatekeepers/HatsGatekeeperBase.sol
index 85ba7c926b..81685e158c 100644
--- a/contracts/contracts/gatekeepers/hatsGatekeepers/HatsGatekeeperBase.sol
+++ b/contracts/contracts/gatekeepers/hatsGatekeepers/HatsGatekeeperBase.sol
@@ -10,22 +10,23 @@ import { IHats } from "../../interfaces/IHats.sol";
 abstract contract HatsGatekeeperBase is SignUpGatekeeper, Ownable {
   /*//////////////////////////////////////////////////////////////
                               CUSTOM ERRORS
-  //////////////////////////////////////////////////////////////*/
+    //////////////////////////////////////////////////////////////*/
 
   error OnlyMACI();
   error NotWearingCriterionHat();
   error AlreadyRegistered();
+  error ZeroAddress();
 
   /*//////////////////////////////////////////////////////////////
                             PUBLIC CONSTANTS
-  //////////////////////////////////////////////////////////////*/
+    //////////////////////////////////////////////////////////////*/
 
   /// @notice The Hats Protocol contract address
   IHats public immutable hats;
 
   /*//////////////////////////////////////////////////////////////
                           PUBLIC STATE VARIABLES
-  //////////////////////////////////////////////////////////////*/
+    //////////////////////////////////////////////////////////////*/
 
   /// @notice the reference to the MACI contract
   address public maci;
@@ -35,7 +36,7 @@ abstract contract HatsGatekeeperBase is SignUpGatekeeper, Ownable {
 
   /*//////////////////////////////////////////////////////////////
                               CONSTRUCTOR
-  //////////////////////////////////////////////////////////////*/
+    //////////////////////////////////////////////////////////////*/
 
   /// @notice Deploy an instance of HatsGatekeeper
   /// @param _hats The Hats Protocol contract
@@ -45,11 +46,12 @@ abstract contract HatsGatekeeperBase is SignUpGatekeeper, Ownable {
 
   /*//////////////////////////////////////////////////////////////
                             OWNER FUNCTIONS
-  //////////////////////////////////////////////////////////////*/
+    //////////////////////////////////////////////////////////////*/
 
   /// @notice Allows to set the MACI contract
   /// @param _maci The MACI contract interface to be stored
   function setMaciInstance(address _maci) public override onlyOwner {
+    if (_maci == address(0)) revert ZeroAddress();
     maci = _maci;
   }
 }
diff --git a/contracts/tests/HatsGatekeeper.test.ts b/contracts/tests/HatsGatekeeper.test.ts
index d6fd203129..f8eeaaca18 100644
--- a/contracts/tests/HatsGatekeeper.test.ts
+++ b/contracts/tests/HatsGatekeeper.test.ts
@@ -29,6 +29,7 @@ describe("HatsProtocol Gatekeeper", () => {
   const hatsContractOP = "0x3bc1A0Ad72417f2d411118085256fC53CBdDd137";
 
   const user = new Keypair();
+  const oneAddress = "0x0000000000000000000000000000000000000001";
 
   let topHat: bigint;
   let hatId: bigint;
@@ -138,6 +139,13 @@ describe("HatsProtocol Gatekeeper", () => {
         expect(await hatsGatekeeperSingle.maci()).to.eq(maciAddress);
       });
 
+      it("should fail to set MACI instance to the zero address", async () => {
+        await expect(hatsGatekeeperSingle.setMaciInstance(ZeroAddress)).to.be.revertedWithCustomError(
+          hatsGatekeeperSingle,
+          "ZeroAddress",
+        );
+      });
+
       it("should fail to set MACI instance when the caller is not the owner", async () => {
         await expect(hatsGatekeeperSingle.connect(voter).setMaciInstance(signerAddress)).to.be.revertedWith(
           "Ownable: caller is not the owner",
@@ -147,7 +155,7 @@ describe("HatsProtocol Gatekeeper", () => {
 
     describe("register", () => {
       it("should not allow to call from a non-registered MACI contract", async () => {
-        await hatsGatekeeperSingle.setMaciInstance(ZeroAddress);
+        await hatsGatekeeperSingle.setMaciInstance(oneAddress);
         await expect(
           maciContract
             .connect(signer)
@@ -233,6 +241,13 @@ describe("HatsProtocol Gatekeeper", () => {
         expect(await hatsGatekeeperMultiple.maci()).to.eq(maciAddress);
       });
 
+      it("should fail to set MACI instance to the zero address", async () => {
+        await expect(hatsGatekeeperMultiple.setMaciInstance(ZeroAddress)).to.be.revertedWithCustomError(
+          hatsGatekeeperMultiple,
+          "ZeroAddress",
+        );
+      });
+
       it("should fail to set MACI instance when the caller is not the owner", async () => {
         await expect(hatsGatekeeperMultiple.connect(voter).setMaciInstance(signerAddress)).to.be.revertedWith(
           "Ownable: caller is not the owner",
@@ -242,7 +257,7 @@ describe("HatsProtocol Gatekeeper", () => {
 
     describe("register", () => {
       it("should not allow to call from a non-registered MACI contract", async () => {
-        await hatsGatekeeperMultiple.connect(signer).setMaciInstance(ZeroAddress);
+        await hatsGatekeeperMultiple.connect(signer).setMaciInstance(oneAddress);
         await expect(
           maciContract
             .connect(signer)