Feat - Quantstamp Report

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();

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,
@@ -285,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
@@ -294,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);
     }
@@ -340,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);
@@ -381,7 +384,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);
@@ -390,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];
         }
     }

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
 {
@@ -158,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();
@@ -242,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 {
@@ -282,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
@@ -291,7 +297,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);
     }
@@ -337,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);
@@ -378,7 +385,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);

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;
@@ -76,6 +77,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);
@@ -119,33 +123,31 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable {
     /// @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 {
-        require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "RegistryFactory: must have admin role to set parameters");
+    function setProfileRegistryAddress(address _hypernetProfileRegistry) external isAdmin {
+        require(address(_hypernetProfileRegistry) != address(0), "NonFungibleRegistry: Invalid hypernetProfileRegistry address.");
         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 {
-        require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "RegistryFactory: must have admin role to set parameters");
+    function setRegistrationToken(address _registrationToken) external isAdmin {
+        require(_registrationToken != address(0), """RegistryFactory: Invalid registrationToken address");
         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");
-        require(registrationFee >= 0, "RegistryFactory: Registration fee must be nonnegative.");
+    function setRegistrationFee(uint256 _registrationFee) external isAdmin {
+        require(_registrationFee >= 0, "RegistryFactory: Registration fee must be nonnegative.");
         registrationFee = _registrationFee;
     }
 
     /// @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;
     }
 
@@ -182,7 +184,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.");
 
@@ -193,7 +195,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.");
 
@@ -214,4 +216,9 @@ contract UpgradeableRegistryFactory is AccessControlEnumerable {
         // 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");
+        _;
+    }
 }

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);
+}

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);
-}

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);
-}

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
@@ -40,6 +41,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
@@ -53,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);
-}