From b54a54228b9fb3bdff05ca046618f99a9f6a04cc Mon Sep 17 00:00:00 2001 From: Alin Mihai Barbatei Date: Thu, 5 Jan 2023 01:46:46 +0200 Subject: [PATCH 1/7] added Updatable and Upgradable Operator Filterer --- .../UpdatableExampleERC721Upgradeable.sol | 60 ++++ .../UpdatableOperatorFiltererUpgradeable.sol | 90 ++++++ .../UpdatableExampleERC721Upgradeable.t.sol | 260 ++++++++++++++++++ 3 files changed, 410 insertions(+) create mode 100644 src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol create mode 100644 src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol create mode 100644 test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol diff --git a/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol b/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol new file mode 100644 index 0000000..5426d01 --- /dev/null +++ b/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol @@ -0,0 +1,60 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +import {ERC721Upgradeable} from "openzeppelin-contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol"; +import {UpdatableOperatorFiltererUpgradeable} from + "../../upgradeable/UpdatableOperatorFiltererUpgradeable.sol"; +import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/access/OwnableUpgradeable.sol"; + +/** + * @title UpdatableExampleERC721Upgradeable + * @notice This example contract is configured to use the UpdatableOperatorFiltererUpgradeable, which registers the + * token and subscribes it to a give register filter. + * Adding the onlyAllowedOperator modifier to the setApprovalForAll, approve, transferFrom, safeTransferFrom (both version) methods ensures that + * the msg.sender (operator) is allowed by the OperatorFilterRegistry. + */ +abstract contract UpdatableExampleERC721Upgradeable is + ERC721Upgradeable, + UpdatableOperatorFiltererUpgradeable, + OwnableUpgradeable +{ + function initialize(address _registry, address subscriptionOrRegistrantToCopy, bool subscribe) public initializer { + __ERC721_init("Example", "EXAMPLE"); + __Ownable_init(); + __UpdatableOperatorFiltererUpgradeable_init(_registry, subscriptionOrRegistrantToCopy, subscribe); + } + + function setApprovalForAll(address operator, bool approved) public override onlyAllowedOperatorApproval(operator) { + super.setApprovalForAll(operator, approved); + } + + function approve(address operator, uint256 tokenId) public override onlyAllowedOperatorApproval(operator) { + super.approve(operator, tokenId); + } + + function transferFrom(address from, address to, uint256 tokenId) public override onlyAllowedOperator(from) { + super.transferFrom(from, to, tokenId); + } + + function safeTransferFrom(address from, address to, uint256 tokenId) public override onlyAllowedOperator(from) { + super.safeTransferFrom(from, to, tokenId); + } + + function safeTransferFrom(address from, address to, uint256 tokenId, bytes memory data) + public + override + onlyAllowedOperator(from) + { + super.safeTransferFrom(from, to, tokenId, data); + } + + function owner() + public + view + virtual + override (OwnableUpgradeable, UpdatableOperatorFiltererUpgradeable) + returns (address) + { + return OwnableUpgradeable.owner(); + } +} diff --git a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol new file mode 100644 index 0000000..939c08e --- /dev/null +++ b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol @@ -0,0 +1,90 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +import {IOperatorFilterRegistry} from "../IOperatorFilterRegistry.sol"; +import {Initializable} from "openzeppelin-contracts-upgradeable/proxy/utils/Initializable.sol"; + + +/** + * @title UpdatableOperatorFiltererUpgradeable + * @notice Abstract contract whose init function automatically registers and optionally subscribes to or copies another + * registrant's entries in the OperatorFilterRegistry. This contract allows the Owner to update the + * OperatorFilterRegistry address via updateOperatorFilterRegistryAddress, including to the zero address, + * which will bypass registry checks. + * Note that OpenSea will still disable creator fee enforcement if filtered operators begin fulfilling orders + * on-chain, eg, if the registry is revoked or bypassed. + * @dev This smart contract is meant to be inherited by token contracts so they can use the following: + * - `onlyAllowedOperator` modifier for `transferFrom` and `safeTransferFrom` methods. + * - `onlyAllowedOperatorApproval` modifier for `approve` and `setApprovalForAll` methods. + * Also use updateOperatorFilterRegistryAddress function to change registry address if needed + */ +abstract contract UpdatableOperatorFiltererUpgradeable is Initializable { + error OperatorNotAllowed(address operator); + error OnlyOwner(); + + IOperatorFilterRegistry public operatorFilterRegistry; + + function __UpdatableOperatorFiltererUpgradeable_init(address _registry, address subscriptionOrRegistrantToCopy, bool subscribe) + internal + onlyInitializing + { + operatorFilterRegistry = IOperatorFilterRegistry(_registry); + // If an inheriting token contract is deployed to a network without the registry deployed, the modifier + // will not revert, but the contract will need to be registered with the registry once it is deployed in + // order for the modifier to filter addresses. + if (address(operatorFilterRegistry).code.length > 0) { + if (subscribe) { + operatorFilterRegistry.registerAndSubscribe(address(this), subscriptionOrRegistrantToCopy); + } else { + if (subscriptionOrRegistrantToCopy != address(0)) { + operatorFilterRegistry.registerAndCopyEntries(address(this), subscriptionOrRegistrantToCopy); + } else { + operatorFilterRegistry.register(address(this)); + } + } + } + } + + modifier onlyAllowedOperator(address from) virtual { + // Check registry code length to facilitate testing in environments without a deployed registry. + if (address(operatorFilterRegistry).code.length > 0) { + // Allow spending tokens from addresses with balance + // Note that this still allows listings and marketplaces with escrow to transfer tokens if transferred + // from an EOA. + if (from == msg.sender) { + _; + return; + } + if (!operatorFilterRegistry.isOperatorAllowed(address(this), msg.sender)) { + revert OperatorNotAllowed(msg.sender); + } + } + _; + } + + modifier onlyAllowedOperatorApproval(address operator) virtual { + // Check registry code length to facilitate testing in environments without a deployed registry. + if (address(operatorFilterRegistry).code.length > 0) { + if (!operatorFilterRegistry.isOperatorAllowed(address(this), operator)) { + revert OperatorNotAllowed(operator); + } + } + _; + } + + /** + * @notice Update the address that the contract will make OperatorFilter checks against. When set to the zero + * address, checks will be bypassed. OnlyOwner. + */ + function updateOperatorFilterRegistryAddress(address newRegistry) public virtual { + if (msg.sender != owner()) { + revert OnlyOwner(); + } + operatorFilterRegistry = IOperatorFilterRegistry(newRegistry); + } + + /** + * @dev assume the contract has an owner, but leave specific Ownable implementation up to inheriting contract + */ + function owner() public view virtual returns (address); +} diff --git a/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol b/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol new file mode 100644 index 0000000..1f3167b --- /dev/null +++ b/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol @@ -0,0 +1,260 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.13; + +import {UpdatableExampleERC721Upgradeable} from + "../../../src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol"; +import {UpdatableOperatorFiltererUpgradeable} from + "../../../src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol"; +import {BaseRegistryTest} from "../../BaseRegistryTest.sol"; + +import {Initializable} from "openzeppelin-contracts-upgradeable/proxy/utils/Initializable.sol"; +import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/access/OwnableUpgradeable.sol"; +import {Vm} from "forge-std/Vm.sol"; + +contract TestableUpdatableExampleERC721Upgradeable is UpdatableExampleERC721Upgradeable { + function mint(address to, uint256 tokenId) external { + _mint(to, tokenId); + } +} + +contract UpdatableExampleERC721UpgradeableForUpgradableTest is BaseRegistryTest, Initializable { + TestableUpdatableExampleERC721Upgradeable example; + address filteredAddress; + address constant DEFAULT_SUBSCRIPTION = address(0x3cc6CddA760b79bAfa08dF41ECFA224f810dCeB6); + + function setUp() public override { + super.setUp(); + + vm.startPrank(DEFAULT_SUBSCRIPTION); + registry.register(DEFAULT_SUBSCRIPTION); + + filteredAddress = makeAddr("filtered address"); + registry.updateOperator(address(DEFAULT_SUBSCRIPTION), filteredAddress, true); + vm.stopPrank(); + + example = new TestableUpdatableExampleERC721Upgradeable(); + example.initialize(address(registry), DEFAULT_SUBSCRIPTION, true); + } + + function testUpgradeable() public { + TestableUpdatableExampleERC721Upgradeable example2 = new TestableUpdatableExampleERC721Upgradeable(); + vm.expectEmit(true, true, false, true, address(example2)); + emit Initialized(1); + example2.initialize(address(registry), DEFAULT_SUBSCRIPTION, true); + vm.expectRevert(bytes("Initializable: contract is already initialized")); + example2.initialize(address(registry), DEFAULT_SUBSCRIPTION, true); + } + + function testFilter() public { + vm.startPrank(address(filteredAddress)); + vm.expectRevert(abi.encodeWithSelector(AddressFiltered.selector, filteredAddress)); + example.transferFrom(makeAddr("from"), makeAddr("to"), 1); + vm.expectRevert(abi.encodeWithSelector(AddressFiltered.selector, filteredAddress)); + example.safeTransferFrom(makeAddr("from"), makeAddr("to"), 1); + vm.expectRevert(abi.encodeWithSelector(AddressFiltered.selector, filteredAddress)); + example.safeTransferFrom(makeAddr("from"), makeAddr("to"), 1, ""); + } + + function testOwnersNotExcluded() public { + address alice = address(0xA11CE); + example.mint(alice, 1); + + vm.prank(DEFAULT_SUBSCRIPTION); + registry.updateOperator(address(DEFAULT_SUBSCRIPTION), alice, true); + + vm.prank(alice); + example.transferFrom(alice, makeAddr("to"), 1); + } + + function testOwnersNotExcludedSafeTransfer() public { + address alice = address(0xA11CE); + example.mint(alice, 1); + example.mint(alice, 2); + + vm.prank(DEFAULT_SUBSCRIPTION); + registry.updateOperator(address(DEFAULT_SUBSCRIPTION), alice, true); + + vm.startPrank(alice); + example.safeTransferFrom(alice, makeAddr("to"), 1); + example.safeTransferFrom(alice, makeAddr("to"), 2, ""); + } + + function testExclusionExceptionDoesNotApplyToOperators() public { + address alice = address(0xA11CE); + address bob = address(0xB0B); + example.mint(bob, 1); + vm.prank(bob); + example.setApprovalForAll(alice, true); + + vm.prank(DEFAULT_SUBSCRIPTION); + registry.updateOperator(address(DEFAULT_SUBSCRIPTION), alice, true); + + vm.startPrank(alice); + vm.expectRevert(abi.encodeWithSelector(AddressFiltered.selector, alice)); + example.transferFrom(bob, makeAddr("to"), 1); + } + + function testExcludeApprovals() public { + address alice = address(0xA11CE); + address bob = address(0xB0B); + example.mint(bob, 1); + + vm.prank(DEFAULT_SUBSCRIPTION); + registry.updateOperator(address(DEFAULT_SUBSCRIPTION), alice, true); + + vm.startPrank(bob); + vm.expectRevert(abi.encodeWithSelector(AddressFiltered.selector, alice)); + example.setApprovalForAll(alice, true); + + vm.expectRevert(abi.encodeWithSelector(AddressFiltered.selector, alice)); + example.approve(alice, 1); + } +} + + +contract ConcreteUpdatableOperatorFiltererUpgradable is UpdatableOperatorFiltererUpgradeable, OwnableUpgradeable { + + function initialize(address registry, address registrant, bool sub) public initializer { + __Ownable_init(); + __UpdatableOperatorFiltererUpgradeable_init(registry, registrant, sub); + } + + function testFilter(address from) public view onlyAllowedOperator(from) returns (bool) { + return true; + } + + function owner() + public + view + virtual + override (OwnableUpgradeable, UpdatableOperatorFiltererUpgradeable) + returns (address) + { + return OwnableUpgradeable.owner(); + } +} + +contract UpdatableExampleERC721UpgradeableForUpdatableTest is BaseRegistryTest { + ConcreteUpdatableOperatorFiltererUpgradable filterer; + address filteredAddress; + address filteredCodeHashAddress; + bytes32 filteredCodeHash; + address notFiltered; + + function setUp() public override { + super.setUp(); + notFiltered = makeAddr("not filtered"); + filterer = new ConcreteUpdatableOperatorFiltererUpgradable(); + filterer.initialize(address(registry), address(0), false); + filteredAddress = makeAddr("filtered address"); + registry.updateOperator(address(filterer), filteredAddress, true); + filteredCodeHashAddress = makeAddr("filtered code hash"); + bytes memory code = hex"deadbeef"; + filteredCodeHash = keccak256(code); + registry.updateCodeHash(address(filterer), filteredCodeHash, true); + vm.etch(filteredCodeHashAddress, code); + } + + function testFilter() public { + assertTrue(filterer.testFilter(notFiltered)); + vm.startPrank(filteredAddress); + vm.expectRevert(abi.encodeWithSelector(AddressFiltered.selector, filteredAddress)); + filterer.testFilter(address(0)); + vm.stopPrank(); + vm.startPrank(filteredCodeHashAddress); + vm.expectRevert(abi.encodeWithSelector(CodeHashFiltered.selector, filteredCodeHashAddress, filteredCodeHash)); + filterer.testFilter(address(0)); + } + + event OwnershipTransferred(address indexed previousOwner, address indexed newOwner); + + function testConstructory_noSubscribeOrCopy() public { + vm.recordLogs(); + ConcreteUpdatableOperatorFiltererUpgradable filterer2 = new ConcreteUpdatableOperatorFiltererUpgradable(); + filterer2.initialize(address(registry), address(0), false); + Vm.Log[] memory logs = vm.getRecordedLogs(); + assertEq(logs.length, 3); + assertEq(logs[0].topics[0], keccak256("OwnershipTransferred(address,address)")); + assertEq(logs[1].topics[0], keccak256("RegistrationUpdated(address,bool)")); + assertEq(address(uint160(uint256(logs[1].topics[1]))), address(filterer2)); + assertEq(logs[2].topics[0], keccak256("Initialized(uint8)")); + + } + + function testConstructor_copy() public { + address deployed = computeCreateAddress(address(this), vm.getNonce(address(this))); + vm.expectEmit(true, false, false, false, address(registry)); + emit RegistrationUpdated(deployed, true); + vm.expectEmit(true, true, true, false, address(registry)); + emit OperatorUpdated(deployed, filteredAddress, true); + vm.expectEmit(true, true, true, false, address(registry)); + emit CodeHashUpdated(deployed, filteredCodeHash, true); + + vm.recordLogs(); + ConcreteUpdatableOperatorFiltererUpgradable filterer2 = new ConcreteUpdatableOperatorFiltererUpgradable(); + filterer2.initialize(address(registry), address(filterer), false); + + Vm.Log[] memory logs = vm.getRecordedLogs(); + assertEq(logs.length, 5); + assertEq(logs[0].topics[0], keccak256("OwnershipTransferred(address,address)")); + assertEq(logs[1].topics[0], keccak256("RegistrationUpdated(address,bool)")); + assertEq(address(uint160(uint256(logs[1].topics[1]))), address(filterer2)); + assertEq(logs[2].topics[0], keccak256("OperatorUpdated(address,address,bool)")); + assertEq(address(uint160(uint256(logs[2].topics[1]))), address(filterer2)); + assertEq(logs[3].topics[0], keccak256("CodeHashUpdated(address,bytes32,bool)")); + assertEq(address(uint160(uint256(logs[3].topics[1]))), address(filterer2)); + assertEq(logs[4].topics[0], keccak256("Initialized(uint8)")); + } + + function testConstructor_subscribe() public { + address deployed = computeCreateAddress(address(this), vm.getNonce(address(this))); + vm.expectEmit(true, false, false, false, address(registry)); + emit RegistrationUpdated(deployed, true); + vm.expectEmit(true, true, true, false, address(registry)); + emit SubscriptionUpdated(deployed, address(filterer), true); + + vm.recordLogs(); + ConcreteUpdatableOperatorFiltererUpgradable filterer2 = new ConcreteUpdatableOperatorFiltererUpgradable(); + filterer2.initialize(address(registry), address(filterer), true); + + Vm.Log[] memory logs = vm.getRecordedLogs(); + assertEq(logs.length, 4); + assertEq(logs[0].topics[0], keccak256("OwnershipTransferred(address,address)")); + assertEq(logs[1].topics[0], keccak256("RegistrationUpdated(address,bool)")); + assertEq(address(uint160(uint256(logs[1].topics[1]))), address(filterer2)); + assertEq(logs[2].topics[0], keccak256("SubscriptionUpdated(address,address,bool)")); + assertEq(address(uint160(uint256(logs[2].topics[1]))), address(filterer2)); + assertEq(logs[3].topics[0], keccak256("Initialized(uint8)")); + } + + function testRegistryNotDeployedDoesNotRevert() public { + vm.etch(address(registry), ""); + ConcreteUpdatableOperatorFiltererUpgradable filterer2 = new ConcreteUpdatableOperatorFiltererUpgradable(); + filterer2.initialize(address(registry), address(0), false); + assertTrue(filterer2.testFilter(notFiltered)); + } + + function testUpdateRegistry() public { + address newRegistry = makeAddr("new registry"); + filterer.updateOperatorFilterRegistryAddress(newRegistry); + assertEq(address(filterer.operatorFilterRegistry()), newRegistry); + } + + function testUpdateRegistry_onlyOwner() public { + vm.startPrank(makeAddr("notOwner")); + vm.expectRevert(abi.encodeWithSignature("OnlyOwner()")); + filterer.updateOperatorFilterRegistryAddress(address(0)); + } + + function testZeroAddressBypass() public { + filterer.updateOperatorFilterRegistryAddress(address(0)); + vm.prank(filteredAddress); + assertTrue(filterer.testFilter(address(0))); + + // can update even if registry is zero address + filterer.updateOperatorFilterRegistryAddress(address(registry)); + vm.startPrank(filteredAddress); + vm.expectRevert(abi.encodeWithSelector(AddressFiltered.selector, filteredAddress)); + filterer.testFilter(address(0)); + } +} From 33b04140dc9732c5f70ff5fe655a5d41ddd88abb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20B=C4=83laj?= Date: Thu, 5 Jan 2023 08:59:51 +0200 Subject: [PATCH 2/7] lint and reorder imports --- .../upgradeable/UpdatableExampleERC721Upgradeable.sol | 3 ++- src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol | 2 +- .../upgradeable/UpdatableExampleERC721Upgradeable.t.sol | 8 ++++---- 3 files changed, 7 insertions(+), 6 deletions(-) diff --git a/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol b/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol index 5426d01..0153c14 100644 --- a/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol +++ b/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol @@ -2,9 +2,10 @@ pragma solidity ^0.8.13; import {ERC721Upgradeable} from "openzeppelin-contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol"; +import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/access/OwnableUpgradeable.sol"; + import {UpdatableOperatorFiltererUpgradeable} from "../../upgradeable/UpdatableOperatorFiltererUpgradeable.sol"; -import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/access/OwnableUpgradeable.sol"; /** * @title UpdatableExampleERC721Upgradeable diff --git a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol index 939c08e..6f2183a 100644 --- a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol +++ b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol @@ -1,9 +1,9 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.13; -import {IOperatorFilterRegistry} from "../IOperatorFilterRegistry.sol"; import {Initializable} from "openzeppelin-contracts-upgradeable/proxy/utils/Initializable.sol"; +import {IOperatorFilterRegistry} from "../IOperatorFilterRegistry.sol"; /** * @title UpdatableOperatorFiltererUpgradeable diff --git a/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol b/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol index 1f3167b..c11e110 100644 --- a/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol +++ b/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol @@ -1,16 +1,16 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.13; +import {Vm} from "forge-std/Vm.sol"; +import {Initializable} from "openzeppelin-contracts-upgradeable/proxy/utils/Initializable.sol"; +import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/access/OwnableUpgradeable.sol"; + import {UpdatableExampleERC721Upgradeable} from "../../../src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol"; import {UpdatableOperatorFiltererUpgradeable} from "../../../src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol"; import {BaseRegistryTest} from "../../BaseRegistryTest.sol"; -import {Initializable} from "openzeppelin-contracts-upgradeable/proxy/utils/Initializable.sol"; -import {OwnableUpgradeable} from "openzeppelin-contracts-upgradeable/access/OwnableUpgradeable.sol"; -import {Vm} from "forge-std/Vm.sol"; - contract TestableUpdatableExampleERC721Upgradeable is UpdatableExampleERC721Upgradeable { function mint(address to, uint256 tokenId) external { _mint(to, tokenId); From 99cfbfc6c29b03b40bfe6485fe405d5b0c48ed83 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marius=20B=C4=83laj?= Date: Thu, 5 Jan 2023 12:03:29 +0200 Subject: [PATCH 3/7] added authors --- src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol | 1 + src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol | 1 + 2 files changed, 2 insertions(+) diff --git a/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol b/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol index 0153c14..cea81ab 100644 --- a/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol +++ b/src/example/upgradeable/UpdatableExampleERC721Upgradeable.sol @@ -9,6 +9,7 @@ import {UpdatableOperatorFiltererUpgradeable} from /** * @title UpdatableExampleERC721Upgradeable + * @author qed.team, abarbatei, balajmarius * @notice This example contract is configured to use the UpdatableOperatorFiltererUpgradeable, which registers the * token and subscribes it to a give register filter. * Adding the onlyAllowedOperator modifier to the setApprovalForAll, approve, transferFrom, safeTransferFrom (both version) methods ensures that diff --git a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol index 6f2183a..51c1f28 100644 --- a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol +++ b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol @@ -7,6 +7,7 @@ import {IOperatorFilterRegistry} from "../IOperatorFilterRegistry.sol"; /** * @title UpdatableOperatorFiltererUpgradeable + * @author qed.team, abarbatei, balajmarius * @notice Abstract contract whose init function automatically registers and optionally subscribes to or copies another * registrant's entries in the OperatorFilterRegistry. This contract allows the Owner to update the * OperatorFilterRegistry address via updateOperatorFilterRegistryAddress, including to the zero address, From 849ce3170fb7e94a122dfaa16793c4101959fb7f Mon Sep 17 00:00:00 2001 From: Alin Mihai Barbatei Date: Fri, 17 Feb 2023 18:32:38 +0200 Subject: [PATCH 4/7] added testRevert_OperatorNotAllowed test to UpdatableExampleERC721Upgradeable tests --- .../UpdatableExampleERC721Upgradeable.t.sol | 20 +++++++++++++++++-- 1 file changed, 18 insertions(+), 2 deletions(-) diff --git a/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol b/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol index c11e110..65cae39 100644 --- a/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol +++ b/test/example/upgradeable/UpdatableExampleERC721Upgradeable.t.sol @@ -11,13 +11,17 @@ import {UpdatableOperatorFiltererUpgradeable} from "../../../src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol"; import {BaseRegistryTest} from "../../BaseRegistryTest.sol"; +import {OperatorFilterRegistryStub} from "../../helpers/OperatorFilterRegistryStub.sol"; + +import {OperatorFilterer} from "../../../src/OperatorFilterer.sol"; + contract TestableUpdatableExampleERC721Upgradeable is UpdatableExampleERC721Upgradeable { function mint(address to, uint256 tokenId) external { _mint(to, tokenId); } } -contract UpdatableExampleERC721UpgradeableForUpgradableTest is BaseRegistryTest, Initializable { +contract UpdatableERC721UpgradeableForUpgradableTest is BaseRegistryTest, Initializable { TestableUpdatableExampleERC721Upgradeable example; address filteredAddress; address constant DEFAULT_SUBSCRIPTION = address(0x3cc6CddA760b79bAfa08dF41ECFA224f810dCeB6); @@ -123,6 +127,10 @@ contract ConcreteUpdatableOperatorFiltererUpgradable is UpdatableOperatorFiltere return true; } + function checkFilterOperator(address operator) public view { + _checkFilterOperator(operator); + } + function owner() public view @@ -134,7 +142,7 @@ contract ConcreteUpdatableOperatorFiltererUpgradable is UpdatableOperatorFiltere } } -contract UpdatableExampleERC721UpgradeableForUpdatableTest is BaseRegistryTest { +contract UpdatableERC721UpgradeableForUpdatableTest is BaseRegistryTest { ConcreteUpdatableOperatorFiltererUpgradable filterer; address filteredAddress; address filteredCodeHashAddress; @@ -257,4 +265,12 @@ contract UpdatableExampleERC721UpgradeableForUpdatableTest is BaseRegistryTest { vm.expectRevert(abi.encodeWithSelector(AddressFiltered.selector, filteredAddress)); filterer.testFilter(address(0)); } + + function testRevert_OperatorNotAllowed() public { + address stubRegistry = address(new OperatorFilterRegistryStub()); + ConcreteUpdatableOperatorFiltererUpgradable updatableFilterer = new ConcreteUpdatableOperatorFiltererUpgradable(); + updatableFilterer.initialize(stubRegistry, address(0), false); + vm.expectRevert(abi.encodeWithSelector(OperatorFilterer.OperatorNotAllowed.selector, address(filteredAddress))); + updatableFilterer.checkFilterOperator(filteredAddress); + } } From c45b3ec2a64c455500db20ffb652d61d2cb2e745 Mon Sep 17 00:00:00 2001 From: Alin Mihai Barbatei Date: Fri, 17 Feb 2023 18:33:59 +0200 Subject: [PATCH 5/7] modified UpdatableOperatorFiltererUpgradeable to respect new project structure, as indicated by the OpenZeppelin audit --- .../UpdatableOperatorFiltererUpgradeable.sol | 115 +++++++++++++----- 1 file changed, 87 insertions(+), 28 deletions(-) diff --git a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol index 51c1f28..e24c3dd 100644 --- a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol +++ b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol @@ -5,6 +5,31 @@ import {Initializable} from "openzeppelin-contracts-upgradeable/proxy/utils/Init import {IOperatorFilterRegistry} from "../IOperatorFilterRegistry.sol"; + +/** + * @title Upgradeable storage layout for UpdatableOperatorFiltererUpgradeable. + * @author qed.team, abarbatei, balajmarius + * @notice Upgradeable contracts must use a storage layout that can be used across upgrades. + * Only append new variables to the end of the layout. + */ +library UpdatableOperatorFiltererUpgradeableStorage { + struct Layout { + /// @dev Address of the opensea filter register contract + IOperatorFilterRegistry _operatorFilterRegistry; + } + + /// @dev The EIP-1967 specific storage slot for the layout + bytes32 internal constant STORAGE_SLOT = bytes32(uint256(keccak256(bytes("UpdatableOperatorFiltererUpgradeable.contracts.storage"))) - 1); + + /// @dev The layout of the storage. + function layout() internal pure returns (Layout storage l) { + bytes32 slot = STORAGE_SLOT; + assembly { + l.slot := slot + } + } +} + /** * @title UpdatableOperatorFiltererUpgradeable * @author qed.team, abarbatei, balajmarius @@ -12,80 +37,114 @@ import {IOperatorFilterRegistry} from "../IOperatorFilterRegistry.sol"; * registrant's entries in the OperatorFilterRegistry. This contract allows the Owner to update the * OperatorFilterRegistry address via updateOperatorFilterRegistryAddress, including to the zero address, * which will bypass registry checks. - * Note that OpenSea will still disable creator fee enforcement if filtered operators begin fulfilling orders + * Note that OpenSea will still disable creator earnings enforcement if filtered operators begin fulfilling orders * on-chain, eg, if the registry is revoked or bypassed. * @dev This smart contract is meant to be inherited by token contracts so they can use the following: * - `onlyAllowedOperator` modifier for `transferFrom` and `safeTransferFrom` methods. * - `onlyAllowedOperatorApproval` modifier for `approve` and `setApprovalForAll` methods. - * Also use updateOperatorFilterRegistryAddress function to change registry address if needed + * Use updateOperatorFilterRegistryAddress function to change registry address if needed */ abstract contract UpdatableOperatorFiltererUpgradeable is Initializable { + + using UpdatableOperatorFiltererUpgradeableStorage for UpdatableOperatorFiltererUpgradeableStorage.Layout; + + /// @notice Emitted when an operator is not allowed. error OperatorNotAllowed(address operator); + /// @notice Emitted when someone other than the owner is trying to call an only owner function. error OnlyOwner(); - IOperatorFilterRegistry public operatorFilterRegistry; + /// @notice Emitted when the operator filter registry address is changed by the owner of the contract + event OperatorFilterRegistryAddressUpdated(address newRegistry); + /** + * @notice Initialization function in accordance with the upgradable pattern + * @dev The upgradeable initialize function specific to proxied contracts + * @param _registry Registry address to which to register to for blocking operators that do not respect royalties + * @param subscriptionOrRegistrantToCopy Subscription address to use as a template for when + * imitating/copying blocked addresses and codehashes + * @param subscribe If to subscribe to the subscriptionOrRegistrantToCopy address or just copy entries from it + */ function __UpdatableOperatorFiltererUpgradeable_init(address _registry, address subscriptionOrRegistrantToCopy, bool subscribe) internal onlyInitializing { - operatorFilterRegistry = IOperatorFilterRegistry(_registry); + UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry = IOperatorFilterRegistry(_registry); // If an inheriting token contract is deployed to a network without the registry deployed, the modifier // will not revert, but the contract will need to be registered with the registry once it is deployed in // order for the modifier to filter addresses. - if (address(operatorFilterRegistry).code.length > 0) { + if (address(UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry).code.length > 0) { if (subscribe) { - operatorFilterRegistry.registerAndSubscribe(address(this), subscriptionOrRegistrantToCopy); + UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry.registerAndSubscribe(address(this), subscriptionOrRegistrantToCopy); } else { if (subscriptionOrRegistrantToCopy != address(0)) { - operatorFilterRegistry.registerAndCopyEntries(address(this), subscriptionOrRegistrantToCopy); + UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry.registerAndCopyEntries(address(this), subscriptionOrRegistrantToCopy); } else { - operatorFilterRegistry.register(address(this)); + UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry.register(address(this)); } } } } + /** + * @dev A helper modifier to check if the operator is allowed. + */ modifier onlyAllowedOperator(address from) virtual { - // Check registry code length to facilitate testing in environments without a deployed registry. - if (address(operatorFilterRegistry).code.length > 0) { - // Allow spending tokens from addresses with balance - // Note that this still allows listings and marketplaces with escrow to transfer tokens if transferred - // from an EOA. - if (from == msg.sender) { - _; - return; - } - if (!operatorFilterRegistry.isOperatorAllowed(address(this), msg.sender)) { - revert OperatorNotAllowed(msg.sender); - } + // Allow spending tokens from addresses with balance + // Note that this still allows listings and marketplaces with escrow to transfer tokens if transferred + // from an EOA. + if (from != msg.sender) { + _checkFilterOperator(msg.sender); } _; } + /** + * @dev A helper modifier to check if the operator approval is allowed. + */ modifier onlyAllowedOperatorApproval(address operator) virtual { - // Check registry code length to facilitate testing in environments without a deployed registry. - if (address(operatorFilterRegistry).code.length > 0) { - if (!operatorFilterRegistry.isOperatorAllowed(address(this), operator)) { - revert OperatorNotAllowed(operator); - } - } + _checkFilterOperator(operator); _; } /** * @notice Update the address that the contract will make OperatorFilter checks against. When set to the zero * address, checks will be bypassed. OnlyOwner. + * @custom:event OperatorFilterRegistryAddressUpdated + * @param newRegistry The address of the registry that will be used for this contract */ function updateOperatorFilterRegistryAddress(address newRegistry) public virtual { if (msg.sender != owner()) { revert OnlyOwner(); } - operatorFilterRegistry = IOperatorFilterRegistry(newRegistry); + UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry = IOperatorFilterRegistry(newRegistry); + emit OperatorFilterRegistryAddressUpdated(newRegistry); } /** - * @dev assume the contract has an owner, but leave specific Ownable implementation up to inheriting contract + * @dev Helper function to return the value of the currently used registry address + */ + function operatorFilterRegistry() public view returns (address) { + return address(UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry); + } + + /** + * @dev Assume the contract has an owner, but leave specific Ownable implementation up to inheriting contract */ function owner() public view virtual returns (address); + + /** + * @dev A helper function to check if the operator is allowed + */ + function _checkFilterOperator(address operator) internal view virtual { + IOperatorFilterRegistry registry = UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry; + // Check registry code length to facilitate testing in environments without a deployed registry. + if (address(registry) != address(0) && address(registry).code.length > 0) { + // under normal circumstances, this function will revert rather than return false, but inheriting or + // upgraded contracts may specify their own OperatorFilterRegistry implementations, which may behave + // differently + if (!registry.isOperatorAllowed(address(this), operator)) { + revert OperatorNotAllowed(operator); + } + } + } } From e5333e45fcf2994ee3d799a324e4657e4179a23e Mon Sep 17 00:00:00 2001 From: Alin Mihai Barbatei Date: Fri, 17 Feb 2023 19:40:21 +0200 Subject: [PATCH 6/7] minor code improvements --- .../UpdatableOperatorFiltererUpgradeable.sol | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol index e24c3dd..a24de1c 100644 --- a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol +++ b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol @@ -15,7 +15,7 @@ import {IOperatorFilterRegistry} from "../IOperatorFilterRegistry.sol"; library UpdatableOperatorFiltererUpgradeableStorage { struct Layout { /// @dev Address of the opensea filter register contract - IOperatorFilterRegistry _operatorFilterRegistry; + address _operatorFilterRegistry; } /// @dev The EIP-1967 specific storage slot for the layout @@ -68,18 +68,19 @@ abstract contract UpdatableOperatorFiltererUpgradeable is Initializable { internal onlyInitializing { - UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry = IOperatorFilterRegistry(_registry); + UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry = _registry; + IOperatorFilterRegistry registry = IOperatorFilterRegistry(_registry); // If an inheriting token contract is deployed to a network without the registry deployed, the modifier // will not revert, but the contract will need to be registered with the registry once it is deployed in // order for the modifier to filter addresses. - if (address(UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry).code.length > 0) { + if (address(registry).code.length > 0) { if (subscribe) { - UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry.registerAndSubscribe(address(this), subscriptionOrRegistrantToCopy); + registry.registerAndSubscribe(address(this), subscriptionOrRegistrantToCopy); } else { if (subscriptionOrRegistrantToCopy != address(0)) { - UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry.registerAndCopyEntries(address(this), subscriptionOrRegistrantToCopy); + registry.registerAndCopyEntries(address(this), subscriptionOrRegistrantToCopy); } else { - UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry.register(address(this)); + registry.register(address(this)); } } } @@ -116,7 +117,7 @@ abstract contract UpdatableOperatorFiltererUpgradeable is Initializable { if (msg.sender != owner()) { revert OnlyOwner(); } - UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry = IOperatorFilterRegistry(newRegistry); + UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry = newRegistry; emit OperatorFilterRegistryAddressUpdated(newRegistry); } @@ -136,7 +137,7 @@ abstract contract UpdatableOperatorFiltererUpgradeable is Initializable { * @dev A helper function to check if the operator is allowed */ function _checkFilterOperator(address operator) internal view virtual { - IOperatorFilterRegistry registry = UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry; + IOperatorFilterRegistry registry = IOperatorFilterRegistry(UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry); // Check registry code length to facilitate testing in environments without a deployed registry. if (address(registry) != address(0) && address(registry).code.length > 0) { // under normal circumstances, this function will revert rather than return false, but inheriting or From a2b8f08196f2527b80e661bbb2e87f638cbc9ce5 Mon Sep 17 00:00:00 2001 From: Operator Filterer Date: Wed, 1 Mar 2023 10:54:59 -0800 Subject: [PATCH 7/7] tweak impl to inherit --- .../UpdatableOperatorFiltererUpgradeable.sol | 49 +++++-------------- 1 file changed, 13 insertions(+), 36 deletions(-) diff --git a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol index a24de1c..1cbdf1c 100644 --- a/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol +++ b/src/upgradeable/UpdatableOperatorFiltererUpgradeable.sol @@ -1,11 +1,9 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.13; -import {Initializable} from "openzeppelin-contracts-upgradeable/proxy/utils/Initializable.sol"; - +import {OperatorFiltererUpgradeable} from "./OperatorFiltererUpgradeable.sol"; import {IOperatorFilterRegistry} from "../IOperatorFilterRegistry.sol"; - /** * @title Upgradeable storage layout for UpdatableOperatorFiltererUpgradeable. * @author qed.team, abarbatei, balajmarius @@ -19,7 +17,8 @@ library UpdatableOperatorFiltererUpgradeableStorage { } /// @dev The EIP-1967 specific storage slot for the layout - bytes32 internal constant STORAGE_SLOT = bytes32(uint256(keccak256(bytes("UpdatableOperatorFiltererUpgradeable.contracts.storage"))) - 1); + bytes32 internal constant STORAGE_SLOT = + bytes32(uint256(keccak256(bytes("UpdatableOperatorFiltererUpgradeable.contracts.storage"))) - 1); /// @dev The layout of the storage. function layout() internal pure returns (Layout storage l) { @@ -44,12 +43,9 @@ library UpdatableOperatorFiltererUpgradeableStorage { * - `onlyAllowedOperatorApproval` modifier for `approve` and `setApprovalForAll` methods. * Use updateOperatorFilterRegistryAddress function to change registry address if needed */ -abstract contract UpdatableOperatorFiltererUpgradeable is Initializable { - +abstract contract UpdatableOperatorFiltererUpgradeable is OperatorFiltererUpgradeable { using UpdatableOperatorFiltererUpgradeableStorage for UpdatableOperatorFiltererUpgradeableStorage.Layout; - /// @notice Emitted when an operator is not allowed. - error OperatorNotAllowed(address operator); /// @notice Emitted when someone other than the owner is trying to call an only owner function. error OnlyOwner(); @@ -64,11 +60,12 @@ abstract contract UpdatableOperatorFiltererUpgradeable is Initializable { * imitating/copying blocked addresses and codehashes * @param subscribe If to subscribe to the subscriptionOrRegistrantToCopy address or just copy entries from it */ - function __UpdatableOperatorFiltererUpgradeable_init(address _registry, address subscriptionOrRegistrantToCopy, bool subscribe) - internal - onlyInitializing - { - UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry = _registry; + function __UpdatableOperatorFiltererUpgradeable_init( + address _registry, + address subscriptionOrRegistrantToCopy, + bool subscribe + ) internal onlyInitializing { + UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry = _registry; IOperatorFilterRegistry registry = IOperatorFilterRegistry(_registry); // If an inheriting token contract is deployed to a network without the registry deployed, the modifier // will not revert, but the contract will need to be registered with the registry once it is deployed in @@ -86,27 +83,6 @@ abstract contract UpdatableOperatorFiltererUpgradeable is Initializable { } } - /** - * @dev A helper modifier to check if the operator is allowed. - */ - modifier onlyAllowedOperator(address from) virtual { - // Allow spending tokens from addresses with balance - // Note that this still allows listings and marketplaces with escrow to transfer tokens if transferred - // from an EOA. - if (from != msg.sender) { - _checkFilterOperator(msg.sender); - } - _; - } - - /** - * @dev A helper modifier to check if the operator approval is allowed. - */ - modifier onlyAllowedOperatorApproval(address operator) virtual { - _checkFilterOperator(operator); - _; - } - /** * @notice Update the address that the contract will make OperatorFilter checks against. When set to the zero * address, checks will be bypassed. OnlyOwner. @@ -136,8 +112,9 @@ abstract contract UpdatableOperatorFiltererUpgradeable is Initializable { /** * @dev A helper function to check if the operator is allowed */ - function _checkFilterOperator(address operator) internal view virtual { - IOperatorFilterRegistry registry = IOperatorFilterRegistry(UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry); + function _checkFilterOperator(address operator) internal view virtual override { + IOperatorFilterRegistry registry = + IOperatorFilterRegistry(UpdatableOperatorFiltererUpgradeableStorage.layout()._operatorFilterRegistry); // Check registry code length to facilitate testing in environments without a deployed registry. if (address(registry) != address(0) && address(registry).code.length > 0) { // under normal circumstances, this function will revert rather than return false, but inheriting or