From fccb61f3d96ab1842a7a0e6635a50a7d5201b3b2 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 16 Aug 2022 08:22:19 -0400 Subject: [PATCH 01/32] Change _owner to internal this way it is accessible via inheritance --- contracts/access/Ownable.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/Ownable.sol b/contracts/access/Ownable.sol index 6d4e866f4da..b49fa4e0997 100644 --- a/contracts/access/Ownable.sol +++ b/contracts/access/Ownable.sol @@ -18,7 +18,7 @@ import "../utils/Context.sol"; * the owner. */ abstract contract Ownable is Context { - address private _owner; + address internal _owner; event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); From 918ab11eff1e42c957923d17b728f77d0301e21d Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 16 Aug 2022 09:35:29 -0400 Subject: [PATCH 02/32] Revert change to _owner variable --- contracts/access/Ownable.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/Ownable.sol b/contracts/access/Ownable.sol index b49fa4e0997..6d4e866f4da 100644 --- a/contracts/access/Ownable.sol +++ b/contracts/access/Ownable.sol @@ -18,7 +18,7 @@ import "../utils/Context.sol"; * the owner. */ abstract contract Ownable is Context { - address internal _owner; + address private _owner; event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); From 00aae1135f9d5639754dac2b998115eaf80d9512 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 16 Aug 2022 09:37:10 -0400 Subject: [PATCH 03/32] New contract SafeOwnable --- contracts/access/SafeOwnable.sol | 41 ++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) create mode 100644 contracts/access/SafeOwnable.sol diff --git a/contracts/access/SafeOwnable.sol b/contracts/access/SafeOwnable.sol new file mode 100644 index 00000000000..12bc70c5bee --- /dev/null +++ b/contracts/access/SafeOwnable.sol @@ -0,0 +1,41 @@ +// SPDX-License-Identifier: MIT +// OpenZeppelin Contracts (last updated v4.7.0) (access/Ownable.sol) + +pragma solidity ^0.8.0; + +import "./Ownable.sol"; + +/** + * @dev Contract module which provides access control mechanism, where + * there is an account (an owner) that can be granted exclusive access to + * specific functions. + * + * By default, the owner account will be the one that deploys the contract. This + * can later be changed with {transferOwnership} and {acceptOwnership}. + * + * This module is used through inheritance. It will make available all functions + * from parent (Ownable). + */ +abstract contract SafeOwnable is Ownable { + address private _newOwner; + + event OwnershipTransferStarted(address indexed newOwner); + + /** + * @dev Starts the ownership transfer of the contract to a new account + * Can only be called by the current owner. + */ + function transferOwnership(address newOwner) public virtual override onlyOwner { + require(newOwner != address(0), "SafeOwnable: new owner is the zero address"); + _newOwner = newOwner; + emit OwnershipTransferStarted(newOwner); + } + + /** + * @dev The new owner accepts the ownership transfer. + */ + function acceptOwnership() external { + require(_newOwner == _msgSender(), "SafeOwnable: caller is not the new owner"); + _transferOwnership(_msgSender()); + } +} From 16f60145d46affa066845102fd01cf8bc8a2451f Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 16 Aug 2022 09:37:20 -0400 Subject: [PATCH 04/32] SafeOwnableMock --- contracts/mocks/SafeOwnableMock.sol | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 contracts/mocks/SafeOwnableMock.sol diff --git a/contracts/mocks/SafeOwnableMock.sol b/contracts/mocks/SafeOwnableMock.sol new file mode 100644 index 00000000000..079fa5bcd6f --- /dev/null +++ b/contracts/mocks/SafeOwnableMock.sol @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: MIT + +pragma solidity ^0.8.0; + +import "../access/SafeOwnable.sol"; + +contract SafeOwnableMock is SafeOwnable {} From fbc33a89842d6ff2fb62d5fd65d8683a92d74255 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 16 Aug 2022 09:40:34 -0400 Subject: [PATCH 05/32] testing the new SafeOwnable contract --- test/access/SafeOwnable.test.js | 44 +++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100644 test/access/SafeOwnable.test.js diff --git a/test/access/SafeOwnable.test.js b/test/access/SafeOwnable.test.js new file mode 100644 index 00000000000..22bcd8947fc --- /dev/null +++ b/test/access/SafeOwnable.test.js @@ -0,0 +1,44 @@ +const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { ZERO_ADDRESS } = constants; + +const { expect } = require('chai'); + +const SafeOwnable = artifacts.require('SafeOwnableMock'); + +contract('SafeOwnable', function (accounts) { + const [ owner, accountA, accountB ] = accounts; + + beforeEach(async function () { + this.safe_ownable = await SafeOwnable.new({ from: owner }); + }); + + describe('transfer ownership', function () { + it('starting a transfer does not change owner', async function () { + const receipt = await this.safe_ownable.transferOwnership(accountA, { from: owner }); + expectEvent(receipt, 'OwnershipTransferStarted'); + expect(await this.safe_ownable.owner()).to.equal(owner); + }); + + it('guards transfer against invalid user', async function () { + await this.safe_ownable.transferOwnership(accountA, { from: owner }); + await expectRevert( + this.safe_ownable.acceptOwnership({ from: accountB }), + 'SafeOwnable: caller is not the new owner', + ); + }); + + it('changes owner after transfer', async function () { + await this.safe_ownable.transferOwnership(accountA, { from: owner }); + const receipt = await this.safe_ownable.acceptOwnership({ from: accountA }); + expectEvent(receipt, 'OwnershipTransferred'); + expect(await this.safe_ownable.owner()).to.equal(accountA); + }); + + it('guards ownership against stuck state', async function () { + await expectRevert( + this.safe_ownable.transferOwnership(ZERO_ADDRESS, { from: owner }), + 'SafeOwnable: new owner is the zero address', + ); + }); + }); +}); From 229b1d0e4a0b91182d3f9d53b26c0a880d6cc6f8 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 16 Aug 2022 09:44:55 -0400 Subject: [PATCH 06/32] format code --- test/access/SafeOwnable.test.js | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/test/access/SafeOwnable.test.js b/test/access/SafeOwnable.test.js index 22bcd8947fc..d3bf652cafc 100644 --- a/test/access/SafeOwnable.test.js +++ b/test/access/SafeOwnable.test.js @@ -19,19 +19,19 @@ contract('SafeOwnable', function (accounts) { expect(await this.safe_ownable.owner()).to.equal(owner); }); - it('guards transfer against invalid user', async function () { - await this.safe_ownable.transferOwnership(accountA, { from: owner }); - await expectRevert( - this.safe_ownable.acceptOwnership({ from: accountB }), - 'SafeOwnable: caller is not the new owner', - ); - }); - it('changes owner after transfer', async function () { - await this.safe_ownable.transferOwnership(accountA, { from: owner }); - const receipt = await this.safe_ownable.acceptOwnership({ from: accountA }); - expectEvent(receipt, 'OwnershipTransferred'); - expect(await this.safe_ownable.owner()).to.equal(accountA); + await this.safe_ownable.transferOwnership(accountA, { from: owner }); + const receipt = await this.safe_ownable.acceptOwnership({ from: accountA }); + expectEvent(receipt, 'OwnershipTransferred'); + expect(await this.safe_ownable.owner()).to.equal(accountA); + }); + + it('guards transfer against invalid user', async function () { + await this.safe_ownable.transferOwnership(accountA, { from: owner }); + await expectRevert( + this.safe_ownable.acceptOwnership({ from: accountB }), + 'SafeOwnable: caller is not the new owner', + ); }); it('guards ownership against stuck state', async function () { From 4b3369ae8f62fa89a2133cb2f4d399fcca4496ae Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 16 Aug 2022 09:58:07 -0400 Subject: [PATCH 07/32] Add entry to changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 25facc7b655..42ef21dfab8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,7 @@ * `SafeCast`: optimize downcasting of signed integers. ([#3565](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3565)) * `ERC20FlashMint`: add an internal `_flashFee` function for overriding. ([#3551](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3551)) * `VestingWallet`: remove unused library `Math.sol`. ([#3605](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3605)) + * `SafeOwnable`: add new contract SafeOwnable, makes the transfers a two step process `SafeOwnable.sol`. ([#3620](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3620)) ### Compatibility Note From 6c7325d66ff0b13a50f2c8b189c957bf7485da58 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 16 Aug 2022 10:28:36 -0400 Subject: [PATCH 08/32] Fix error 'safe_ownable' is not in camel case --- test/access/SafeOwnable.test.js | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/test/access/SafeOwnable.test.js b/test/access/SafeOwnable.test.js index d3bf652cafc..cbc45e88e71 100644 --- a/test/access/SafeOwnable.test.js +++ b/test/access/SafeOwnable.test.js @@ -9,34 +9,34 @@ contract('SafeOwnable', function (accounts) { const [ owner, accountA, accountB ] = accounts; beforeEach(async function () { - this.safe_ownable = await SafeOwnable.new({ from: owner }); + this.safeOwnable = await SafeOwnable.new({ from: owner }); }); describe('transfer ownership', function () { it('starting a transfer does not change owner', async function () { - const receipt = await this.safe_ownable.transferOwnership(accountA, { from: owner }); + const receipt = await this.safeOwnable.transferOwnership(accountA, { from: owner }); expectEvent(receipt, 'OwnershipTransferStarted'); - expect(await this.safe_ownable.owner()).to.equal(owner); + expect(await this.safeOwnable.owner()).to.equal(owner); }); it('changes owner after transfer', async function () { - await this.safe_ownable.transferOwnership(accountA, { from: owner }); - const receipt = await this.safe_ownable.acceptOwnership({ from: accountA }); + await this.safeOwnable.transferOwnership(accountA, { from: owner }); + const receipt = await this.safeOwnable.acceptOwnership({ from: accountA }); expectEvent(receipt, 'OwnershipTransferred'); - expect(await this.safe_ownable.owner()).to.equal(accountA); + expect(await this.safeOwnable.owner()).to.equal(accountA); }); it('guards transfer against invalid user', async function () { - await this.safe_ownable.transferOwnership(accountA, { from: owner }); + await this.safeOwnable.transferOwnership(accountA, { from: owner }); await expectRevert( - this.safe_ownable.acceptOwnership({ from: accountB }), + this.safeOwnable.acceptOwnership({ from: accountB }), 'SafeOwnable: caller is not the new owner', ); }); it('guards ownership against stuck state', async function () { await expectRevert( - this.safe_ownable.transferOwnership(ZERO_ADDRESS, { from: owner }), + this.safeOwnable.transferOwnership(ZERO_ADDRESS, { from: owner }), 'SafeOwnable: new owner is the zero address', ); }); From 80b52602bfd99258e037828d152b0b69e290c639 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 16 Aug 2022 10:41:18 -0400 Subject: [PATCH 09/32] Fix mix of tabs and spaces --- contracts/access/SafeOwnable.sol | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contracts/access/SafeOwnable.sol b/contracts/access/SafeOwnable.sol index 12bc70c5bee..a964b54cb4f 100644 --- a/contracts/access/SafeOwnable.sol +++ b/contracts/access/SafeOwnable.sol @@ -35,7 +35,7 @@ abstract contract SafeOwnable is Ownable { * @dev The new owner accepts the ownership transfer. */ function acceptOwnership() external { - require(_newOwner == _msgSender(), "SafeOwnable: caller is not the new owner"); + require(_newOwner == _msgSender(), "SafeOwnable: caller is not the new owner"); _transferOwnership(_msgSender()); - } + } } From db4af818eba3c9a5c3e286845856d081e1b4eb64 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 16 Aug 2022 10:49:59 -0400 Subject: [PATCH 10/32] Allow address(0) as new owner it could be a way for owner to cancel a transfer --- contracts/access/SafeOwnable.sol | 1 - test/access/SafeOwnable.test.js | 7 ------- 2 files changed, 8 deletions(-) diff --git a/contracts/access/SafeOwnable.sol b/contracts/access/SafeOwnable.sol index a964b54cb4f..ee1d451a9a0 100644 --- a/contracts/access/SafeOwnable.sol +++ b/contracts/access/SafeOwnable.sol @@ -26,7 +26,6 @@ abstract contract SafeOwnable is Ownable { * Can only be called by the current owner. */ function transferOwnership(address newOwner) public virtual override onlyOwner { - require(newOwner != address(0), "SafeOwnable: new owner is the zero address"); _newOwner = newOwner; emit OwnershipTransferStarted(newOwner); } diff --git a/test/access/SafeOwnable.test.js b/test/access/SafeOwnable.test.js index cbc45e88e71..f799175f2af 100644 --- a/test/access/SafeOwnable.test.js +++ b/test/access/SafeOwnable.test.js @@ -33,12 +33,5 @@ contract('SafeOwnable', function (accounts) { 'SafeOwnable: caller is not the new owner', ); }); - - it('guards ownership against stuck state', async function () { - await expectRevert( - this.safeOwnable.transferOwnership(ZERO_ADDRESS, { from: owner }), - 'SafeOwnable: new owner is the zero address', - ); - }); }); }); From 2530cd57e9ae8115970b68f40f7371632f357523 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 17 Aug 2022 04:10:29 -0400 Subject: [PATCH 11/32] Rename _newOwner to _pendingOwner --- contracts/access/SafeOwnable.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/access/SafeOwnable.sol b/contracts/access/SafeOwnable.sol index ee1d451a9a0..a629e86421b 100644 --- a/contracts/access/SafeOwnable.sol +++ b/contracts/access/SafeOwnable.sol @@ -17,7 +17,7 @@ import "./Ownable.sol"; * from parent (Ownable). */ abstract contract SafeOwnable is Ownable { - address private _newOwner; + address private _pendingOwner; event OwnershipTransferStarted(address indexed newOwner); @@ -26,7 +26,7 @@ abstract contract SafeOwnable is Ownable { * Can only be called by the current owner. */ function transferOwnership(address newOwner) public virtual override onlyOwner { - _newOwner = newOwner; + _pendingOwner = newOwner; emit OwnershipTransferStarted(newOwner); } @@ -34,7 +34,7 @@ abstract contract SafeOwnable is Ownable { * @dev The new owner accepts the ownership transfer. */ function acceptOwnership() external { - require(_newOwner == _msgSender(), "SafeOwnable: caller is not the new owner"); + require(_pendingOwner == _msgSender(), "SafeOwnable: caller is not the new owner"); _transferOwnership(_msgSender()); } } From de9beed6de2a8ff0487a7fae9b1e5921a6267f50 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 17 Aug 2022 04:19:10 -0400 Subject: [PATCH 12/32] Remove unused const --- test/access/SafeOwnable.test.js | 1 - 1 file changed, 1 deletion(-) diff --git a/test/access/SafeOwnable.test.js b/test/access/SafeOwnable.test.js index f799175f2af..8cc896de603 100644 --- a/test/access/SafeOwnable.test.js +++ b/test/access/SafeOwnable.test.js @@ -1,5 +1,4 @@ const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); -const { ZERO_ADDRESS } = constants; const { expect } = require('chai'); From c06b773982f9ba8a458ff629870deb3ecd940919 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 17 Aug 2022 04:31:18 -0400 Subject: [PATCH 13/32] Remove unused constants --- test/access/SafeOwnable.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/access/SafeOwnable.test.js b/test/access/SafeOwnable.test.js index 8cc896de603..a6e56302707 100644 --- a/test/access/SafeOwnable.test.js +++ b/test/access/SafeOwnable.test.js @@ -1,4 +1,4 @@ -const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); const { expect } = require('chai'); From 3279163f80fa591ab0c4e372e95c626ff855f3bd Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 17 Aug 2022 06:23:22 -0400 Subject: [PATCH 14/32] override _transferOwnership to delete _pendingOwner --- contracts/access/SafeOwnable.sol | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/contracts/access/SafeOwnable.sol b/contracts/access/SafeOwnable.sol index a629e86421b..b09cb0d34b3 100644 --- a/contracts/access/SafeOwnable.sol +++ b/contracts/access/SafeOwnable.sol @@ -30,6 +30,15 @@ abstract contract SafeOwnable is Ownable { emit OwnershipTransferStarted(newOwner); } + /** + * @dev Transfers ownership of the contract to a new account (`newOwner`). + * Internal function without access restriction. + */ + function _transferOwnership(address newOwner) internal virtual override { + delete _pendingOwner; + super._transferOwnership(newOwner); + } + /** * @dev The new owner accepts the ownership transfer. */ From d995d7c859f469e494af934ee042cff9ba87dc23 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 17 Aug 2022 07:02:17 -0400 Subject: [PATCH 15/32] Update contracts/access/SafeOwnable.sol Co-authored-by: Hadrien Croubois --- contracts/access/SafeOwnable.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contracts/access/SafeOwnable.sol b/contracts/access/SafeOwnable.sol index b09cb0d34b3..ff07cae6fc3 100644 --- a/contracts/access/SafeOwnable.sol +++ b/contracts/access/SafeOwnable.sol @@ -21,6 +21,12 @@ abstract contract SafeOwnable is Ownable { event OwnershipTransferStarted(address indexed newOwner); + /** + * @dev Returns the address of the pending owner. + */ + function pendingOwner() public view virtual returns (address) { + return _pendingOwner; + } /** * @dev Starts the ownership transfer of the contract to a new account * Can only be called by the current owner. From b12b1e7f051c3d860aa923c453cc309b1426d8fe Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 17 Aug 2022 07:02:49 -0400 Subject: [PATCH 16/32] Update contracts/access/SafeOwnable.sol Co-authored-by: Hadrien Croubois --- contracts/access/SafeOwnable.sol | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/contracts/access/SafeOwnable.sol b/contracts/access/SafeOwnable.sol index ff07cae6fc3..bd4801c643e 100644 --- a/contracts/access/SafeOwnable.sol +++ b/contracts/access/SafeOwnable.sol @@ -36,6 +36,12 @@ abstract contract SafeOwnable is Ownable { emit OwnershipTransferStarted(newOwner); } +/** + * @dev Make the direct ownership transfer available as a fallback when the receiver cannot accept. + */ +function forceTransferOwnership(address newOwner) public virtual { + super.transferOwnership(newOwner); +} /** * @dev Transfers ownership of the contract to a new account (`newOwner`). * Internal function without access restriction. From 8d62986981cfc1f1aa67c26510659ae76cf798a9 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 17 Aug 2022 07:13:43 -0400 Subject: [PATCH 17/32] format code --- contracts/access/SafeOwnable.sol | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/contracts/access/SafeOwnable.sol b/contracts/access/SafeOwnable.sol index bd4801c643e..4b751d4872f 100644 --- a/contracts/access/SafeOwnable.sol +++ b/contracts/access/SafeOwnable.sol @@ -27,6 +27,7 @@ abstract contract SafeOwnable is Ownable { function pendingOwner() public view virtual returns (address) { return _pendingOwner; } + /** * @dev Starts the ownership transfer of the contract to a new account * Can only be called by the current owner. @@ -36,12 +37,13 @@ abstract contract SafeOwnable is Ownable { emit OwnershipTransferStarted(newOwner); } -/** - * @dev Make the direct ownership transfer available as a fallback when the receiver cannot accept. - */ -function forceTransferOwnership(address newOwner) public virtual { - super.transferOwnership(newOwner); -} + /** + * @dev Make the direct ownership transfer available as a fallback when the receiver cannot accept. + */ + function forceTransferOwnership(address newOwner) public virtual { + super.transferOwnership(newOwner); + } + /** * @dev Transfers ownership of the contract to a new account (`newOwner`). * Internal function without access restriction. From 320aa21ecae690db0b01dcf7dbc014d8b9e7112b Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 17 Aug 2022 07:13:51 -0400 Subject: [PATCH 18/32] Add test to forceTransferOwnership --- test/access/SafeOwnable.test.js | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/test/access/SafeOwnable.test.js b/test/access/SafeOwnable.test.js index a6e56302707..88a99d3d371 100644 --- a/test/access/SafeOwnable.test.js +++ b/test/access/SafeOwnable.test.js @@ -5,7 +5,7 @@ const { expect } = require('chai'); const SafeOwnable = artifacts.require('SafeOwnableMock'); contract('SafeOwnable', function (accounts) { - const [ owner, accountA, accountB ] = accounts; + const [owner, accountA, accountB] = accounts; beforeEach(async function () { this.safeOwnable = await SafeOwnable.new({ from: owner }); @@ -23,6 +23,13 @@ contract('SafeOwnable', function (accounts) { const receipt = await this.safeOwnable.acceptOwnership({ from: accountA }); expectEvent(receipt, 'OwnershipTransferred'); expect(await this.safeOwnable.owner()).to.equal(accountA); + expect(await this.safeOwnable.pendingOwner()).to.not.equal(accountA); + }); + + it('changes owner after force transfer', async function () { + await this.safeOwnable.forceTransferOwnership(accountA, { from: owner }); + expect(await this.safeOwnable.owner()).to.equal(accountA); + expect(await this.safeOwnable.pendingOwner()).to.not.equal(accountA); }); it('guards transfer against invalid user', async function () { From fbf41df1cc02c5b2d6dc4610c448b0c4cba2b483 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 17 Aug 2022 07:14:53 -0400 Subject: [PATCH 19/32] Add check on pendingOwner --- test/access/SafeOwnable.test.js | 1 + 1 file changed, 1 insertion(+) diff --git a/test/access/SafeOwnable.test.js b/test/access/SafeOwnable.test.js index 88a99d3d371..48d74074d5b 100644 --- a/test/access/SafeOwnable.test.js +++ b/test/access/SafeOwnable.test.js @@ -16,6 +16,7 @@ contract('SafeOwnable', function (accounts) { const receipt = await this.safeOwnable.transferOwnership(accountA, { from: owner }); expectEvent(receipt, 'OwnershipTransferStarted'); expect(await this.safeOwnable.owner()).to.equal(owner); + expect(await this.safeOwnable.pendingOwner()).to.equal(accountA); }); it('changes owner after transfer', async function () { From 9bbeb47a585a3ae2e16882830db3c80182e48964 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 24 Aug 2022 12:18:29 -0400 Subject: [PATCH 20/32] Rename to Ownable2Step --- CHANGELOG.md | 2 +- .../{SafeOwnable.sol => Ownable2Step.sol} | 4 +- contracts/mocks/SafeOwnableMock.sol | 4 +- test/access/Ownable2Step.test.js | 44 +++++++++++++++++++ test/access/SafeOwnable.test.js | 44 ------------------- 5 files changed, 49 insertions(+), 49 deletions(-) rename contracts/access/{SafeOwnable.sol => Ownable2Step.sol} (93%) create mode 100644 test/access/Ownable2Step.test.js delete mode 100644 test/access/SafeOwnable.test.js diff --git a/CHANGELOG.md b/CHANGELOG.md index 8bdb98fa5ae..2ebc3fc0985 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ * `VestingWallet`: add `releasable` getters. ([#3580](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3580)) * `Create2`: optimize address computation by using assembly instead of `abi.encodePacked`. ([#3600](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3600)) * `Clones`: optimized the assembly to use only the scratch space during deployments, and optimized `predictDeterministicAddress` to use lesser operations. ([#3640](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3640)) - * `SafeOwnable`: add new contract SafeOwnable, makes the transfers a two step process `SafeOwnable.sol`. ([#3620](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3620)) + * `Ownable2Step`: add new contract Ownable2Step, makes the transfers a two step process `Ownable2Step.sol`. ([#3620](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3620)) ### Deprecations diff --git a/contracts/access/SafeOwnable.sol b/contracts/access/Ownable2Step.sol similarity index 93% rename from contracts/access/SafeOwnable.sol rename to contracts/access/Ownable2Step.sol index 4b751d4872f..139ea2e813e 100644 --- a/contracts/access/SafeOwnable.sol +++ b/contracts/access/Ownable2Step.sol @@ -16,7 +16,7 @@ import "./Ownable.sol"; * This module is used through inheritance. It will make available all functions * from parent (Ownable). */ -abstract contract SafeOwnable is Ownable { +abstract contract Ownable2Step is Ownable { address private _pendingOwner; event OwnershipTransferStarted(address indexed newOwner); @@ -57,7 +57,7 @@ abstract contract SafeOwnable is Ownable { * @dev The new owner accepts the ownership transfer. */ function acceptOwnership() external { - require(_pendingOwner == _msgSender(), "SafeOwnable: caller is not the new owner"); + require(_pendingOwner == _msgSender(), "Ownable2Step: caller is not the new owner"); _transferOwnership(_msgSender()); } } diff --git a/contracts/mocks/SafeOwnableMock.sol b/contracts/mocks/SafeOwnableMock.sol index 079fa5bcd6f..606d0c96436 100644 --- a/contracts/mocks/SafeOwnableMock.sol +++ b/contracts/mocks/SafeOwnableMock.sol @@ -2,6 +2,6 @@ pragma solidity ^0.8.0; -import "../access/SafeOwnable.sol"; +import "../access/Ownable2Step.sol"; -contract SafeOwnableMock is SafeOwnable {} +contract Ownable2StepMock is Ownable2Step {} diff --git a/test/access/Ownable2Step.test.js b/test/access/Ownable2Step.test.js new file mode 100644 index 00000000000..a6a818888e4 --- /dev/null +++ b/test/access/Ownable2Step.test.js @@ -0,0 +1,44 @@ +const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); + +const { expect } = require('chai'); + +const Ownable2Step = artifacts.require('Ownable2StepMock'); + +contract('Ownable2Step', function (accounts) { + const [owner, accountA, accountB] = accounts; + + beforeEach(async function () { + this.ownable2Step = await Ownable2Step.new({ from: owner }); + }); + + describe('transfer ownership', function () { + it('starting a transfer does not change owner', async function () { + const receipt = await this.ownable2Step.transferOwnership(accountA, { from: owner }); + expectEvent(receipt, 'OwnershipTransferStarted'); + expect(await this.ownable2Step.owner()).to.equal(owner); + expect(await this.ownable2Step.pendingOwner()).to.equal(accountA); + }); + + it('changes owner after transfer', async function () { + await this.ownable2Step.transferOwnership(accountA, { from: owner }); + const receipt = await this.ownable2Step.acceptOwnership({ from: accountA }); + expectEvent(receipt, 'OwnershipTransferred'); + expect(await this.ownable2Step.owner()).to.equal(accountA); + expect(await this.ownable2Step.pendingOwner()).to.not.equal(accountA); + }); + + it('changes owner after force transfer', async function () { + await this.ownable2Step.forceTransferOwnership(accountA, { from: owner }); + expect(await this.ownable2Step.owner()).to.equal(accountA); + expect(await this.ownable2Step.pendingOwner()).to.not.equal(accountA); + }); + + it('guards transfer against invalid user', async function () { + await this.ownable2Step.transferOwnership(accountA, { from: owner }); + await expectRevert( + this.ownable2Step.acceptOwnership({ from: accountB }), + 'Ownable2Step: caller is not the new owner', + ); + }); + }); +}); diff --git a/test/access/SafeOwnable.test.js b/test/access/SafeOwnable.test.js deleted file mode 100644 index 48d74074d5b..00000000000 --- a/test/access/SafeOwnable.test.js +++ /dev/null @@ -1,44 +0,0 @@ -const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); - -const { expect } = require('chai'); - -const SafeOwnable = artifacts.require('SafeOwnableMock'); - -contract('SafeOwnable', function (accounts) { - const [owner, accountA, accountB] = accounts; - - beforeEach(async function () { - this.safeOwnable = await SafeOwnable.new({ from: owner }); - }); - - describe('transfer ownership', function () { - it('starting a transfer does not change owner', async function () { - const receipt = await this.safeOwnable.transferOwnership(accountA, { from: owner }); - expectEvent(receipt, 'OwnershipTransferStarted'); - expect(await this.safeOwnable.owner()).to.equal(owner); - expect(await this.safeOwnable.pendingOwner()).to.equal(accountA); - }); - - it('changes owner after transfer', async function () { - await this.safeOwnable.transferOwnership(accountA, { from: owner }); - const receipt = await this.safeOwnable.acceptOwnership({ from: accountA }); - expectEvent(receipt, 'OwnershipTransferred'); - expect(await this.safeOwnable.owner()).to.equal(accountA); - expect(await this.safeOwnable.pendingOwner()).to.not.equal(accountA); - }); - - it('changes owner after force transfer', async function () { - await this.safeOwnable.forceTransferOwnership(accountA, { from: owner }); - expect(await this.safeOwnable.owner()).to.equal(accountA); - expect(await this.safeOwnable.pendingOwner()).to.not.equal(accountA); - }); - - it('guards transfer against invalid user', async function () { - await this.safeOwnable.transferOwnership(accountA, { from: owner }); - await expectRevert( - this.safeOwnable.acceptOwnership({ from: accountB }), - 'SafeOwnable: caller is not the new owner', - ); - }); - }); -}); From 16d5c1658ab0fbc0a299872d40f70a9a543be12c Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 24 Aug 2022 12:19:46 -0400 Subject: [PATCH 21/32] Rename mock to Ownable2StepMock --- contracts/mocks/{SafeOwnableMock.sol => Ownable2StepMock.sol} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename contracts/mocks/{SafeOwnableMock.sol => Ownable2StepMock.sol} (100%) diff --git a/contracts/mocks/SafeOwnableMock.sol b/contracts/mocks/Ownable2StepMock.sol similarity index 100% rename from contracts/mocks/SafeOwnableMock.sol rename to contracts/mocks/Ownable2StepMock.sol From baf063e132625c61f7130208e48437a9fd5ce129 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 24 Aug 2022 13:05:18 -0400 Subject: [PATCH 22/32] Update CHANGELOG.md Co-authored-by: Hadrien Croubois --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2ebc3fc0985..8c2edce4158 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ * `VestingWallet`: add `releasable` getters. ([#3580](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3580)) * `Create2`: optimize address computation by using assembly instead of `abi.encodePacked`. ([#3600](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3600)) * `Clones`: optimized the assembly to use only the scratch space during deployments, and optimized `predictDeterministicAddress` to use lesser operations. ([#3640](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3640)) - * `Ownable2Step`: add new contract Ownable2Step, makes the transfers a two step process `Ownable2Step.sol`. ([#3620](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3620)) + * `Ownable2Step`: extension of `Ownable` that makes the ownership transfers a two step process. ([#3620](https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3620)) ### Deprecations From adcf9aa3f68ddc1ec1f29c9ff70d0b1fdeb5c088 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 24 Aug 2022 13:07:22 -0400 Subject: [PATCH 23/32] Update contracts/access/Ownable2Step.sol Co-authored-by: Hadrien Croubois --- contracts/access/Ownable2Step.sol | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/contracts/access/Ownable2Step.sol b/contracts/access/Ownable2Step.sol index 139ea2e813e..fb49f393949 100644 --- a/contracts/access/Ownable2Step.sol +++ b/contracts/access/Ownable2Step.sol @@ -57,7 +57,8 @@ abstract contract Ownable2Step is Ownable { * @dev The new owner accepts the ownership transfer. */ function acceptOwnership() external { - require(_pendingOwner == _msgSender(), "Ownable2Step: caller is not the new owner"); - _transferOwnership(_msgSender()); + address sender = _msgSender(); + require(_pendingOwner == sender, "Ownable2Step: caller is not the new owner"); + _transferOwnership(sender); } } From 0b46f49c965658da4ef32ae4c0d874eae0660372 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 24 Aug 2022 13:12:13 -0400 Subject: [PATCH 24/32] check the event arguments --- test/access/Ownable2Step.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/access/Ownable2Step.test.js b/test/access/Ownable2Step.test.js index a6a818888e4..055a5b57616 100644 --- a/test/access/Ownable2Step.test.js +++ b/test/access/Ownable2Step.test.js @@ -14,7 +14,7 @@ contract('Ownable2Step', function (accounts) { describe('transfer ownership', function () { it('starting a transfer does not change owner', async function () { const receipt = await this.ownable2Step.transferOwnership(accountA, { from: owner }); - expectEvent(receipt, 'OwnershipTransferStarted'); + expectEvent(receipt, 'OwnershipTransferStarted', {newOwner: accountA}); expect(await this.ownable2Step.owner()).to.equal(owner); expect(await this.ownable2Step.pendingOwner()).to.equal(accountA); }); @@ -22,7 +22,7 @@ contract('Ownable2Step', function (accounts) { it('changes owner after transfer', async function () { await this.ownable2Step.transferOwnership(accountA, { from: owner }); const receipt = await this.ownable2Step.acceptOwnership({ from: accountA }); - expectEvent(receipt, 'OwnershipTransferred'); + expectEvent(receipt, 'OwnershipTransferred', {previousOwner: owner, newOwner: accountA}); expect(await this.ownable2Step.owner()).to.equal(accountA); expect(await this.ownable2Step.pendingOwner()).to.not.equal(accountA); }); From f22673df4b262752a8ccd841d85f9ae33d4e288f Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 24 Aug 2022 13:18:32 -0400 Subject: [PATCH 25/32] format code --- test/access/Ownable2Step.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/access/Ownable2Step.test.js b/test/access/Ownable2Step.test.js index 055a5b57616..6dbf8b31650 100644 --- a/test/access/Ownable2Step.test.js +++ b/test/access/Ownable2Step.test.js @@ -14,7 +14,7 @@ contract('Ownable2Step', function (accounts) { describe('transfer ownership', function () { it('starting a transfer does not change owner', async function () { const receipt = await this.ownable2Step.transferOwnership(accountA, { from: owner }); - expectEvent(receipt, 'OwnershipTransferStarted', {newOwner: accountA}); + expectEvent(receipt, 'OwnershipTransferStarted', { newOwner: accountA }); expect(await this.ownable2Step.owner()).to.equal(owner); expect(await this.ownable2Step.pendingOwner()).to.equal(accountA); }); @@ -22,7 +22,7 @@ contract('Ownable2Step', function (accounts) { it('changes owner after transfer', async function () { await this.ownable2Step.transferOwnership(accountA, { from: owner }); const receipt = await this.ownable2Step.acceptOwnership({ from: accountA }); - expectEvent(receipt, 'OwnershipTransferred', {previousOwner: owner, newOwner: accountA}); + expectEvent(receipt, 'OwnershipTransferred', { previousOwner: owner, newOwner: accountA }); expect(await this.ownable2Step.owner()).to.equal(accountA); expect(await this.ownable2Step.pendingOwner()).to.not.equal(accountA); }); From ac7e601d3194dc41683625e6ac5b104dacd47836 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 24 Aug 2022 13:31:09 -0400 Subject: [PATCH 26/32] Add check pending owner resets on force transfer --- test/access/Ownable2Step.test.js | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/test/access/Ownable2Step.test.js b/test/access/Ownable2Step.test.js index 6dbf8b31650..7d3f1fc6cf6 100644 --- a/test/access/Ownable2Step.test.js +++ b/test/access/Ownable2Step.test.js @@ -1,5 +1,5 @@ -const { expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); - +const { constants, expectEvent, expectRevert } = require('@openzeppelin/test-helpers'); +const { ZERO_ADDRESS } = constants; const { expect } = require('chai'); const Ownable2Step = artifacts.require('Ownable2StepMock'); @@ -33,6 +33,17 @@ contract('Ownable2Step', function (accounts) { expect(await this.ownable2Step.pendingOwner()).to.not.equal(accountA); }); + it('pending owner resets on force transfer', async function () { + await this.ownable2Step.transferOwnership(accountA, { from: owner }); + expect(await this.ownable2Step.pendingOwner()).to.equal(accountA); + await this.ownable2Step.forceTransferOwnership(accountB, { from: owner }); + expect(await this.ownable2Step.pendingOwner()).to.equal(ZERO_ADDRESS); + await expectRevert( + this.ownable2Step.acceptOwnership({ from: accountB }), + 'Ownable2Step: caller is not the new owner', + ); + }); + it('guards transfer against invalid user', async function () { await this.ownable2Step.transferOwnership(accountA, { from: owner }); await expectRevert( From b7814270a61e90834f62ce6217293e91722e9850 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 24 Aug 2022 15:36:25 -0400 Subject: [PATCH 27/32] Update Ownable2Step.test.js --- test/access/Ownable2Step.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/access/Ownable2Step.test.js b/test/access/Ownable2Step.test.js index 7d3f1fc6cf6..cd0253842b9 100644 --- a/test/access/Ownable2Step.test.js +++ b/test/access/Ownable2Step.test.js @@ -39,7 +39,7 @@ contract('Ownable2Step', function (accounts) { await this.ownable2Step.forceTransferOwnership(accountB, { from: owner }); expect(await this.ownable2Step.pendingOwner()).to.equal(ZERO_ADDRESS); await expectRevert( - this.ownable2Step.acceptOwnership({ from: accountB }), + this.ownable2Step.acceptOwnership({ from: accountA }), 'Ownable2Step: caller is not the new owner', ); }); From 81c1f6c0f01cad6dae89968b85564cc7beb949ed Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Mon, 29 Aug 2022 18:43:13 +0200 Subject: [PATCH 28/32] Update contracts/access/Ownable2Step.sol Co-authored-by: Hadrien Croubois --- contracts/access/Ownable2Step.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/Ownable2Step.sol b/contracts/access/Ownable2Step.sol index fb49f393949..633d243e8a1 100644 --- a/contracts/access/Ownable2Step.sol +++ b/contracts/access/Ownable2Step.sol @@ -58,7 +58,7 @@ abstract contract Ownable2Step is Ownable { */ function acceptOwnership() external { address sender = _msgSender(); - require(_pendingOwner == sender, "Ownable2Step: caller is not the new owner"); + require(pendingOwner() == sender, "Ownable2Step: caller is not the new owner"); _transferOwnership(sender); } } From a36cc8e4b1c0d5017008f2e0dfeb34d8b1e95f0e Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 30 Aug 2022 04:53:42 -0400 Subject: [PATCH 29/32] Update contracts/access/Ownable2Step.sol Co-authored-by: Francisco --- contracts/access/Ownable2Step.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/Ownable2Step.sol b/contracts/access/Ownable2Step.sol index 633d243e8a1..1fd33645da3 100644 --- a/contracts/access/Ownable2Step.sol +++ b/contracts/access/Ownable2Step.sol @@ -45,7 +45,7 @@ abstract contract Ownable2Step is Ownable { } /** - * @dev Transfers ownership of the contract to a new account (`newOwner`). + * @dev Transfers ownership of the contract to a new account (`newOwner`) and deletes any pending owner. * Internal function without access restriction. */ function _transferOwnership(address newOwner) internal virtual override { From b0e363aee9986bb32f4672917579281513d23aa5 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 30 Aug 2022 04:58:23 -0400 Subject: [PATCH 30/32] Update contracts/access/Ownable2Step.sol Co-authored-by: Francisco --- contracts/access/Ownable2Step.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/access/Ownable2Step.sol b/contracts/access/Ownable2Step.sol index 1fd33645da3..ee2da541cc3 100644 --- a/contracts/access/Ownable2Step.sol +++ b/contracts/access/Ownable2Step.sol @@ -29,7 +29,7 @@ abstract contract Ownable2Step is Ownable { } /** - * @dev Starts the ownership transfer of the contract to a new account + * @dev Starts the ownership transfer of the contract to a new account. Replaces the pending transfer if there is one. * Can only be called by the current owner. */ function transferOwnership(address newOwner) public virtual override onlyOwner { From 8ff062167e4ca2bdd0dd42bc5afca03455d14963 Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Tue, 30 Aug 2022 05:03:37 -0400 Subject: [PATCH 31/32] Add previousOwner to event OwnershipTransferStarted --- contracts/access/Ownable2Step.sol | 4 ++-- test/access/Ownable2Step.test.js | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/access/Ownable2Step.sol b/contracts/access/Ownable2Step.sol index ee2da541cc3..0d5aa857ecb 100644 --- a/contracts/access/Ownable2Step.sol +++ b/contracts/access/Ownable2Step.sol @@ -19,7 +19,7 @@ import "./Ownable.sol"; abstract contract Ownable2Step is Ownable { address private _pendingOwner; - event OwnershipTransferStarted(address indexed newOwner); + event OwnershipTransferStarted(address indexed previousOwner, address indexed newOwner); /** * @dev Returns the address of the pending owner. @@ -34,7 +34,7 @@ abstract contract Ownable2Step is Ownable { */ function transferOwnership(address newOwner) public virtual override onlyOwner { _pendingOwner = newOwner; - emit OwnershipTransferStarted(newOwner); + emit OwnershipTransferStarted(owner(), newOwner); } /** diff --git a/test/access/Ownable2Step.test.js b/test/access/Ownable2Step.test.js index cd0253842b9..2f4c45b9494 100644 --- a/test/access/Ownable2Step.test.js +++ b/test/access/Ownable2Step.test.js @@ -14,7 +14,7 @@ contract('Ownable2Step', function (accounts) { describe('transfer ownership', function () { it('starting a transfer does not change owner', async function () { const receipt = await this.ownable2Step.transferOwnership(accountA, { from: owner }); - expectEvent(receipt, 'OwnershipTransferStarted', { newOwner: accountA }); + expectEvent(receipt, 'OwnershipTransferStarted', { previousOwner: owner, newOwner: accountA }); expect(await this.ownable2Step.owner()).to.equal(owner); expect(await this.ownable2Step.pendingOwner()).to.equal(accountA); }); From 84f3b9f5a8e3b53bad24620583347ebdb787ceaa Mon Sep 17 00:00:00 2001 From: Helder Sepulveda Date: Wed, 31 Aug 2022 03:46:54 -0400 Subject: [PATCH 32/32] Remove forceTransferOwnership --- contracts/access/Ownable2Step.sol | 7 ------- test/access/Ownable2Step.test.js | 14 ++++++++------ 2 files changed, 8 insertions(+), 13 deletions(-) diff --git a/contracts/access/Ownable2Step.sol b/contracts/access/Ownable2Step.sol index 0d5aa857ecb..5ee5cf8c7f6 100644 --- a/contracts/access/Ownable2Step.sol +++ b/contracts/access/Ownable2Step.sol @@ -37,13 +37,6 @@ abstract contract Ownable2Step is Ownable { emit OwnershipTransferStarted(owner(), newOwner); } - /** - * @dev Make the direct ownership transfer available as a fallback when the receiver cannot accept. - */ - function forceTransferOwnership(address newOwner) public virtual { - super.transferOwnership(newOwner); - } - /** * @dev Transfers ownership of the contract to a new account (`newOwner`) and deletes any pending owner. * Internal function without access restriction. diff --git a/test/access/Ownable2Step.test.js b/test/access/Ownable2Step.test.js index 2f4c45b9494..0aeb2246541 100644 --- a/test/access/Ownable2Step.test.js +++ b/test/access/Ownable2Step.test.js @@ -27,16 +27,18 @@ contract('Ownable2Step', function (accounts) { expect(await this.ownable2Step.pendingOwner()).to.not.equal(accountA); }); - it('changes owner after force transfer', async function () { - await this.ownable2Step.forceTransferOwnership(accountA, { from: owner }); - expect(await this.ownable2Step.owner()).to.equal(accountA); - expect(await this.ownable2Step.pendingOwner()).to.not.equal(accountA); + it('changes owner after renouncing ownership', async function () { + await this.ownable2Step.renounceOwnership({ from: owner }); + // If renounceOwnership is removed from parent an alternative is needed ... + // without it is difficult to cleanly renounce with the two step process + // see: https://github.com/OpenZeppelin/openzeppelin-contracts/pull/3620#discussion_r957930388 + expect(await this.ownable2Step.owner()).to.equal(ZERO_ADDRESS); }); - it('pending owner resets on force transfer', async function () { + it('pending owner resets after renouncing ownership', async function () { await this.ownable2Step.transferOwnership(accountA, { from: owner }); expect(await this.ownable2Step.pendingOwner()).to.equal(accountA); - await this.ownable2Step.forceTransferOwnership(accountB, { from: owner }); + await this.ownable2Step.renounceOwnership({ from: owner }); expect(await this.ownable2Step.pendingOwner()).to.equal(ZERO_ADDRESS); await expectRevert( this.ownable2Step.acceptOwnership({ from: accountA }),