Make Ownable's initial owner explicit (#4267)

Co-authored-by: Ernesto García <ernestognw@gmail.com>

.changeset/clever-pumas-beg.md

@@ -0,0 +1,5 @@
+---
+'openzeppelin-solidity': major
+---
+
+`Ownable`: Add an `initialOwner` parameter to the constructor, making the ownership initialization explicit.

contracts/access/Ownable.sol

@@ -25,8 +25,8 @@ abstract contract Ownable is Context {
     /**
      * @dev Initializes the contract setting the deployer as the initial owner.
      */
-    constructor() {
+    constructor(address initialOwner) {
-        _transferOwnership(_msgSender());
+        _transferOwnership(initialOwner);
     }
 
     /**

contracts/mocks/ERC1271WalletMock.sol

@@ -7,9 +7,7 @@ import "../interfaces/IERC1271.sol";
 import "../utils/cryptography/ECDSA.sol";
 
 contract ERC1271WalletMock is Ownable, IERC1271 {
-    constructor(address originalOwner) {
+    constructor(address originalOwner) Ownable(originalOwner) {}
-        transferOwnership(originalOwner);
-    }
 
     function isValidSignature(bytes32 hash, bytes memory signature) public view override returns (bytes4 magicValue) {
         return ECDSA.recover(hash, signature) == owner() ? this.isValidSignature.selector : bytes4(0);

contracts/proxy/beacon/UpgradeableBeacon.sol

@@ -21,10 +21,9 @@ contract UpgradeableBeacon is IBeacon, Ownable {
     event Upgraded(address indexed implementation);
 
     /**
-     * @dev Sets the address of the initial implementation, and the deployer account as the owner who can upgrade the
+     * @dev Sets the address of the initial implementation, and the initial owner who can upgrade the beacon.
-     * beacon.
      */
-    constructor(address implementation_) {
+    constructor(address implementation_, address initialOwner) Ownable(initialOwner) {
         _setImplementation(implementation_);
     }
 

contracts/proxy/transparent/ProxyAdmin.sol

@@ -11,6 +11,11 @@ import "../../access/Ownable.sol";
  * explanation of why you would want to use this see the documentation for {TransparentUpgradeableProxy}.
  */
 contract ProxyAdmin is Ownable {
+    /**
+     * @dev Sets the initial owner who can perform upgrades.
+     */
+    constructor(address initialOwner) Ownable(initialOwner) {}
+
     /**
      * @dev Changes the admin of `proxy` to `newAdmin`.
      *

test/access/Ownable.test.js

@@ -9,7 +9,7 @@ contract('Ownable', function (accounts) {
   const [owner, other] = accounts;
 
   beforeEach(async function () {
-    this.ownable = await Ownable.new({ from: owner });
+    this.ownable = await Ownable.new(owner);
   });
 
   it('has an owner', async function () {

test/access/Ownable2Step.test.js

@@ -8,7 +8,7 @@ contract('Ownable2Step', function (accounts) {
   const [owner, accountA, accountB] = accounts;
 
   beforeEach(async function () {
-    this.ownable2Step = await Ownable2Step.new({ from: owner });
+    this.ownable2Step = await Ownable2Step.new(owner);
   });
 
   describe('transfer ownership', function () {

test/proxy/beacon/BeaconProxy.test.js

@@ -11,7 +11,7 @@ const BadBeaconNoImpl = artifacts.require('BadBeaconNoImpl');
 const BadBeaconNotContract = artifacts.require('BadBeaconNotContract');
 
 contract('BeaconProxy', function (accounts) {
-  const [anotherAccount] = accounts;
+  const [upgradeableBeaconAdmin, anotherAccount] = accounts;
 
   describe('bad beacon is not accepted', async function () {
     it('non-contract beacon', async function () {
@@ -49,7 +49,7 @@ contract('BeaconProxy', function (accounts) {
     });
 
     beforeEach('deploy beacon', async function () {
-      this.beacon = await UpgradeableBeacon.new(this.implementationV0.address);
+      this.beacon = await UpgradeableBeacon.new(this.implementationV0.address, upgradeableBeaconAdmin);
     });
 
     it('no initialization', async function () {
@@ -81,7 +81,7 @@ contract('BeaconProxy', function (accounts) {
   });
 
   it('upgrade a proxy by upgrading its beacon', async function () {
-    const beacon = await UpgradeableBeacon.new(this.implementationV0.address);
+    const beacon = await UpgradeableBeacon.new(this.implementationV0.address, upgradeableBeaconAdmin);
 
     const value = '10';
     const data = this.implementationV0.contract.methods.initializeNonPayableWithValue(value).encodeABI();
@@ -96,7 +96,7 @@ contract('BeaconProxy', function (accounts) {
     expect(await dummy.version()).to.eq('V1');
 
     // upgrade beacon
-    await beacon.upgradeTo(this.implementationV1.address);
+    await beacon.upgradeTo(this.implementationV1.address, { from: upgradeableBeaconAdmin });
 
     // test upgraded version
     expect(await dummy.version()).to.eq('V2');
@@ -106,7 +106,7 @@ contract('BeaconProxy', function (accounts) {
     const value1 = '10';
     const value2 = '42';
 
-    const beacon = await UpgradeableBeacon.new(this.implementationV0.address);
+    const beacon = await UpgradeableBeacon.new(this.implementationV0.address, upgradeableBeaconAdmin);
 
     const proxy1InitializeData = this.implementationV0.contract.methods
       .initializeNonPayableWithValue(value1)
@@ -130,7 +130,7 @@ contract('BeaconProxy', function (accounts) {
     expect(await dummy2.version()).to.eq('V1');
 
     // upgrade beacon
-    await beacon.upgradeTo(this.implementationV1.address);
+    await beacon.upgradeTo(this.implementationV1.address, { from: upgradeableBeaconAdmin });
 
     // test upgraded version
     expect(await dummy1.version()).to.eq('V2');

test/proxy/beacon/UpgradeableBeacon.test.js

@@ -9,13 +9,16 @@ contract('UpgradeableBeacon', function (accounts) {
   const [owner, other] = accounts;
 
   it('cannot be created with non-contract implementation', async function () {
-    await expectRevert(UpgradeableBeacon.new(accounts[0]), 'UpgradeableBeacon: implementation is not a contract');
+    await expectRevert(
+      UpgradeableBeacon.new(accounts[0], owner),
+      'UpgradeableBeacon: implementation is not a contract',
+    );
   });
 
   context('once deployed', async function () {
     beforeEach('deploying beacon', async function () {
       this.v1 = await Implementation1.new();
-      this.beacon = await UpgradeableBeacon.new(this.v1.address, { from: owner });
+      this.beacon = await UpgradeableBeacon.new(this.v1.address, owner);
     });
 
     it('returns implementation', async function () {

test/proxy/transparent/ProxyAdmin.test.js

@@ -17,12 +17,11 @@ contract('ProxyAdmin', function (accounts) {
 
   beforeEach(async function () {
     const initializeData = Buffer.from('');
-    this.proxyAdmin = await ProxyAdmin.new({ from: proxyAdminOwner });
+    this.proxyAdmin = await ProxyAdmin.new(proxyAdminOwner);
     const proxy = await TransparentUpgradeableProxy.new(
       this.implementationV1.address,
       this.proxyAdmin.address,
       initializeData,
-      { from: proxyAdminOwner },
     );
     this.proxy = await ITransparentUpgradeableProxy.at(proxy.address);
   });