From 14d89c291d47ee1202eb810b580da1a27d83e009 Mon Sep 17 00:00:00 2001
From: NishantKoyalwar <122688383+NishantKoyalwar@users.noreply.github.com>
Date: Tue, 5 Sep 2023 01:47:16 +0530
Subject: [PATCH] Move beneficiary zero address check to Ownable (#4531)

Co-authored-by: Hadrien Croubois <hadrien.croubois@gmail.com>
Co-authored-by: Francisco <fg@frang.io>
---
 .changeset/clever-bats-kick.md      | 5 +++++
 contracts/access/Ownable.sol        | 3 +++
 contracts/finance/VestingWallet.sol | 9 ---------
 test/access/Ownable.test.js         | 4 ++++
 test/finance/VestingWallet.test.js  | 2 +-
 5 files changed, 13 insertions(+), 10 deletions(-)
 create mode 100644 .changeset/clever-bats-kick.md

diff --git a/.changeset/clever-bats-kick.md b/.changeset/clever-bats-kick.md
new file mode 100644
index 00000000000..b35301b73de
--- /dev/null
+++ b/.changeset/clever-bats-kick.md
@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': patch
+---
+
+`Ownable`: Prevent using address(0) as the initial owner.
diff --git a/contracts/access/Ownable.sol b/contracts/access/Ownable.sol
index aa495ff4de7..3cf11378261 100644
--- a/contracts/access/Ownable.sol
+++ b/contracts/access/Ownable.sol
@@ -36,6 +36,9 @@ abstract contract Ownable is Context {
      * @dev Initializes the contract setting the address provided by the deployer as the initial owner.
      */
     constructor(address initialOwner) {
+        if (initialOwner == address(0)) {
+            revert OwnableInvalidOwner(address(0));
+        }
         _transferOwnership(initialOwner);
     }
 
diff --git a/contracts/finance/VestingWallet.sol b/contracts/finance/VestingWallet.sol
index 7b50e699e1f..f014e59c4bd 100644
--- a/contracts/finance/VestingWallet.sol
+++ b/contracts/finance/VestingWallet.sol
@@ -31,11 +31,6 @@ contract VestingWallet is Context, Ownable {
     event EtherReleased(uint256 amount);
     event ERC20Released(address indexed token, uint256 amount);
 
-    /**
-     * @dev The `beneficiary` is not a valid account.
-     */
-    error VestingWalletInvalidBeneficiary(address beneficiary);
-
     uint256 private _released;
     mapping(address token => uint256) private _erc20Released;
     uint64 private immutable _start;
@@ -46,10 +41,6 @@ contract VestingWallet is Context, Ownable {
      * vesting duration of the vesting wallet.
      */
     constructor(address beneficiary, uint64 startTimestamp, uint64 durationSeconds) payable Ownable(beneficiary) {
-        if (beneficiary == address(0)) {
-            revert VestingWalletInvalidBeneficiary(address(0));
-        }
-
         _start = startTimestamp;
         _duration = durationSeconds;
     }
diff --git a/test/access/Ownable.test.js b/test/access/Ownable.test.js
index 079d694d730..f85daec5d28 100644
--- a/test/access/Ownable.test.js
+++ b/test/access/Ownable.test.js
@@ -14,6 +14,10 @@ contract('Ownable', function (accounts) {
     this.ownable = await Ownable.new(owner);
   });
 
+  it('rejects zero address for initialOwner', async function () {
+    await expectRevertCustomError(Ownable.new(constants.ZERO_ADDRESS), 'OwnableInvalidOwner', [constants.ZERO_ADDRESS]);
+  });
+
   it('has an owner', async function () {
     expect(await this.ownable.owner()).to.equal(owner);
   });
diff --git a/test/finance/VestingWallet.test.js b/test/finance/VestingWallet.test.js
index d79aea1955a..918e56345fb 100644
--- a/test/finance/VestingWallet.test.js
+++ b/test/finance/VestingWallet.test.js
@@ -23,7 +23,7 @@ contract('VestingWallet', function (accounts) {
   it('rejects zero address for beneficiary', async function () {
     await expectRevertCustomError(
       VestingWallet.new(constants.ZERO_ADDRESS, this.start, duration),
-      'VestingWalletInvalidBeneficiary',
+      'OwnableInvalidOwner',
       [constants.ZERO_ADDRESS],
     );
   });