Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Jan 17, 2024
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
189 changes: 189 additions & 0 deletions contracts/allowlist/OperatorAllowlistUpgradeable.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,189 @@
// Copyright Immutable Pty Ltd 2018 - 2023
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@jasonzwli jasonzwli Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// SPDX-License-Identifier: Apache 2.0
pragma solidity 0.8.19;

Check warning on line 3 in contracts/allowlist/OperatorAllowlistUpgradeable.sol

View workflow job for this annotation

GitHub Actions / Run eslint

Found more than One contract per file. 2 contracts found!

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

Check warning on line 5 in contracts/allowlist/OperatorAllowlistUpgradeable.sol

View workflow job for this annotation

GitHub Actions / Run eslint

global import of path @openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol is not allowed. Specify names to import individually or bind all exports of the module into a name (import "path" as Name)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix linter issue by importing specific contract

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

Check warning on line 6 in contracts/allowlist/OperatorAllowlistUpgradeable.sol

View workflow job for this annotation

GitHub Actions / Run eslint

global import of path @openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol is not allowed. Specify names to import individually or bind all exports of the module into a name (import "path" as Name)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fix linter issue by importing {AccessControlUpgradeable} from

// Introspection
import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";

// Interfaces
import {IOperatorAllowlist} from "./IOperatorAllowlist.sol";

// Interface to retrieve the implemention stored inside the Proxy contract
interface IProxy {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Returns the current implementation address used by the proxy contract
function PROXY_getImplementation() external view returns (address);

Check warning on line 17 in contracts/allowlist/OperatorAllowlistUpgradeable.sol

View workflow job for this annotation

GitHub Actions / Run eslint

Function name must be in mixedCase
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/*
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.
Copy link
Contributor

@allan-almeida-imtbl allan-almeida-imtbl Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

*/

contract OperatorAllowlistUpgradeable is ERC165, AccessControlUpgradeable, UUPSUpgradeable, IOperatorAllowlist {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

/// ===== State Variables =====

/// @notice Only REGISTRAR_ROLE can invoke white listing registration and removal
bytes32 public constant REGISTRAR_ROLE = bytes32("REGISTRAR_ROLE");

/// @notice Only UPGRADE_ROLE can upgrade the contract
bytes32 public constant UPGRADE_ROLE = bytes32("UPGRADE_ROLE");

/// @notice Mapping of Allowlisted addresses
mapping(address => bool) private addressAllowlist;

/// @notice Mapping of Allowlisted implementation addresses
mapping(address => bool) private addressImplementationAllowlist;

/// @notice Mapping of Allowlisted bytecodes
mapping(bytes32 => bool) private bytecodeAllowlist;

/// @notice storage gap for additional variables for upgrades
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

uint256[20] __gap;

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Events should be before storage variables.

event AddressAllowlistChanged(address indexed target, bool added);

/// @notice Emitted when a target smart contract wallet is added or removed from the Allowlist
event WalletAllowlistChanged(bytes32 indexed targetBytes, address indexed targetAddress, bool added);

/// ===== Initializer =====

/**
* @notice Grants `DEFAULT_ADMIN_ROLE` to the supplied `admin` address
* @param _roleAdmin the address to grant `DEFAULT_ADMIN_ROLE` to
* @param _upgradeAdmin the address to grant `UPGRADE_ROLE` to
*/
function initialize(address _roleAdmin, address _upgradeAdmin) public initializer {
__UUPSUpgradeable_init();
__AccessControl_init();
_grantRole(DEFAULT_ADMIN_ROLE, _roleAdmin);
_grantRole(UPGRADE_ROLE, _upgradeAdmin);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should also initialize the REGISTRAR role here.


/// ===== External functions =====

/**
* @notice Add a target address to Allowlist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @param addressTargets the addresses to be added to the allowlist
*/
function addAddressToAllowlist(address[] calldata addressTargets) external onlyRole(REGISTRAR_ROLE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

would addAddressesToAllowList be clearer, as an alternative

for (uint256 i; i < addressTargets.length; i++) {
addressAllowlist[addressTargets[i]] = true;
emit AddressAllowlistChanged(addressTargets[i], true);
}
}

/**
* @notice Remove a target address from Allowlist
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

* @param addressTargets the addresses to be removed from the allowlist
*/
function removeAddressFromAllowlist(address[] calldata addressTargets) external onlyRole(REGISTRAR_ROLE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above

for (uint256 i; i < addressTargets.length; i++) {
delete addressAllowlist[addressTargets[i]];
emit AddressAllowlistChanged(addressTargets[i], false);
}
}

/**
* @notice Add a smart contract wallet to the Allowlist.
* This will allowlist the proxy and implementation contract pair.
* First, the bytecode of the proxy is added to the bytecode allowlist.
* Second, the implementation address stored in the proxy is stored in the
* implementation address allowlist.
* @param walletAddr the wallet address to be added to the allowlist
*/
function addWalletToAllowlist(address walletAddr) external onlyRole(REGISTRAR_ROLE) {
// get bytecode of wallet
bytes32 codeHash;
assembly {
codeHash := extcodehash(walletAddr)
}
bytecodeAllowlist[codeHash] = true;
// get address of wallet module
address impl = IProxy(walletAddr).PROXY_getImplementation();
addressImplementationAllowlist[impl] = true;

emit WalletAllowlistChanged(codeHash, walletAddr, true);
}

/**
* @notice Remove a smart contract wallet from the Allowlist
* This will remove the proxy bytecode hash and implementation contract address pair from the allowlist
* @param walletAddr the wallet address to be removed from the allowlist
*/
function removeWalletFromAllowlist(address walletAddr) external onlyRole(REGISTRAR_ROLE) {
// get bytecode of wallet
bytes32 codeHash;
assembly {
codeHash := extcodehash(walletAddr)
}
delete bytecodeAllowlist[codeHash];
// get address of wallet module
address impl = IProxy(walletAddr).PROXY_getImplementation();
delete addressImplementationAllowlist[impl];

emit WalletAllowlistChanged(codeHash, walletAddr, false);
}

/**
* @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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

grantRole(REGISTRAR_ROLE, user);
}

/**
* @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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

revokeRole(REGISTRAR_ROLE, user);
}

/// ===== View functions =====

/**
* @notice Returns true if an address is Allowlisted, false otherwise
* @param target the address that will be checked for presence in the allowlist
*/
function isAllowlisted(address target) external view override returns (bool) {
if (addressAllowlist[target]) {
return true;
}

// Check if caller is a Allowlisted smart contract wallet
bytes32 codeHash;
assembly {
codeHash := extcodehash(target)
}
if (bytecodeAllowlist[codeHash]) {
// If wallet proxy bytecode is approved, check addr of implementation contract
address impl = IProxy(target).PROXY_getImplementation();

return addressImplementationAllowlist[impl];
}

return false;
}

/**
* @notice ERC-165 interface support
* @param interfaceId The interface identifier, which is a 4-byte selector.
*/
function supportsInterface(
bytes4 interfaceId
) public view virtual override(ERC165, AccessControlUpgradeable) returns (bool) {
return interfaceId == type(IOperatorAllowlist).interfaceId || super.supportsInterface(interfaceId);
}

// Override the _authorizeUpgrade function
function _authorizeUpgrade(address newImplementation) internal override onlyRole(UPGRADE_ROLE) {}
}
197 changes: 197 additions & 0 deletions contracts/mocks/MockOAL.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,197 @@
// Copyright Immutable Pty Ltd 2018 - 2023
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// SPDX-License-Identifier: Apache 2.0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

pragma solidity 0.8.19;

// Access Control
import {AccessControl} from "@openzeppelin/contracts/access/AccessControl.sol";

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

// Introspection
import {ERC165} from "@openzeppelin/contracts/utils/introspection/ERC165.sol";

// Interfaces
import {IOperatorAllowlist} from "../allowlist/IOperatorAllowlist.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);
}

/*
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.
*/

contract MockOperatorAllowlistUpgradeable is ERC165, AccessControlUpgradeable, UUPSUpgradeable, IOperatorAllowlist {
/// ===== State Variables =====

/// @notice Only REGISTRAR_ROLE can invoke white listing registration and removal
bytes32 public constant REGISTRAR_ROLE = bytes32("REGISTRAR_ROLE");

/// @notice Only UPGRADE_ROLE can upgrade the contract
bytes32 public constant UPGRADE_ROLE = bytes32("UPGRADE_ROLE");

/// @notice Mapping of Allowlisted addresses
mapping(address => bool) private addressAllowlist;

/// @notice Mapping of Allowlisted implementation addresses
mapping(address => bool) private addressImplementationAllowlist;

/// @notice Mapping of Allowlisted bytecodes
mapping(bytes32 => bool) private bytecodeAllowlist;

uint256 public mockInt;

/// @notice storage gap for additional variables for upgrades
uint256[19] __gap;

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

/// @notice Emitted when a target address is added or removed from the Allowlist
event AddressAllowlistChanged(address indexed target, bool added);

/// @notice Emitted when a target smart contract wallet is added or removed from the Allowlist
event WalletAllowlistChanged(bytes32 indexed targetBytes, address indexed targetAddress, bool added);

/// ===== Initializer =====

/**
* @notice Grants `DEFAULT_ADMIN_ROLE` to the supplied `admin` address
* @param _roleAdmin the address to grant `DEFAULT_ADMIN_ROLE` to
*/
function initialize(address _roleAdmin, address _upgradeAdmin) public initializer {
__UUPSUpgradeable_init();
__AccessControl_init();
_grantRole(DEFAULT_ADMIN_ROLE, _roleAdmin);
_grantRole(UPGRADE_ROLE, _upgradeAdmin);
}

/// ===== External functions =====

/**
* @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) {
for (uint256 i; i < addressTargets.length; i++) {
addressAllowlist[addressTargets[i]] = true;
emit AddressAllowlistChanged(addressTargets[i], true);
}
}

/**
* @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) {
for (uint256 i; i < addressTargets.length; i++) {
delete addressAllowlist[addressTargets[i]];
emit AddressAllowlistChanged(addressTargets[i], false);
}
}

/**
* @notice Add a smart contract wallet to the Allowlist.
* This will allowlist the proxy and implementation contract pair.
* First, the bytecode of the proxy is added to the bytecode allowlist.
* Second, the implementation address stored in the proxy is stored in the
* implementation address allowlist.
* @param walletAddr the wallet address to be added to the allowlist
*/
function addWalletToAllowlist(address walletAddr) external onlyRole(REGISTRAR_ROLE) {
// get bytecode of wallet
bytes32 codeHash;
assembly {
codeHash := extcodehash(walletAddr)
}
bytecodeAllowlist[codeHash] = true;
// get address of wallet module
address impl = IProxy(walletAddr).PROXY_getImplementation();
addressImplementationAllowlist[impl] = true;

emit WalletAllowlistChanged(codeHash, walletAddr, true);
}

/**
* @notice Remove a smart contract wallet from the Allowlist
* This will remove the proxy bytecode hash and implementation contract address pair from the allowlist
* @param walletAddr the wallet address to be removed from the allowlist
*/
function removeWalletFromAllowlist(address walletAddr) external onlyRole(REGISTRAR_ROLE) {
// get bytecode of wallet
bytes32 codeHash;
assembly {
codeHash := extcodehash(walletAddr)
}
delete bytecodeAllowlist[codeHash];
// get address of wallet module
address impl = IProxy(walletAddr).PROXY_getImplementation();
delete addressImplementationAllowlist[impl];

emit WalletAllowlistChanged(codeHash, walletAddr, false);
}

/**
* @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) {
grantRole(REGISTRAR_ROLE, user);
}

/**
* @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) {
revokeRole(REGISTRAR_ROLE, user);
}

/// ===== View functions =====

/**
* @notice Returns true if an address is Allowlisted, false otherwise
* @param target the address that will be checked for presence in the allowlist
*/
function isAllowlisted(address target) external view override returns (bool) {
if (addressAllowlist[target]) {
return true;
}

// Check if caller is a Allowlisted smart contract wallet
bytes32 codeHash;
assembly {
codeHash := extcodehash(target)
}
if (bytecodeAllowlist[codeHash]) {
// If wallet proxy bytecode is approved, check addr of implementation contract
address impl = IProxy(target).PROXY_getImplementation();

return addressImplementationAllowlist[impl];
}

return false;
}

function setMockValue(uint256 val) public {
mockInt = val;
}

/**
* @notice ERC-165 interface support
* @param interfaceId The interface identifier, which is a 4-byte selector.
*/
function supportsInterface(
bytes4 interfaceId
) public view virtual override(ERC165, AccessControlUpgradeable) returns (bool) {
return interfaceId == type(IOperatorAllowlist).interfaceId || super.supportsInterface(interfaceId);
}

// Override the _authorizeUpgrade function
function _authorizeUpgrade(address newImplementation) internal override onlyRole(UPGRADE_ROLE) {}
}
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,7 @@
},
"dependencies": {
"@openzeppelin/contracts": "^4.9.3",
"@openzeppelin/contracts-upgradeable": "^4.9.3",
"@rari-capital/solmate": "^6.4.0",
"seaport": "https://github.com/immutable/seaport.git#1.5.0+im.1.3",
"solidity-bits": "^0.4.0",
Expand Down
Loading
Loading