[gms-1363] Add OALUpgradeable to be used behind proxy #163

jasonzwli · 2024-01-17T02:57:48Z

Adding OperaterAllowlistUpgradeable. The reasoning for not just overwriting the original OAL code is because the repo is fairly out of date with its packages, i.e overwriting the existing contract requires ethers ^6.6.0, which will break 95% of our hardhat tests. This is time consuming to fix, so I made the decision to write a new contract and test the essential proxy functions in forge. I've also replaced the OAL in the ERC1155 test with the new OALUpgradeable to test its interactions with collections. The regular Allowlist contract will be removed from the repo once we fix the tests that are written in hardhat

openzeppelin-code · 2024-01-17T03:03:50Z

[gms-1363] Add OALUpgradeable to be used behind proxy

Generated at commit: 9bfb7001f2d454e503688005cf95fd8c372f4b1b

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	2 0 0 8 30 40
Dependencies	Critical High Medium Low Note Total	0 0 0 1 54 55

For more details view the full report in OpenZeppelin Code Inspector

drinkcoffee · 2024-01-17T03:39:30Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+
+import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
+import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";
+


fix linter issue by importing {AccessControlUpgradeable} from

drinkcoffee · 2024-01-17T03:39:51Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+// SPDX-License-Identifier: Apache 2.0
+pragma solidity 0.8.19;
+
+import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";


fix linter issue by importing specific contract

drinkcoffee · 2024-01-17T03:43:28Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+// Interface to retrieve the implemention stored inside the Proxy contract
+interface IProxy {
+    // Returns the current implementation address used by the proxy contract
+    function PROXY_getImplementation() external view returns (address);


Remove linter issue by adding:
// solhint-disable-next-line var-name-mixedcase

drinkcoffee · 2024-01-17T03:45:04Z

contracts/mocks/MockOAL.sol

@@ -0,0 +1,197 @@
+// Copyright Immutable Pty Ltd 2018 - 2023


This contract should be with the test code. That is in /test/allowlist

drinkcoffee · 2024-01-17T03:46:40Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+    /// @notice Mapping of Allowlisted bytecodes
+    mapping(bytes32 => bool) private bytecodeAllowlist;
+
+    /// @notice storage gap for additional variables for upgrades


Storage gap should be at the end of the file.
Storage gap variable's name should be __OperatorAllowlistUpgradeableGap
Naming the gap variable makes it easier to read storage maps.

drinkcoffee · 2024-01-17T03:46:57Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+
+    ///     =====       Events       =====
+
+    /// @notice Emitted when a target address is added or removed from the Allowlist


Events should be before storage variables.

allan-almeida-imtbl · 2024-01-17T03:35:20Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+    OperatorAllowlist is an implementation of a Allowlist registry, storing addresses and bytecode
+    which are allowed to be approved operators and execute transfers of interfacing token contracts (e.g. ERC721/ERC1155).
+    The registry will be a deployed contract that tokens may interface with and point to.
+    OperatorAllowlist is not designed to be upgradeable or extended.


Can probably remove OperatorAllowlist is not designed to be upgradeable or extended.

drinkcoffee · 2024-01-17T03:50:16Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+import {IOperatorAllowlist} from "./IOperatorAllowlist.sol";
+
+// Interface to retrieve the implemention stored inside the Proxy contract
+interface IProxy {


This should be a separate file. Doing this will allow the same interface file to be used by all usages, rather than the code being copy and pasted.

The file name should be IWalletProxy. The description should be:

Interface for Passport Wallet's proxy contract.

drinkcoffee · 2024-01-17T03:51:38Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+        __AccessControl_init();
+        _grantRole(DEFAULT_ADMIN_ROLE, _roleAdmin);
+        _grantRole(UPGRADE_ROLE, _upgradeAdmin);
+    }


We should also initialize the REGISTRAR role here.

drinkcoffee · 2024-01-17T03:53:09Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+    ///     =====  External functions  =====
+
+    /**
+     * @notice Add a target address to Allowlist


Update notice to reflect that a set of addresses is being passed in.

drinkcoffee · 2024-01-17T03:53:49Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+     * @notice Add a target address to Allowlist
+     * @param addressTargets the addresses to be added to the allowlist
+     */
+    function addAddressToAllowlist(address[] calldata addressTargets) external onlyRole(REGISTRAR_ROLE) {


The function name should be addAddressToAllowLists. That is with an s at the end to imply multiple addresses.

would addAddressesToAllowList be clearer, as an alternative

drinkcoffee · 2024-01-17T03:54:00Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+    }
+
+    /**
+     * @notice Remove a target address from Allowlist


drinkcoffee · 2024-01-17T03:54:10Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+     * @notice Remove a target address from Allowlist
+     * @param addressTargets the addresses to be removed from the allowlist
+     */
+    function removeAddressFromAllowlist(address[] calldata addressTargets) external onlyRole(REGISTRAR_ROLE) {


drinkcoffee · 2024-01-17T03:56:51Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+     * @notice Allows admin to grant `user` `REGISTRAR_ROLE` role
+     * @param user the address that `REGISTRAR_ROLE` will be granted to
+     */
+    function grantRegistrarRole(address user) external onlyRole(DEFAULT_ADMIN_ROLE) {


This function is not needed. It is available in the AccessControl Open Zeppelin contract.

drinkcoffee · 2024-01-17T03:57:21Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+     * @notice Allows admin to revoke `REGISTRAR_ROLE` role from `user`
+     * @param user the address that `REGISTRAR_ROLE` will be revoked from
+     */
+    function revokeRegistrarRole(address user) external onlyRole(DEFAULT_ADMIN_ROLE) {


The function is not needed. The same functionality is available in AccessControl

drinkcoffee · 2024-01-17T04:01:21Z

contracts/mocks/MockOAL.sol

@@ -0,0 +1,197 @@
+// Copyright Immutable Pty Ltd 2018 - 2023
+// SPDX-License-Identifier: Apache 2.0


I think this contract could just extend the OperatorAllowlist contract, just adding the extra storage location and function(s)

drinkcoffee · 2024-01-17T04:01:54Z

test/allowlist/OperatorAllowlistUpgradeable.t.sol

@@ -0,0 +1,67 @@
+pragma solidity 0.8.19;


Add copyright and license

drinkcoffee · 2024-01-17T04:03:20Z

test/utils/DeployAllowlistProxy.sol

@@ -0,0 +1,25 @@
+// SPDX-License-Identifier: Apache 2.0


If this contract is only needed by allow list tests, put it in test/allowlist

This is also used by the erc1155 test

drinkcoffee · 2024-01-17T04:03:47Z

test/utils/DeployAllowlistProxy.sol

+import "@openzeppelin/contracts/proxy/ERC1967/ERC1967Proxy.sol";
+import {OperatorAllowlistUpgradeable} from "../../contracts/allowlist/OperatorAllowlistUpgradeable.sol";
+
+contract DeployOperatorAllowlist {


Add a comment to the class so it is obvious what this class is for.

drinkcoffee · 2024-01-17T04:08:16Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

@@ -0,0 +1,189 @@
+// Copyright Immutable Pty Ltd 2018 - 2023


This contract has a lot in common with OperatorAllowlist. The two contracts should inherit from a common OperatorAllowlistBase that contains the common code and is written to be used by both of them.

We will delete the regular OperatorAllowlist contract once the repo's dependencies are fixed

drinkcoffee · 2024-01-17T04:16:40Z

test/allowlist/OperatorAllowlistUpgradeable.t.sol

+    uint256 registerarPrivateKey = 3;
+    uint256 feeReceiverKey = 4;
+
+    address admin = vm.addr(adminPrivateKey);


Remove and use:
address public roleAdmin = makeAddr("roleAdmin");

drinkcoffee · 2024-01-17T04:22:55Z

test/token/erc1155/ImmutableERC1155.t.sol

@@ -5,15 +5,16 @@ import "forge-std/Test.sol";
 import {ImmutableERC1155} from "../../../contracts/token/erc1155/preset/draft-ImmutableERC1155.sol";
 import {IImmutableERC1155Errors} from "../../../contracts/errors/Errors.sol";
 import {OperatorAllowlistEnforcementErrors} from "../../../contracts/errors/Errors.sol";
-import {OperatorAllowlist} from "../../../contracts/allowlist/OperatorAllowlist.sol";
+import {OperatorAllowlistUpgradeable} from "../../../contracts/allowlist/OperatorAllowlistUpgradeable.sol";


This implies that we are no longer testing OperatorAllowlist, and only testing the Upgradeable version. ???

Yes, the plan is to no longer use and test OperatorAllowlist but it is a huge effort to rewrite all the tests for it in forge since ethers ^6.6.0(which we need to help test proxy deploys) will break a lot of the hardhat tests

The basic functionalities that are common between OperatorAllowlist and OperatorAllowlistUpgradeable are tested by existing hardhat tests

drinkcoffee · 2024-01-17T04:24:37Z

test/allowlist/OperatorAllowlistUpgradeable.t.sol

+        assertEq(address(immutableERC721.operatorAllowlist()), proxyAddr);
+    }
+
+    function testUpgradeToV2() public {


Add a test where the authorisation fails due to the account not being authorised.

drinkcoffee · 2024-01-17T05:07:57Z

contracts/allowlist/OperatorAllowlistUpgradeable.sol

+    The registry will be a deployed contract that tokens may interface with and point to.
+*/
+
+contract OperatorAllowlistUpgradeable is ERC165, AccessControlUpgradeable, UUPSUpgradeable, IOperatorAllowlist {


We must used AccessControlEnumerableUpgradeable
Being able to enumerate who the admins are is very important.

jasonzwli requested a review from a team as a code owner January 17, 2024 02:57

jasonzwli force-pushed the jason/proxy branch 2 times, most recently from 89df76c to 6da134f Compare January 17, 2024 03:01

jasonzwli force-pushed the jason/proxy branch from 6da134f to 8b1403f Compare January 17, 2024 03:11

[gms-1363] Add OALUpgradeable to be used behind proxy

f1029b6

jasonzwli force-pushed the jason/proxy branch from 8b1403f to f1029b6 Compare January 17, 2024 03:15

drinkcoffee reviewed Jan 17, 2024

View reviewed changes

allan-almeida-imtbl reviewed Jan 17, 2024

View reviewed changes

drinkcoffee reviewed Jan 17, 2024

View reviewed changes

jasonzwli force-pushed the jason/proxy branch from e9fe5bb to d56b407 Compare January 17, 2024 04:57

drinkcoffee reviewed Jan 17, 2024

View reviewed changes

jasonzwli force-pushed the jason/proxy branch 2 times, most recently from 951c8e0 to 948f228 Compare January 17, 2024 05:31

[chore] comments

9bfb700

jasonzwli force-pushed the jason/proxy branch from 948f228 to 9bfb700 Compare January 17, 2024 05:53

jasonzwli requested review from drinkcoffee and allan-almeida-imtbl January 17, 2024 23:19

allan-almeida-imtbl approved these changes Jan 17, 2024

View reviewed changes

jasonzwli enabled auto-merge (squash) January 17, 2024 23:25

ipekt approved these changes Jan 17, 2024

View reviewed changes

jasonzwli merged commit 929cbbb into main Jan 17, 2024
4 checks passed

drinkcoffee deleted the jason/proxy branch April 1, 2024 22:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[gms-1363] Add OALUpgradeable to be used behind proxy #163

[gms-1363] Add OALUpgradeable to be used behind proxy #163

jasonzwli commented Jan 17, 2024 •

edited

Loading

openzeppelin-code bot commented Jan 17, 2024 •

edited

Loading

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

allan-almeida-imtbl Jan 17, 2024 •

edited

Loading

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

allan-almeida-imtbl Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

jasonzwli Jan 17, 2024 •

edited

Loading

drinkcoffee Jan 17, 2024 •

edited

Loading

drinkcoffee Jan 17, 2024

jasonzwli Jan 17, 2024 •

edited

Loading

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024

jasonzwli Jan 17, 2024

jasonzwli Jan 17, 2024

drinkcoffee Jan 17, 2024

drinkcoffee Jan 17, 2024


		import "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
		import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";


		/// ===== Events =====

		/// @notice Emitted when a target address is added or removed from the Allowlist

		@@ -0,0 +1,197 @@
		// Copyright Immutable Pty Ltd 2018 - 2023
		// SPDX-License-Identifier: Apache 2.0

[gms-1363] Add OALUpgradeable to be used behind proxy #163

[gms-1363] Add OALUpgradeable to be used behind proxy #163

Conversation

jasonzwli commented Jan 17, 2024 • edited Loading

openzeppelin-code bot commented Jan 17, 2024 • edited Loading

[gms-1363] Add OALUpgradeable to be used behind proxy

🚨 Report Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

allan-almeida-imtbl Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonzwli Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

drinkcoffee Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonzwli Jan 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonzwli commented Jan 17, 2024 •

edited

Loading

openzeppelin-code bot commented Jan 17, 2024 •

edited

Loading

allan-almeida-imtbl Jan 17, 2024 •

edited

Loading

jasonzwli Jan 17, 2024 •

edited

Loading

drinkcoffee Jan 17, 2024 •

edited

Loading

jasonzwli Jan 17, 2024 •

edited

Loading