fix: [audit] ZNS-12 Missing input validation (#65)

zer0-os · Oct 31, 2023 · e2c258b · e2c258b
2 parents 4824679 + 8390856
commit e2c258b
Show file tree

Hide file tree

Showing 7 changed files with 376 additions and 336 deletions.
diff --git a/contracts/access/ZNSAccessController.sol b/contracts/access/ZNSAccessController.sol
@@ -77,6 +77,10 @@ contract ZNSAccessController is AccessControl, ZNSRoles, IZNSAccessController {
     function _grantRoleToMany(bytes32 role, address[] memory addresses) internal {
         uint256 length = addresses.length;
         for (uint256 i = 0; i < length; ++i) {
+            require(
+                addresses[i] != address(0),
+                "ZNSAccessController: Can't grant role to zero address"
+            );
             _grantRole(role, addresses[i]);
         }
     }

diff --git a/contracts/price/ZNSCurvePricer.sol b/contracts/price/ZNSCurvePricer.sol
diff --git a/contracts/price/ZNSFixedPricer.sol b/contracts/price/ZNSFixedPricer.sol
@@ -140,6 +140,11 @@ contract ZNSFixedPricer is AAccessControlled, ARegistryWired, UUPSUpgradeable, I
      * @param feePercentage The new feePercentage
      */
     function _setFeePercentage(bytes32 domainHash, uint256 feePercentage) internal {
+        require(
+            feePercentage <= PERCENTAGE_BASIS,
+            "ZNSFixedPricer: feePercentage cannot be greater than PERCENTAGE_BASIS"
+        );
+
         priceConfigs[domainHash].feePercentage = feePercentage;
         emit FeePercentageSet(domainHash, feePercentage);
     }

diff --git a/contracts/registry/ZNSRegistry.sol b/contracts/registry/ZNSRegistry.sol
@@ -59,7 +59,7 @@ contract ZNSRegistry is AAccessControlled, UUPSUpgradeable, IZNSRegistry {
     /**
      * @notice Initializer for the `ZNSRegistry` proxy.
      * @param accessController_ The address of the `ZNSAccessController` contract
-     * @dev ! The owner of the 0x0 hash should be a multisig !
+     * @dev ! The owner of the 0x0 hash should be a multisig ideally, but EOA can be used to deploy !
      * > Admin account deploying the contract will be the owner of the 0x0 hash !
      */
     function initialize(address accessController_) external override initializer {

diff --git a/test/ZNSAccessController.test.ts b/test/ZNSAccessController.test.ts
@@ -5,6 +5,7 @@ import { deployAccessController } from "./helpers";
 import { expect } from "chai";
 import { ADMIN_ROLE, GOVERNOR_ROLE, EXECUTOR_ROLE, REGISTRAR_ROLE } from "./helpers/access";
 import { getAccessRevertMsg } from "./helpers/errors";
+import { ethers } from "hardhat";
 
 
 describe("ZNSAccessController", () => {
@@ -46,6 +47,16 @@ describe("ZNSAccessController", () => {
         }, Promise.resolve()
       );
     });
+
+    it("Should revert when passing 0x0 address to assing roles", async () => {
+      await expect(
+        deployAccessController({
+          deployer,
+          governorAddresses: [ ethers.constants.AddressZero ],
+          adminAddresses: [ ethers.constants.AddressZero ],
+        })
+      ).to.be.revertedWith("ZNSAccessController: Can't grant role to zero address");
+    });
   });
 
   describe("Role Management from the Initial Setup", () => {

diff --git a/test/ZNSCurvePricer.test.ts b/test/ZNSCurvePricer.test.ts
@@ -848,7 +848,7 @@ describe("ZNSCurvePricer", () => {
     });
   });
 
-  describe("#setRegistrationFeePercentage", () => {
+  describe("#setFeePercentage", () => {
     it("Successfully sets the fee percentage", async () => {
       const newFeePerc = BigNumber.from(222);
       await zns.curvePricer.connect(user).setFeePercentage(domainHash, newFeePerc);
@@ -863,6 +863,13 @@ describe("ZNSCurvePricer", () => {
         .setFeePercentage(domainHash, newFeePerc);
       await expect(tx).to.be.revertedWith(NOT_AUTHORIZED_REG_WIRED_ERR);
     });
+
+    it("should revert when trying to set feePercentage higher than PERCENTAGE_BASIS", async () => {
+      const newFeePerc = BigNumber.from(10001);
+      await expect(
+        zns.curvePricer.connect(user).setFeePercentage(domainHash, newFeePerc)
+      ).to.be.revertedWith("ZNSCurvePricer: feePercentage cannot be greater than PERCENTAGE_BASIS");
+    });
   });
 
   describe("#getRegistrationFee", () => {

diff --git a/test/ZNSFixedPricer.test.ts b/test/ZNSFixedPricer.test.ts
@@ -205,6 +205,14 @@ describe("ZNSFixedPricer", () => {
     );
   });
 
+  it("#setFeePercentage() should revert when trying to set feePercentage higher than PERCENTAGE_BASIS", async () => {
+    await expect(
+      zns.fixedPricer.connect(user).setFeePercentage(domainHash, PERCENTAGE_BASIS.add(1))
+    ).to.be.revertedWith(
+      "ZNSFixedPricer: feePercentage cannot be greater than PERCENTAGE_BASIS"
+    );
+  });
+
   // eslint-disable-next-line max-len
   it("#setPriceConfig() should set the price config correctly and emit #PriceSet and #FeePercentageSet events", async () => {
     const newPrice = ethers.utils.parseEther("1823");