refactor: use custom error for onlyOwner and address(0) check (#236)

* refactor: use custom error for `address(0)` check

* test: update tests for custom error

* refactor: use custom error for `onlyOwner` check

* ci: add different solc compiler versions to test in CI

* build!: increase min pragma to `0.8.4` for contracts using custom `error`

* fix!: set min pragma to `0.8.5` because of `bytes -> bytesN` conversion feature

* test: add tests for new custom error

.github/workflows/solc_version.yml

@@ -17,16 +17,23 @@ jobs:
     strategy:
       matrix:
         solc: [
+            "0.8.5",
+            "0.8.6",
+            "0.8.7",
             "0.8.8",
             "0.8.9",
-            # "0.8.10" skipped as default in hardhat.config.ts
+            "0.8.10",
             "0.8.11",
             "0.8.12",
             "0.8.13",
             "0.8.14",
             "0.8.15",
             "0.8.16",
-            "0.8.17",
+            # "0.8.17", # skipped as default in hardhat.config.ts
+            "0.8.18",
+            "0.8.19",
+            "0.8.20",
+            "0.8.21"
           ]
     steps:
       - uses: actions/checkout@v3
@@ -51,6 +58,7 @@ jobs:
 
       - name: Compile Smart Contracts
         run: |
-          solc contracts/**/*.sol \
-              @openzeppelin/=node_modules/@openzeppelin/ \
-              solidity-bytes-utils/=node_modules/solidity-bytes-utils/
+          solc contracts/**/*.sol --allow-paths $(pwd)/node_modules/ \
+              @openzeppelin/=$(pwd)/node_modules/@openzeppelin/ \
+              solidity-bytes-utils/=$(pwd)/node_modules/solidity-bytes-utils/ \
+              ../=$(pwd)/contracts/

implementations/contracts/ERC725.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: CC0-1.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.5;
 
 // modules
 import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
@@ -10,6 +10,9 @@ import {ERC725YCore} from "./ERC725YCore.sol";
 // constants
 import {_INTERFACEID_ERC725X, _INTERFACEID_ERC725Y} from "./constants.sol";
 
+// errors
+import {OwnableCannotSetZeroAddressAsOwner} from "./errors.sol";
+
 /**
  * @title ERC725 bundle.
  * @author Fabian Vogelsteller <fabian@lukso.network>
@@ -27,10 +30,9 @@ contract ERC725 is ERC725XCore, ERC725YCore {
      * - `initialOwner` CANNOT be the zero address.
      */
     constructor(address initialOwner) payable {
-        require(
-            initialOwner != address(0),
-            "Ownable: new owner is the zero address"
-        );
+        if (initialOwner == address(0)) {
+            revert OwnableCannotSetZeroAddressAsOwner();
+        }
         OwnableUnset._setOwner(initialOwner);
     }
 

implementations/contracts/ERC725Init.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: CC0-1.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.5;
 
 // modules
 import {ERC725InitAbstract} from "./ERC725InitAbstract.sol";

implementations/contracts/ERC725InitAbstract.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: CC0-1.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.5;
 
 // modules
 import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
@@ -13,6 +13,9 @@ import {ERC725YCore} from "./ERC725YCore.sol";
 // constants
 import {_INTERFACEID_ERC725X, _INTERFACEID_ERC725Y} from "./constants.sol";
 
+// errors
+import {OwnableCannotSetZeroAddressAsOwner} from "./errors.sol";
+
 /**
  * @title Inheritable Proxy Implementation of ERC725 bundle
  * @author Fabian Vogelsteller <fabian@lukso.network>
@@ -35,10 +38,9 @@ abstract contract ERC725InitAbstract is
     function _initialize(
         address initialOwner
     ) internal virtual onlyInitializing {
-        require(
-            initialOwner != address(0),
-            "Ownable: new owner is the zero address"
-        );
+        if (initialOwner == address(0)) {
+            revert OwnableCannotSetZeroAddressAsOwner();
+        }
         OwnableUnset._setOwner(initialOwner);
     }
 

implementations/contracts/ERC725X.sol

@@ -1,10 +1,13 @@
 // SPDX-License-Identifier: Apache-2.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.5;
 
 // modules
 import {OwnableUnset} from "./custom/OwnableUnset.sol";
 import {ERC725XCore} from "./ERC725XCore.sol";
 
+// errors
+import {OwnableCannotSetZeroAddressAsOwner} from "./errors.sol";
+
 /**
  * @title Deployable implementation with `constructor` of ERC725X, a generic executor.
  * @author Fabian Vogelsteller <fabian@lukso.network>
@@ -23,10 +26,9 @@ contract ERC725X is ERC725XCore {
      * - `initialOwner` CANNOT be the zero address.
      */
     constructor(address initialOwner) payable {
-        require(
-            initialOwner != address(0),
-            "Ownable: new owner is the zero address"
-        );
+        if (initialOwner == address(0)) {
+            revert OwnableCannotSetZeroAddressAsOwner();
+        }
         OwnableUnset._setOwner(initialOwner);
     }
 }

implementations/contracts/ERC725XCore.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: Apache-2.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.5;
 
 // interfaces
 import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";

implementations/contracts/ERC725XInit.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: Apache-2.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.5;
 
 // modules
 import {ERC725XInitAbstract} from "./ERC725XInitAbstract.sol";

implementations/contracts/ERC725XInitAbstract.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: Apache-2.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.5;
 
 // modules
 import {
@@ -8,6 +8,9 @@ import {
 import {OwnableUnset} from "./custom/OwnableUnset.sol";
 import {ERC725XCore} from "./ERC725XCore.sol";
 
+// errors
+import {OwnableCannotSetZeroAddressAsOwner} from "./errors.sol";
+
 /**
  * @title Inheritable Proxy Implementation of ERC725X, a generic executor.
  * @author Fabian Vogelsteller <fabian@lukso.network>
@@ -27,10 +30,9 @@ abstract contract ERC725XInitAbstract is Initializable, ERC725XCore {
     function _initialize(
         address initialOwner
     ) internal virtual onlyInitializing {
-        require(
-            initialOwner != address(0),
-            "Ownable: new owner is the zero address"
-        );
+        if (initialOwner == address(0)) {
+            revert OwnableCannotSetZeroAddressAsOwner();
+        }
         OwnableUnset._setOwner(initialOwner);
     }
 }

implementations/contracts/ERC725Y.sol

@@ -1,11 +1,14 @@
 // SPDX-License-Identifier: Apache-2.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.4;
 
 // modules
 import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";
 import {OwnableUnset} from "./custom/OwnableUnset.sol";
 import {ERC725YCore} from "./ERC725YCore.sol";
 
+// errors
+import {OwnableCannotSetZeroAddressAsOwner} from "./errors.sol";
+
 /**
  * @title Deployable implementation with `constructor` of ERC725Y, a generic data key/value store.
  * @author Fabian Vogelsteller <fabian@lukso.network>
@@ -22,10 +25,9 @@ contract ERC725Y is ERC725YCore {
      * - `initialOwner` CANNOT be the zero address.
      */
     constructor(address initialOwner) payable {
-        require(
-            initialOwner != address(0),
-            "Ownable: new owner is the zero address"
-        );
+        if (initialOwner == address(0)) {
+            revert OwnableCannotSetZeroAddressAsOwner();
+        }
         OwnableUnset._setOwner(initialOwner);
     }
 }

implementations/contracts/ERC725YCore.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: Apache-2.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.4;
 
 // interfaces
 import {IERC165} from "@openzeppelin/contracts/utils/introspection/IERC165.sol";

implementations/contracts/ERC725YInit.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: Apache-2.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.4;
 
 // modules
 import {ERC725YInitAbstract} from "./ERC725YInitAbstract.sol";

implementations/contracts/ERC725YInitAbstract.sol

@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: Apache-2.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.4;
 
 // modules
 import {
@@ -8,6 +8,9 @@ import {
 import {OwnableUnset} from "./custom/OwnableUnset.sol";
 import {ERC725YCore} from "./ERC725YCore.sol";
 
+// errors
+import {OwnableCannotSetZeroAddressAsOwner} from "./errors.sol";
+
 /**
  * @title Inheritable Proxy Implementation of ERC725Y, a generic data key/value store
  * @author Fabian Vogelsteller <fabian@lukso.network>
@@ -25,10 +28,9 @@ abstract contract ERC725YInitAbstract is Initializable, ERC725YCore {
     function _initialize(
         address initialOwner
     ) internal virtual onlyInitializing {
-        require(
-            initialOwner != address(0),
-            "Ownable: new owner is the zero address"
-        );
+        if (initialOwner == address(0)) {
+            revert OwnableCannotSetZeroAddressAsOwner();
+        }
         OwnableUnset._setOwner(initialOwner);
     }
 }

implementations/contracts/custom/OwnableUnset.sol

@@ -1,5 +1,11 @@
 // SPDX-License-Identifier: MIT
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.4;
+
+// errors
+import {
+    OwnableCannotSetZeroAddressAsOwner,
+    OwnableCallerNotTheOwner
+} from "../errors.sol";
 
 /**
  * @title OwnableUnset
@@ -47,18 +53,19 @@ abstract contract OwnableUnset {
      * Can only be called by the current owner.
      */
     function transferOwnership(address newOwner) public virtual onlyOwner {
-        require(
-            newOwner != address(0),
-            "Ownable: new owner is the zero address"
-        );
+        if (newOwner == address(0)) {
+            revert OwnableCannotSetZeroAddressAsOwner();
+        }
         _setOwner(newOwner);
     }
 
     /**
      * @dev Throws if the sender is not the owner.
      */
     function _checkOwner() internal view virtual {
-        require(owner() == msg.sender, "Ownable: caller is not the owner");
+        if (owner() != msg.sender) {
+            revert OwnableCallerNotTheOwner(msg.sender);
+        }
     }
 
     /**

implementations/contracts/errors.sol

@@ -1,5 +1,17 @@
 // SPDX-License-Identifier: Apache-2.0
-pragma solidity ^0.8.0;
+pragma solidity ^0.8.4;
+
+/**
+ * @dev Reverts when trying to set `address(0)` as the contract owner when deploying the contract,
+ * initializing it or transferring ownership of the contract.
+ */
+error OwnableCannotSetZeroAddressAsOwner();
+
+/**
+ * @dev Reverts when only the owner is allowed to call the function.
+ * @param callerAddress The address that tried to make the call.
+ */
+error OwnableCallerNotTheOwner(address callerAddress);
 
 /**
  * @dev Reverts when trying to send more native tokens `value` than available in current `balance`.

implementations/test/ERC725.test.ts

@@ -30,9 +30,12 @@ describe('ERC725', () => {
           newOwner: ethers.constants.AddressZero,
         };
 
-        await expect(
-          new ERC725__factory(accounts[0]).deploy(deployParams.newOwner),
-        ).to.be.revertedWith('Ownable: new owner is the zero address');
+        const contractToDeploy = new ERC725__factory(accounts[0]);
+
+        await expect(contractToDeploy.deploy(deployParams.newOwner)).to.be.revertedWithCustomError(
+          contractToDeploy,
+          'OwnableCannotSetZeroAddressAsOwner',
+        );
       });
 
       it("should deploy the contract with the owner's address", async () => {
@@ -108,7 +111,7 @@ describe('ERC725', () => {
       it('should revert when initializing with address(0) as owner', async () => {
         await expect(
           context.erc725['initialize(address)'](ethers.constants.AddressZero),
-        ).to.be.revertedWith('Ownable: new owner is the zero address');
+        ).to.be.revertedWithCustomError(context.erc725, 'OwnableCannotSetZeroAddressAsOwner');
       });
 
       it("should initialize the contract with the owner's address", async () => {

implementations/test/ERC725X.behaviour.ts

@@ -118,6 +118,14 @@ export const shouldBehaveLikeERC725X = (buildContext: () => Promise<ERC725XTestC
 
         expect(accountOwner).to.equal(context.accounts.anyone.address);
       });
+
+      it('should revert when transferring ownership to `address(0)`', async () => {
+        await expect(
+          context.erc725X
+            .connect(context.accounts.owner)
+            .transferOwnership(ethers.constants.AddressZero),
+        ).to.be.revertedWithCustomError(context.erc725X, 'OwnableCannotSetZeroAddressAsOwner');
+      });
     });
 
     describe('When non-owner is transferring ownership', () => {
@@ -126,7 +134,7 @@ export const shouldBehaveLikeERC725X = (buildContext: () => Promise<ERC725XTestC
           context.erc725X
             .connect(context.accounts.anyone)
             .transferOwnership(context.accounts.anyone.address),
-        ).to.be.revertedWith('Ownable: caller is not the owner');
+        ).to.be.revertedWithCustomError(context.erc725X, 'OwnableCallerNotTheOwner');
       });
     });
 
@@ -146,7 +154,7 @@ export const shouldBehaveLikeERC725X = (buildContext: () => Promise<ERC725XTestC
       it('should revert', async () => {
         await expect(
           context.erc725X.connect(context.accounts.anyone).renounceOwnership(),
-        ).to.be.revertedWith('Ownable: caller is not the owner');
+        ).to.be.revertedWithCustomError(context.erc725X, 'OwnableCallerNotTheOwner');
       });
     });
   });
@@ -196,7 +204,7 @@ export const shouldBehaveLikeERC725X = (buildContext: () => Promise<ERC725XTestC
               context.erc725X
                 .connect(context.accounts.anyone)
                 .execute(txParams.Operation, txParams.to, txParams.value, txParams.data),
-            ).to.be.revertedWith('Ownable: caller is not the owner');
+            ).to.be.revertedWithCustomError(context.erc725X, 'OwnableCallerNotTheOwner');
           });
         });
       });
@@ -1624,7 +1632,7 @@ export const shouldBehaveLikeERC725X = (buildContext: () => Promise<ERC725XTestC
               context.erc725X
                 .connect(context.accounts.anyone)
                 .executeBatch(txParams.Operations, txParams.to, txParams.values, txParams.data),
-            ).to.be.revertedWith('Ownable: caller is not the owner');
+            ).to.be.revertedWithCustomError(context.erc725X, 'OwnableCallerNotTheOwner');
           });
         });
       });