From b4f273ff33e1a847ddd7f4108de12026b7f7f112 Mon Sep 17 00:00:00 2001 From: PonyJackal Date: Thu, 16 Jun 2022 15:18:32 -0800 Subject: [PATCH 01/10] fix: reentrancy in NonFungibleRegistryUpgradeable.burn --- .../identity/NonFungibleRegistryEnumerableUpgradeable.sol | 4 +++- .../contracts/identity/NonFungibleRegistryUpgradeable.sol | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol index 002fcdce..ce4ec359 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol @@ -6,6 +6,7 @@ import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeab import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721EnumerableUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721URIStorageUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/access/AccessControlEnumerableUpgradeable.sol"; +import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; @@ -28,6 +29,7 @@ contract NonFungibleRegistryEnumerableUpgradeable is Initializable, ContextUpgradeable, AccessControlEnumerableUpgradeable, + ReentrancyGuardUpgradeable, ERC721EnumerableUpgradeable, ERC721URIStorageUpgradeable, OwnableUpgradeable, @@ -381,7 +383,7 @@ contract NonFungibleRegistryEnumerableUpgradeable is /// @notice burn removes a token from the registry enumeration and refunds registration fee to burner /// @dev only callable by the owner, approved caller when allowTransfers is true or REGISTRAR_ROLE /// @param tokenId unique id to refence the target token - function burn(uint256 tokenId) public virtual { + function burn(uint256 tokenId) public virtual nonReentrant { //solhint-disable-next-line max-line-length require(_isApprovedOrOwner(_msgSender(), tokenId), "NonFungibleRegistry: caller is not owner nor approved nor registrar."); _burn(tokenId); diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol index ced2ec9d..ba835fdb 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol @@ -5,6 +5,7 @@ pragma solidity ^0.8.4; import "@openzeppelin/contracts-upgradeable/token/ERC20/utils/SafeERC20Upgradeable.sol"; import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721URIStorageUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/access/AccessControlEnumerableUpgradeable.sol"; +import "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/utils/ContextUpgradeable.sol"; import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; import "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; @@ -28,6 +29,7 @@ contract NonFungibleRegistryUpgradeable is ContextUpgradeable, AccessControlEnumerableUpgradeable, ERC721URIStorageUpgradeable, + ReentrancyGuardUpgradeable, OwnableUpgradeable, RoyaltiesV2Impl { @@ -378,7 +380,7 @@ contract NonFungibleRegistryUpgradeable is /// @notice burn removes a token from the registry enumeration and refunds registration fee to burner /// @dev only callable by the owner, approved caller when allowTransfers is true or REGISTRAR_ROLE /// @param tokenId unique id to refence the target token - function burn(uint256 tokenId) public virtual { + function burn(uint256 tokenId) public virtual nonReentrant { //solhint-disable-next-line max-line-length require(_isApprovedOrOwner(_msgSender(), tokenId), "NonFungibleRegistry: caller is not owner nor approved nor registrar."); _burn(tokenId); From 0046b1edf9d16c37f8c2c0e0f7f4089c33559445 Mon Sep 17 00:00:00 2001 From: PonyJackal Date: Thu, 16 Jun 2022 17:23:46 -0800 Subject: [PATCH 02/10] feat: add chainId and nonce in signature --- packages/contracts/contracts/modules/LazyMintModule.sol | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/packages/contracts/contracts/modules/LazyMintModule.sol b/packages/contracts/contracts/modules/LazyMintModule.sol index 8183f3ac..000e9f7d 100644 --- a/packages/contracts/contracts/modules/LazyMintModule.sol +++ b/packages/contracts/contracts/modules/LazyMintModule.sol @@ -38,6 +38,8 @@ contract LazyMintModule is Context { string calldata label, string calldata registrationData, uint256 tokenId, + uint256 chainId, + uint256 nonce, bytes calldata signature, address registry) external { @@ -45,19 +47,19 @@ contract LazyMintModule is Context { require(_msgSender() == to, "LazyMintModule: Caller is not recipient."); // require a valid signature from a member of REGISTRAR_ROLE - require(_isValidSignature(to, label, registrationData, tokenId, signature, registry), "LazyMintModule: signature failure."); + require(_isValidSignature(to, label, registrationData, tokenId, chainId, nonce, signature, registry), "LazyMintModule: signature failure."); // issue new token here INfr(registry).register(to, label, registrationData, tokenId); } - function _isValidSignature(address to, string memory label, string memory registrationData, uint256 tokenId, bytes memory signature, address registry) + function _isValidSignature(address to, string memory label, string memory registrationData, uint256 tokenId, uint256 chainId, uint256 nonce, bytes memory signature, address registry) internal view returns (bool) { // convert the payload to a 32 byte hash - bytes32 hash = ECDSA.toEthSignedMessageHash(keccak256(abi.encodePacked(to, label, registrationData, tokenId))); + bytes32 hash = ECDSA.toEthSignedMessageHash(keccak256(abi.encodePacked(to, label, registrationData, tokenId, chainId, nonce))); // check that the signature is from REGISTRAR_ROLE address signer = ECDSA.recover(hash, signature); From cb21104de79061a68faf8d92ca3e019e89ebf090 Mon Sep 17 00:00:00 2001 From: PonyJackal Date: Fri, 17 Jun 2022 06:25:21 -0700 Subject: [PATCH 03/10] fix: check transfer return value in NonFungibleRegistryEnumerableUpgradeable --- .../identity/NonFungibleRegistryEnumerableUpgradeable.sol | 2 +- .../contracts/identity/NonFungibleRegistryUpgradeable.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol index ce4ec359..78b23397 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol @@ -296,7 +296,7 @@ contract NonFungibleRegistryEnumerableUpgradeable is _createLabeledToken(to, label, registrationData, tokenId); uint256 burnAmount = registrationFee * burnFee / 10000; - IERC20Upgradeable(registrationToken).transfer(burnAddress, burnAmount); + require(IERC20Upgradeable(registrationToken).transfer(burnAddress, burnAmount), "NonFungibleRegistry: token transfer failed."); // the fee stays with the token, not the token owner identityStakes[tokenId] = Fee(registrationToken, registrationFee-burnAmount); } diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol index ba835fdb..f321efc6 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol @@ -293,7 +293,7 @@ contract NonFungibleRegistryUpgradeable is _createLabeledToken(to, label, registrationData, tokenId); uint256 burnAmount = registrationFee * burnFee / 10000; - IERC20Upgradeable(registrationToken).transfer(burnAddress, burnAmount); + require(IERC20Upgradeable(registrationToken).transfer(burnAddress, burnAmount), "NonFungibleRegistry: token transfer failed."); // the fee stays with the token, not the token owner identityStakes[tokenId] = Fee(registrationToken, registrationFee-burnAmount); } From e69dcef227e5c4f3ede38963e29f6150b8892b3f Mon Sep 17 00:00:00 2001 From: PonyJackal Date: Fri, 17 Jun 2022 08:07:13 -0700 Subject: [PATCH 04/10] fix: QSP-6 add input validations --- .../contracts/access/NFTAccessControlUpgradeable.sol | 1 + .../contracts/identity/NonFungibleRegistryUpgradeable.sol | 4 ++++ .../contracts/identity/UpgradeableRegistryFactory.sol | 5 +++++ packages/contracts/contracts/modules/BuyModule.sol | 1 + packages/contracts/contracts/modules/MerkleModule.sol | 2 ++ packages/contracts/contracts/utils/Vesting.sol | 4 ++++ 6 files changed, 17 insertions(+) diff --git a/packages/contracts/contracts/access/NFTAccessControlUpgradeable.sol b/packages/contracts/contracts/access/NFTAccessControlUpgradeable.sol index d976d3cb..4267847b 100644 --- a/packages/contracts/contracts/access/NFTAccessControlUpgradeable.sol +++ b/packages/contracts/contracts/access/NFTAccessControlUpgradeable.sol @@ -52,6 +52,7 @@ import "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721Enumer */ abstract contract NFTAccessControlUpgradeable is Initializable, ContextUpgradeable, INFTAccessControlUpgradeable, ERC165Upgradeable { function __NFTAccessControl_init(address registry) internal initializer { + require(registry != address(0), "NFTAccessControlUpgradeable: Invalid registry address."); __Context_init_unchained(); __ERC165_init_unchained(); __NFTAccessControl_init_unchained(); diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol index f321efc6..db65daf5 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol @@ -160,6 +160,10 @@ contract NonFungibleRegistryUpgradeable is address _admin ) public initializer { + require(address(_primaryRegistry) != address(0), "NonFungibleRegistry: Invalid primaryRegistry address."); + require(address(_registrar) != address(0), "NonFungibleRegistry: Invalid registrar address."); + require(address(_admin) != address(0), "NonFungibleRegistry: Invalid admin address."); + __Context_init(); __AccessControlEnumerable_init(); __ERC721URIStorage_init(); diff --git a/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol b/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol index e0f45e1d..7b7023a1 100644 --- a/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol +++ b/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol @@ -76,6 +76,9 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable { address _registrationToken) { require(_names.length == _symbols.length, "RegistryFactory: Initializer arrays must be equal length."); require(_symbols.length == _registrars.length, "RegistryFactory: Initializer arrays must be equal length."); + require(address(_enumerableRegistry) != address(0), "RegistryFactory: Invalid enumerableRegistry address."); + require(address(registry) != address(0), "RegistryFactory: Invalid registry address."); + require(address(registrationToken) != address(0), "RegistryFactory: Invalid registrationToken address."); // set the administrator of the registry factory _setupRole(DEFAULT_ADMIN_ROLE, _admin); @@ -120,6 +123,7 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable { /// @dev can only be called by the DEFAULT_ADMIN_ROLE /// @param _hypernetProfileRegistry address of ERC721 token to use as profile contract function setProfileRegistryAddress(address _hypernetProfileRegistry) external { + require(address(_hypernetProfileRegistry) != address(0), "NonFungibleRegistry: Invalid hypernetProfileRegistry address."); require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "RegistryFactory: must have admin role to set parameters"); hypernetProfileRegistry = _hypernetProfileRegistry; } @@ -128,6 +132,7 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable { /// @dev can only be called by the DEFAULT_ADMIN_ROLE /// @param _registrationToken address of ERC20 token burned during registration function setRegistrationToken(address _registrationToken) external { + require(_registrationToken != address(0), """RegistryFactory: Invalid registrationToken address"); require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "RegistryFactory: must have admin role to set parameters"); registrationToken = _registrationToken; } diff --git a/packages/contracts/contracts/modules/BuyModule.sol b/packages/contracts/contracts/modules/BuyModule.sol index bef83dae..053325e3 100644 --- a/packages/contracts/contracts/modules/BuyModule.sol +++ b/packages/contracts/contracts/modules/BuyModule.sol @@ -40,6 +40,7 @@ contract BuyModule is Context { external virtual { + require(registry != address(0), "BuyModule: Invalid registry address."); address seller = INfr(registry).ownerOf(tokenId); uint256 price = INfr(registry).registrationFee(); // the current owner of the tokenid must be an account in the REGISTRAR_ROLE diff --git a/packages/contracts/contracts/modules/MerkleModule.sol b/packages/contracts/contracts/modules/MerkleModule.sol index 96fadc49..e7f1f06a 100644 --- a/packages/contracts/contracts/modules/MerkleModule.sol +++ b/packages/contracts/contracts/modules/MerkleModule.sol @@ -37,6 +37,8 @@ contract MerkleModule { function redeem(address to, string calldata label, string calldata registrationData, uint256 tokenId, bytes32[] calldata proof, address registry) external { + require(to != address(0), "MerkleModule: Invalid to address"); + require(registry != address(0), "MerkleModule: Invalid registry address"); bytes32 root = INfr(registry).merkleRoot(); require(_verify(_leaf(to, label, registrationData, tokenId), proof, root), "MerkleModule: Invalid merkle proof"); INfr(registry).register(to, label, registrationData, tokenId); diff --git a/packages/contracts/contracts/utils/Vesting.sol b/packages/contracts/contracts/utils/Vesting.sol index f1511694..cc3a2148 100644 --- a/packages/contracts/contracts/utils/Vesting.sol +++ b/packages/contracts/contracts/utils/Vesting.sol @@ -65,6 +65,8 @@ contract Vester { require(vestingBegin_ >= block.timestamp, 'Vester::constructor: vesting begin too early'); require(vestingCliff_ >= vestingBegin_, 'Vester::constructor: cliff is too early'); require(vestingEnd_ > vestingCliff_, 'Vester::constructor: end is too early'); + require(h_ != address(0), "Vester::constructor: Invalid token address"); + require(recipient_ != address(0), "Vester::constructor: Invalid recipient address"); h = h_; recipient = recipient_; @@ -88,6 +90,7 @@ contract Vester { /// @dev This function can only be called by the account set in the recipient variable /// @param recipient_ address to set as the new beneficiary function setRecipient(address recipient_) public { + require(recipient_ != address(0), "Vester::setRecipient: Invalid recipient address."); require(msg.sender == recipient, 'Vester::setRecipient: unauthorized'); recipient = recipient_; } @@ -96,6 +99,7 @@ contract Vester { /// @dev The function allows for beneficiaries to have voting rights before they take possession of their tokens /// @param delegate_ address to recieve the voting rights, does not necessarly have to be the beneficiary function delegate(address delegate_) public { + require(delegate_ != address(0), "Vester::setRecipient: Invalid delegate address."); require(msg.sender == recipient, 'Vester::setRecipient: unauthorized'); IHypertoken(h).delegate(delegate_); } From 65650e011b30f63fe501dd4e161eafac65e46296 Mon Sep 17 00:00:00 2001 From: PonyJackal Date: Wed, 22 Jun 2022 10:33:06 -0700 Subject: [PATCH 05/10] feat: add INfr interface --- .../identity/UpgradeableRegistryFactory.sol | 4 ++-- packages/contracts/contracts/interfaces/INfr.sol | 15 +++++++++++++++ .../contracts/contracts/modules/BatchModule.sol | 8 +------- .../contracts/modules/BulkTransferModule.sol | 8 +------- .../contracts/contracts/modules/BuyModule.sol | 12 +----------- .../contracts/modules/LazyMintModule.sol | 8 +------- 6 files changed, 21 insertions(+), 34 deletions(-) create mode 100644 packages/contracts/contracts/interfaces/INfr.sol diff --git a/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol b/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol index 7b7023a1..f3c0b133 100644 --- a/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol +++ b/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol @@ -77,8 +77,8 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable { require(_names.length == _symbols.length, "RegistryFactory: Initializer arrays must be equal length."); require(_symbols.length == _registrars.length, "RegistryFactory: Initializer arrays must be equal length."); require(address(_enumerableRegistry) != address(0), "RegistryFactory: Invalid enumerableRegistry address."); - require(address(registry) != address(0), "RegistryFactory: Invalid registry address."); - require(address(registrationToken) != address(0), "RegistryFactory: Invalid registrationToken address."); + require(address(_registry) != address(0), "RegistryFactory: Invalid registry address."); + require(address(_registrationToken) != address(0), "RegistryFactory: Invalid registrationToken address."); // set the administrator of the registry factory _setupRole(DEFAULT_ADMIN_ROLE, _admin); diff --git a/packages/contracts/contracts/interfaces/INfr.sol b/packages/contracts/contracts/interfaces/INfr.sol new file mode 100644 index 00000000..1073c2da --- /dev/null +++ b/packages/contracts/contracts/interfaces/INfr.sol @@ -0,0 +1,15 @@ +// SPDX-License-Identifier: MIT +pragma solidity ^0.8.4; + +/// @dev a minimal interface for interacting with Hypernet Protocol NFRs +interface INfr { + function ownerOf(uint256 tokenId) external view returns (address); + function hasRole(bytes32 role, address account) external view returns (bool); + function safeTransferFrom(address from, address to, uint256 tokenId) external; + function REGISTRAR_ROLE() external view returns (bytes32); + function registrationFee() external view returns (uint256); + function registrationToken() external view returns (address); + function burnAddress() external view returns (address); + function register(address to, string calldata label, string calldata registrationData, uint256 tokenId) external; + function isApprovedForAll(address owner, address operator) external view returns (bool); +} \ No newline at end of file diff --git a/packages/contracts/contracts/modules/BatchModule.sol b/packages/contracts/contracts/modules/BatchModule.sol index cb4a821c..074f5771 100644 --- a/packages/contracts/contracts/modules/BatchModule.sol +++ b/packages/contracts/contracts/modules/BatchModule.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.4; import "@openzeppelin/contracts/utils/Context.sol"; +import "../interfaces/INfr.sol"; /** * @title Hypernet Protocol Batch Minting Module for NFRs @@ -53,10 +54,3 @@ contract BatchModule is Context { } } } - -/// @dev a minimal interface for interacting with Hypernet Protocol NFRs -interface INfr { - function register(address to, string calldata label, string calldata registrationData, uint256 tokenId) external; - function hasRole(bytes32 role, address account) external view returns (bool); - function REGISTRAR_ROLE() external view returns (bytes32); -} \ No newline at end of file diff --git a/packages/contracts/contracts/modules/BulkTransferModule.sol b/packages/contracts/contracts/modules/BulkTransferModule.sol index 5543af66..5511eba4 100644 --- a/packages/contracts/contracts/modules/BulkTransferModule.sol +++ b/packages/contracts/contracts/modules/BulkTransferModule.sol @@ -3,6 +3,7 @@ pragma solidity ^0.8.4; import "@openzeppelin/contracts/utils/Context.sol"; +import "../interfaces/INfr.sol"; /** * @title Hypernet Protocol Bulk Transfer Module for NFRs @@ -49,10 +50,3 @@ contract BulkTransferModule is Context { } } } - -/// @dev a minimal interface for interacting with Hypernet Protocol NFRs -interface INfr { - function safeTransferFrom(address from, address to, uint256 tokenId) external; - function isApprovedForAll(address owner, address operator) external view returns (bool); - function REGISTRAR_ROLE() external view returns (bytes32); -} \ No newline at end of file diff --git a/packages/contracts/contracts/modules/BuyModule.sol b/packages/contracts/contracts/modules/BuyModule.sol index 053325e3..44177356 100644 --- a/packages/contracts/contracts/modules/BuyModule.sol +++ b/packages/contracts/contracts/modules/BuyModule.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.4; import "@openzeppelin/contracts/utils/Context.sol"; import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol"; +import "../interfaces/INfr.sol"; /** * @title Hypernet Protocol Buy Module for NFRs @@ -54,14 +55,3 @@ contract BuyModule is Context { require(INfr(registry).ownerOf(tokenId) == _msgSender(), "BuyModule: NFI purchase transfer failed"); } } - -/// @dev a minimal interface for interacting with Hypernet Protocol NFRs -interface INfr { - function ownerOf(uint256 tokenId) external view returns (address); - function hasRole(bytes32 role, address account) external view returns (bool); - function safeTransferFrom(address from, address to, uint256 tokenId) external; - function REGISTRAR_ROLE() external view returns (bytes32); - function registrationFee() external view returns (uint256); - function registrationToken() external view returns (address); - function burnAddress() external view returns (address); -} \ No newline at end of file diff --git a/packages/contracts/contracts/modules/LazyMintModule.sol b/packages/contracts/contracts/modules/LazyMintModule.sol index 000e9f7d..68a54492 100644 --- a/packages/contracts/contracts/modules/LazyMintModule.sol +++ b/packages/contracts/contracts/modules/LazyMintModule.sol @@ -4,6 +4,7 @@ pragma solidity ^0.8.4; import "@openzeppelin/contracts/utils/cryptography/ECDSA.sol"; import "@openzeppelin/contracts/utils/Context.sol"; +import "../interfaces/INfr.sol"; /** * @title Hypernet Protocol Lazy Minting Module for NFRs @@ -67,10 +68,3 @@ contract LazyMintModule is Context { return INfr(registry).hasRole(INfr(registry).REGISTRAR_ROLE(), signer); } } - -/// @dev a minimal interface for interacting with Hypernet Protocol NFRs -interface INfr { - function register(address to, string calldata label, string calldata registrationData, uint256 tokenId) external; - function hasRole(bytes32 role, address account) external view returns (bool); - function REGISTRAR_ROLE() external view returns (bytes32); -} \ No newline at end of file From 9ee21fda4538fcf1caed256710c4f4d18119326d Mon Sep 17 00:00:00 2001 From: PonyJackal Date: Wed, 22 Jun 2022 10:57:14 -0700 Subject: [PATCH 06/10] fix: QSP-10 Token May Have Multiple Labels --- .../identity/NonFungibleRegistryEnumerableUpgradeable.sol | 1 + .../contracts/identity/NonFungibleRegistryUpgradeable.sol | 1 + 2 files changed, 2 insertions(+) diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol index 78b23397..cb00667b 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol @@ -342,6 +342,7 @@ contract NonFungibleRegistryEnumerableUpgradeable is require(_isApprovedOrOwner(_msgSender(), tokenId), "NonFungibleRegistry: caller is not owner nor approved nor registrar."); require(!_mappingExists(label), "NonFungibleRegistry: label is already registered."); + delete registryMap[reverseRegistryMap[tokenId]]; registryMap[label] = tokenId; reverseRegistryMap[tokenId] = label; emit LabelUpdated(tokenId, label); diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol index db65daf5..d65aa3cb 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol @@ -343,6 +343,7 @@ contract NonFungibleRegistryUpgradeable is require(_isApprovedOrOwner(_msgSender(), tokenId), "NonFungibleRegistry: caller is not owner nor approved nor registrar."); require(!_mappingExists(label), "NonFungibleRegistry: label is already registered."); + delete registryMap[reverseRegistryMap[tokenId]]; registryMap[label] = tokenId; reverseRegistryMap[tokenId] = label; emit LabelUpdated(tokenId, label); From d4ca94fd24d27bc6eab2f9e8456b871ab68d90be Mon Sep 17 00:00:00 2001 From: PonyJackal Date: Wed, 22 Jun 2022 11:09:15 -0700 Subject: [PATCH 07/10] feat: prevent reentrancy in NonFungibleRegistryEnumerableUpgradeable, UpgradeableRegistryFactory --- .../identity/NonFungibleRegistryEnumerableUpgradeable.sol | 2 +- .../contracts/identity/NonFungibleRegistryUpgradeable.sol | 2 +- .../contracts/identity/UpgradeableRegistryFactory.sol | 7 ++++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol index cb00667b..88262658 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol @@ -287,7 +287,7 @@ contract NonFungibleRegistryEnumerableUpgradeable is /// @param label a unique label to attach to the token /// @param registrationData data to store in the tokenURI /// @param tokenId unique uint256 identifier for the newly created token - function registerByToken(address to, string calldata label, string calldata registrationData, uint256 tokenId) external virtual { + function registerByToken(address to, string calldata label, string calldata registrationData, uint256 tokenId) external virtual nonReentrant { require(registrationToken != address(0), "NonFungibleRegistry: registration by token not enabled."); require(!_mappingExists(label), "NonFungibleRegistry: label is already registered."); // user must approve the registry to collect the registration fee from their wallet diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol index d65aa3cb..d8221084 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol @@ -288,7 +288,7 @@ contract NonFungibleRegistryUpgradeable is /// @param label a unique label to attach to the token /// @param registrationData data to store in the tokenURI /// @param tokenId unique uint256 identifier for the newly created token - function registerByToken(address to, string calldata label, string calldata registrationData, uint256 tokenId) external virtual { + function registerByToken(address to, string calldata label, string calldata registrationData, uint256 tokenId) external virtual nonReentrant { require(registrationToken != address(0), "NonFungibleRegistry: registration by token not enabled."); require(!_mappingExists(label), "NonFungibleRegistry: label is already registered."); // user must approve the registry to collect the registration fee from their wallet diff --git a/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol b/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol index f3c0b133..6bad826d 100644 --- a/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol +++ b/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol @@ -8,6 +8,7 @@ import "./NonFungibleRegistryUpgradeable.sol"; import "@openzeppelin/contracts/access/AccessControlEnumerable.sol"; import "@openzeppelin/contracts/proxy/beacon/UpgradeableBeacon.sol"; import "@openzeppelin/contracts/proxy/beacon/BeaconProxy.sol"; +import "@openzeppelin/contracts/security/ReentrancyGuard.sol"; /** * @title Hypernet Protocol Non Fungible Registry Factory @@ -21,7 +22,7 @@ import "@openzeppelin/contracts/proxy/beacon/BeaconProxy.sol"; * See unit tests for example usage: * https://github.com/GoHypernet/hypernet-protocol/blob/dev/packages/contracts/test/upgradeable-factory-test.js */ -contract UpgradeableRegistryFactory is AccessControlEnumerable { +contract UpgradeableRegistryFactory is AccessControlEnumerable, ReentrancyGuard { /// @dev address of our upgradeble registry with enumeration proxy beacon address public enumerableRegistryBeacon; @@ -187,7 +188,7 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable { } } - function _createEnumerableRegistry(string memory _name, string memory _symbol, address _registrar) private { + function _createEnumerableRegistry(string memory _name, string memory _symbol, address _registrar) private nonReentrant { require(_registrar != address(0), "RegistryFactory: Registrar address must not be 0."); require(!_registryExists(_name), "RegistryFactory: Registry by that name exists."); @@ -198,7 +199,7 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable { emit RegistryCreated(address(proxy)); } - function _createRegistry(string memory _name, string memory _symbol, address _registrar) private { + function _createRegistry(string memory _name, string memory _symbol, address _registrar) private nonReentrant { require(_registrar != address(0), "RegistryFactory: Registrar address must not be 0."); require(!_registryExists(_name), "RegistryFactory: Registry by that name exists."); From de6c251bf89a68bd06b5da8a483b28480836aa61 Mon Sep 17 00:00:00 2001 From: PonyJackal Date: Wed, 22 Jun 2022 12:07:25 -0700 Subject: [PATCH 08/10] fix: Tautology in UpgradeableRegistryFactory.sol#140 --- .../contracts/contracts/identity/UpgradeableRegistryFactory.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol b/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol index 6bad826d..033e22f2 100644 --- a/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol +++ b/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol @@ -143,7 +143,7 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable, ReentrancyGuard /// @param _registrationFee burn fee amount function setRegistrationFee(uint256 _registrationFee) external { require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "RegistryFactory: must have admin role to set parameters"); - require(registrationFee >= 0, "RegistryFactory: Registration fee must be nonnegative."); + require(_registrationFee >= 0, "RegistryFactory: Registration fee must be nonnegative."); registrationFee = _registrationFee; } From bbe03553cadd18e7fc0a7c290b7f1760b656e147 Mon Sep 17 00:00:00 2001 From: PonyJackal Date: Wed, 22 Jun 2022 12:23:33 -0700 Subject: [PATCH 09/10] fix: typos --- .../identity/NonFungibleRegistryEnumerableUpgradeable.sol | 2 +- .../contracts/identity/NonFungibleRegistryUpgradeable.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol index 88262658..6936d6de 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryEnumerableUpgradeable.sol @@ -393,7 +393,7 @@ contract NonFungibleRegistryEnumerableUpgradeable is if (identityStakes[tokenId].amount != 0) { // send the registration fee to the token burner // don't set a registration token you do not control/trust, otherwise, this could be used for re-entrancy attack - require(IERC20Upgradeable(identityStakes[tokenId].token).transfer(_msgSender(), identityStakes[tokenId].amount), "NonFungibleRegistry: token tansfer failed."); + require(IERC20Upgradeable(identityStakes[tokenId].token).transfer(_msgSender(), identityStakes[tokenId].amount), "NonFungibleRegistry: token transfer failed."); delete identityStakes[tokenId]; } } diff --git a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol index d8221084..6a89349c 100644 --- a/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol +++ b/packages/contracts/contracts/identity/NonFungibleRegistryUpgradeable.sol @@ -248,7 +248,7 @@ contract NonFungibleRegistryUpgradeable is /// @param _primaryRegistry address to set as the primary registry function setPrimaryRegistry(address _primaryRegistry) external { require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "NonFungibleRegistry: must be admin."); - // allow this feature to be disablled by setting to 0 address + // allow this feature to be disabled by setting to 0 address if (address(_primaryRegistry) == address(0)) { primaryRegistry = address(0); } else { From 12729f690675c6f328711a1614616d712eff6aeb Mon Sep 17 00:00:00 2001 From: PonyJackal Date: Wed, 22 Jun 2022 12:26:56 -0700 Subject: [PATCH 10/10] feat: add isAdmin modifier --- .../identity/UpgradeableRegistryFactory.sol | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol b/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol index 033e22f2..bf547f4a 100644 --- a/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol +++ b/packages/contracts/contracts/identity/UpgradeableRegistryFactory.sol @@ -123,26 +123,23 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable, ReentrancyGuard /// @notice setProfileRegistryAddress change the address of the profile registry contract /// @dev can only be called by the DEFAULT_ADMIN_ROLE /// @param _hypernetProfileRegistry address of ERC721 token to use as profile contract - function setProfileRegistryAddress(address _hypernetProfileRegistry) external { + function setProfileRegistryAddress(address _hypernetProfileRegistry) external isAdmin { require(address(_hypernetProfileRegistry) != address(0), "NonFungibleRegistry: Invalid hypernetProfileRegistry address."); - require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "RegistryFactory: must have admin role to set parameters"); hypernetProfileRegistry = _hypernetProfileRegistry; } /// @notice setRegistrationToken setter function for configuring which ERC20 token is burned when adding new apps /// @dev can only be called by the DEFAULT_ADMIN_ROLE /// @param _registrationToken address of ERC20 token burned during registration - function setRegistrationToken(address _registrationToken) external { + function setRegistrationToken(address _registrationToken) external isAdmin { require(_registrationToken != address(0), """RegistryFactory: Invalid registrationToken address"); - require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "RegistryFactory: must have admin role to set parameters"); registrationToken = _registrationToken; } /// @notice setRegistrationFee setter function for configuring how much token is burned when adding new apps /// @dev can only be called by the DEFAULT_ADMIN_ROLE /// @param _registrationFee burn fee amount - function setRegistrationFee(uint256 _registrationFee) external { - require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "RegistryFactory: must have admin role to set parameters"); + function setRegistrationFee(uint256 _registrationFee) external isAdmin { require(_registrationFee >= 0, "RegistryFactory: Registration fee must be nonnegative."); registrationFee = _registrationFee; } @@ -150,8 +147,7 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable, ReentrancyGuard /// @notice setBurnAddress setter function for configuring where tokens are sent when calling createRegistryByToken /// @dev can only be called by the DEFAULT_ADMIN_ROLE /// @param _burnAddress address where creation fee is to be sent - function setBurnAddress(address _burnAddress) external { - require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "RegistryFactory: must have admin role to set parameters"); + function setBurnAddress(address _burnAddress) external isAdmin { burnAddress = _burnAddress; } @@ -220,4 +216,9 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable, ReentrancyGuard // does the recipient have a non-zero balance. return ((hypernetProfileRegistry == address(0)) || (IERC721Upgradeable(hypernetProfileRegistry).balanceOf(owner) > 0)); } + + modifier isAdmin() { + require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "RegistryFactory: must have admin role to set parameters"); + _; + } } \ No newline at end of file