From c686d3441357413113ce39193542361e21a17473 Mon Sep 17 00:00:00 2001 From: Lukas Date: Mon, 27 Jan 2025 14:34:15 +0100 Subject: [PATCH] refactor: use oz ownable opposed to custom copy (#60) --- .../access-control/OwnableWithGuardian.sol | 9 +- .../UpgradeableOwnableWithGuardian.sol | 10 --- .../interfaces/IWithGuardian.sol | 10 +++ src/contracts/oz-common/Ownable.sol | 84 ------------------- test/OwnableWithGuardian.t.sol | 14 +++- test/UpgradeableOwnableWithGuardian.t.sol | 17 +--- test/create3Test.t.sol | 5 +- 7 files changed, 31 insertions(+), 118 deletions(-) delete mode 100644 src/contracts/oz-common/Ownable.sol diff --git a/src/contracts/access-control/OwnableWithGuardian.sol b/src/contracts/access-control/OwnableWithGuardian.sol index 31c434e..8505cac 100644 --- a/src/contracts/access-control/OwnableWithGuardian.sol +++ b/src/contracts/access-control/OwnableWithGuardian.sol @@ -1,13 +1,13 @@ // SPDX-License-Identifier: MIT pragma solidity >=0.7.0; +import {Ownable} from 'openzeppelin-contracts/contracts/access/Ownable.sol'; import {IWithGuardian} from './interfaces/IWithGuardian.sol'; -import {Ownable} from '../oz-common/Ownable.sol'; abstract contract OwnableWithGuardian is Ownable, IWithGuardian { address private _guardian; - constructor() { + constructor(address initialOwner) Ownable(initialOwner) { _updateGuardian(_msgSender()); } @@ -41,10 +41,11 @@ abstract contract OwnableWithGuardian is Ownable, IWithGuardian { } function _checkGuardian() internal view { - require(guardian() == _msgSender(), 'ONLY_BY_GUARDIAN'); + if (guardian() != _msgSender()) revert OnlyGuardianInvalidCaller(_msgSender()); } function _checkOwnerOrGuardian() internal view { - require(_msgSender() == owner() || _msgSender() == guardian(), 'ONLY_BY_OWNER_OR_GUARDIAN'); + if (_msgSender() != owner() && _msgSender() != guardian()) + revert OnlyGuardianOrOwnerInvalidCaller(_msgSender()); } } diff --git a/src/contracts/access-control/UpgradeableOwnableWithGuardian.sol b/src/contracts/access-control/UpgradeableOwnableWithGuardian.sol index a9ae741..28bcda7 100644 --- a/src/contracts/access-control/UpgradeableOwnableWithGuardian.sol +++ b/src/contracts/access-control/UpgradeableOwnableWithGuardian.sol @@ -24,16 +24,6 @@ abstract contract UpgradeableOwnableWithGuardian is OwnableUpgradeable, IWithGua } } - /** - * @dev The caller account is not authorized to perform an operation. - */ - error OnlyGuardianInvalidCaller(address account); - - /** - * @dev The caller account is not authorized to perform an operation. - */ - error OnlyGuardianOrOwnerInvalidCaller(address account); - /** * @dev Initializes the contract setting the address provided by the deployer as the initial owner. */ diff --git a/src/contracts/access-control/interfaces/IWithGuardian.sol b/src/contracts/access-control/interfaces/IWithGuardian.sol index f2102dd..f7271c6 100644 --- a/src/contracts/access-control/interfaces/IWithGuardian.sol +++ b/src/contracts/access-control/interfaces/IWithGuardian.sol @@ -9,6 +9,16 @@ interface IWithGuardian { */ event GuardianUpdated(address oldGuardian, address newGuardian); + /** + * @dev The caller account is not authorized to perform an operation. + */ + error OnlyGuardianInvalidCaller(address account); + + /** + * @dev The caller account is not authorized to perform an operation. + */ + error OnlyGuardianOrOwnerInvalidCaller(address account); + /** * @dev get guardian address; */ diff --git a/src/contracts/oz-common/Ownable.sol b/src/contracts/oz-common/Ownable.sol deleted file mode 100644 index e1f2ad8..0000000 --- a/src/contracts/oz-common/Ownable.sol +++ /dev/null @@ -1,84 +0,0 @@ -// SPDX-License-Identifier: MIT -// OpenZeppelin Contracts (last updated v4.7.0) (access/Ownable.sol) -// From commit https://github.com/OpenZeppelin/openzeppelin-contracts/commit/8b778fa20d6d76340c5fac1ed66c80273f05b95a - -pragma solidity ^0.8.0; - -import './Context.sol'; - -/** - * @dev Contract module which provides a basic 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}. - * - * This module is used through inheritance. It will make available the modifier - * `onlyOwner`, which can be applied to your functions to restrict their use to - * the owner. - */ -abstract contract Ownable is Context { - address private _owner; - - event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); - - /** - * @dev Initializes the contract setting the deployer as the initial owner. - */ - constructor() { - _transferOwnership(_msgSender()); - } - - /** - * @dev Throws if called by any account other than the owner. - */ - modifier onlyOwner() { - _checkOwner(); - _; - } - - /** - * @dev Returns the address of the current owner. - */ - function owner() public view virtual returns (address) { - return _owner; - } - - /** - * @dev Throws if the sender is not the owner. - */ - function _checkOwner() internal view virtual { - require(owner() == _msgSender(), 'Ownable: caller is not the owner'); - } - - /** - * @dev Leaves the contract without owner. It will not be possible to call - * `onlyOwner` functions anymore. Can only be called by the current owner. - * - * NOTE: Renouncing ownership will leave the contract without an owner, - * thereby removing any functionality that is only available to the owner. - */ - function renounceOwnership() public virtual onlyOwner { - _transferOwnership(address(0)); - } - - /** - * @dev Transfers ownership of the contract to a new account (`newOwner`). - * 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'); - _transferOwnership(newOwner); - } - - /** - * @dev Transfers ownership of the contract to a new account (`newOwner`). - * Internal function without access restriction. - */ - function _transferOwnership(address newOwner) internal virtual { - address oldOwner = _owner; - _owner = newOwner; - emit OwnershipTransferred(oldOwner, newOwner); - } -} diff --git a/test/OwnableWithGuardian.t.sol b/test/OwnableWithGuardian.t.sol index 02d794f..3e30c0e 100644 --- a/test/OwnableWithGuardian.t.sol +++ b/test/OwnableWithGuardian.t.sol @@ -2,9 +2,11 @@ pragma solidity ^0.8.0; import 'forge-std/Test.sol'; -import {OwnableWithGuardian} from '../src/contracts/access-control/OwnableWithGuardian.sol'; +import {OwnableWithGuardian, IWithGuardian} from '../src/contracts/access-control/OwnableWithGuardian.sol'; contract ImplOwnableWithGuardian is OwnableWithGuardian { + constructor(address initialOwner) OwnableWithGuardian(initialOwner) {} + function mock_onlyGuardian() external onlyGuardian {} function mock_onlyOwnerOrGuardian() external onlyOwnerOrGuardian {} @@ -17,7 +19,7 @@ contract TestOfOwnableWithGuardian is Test { address guardian = makeAddr('guardian'); function setUp() public { - withGuardian = new ImplOwnableWithGuardian(); + withGuardian = new ImplOwnableWithGuardian(address(this)); assertEq(withGuardian.owner(), address(this)); assertEq(withGuardian.guardian(), address(this)); withGuardian.transferOwnership(owner); @@ -40,7 +42,9 @@ contract TestOfOwnableWithGuardian is Test { } function testGuardianUpdateNoAccess() external { - vm.expectRevert('ONLY_BY_OWNER_OR_GUARDIAN'); + vm.expectRevert( + abi.encodeWithSelector(IWithGuardian.OnlyGuardianOrOwnerInvalidCaller.selector, address(this)) + ); withGuardian.updateGuardian(guardian); } @@ -50,7 +54,9 @@ contract TestOfOwnableWithGuardian is Test { } function test_onlyGuardianGuard_shouldRevert() external { - vm.expectRevert('ONLY_BY_GUARDIAN'); + vm.expectRevert( + abi.encodeWithSelector(IWithGuardian.OnlyGuardianInvalidCaller.selector, address(this)) + ); withGuardian.mock_onlyGuardian(); } } diff --git a/test/UpgradeableOwnableWithGuardian.t.sol b/test/UpgradeableOwnableWithGuardian.t.sol index 61b0915..38de261 100644 --- a/test/UpgradeableOwnableWithGuardian.t.sol +++ b/test/UpgradeableOwnableWithGuardian.t.sol @@ -2,7 +2,7 @@ pragma solidity ^0.8.20; import 'forge-std/Test.sol'; -import {UpgradeableOwnableWithGuardian} from '../src/contracts/access-control/UpgradeableOwnableWithGuardian.sol'; +import {UpgradeableOwnableWithGuardian, IWithGuardian} from '../src/contracts/access-control/UpgradeableOwnableWithGuardian.sol'; contract ImplOwnableWithGuardian is UpgradeableOwnableWithGuardian { function initialize(address owner, address guardian) public initializer { @@ -33,20 +33,14 @@ contract TestOfUpgradableOwnableWithGuardian is Test { function test_onlyGuardian() external { vm.expectRevert( - abi.encodeWithSelector( - UpgradeableOwnableWithGuardian.OnlyGuardianInvalidCaller.selector, - address(this) - ) + abi.encodeWithSelector(IWithGuardian.OnlyGuardianInvalidCaller.selector, address(this)) ); ImplOwnableWithGuardian(address(withGuardian)).mock_onlyGuardian(); } function test_onlyOwnerOrGuardian() external { vm.expectRevert( - abi.encodeWithSelector( - UpgradeableOwnableWithGuardian.OnlyGuardianOrOwnerInvalidCaller.selector, - address(this) - ) + abi.encodeWithSelector(IWithGuardian.OnlyGuardianOrOwnerInvalidCaller.selector, address(this)) ); ImplOwnableWithGuardian(address(withGuardian)).mock_onlyOwnerOrGuardian(); } @@ -66,10 +60,7 @@ contract TestOfUpgradableOwnableWithGuardian is Test { vm.prank(eoa); vm.expectRevert( - abi.encodeWithSelector( - UpgradeableOwnableWithGuardian.OnlyGuardianOrOwnerInvalidCaller.selector, - eoa - ) + abi.encodeWithSelector(IWithGuardian.OnlyGuardianOrOwnerInvalidCaller.selector, eoa) ); withGuardian.updateGuardian(newGuardian); } diff --git a/test/create3Test.t.sol b/test/create3Test.t.sol index 7f7dec6..b124354 100644 --- a/test/create3Test.t.sol +++ b/test/create3Test.t.sol @@ -2,16 +2,15 @@ pragma solidity ^0.8.0; import 'forge-std/Test.sol'; +import {Ownable} from 'openzeppelin-contracts/contracts/access/Ownable.sol'; import {Create3Factory, ICreate3Factory, Create3} from '../src/contracts/create3/Create3Factory.sol'; -import {Ownable} from '../src/contracts/oz-common/Ownable.sol'; contract MockContract is Ownable { address public immutable SOME_ADDRESS; address internal _someOtherAddress; - constructor(address someAddress, address owner) { + constructor(address someAddress, address owner) Ownable(owner) { SOME_ADDRESS = someAddress; - _transferOwnership(owner); } function getOtherAddress() external view returns (address) {