Improvements (#15)

naszam · Sep 10, 2022 · ff3350f · ff3350f
1 parent 50deb01
commit ff3350f
Show file tree

Hide file tree

Showing 15 changed files with 701 additions and 618 deletions.
diff --git a/.github/workflows/echidna.yml b/.github/workflows/echidna.yml
@@ -39,6 +39,6 @@ jobs:
           test-mode: assertion
           test-limit: 50000
           seq-len: 100
-          solc-version: 0.8.4
+          solc-version: 0.8.9
           echidna-version: v2.0.0
           crytic-args: --hardhat-ignore-compile
diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml
@@ -1,10 +1,6 @@
 name: Lint
 
-on:
-  push:
-    branches:
-      - master
-  pull_request:
+on: push
 
 jobs:
   run-linters:
@@ -18,17 +14,7 @@ jobs:
         uses: actions/setup-node@v2
         with:
           node-version: 12
-
-      - name: Get yarn cache directory path
-        id: yarn-cache-dir-path
-        run: echo "::set-output name=dir::$(yarn cache dir)"
-
-      - uses: actions/cache@v2
-        with:
-          path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
-          key: yarn-${{ hashFiles('**/yarn.lock') }}
-          restore-keys: |
-            yarn-
+          cache: 'yarn'
 
       - name: Install dependencies
         run: yarn --no-progress --non-interactive --frozen-lockfile

diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml
@@ -8,7 +8,7 @@ jobs:
     runs-on: ubuntu-latest
     steps:
     - name: Checkout repository
-      uses: actions/checkout@v2
+      uses: actions/checkout@v3
 
     - name: Run Slither
       uses: crytic/[email protected]

diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
@@ -1,10 +1,6 @@
 name: Tests
 
-on:
-  push:
-    branches:
-      - master
-  pull_request:
+on: push
 
 jobs:
   unit-tests:
@@ -18,17 +14,7 @@ jobs:
         uses: actions/setup-node@v2
         with:
           node-version: 12
-
-      - name: Get yarn cache directory path
-        id: yarn-cache-dir-path
-        run: echo "::set-output name=dir::$(yarn cache dir)"
-
-      - uses: actions/cache@v2
-        with:
-          path: ${{ steps.yarn-cache-dir-path.outputs.dir }}
-          key: yarn-${{ hashFiles('**/yarn.lock') }}
-          restore-keys: |
-            yarn-
+          cache: 'yarn'
 
       - name: Install dependencies
         run: yarn --no-progress --non-interactive --frozen-lockfile

diff --git a/README.md b/README.md
@@ -1,8 +1,9 @@
-[![#ubuntu 20.04](https://img.shields.io/badge/ubuntu-v20.04-orange?style=plastic)](https://ubuntu.com/download/desktop)
-[![#node 12](https://img.shields.io/badge/node-v12-blue?style=plastic)](https://github.com/nvm-sh/nvm#installation-and-update)
+[![#ubuntu 20.04.3](https://img.shields.io/badge/ubuntu-v20.04.3-orange?style=plastic)](https://wiki.ubuntu.com/Releases)
+[![#node 16.13.1](https://img.shields.io/badge/node-v16.13.1-blue?style=plastic)](https://github.com/nvm-sh/nvm#installation-and-update)
 [![built-with openzeppelin](https://img.shields.io/badge/built%20with-OpenZeppelin-3677FF?style=plastic)](https://docs.openzeppelin.com/)
-[![#solc 0.6.12](https://img.shields.io/badge/solc-v0.6.12-brown?style=plastic)](https://github.com/ethereum/solidity/releases/tag/v0.6.12)
+[![#solc 0.8.9](https://img.shields.io/badge/solc-v0.8.9-brown?style=plastic)](https://github.com/ethereum/solidity/releases/tag/v0.8.9)
 [![#testnet sokol](https://img.shields.io/badge/testnet-Sokol-grey?style=plastic&logo=Ethereum)](#development-deployments)
+[![#license AGPLv3](https://img.shields.io/badge/license-AGPLv3-purple?style=plastic)](https://www.gnu.org/licenses/agpl-3.0)
 
 [![Lint](https://github.com/naszam/maker-badges/actions/workflows/lint.yml/badge.svg)](https://github.com/naszam/maker-badges/actions/workflows/lint.yml)
 [![Slither](https://github.com/naszam/maker-badges/actions/workflows/slither.yml/badge.svg)](https://github.com/naszam/maker-badges/actions/workflows/slither.yml)

diff --git a/contracts/BadgeRoles.sol b/contracts/BadgeRoles.sol
@@ -1,25 +1,24 @@
 /// SPDX-License-Identifier: AGPL-3.0-or-later
 
-pragma solidity 0.8.4;
+pragma solidity 0.8.9;
 
 import "@openzeppelin/contracts/access/AccessControlEnumerable.sol";
-import "@openzeppelin/contracts/security/Pausable.sol";
 import "@openzeppelin/contracts/metatx/MinimalForwarder.sol";
 import "@openzeppelin/contracts/metatx/ERC2771Context.sol";
 
 /// @title Non-transferable Badges for Maker Ecosystem Activity, CDIP 18, 29, 38
 /// @author Nazzareno Massari @naszam
-/// @notice BadgeRoles Access Management for Default Admin, Templater and Pauser Role
-/// @dev See https://github.com/makerdao/community/issues/537
-/// @dev See https://github.com/makerdao/community/issues/721
-/// @dev See https://github.com/makerdao/community/issues/1180
-/// @dev All function calls are currently implemented without side effects through TDD approach
-/// @dev OpenZeppelin Library is used for secure contract development
-contract BadgeRoles is AccessControlEnumerable, Pausable, ERC2771Context {
+/// @notice BadgeRoles Access Management for Default Admin and Templater Roles
+
+contract BadgeRoles is AccessControlEnumerable, ERC2771Context {
     /// @dev Roles
     bytes32 public constant ADMIN_ROLE = keccak256("ADMIN_ROLE");
     bytes32 public constant TEMPLATER_ROLE = keccak256("TEMPLATER_ROLE");
-    bytes32 public constant PAUSER_ROLE = keccak256("PAUSER_ROLE");
+
+    /// @dev Errors
+    error OnlyDefAdmin();
+    error ZeroAddress();
+    error OnlyPauser();
 
     constructor(MinimalForwarder forwarder, address multisig) ERC2771Context(address(forwarder)) {
         require(multisig != address(0), "MakerBadges/invalid-multisig-address");
@@ -28,7 +27,6 @@ contract BadgeRoles is AccessControlEnumerable, Pausable, ERC2771Context {
 
         _setupRole(ADMIN_ROLE, multisig);
         _setupRole(TEMPLATER_ROLE, multisig);
-        _setupRole(PAUSER_ROLE, multisig);
     }
 
     /// @dev Functions
@@ -38,8 +36,8 @@ contract BadgeRoles is AccessControlEnumerable, Pausable, ERC2771Context {
     /// @param account Address of the new Admin
     /// @return True if account is added as Admin
     function addAdmin(address account) external returns (bool) {
-        require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "MakerBadges/only-def-admin");
-        require(account != address(0), "MakerBadges/invalid-account-address");
+        if (!hasRole(DEFAULT_ADMIN_ROLE, _msgSender())) revert OnlyDefAdmin();
+        if (account == address(0)) revert ZeroAddress();
         grantRole(ADMIN_ROLE, account);
         return true;
     }
@@ -49,7 +47,7 @@ contract BadgeRoles is AccessControlEnumerable, Pausable, ERC2771Context {
     /// @param account Address of the Admin
     /// @return True if account is removed as Admin
     function removeAdmin(address account) external returns (bool) {
-        require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "MakerBadges/only-def-admin");
+        if (!hasRole(DEFAULT_ADMIN_ROLE, _msgSender())) revert OnlyDefAdmin();
         revokeRole(ADMIN_ROLE, account);
         return true;
     }
@@ -59,8 +57,8 @@ contract BadgeRoles is AccessControlEnumerable, Pausable, ERC2771Context {
     /// @param account Address of the new Templater
     /// @return True if account is added as Templater
     function addTemplater(address account) external returns (bool) {
-        require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "MakerBadges/only-def-admin");
-        require(account != address(0), "MakerBadges/invalid-account-address");
+        if (!hasRole(DEFAULT_ADMIN_ROLE, _msgSender())) revert OnlyDefAdmin();
+        if (account == address(0)) revert ZeroAddress();
         grantRole(TEMPLATER_ROLE, account);
         return true;
     }
@@ -70,25 +68,11 @@ contract BadgeRoles is AccessControlEnumerable, Pausable, ERC2771Context {
     /// @param account Address of the Templater
     /// @return True if account is removed as Templater
     function removeTemplater(address account) external returns (bool) {
-        require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "MakerBadges/only-def-admin");
+        if (!hasRole(DEFAULT_ADMIN_ROLE, _msgSender())) revert OnlyDefAdmin();
         revokeRole(TEMPLATER_ROLE, account);
         return true;
     }
 
-    /// @notice Pause all the functions
-    /// @dev the caller must have the 'PAUSER_ROLE'
-    function pause() external {
-        require(hasRole(PAUSER_ROLE, _msgSender()), "MakerBadges/only-pauser");
-        _pause();
-    }
-
-    /// @notice Unpause all the functions
-    /// @dev the caller must have the 'PAUSER_ROLE'
-    function unpause() external {
-        require(hasRole(PAUSER_ROLE, _msgSender()), "MakerBadges/only-pauser");
-        _unpause();
-    }
-
     function _msgSender() internal view virtual override(Context, ERC2771Context) returns (address sender) {
         return super._msgSender();
     }

diff --git a/contracts/MakerBadges.sol b/contracts/MakerBadges.sol
@@ -6,7 +6,7 @@
 
 /// SPDX-License-Identifier: AGPL-3.0-or-later
 
-pragma solidity 0.8.4;
+pragma solidity 0.8.9;
 
 import "./BadgeRoles.sol";
 import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";
@@ -15,11 +15,7 @@ import "@openzeppelin/contracts/utils/cryptography/MerkleProof.sol";
 /// @title Non-transferable Badges for Maker Ecosystem Activity, CDIP 18, 29, 38
 /// @author Nazzareno Massari @naszam
 /// @notice MakerBadges to manage Templates and activate Non-transferable MakerBadges by redeemers
-/// @dev See https://github.com/makerdao/community/issues/537
-/// @dev See https://github.com/makerdao/community/issues/721
-/// @dev See https://github.com/makerdao/community/issues/1180
-/// @dev All function calls are currently implemented without side effects through TDD approach
-/// @dev OpenZeppelin Library is used for secure contract development
+
 contract MakerBadges is BadgeRoles, ERC721URIStorage {
     /// @dev Libraries
     using MerkleProof for bytes32[];
@@ -44,6 +40,17 @@ contract MakerBadges is BadgeRoles, ERC721URIStorage {
     event TemplateUpdated(uint256 indexed templateId, string name, string description, string image);
     event BadgeActivated(uint256 indexed tokenId, uint256 indexed templateId);
 
+    /// @dev Errors
+    error OverflowUint96();
+    error OnlyDefAmin();
+    error OnlyAdmin();
+    error OnlyTemplater();
+    error InvalidTemplateId();
+    error OnlyRedeemer();
+    error AlreadyClaimed();
+    error InvalidTokenId();
+    error TransferDisabled();
+
     constructor(MinimalForwarder forwarder, address multisig)
         ERC721("MakerBadges", "MAKER")
         BadgeRoles(forwarder, multisig)
@@ -55,22 +62,23 @@ contract MakerBadges is BadgeRoles, ERC721URIStorage {
     /// @dev Revert on overflow
     /// @param x Value to cast
     function toUint96(uint256 x) internal pure returns (uint96 z) {
-        require((z = uint96(x)) == x, "MakerBadges/uint96-overflow");
+        z = uint96(x);
+        if (z != x) revert OverflowUint96();
     }
 
     /// @notice Set the baseURI
     /// @dev Update the baseURI specified in the constructor
     /// @param baseURI New baseURI
     function setBaseURI(string calldata baseURI) external {
-        require(hasRole(DEFAULT_ADMIN_ROLE, _msgSender()), "MakerBadges/only-def-admin");
+        if (!hasRole(DEFAULT_ADMIN_ROLE, _msgSender())) revert OnlyDefAdmin();
         baseTokenURI = baseURI;
     }
 
     /// @notice Set Merkle Tree Root Hashes array
     /// @dev Called by admin to update roots for different address batches by templateId
     /// @param _roots Root hashes of the Merkle Trees by templateId
-    function setRootHashes(bytes32[] calldata _roots) external whenNotPaused {
-        require(hasRole(ADMIN_ROLE, _msgSender()), "MakerBadges/only-admin");
+    function setRootHashes(bytes32[] calldata _roots) external {
+        if (!hasRole(ADMIN_ROLE, _msgSender())) revert OnlyAdmin();
         roots = _roots;
     }
 
@@ -85,8 +93,8 @@ contract MakerBadges is BadgeRoles, ERC721URIStorage {
         string calldata name,
         string calldata description,
         string calldata image
-    ) external whenNotPaused {
-        require(hasRole(TEMPLATER_ROLE, _msgSender()), "MakerBadges/only-templater");
+    ) external {
+        if (!hasRole(TEMPLATER_ROLE, _msgSender())) revert OnlyTemplater();
 
         uint256 id = templateIds++;
 
@@ -108,9 +116,9 @@ contract MakerBadges is BadgeRoles, ERC721URIStorage {
         string calldata name,
         string calldata description,
         string calldata image
-    ) external whenNotPaused {
-        require(hasRole(TEMPLATER_ROLE, _msgSender()), "MakerBadges/only-templater");
-        require(templateIds > templateId, "MakerBadges/invalid-template-id");
+    ) external {
+        if (!hasRole(TEMPLATER_ROLE, _msgSender())) revert OnlyTemplater();
+        if (templateIds <= templateId) revert InvalidTemplateId();
         templates[templateId].name = name;
         templates[templateId].description = description;
         templates[templateId].image = image;
@@ -129,19 +137,16 @@ contract MakerBadges is BadgeRoles, ERC721URIStorage {
         bytes32[] calldata proof,
         uint256 templateId,
         string calldata tokenURI
-    ) external whenNotPaused returns (bool) {
-        require(templateIds > templateId, "MakerBadges/invalid-template-id");
-        require(
-            proof.verify(roots[templateId], keccak256(abi.encodePacked(_msgSender()))),
-            "MakerBadges/only-redeemer"
-        );
+    ) external returns (bool) {
+        if (templateIds <= templateId) revert InvalidTemplateId();
+        if (!proof.verify(roots[templateId], keccak256(abi.encodePacked(_msgSender())))) revert OnlyRedeemer();
 
         uint256 _tokenId = _getTokenId(_msgSender(), templateId);
 
         /// @dev Increase the quantities
         templateQuantities[templateId] += 1;
 
-        require(_mintWithTokenURI(_msgSender(), _tokenId, tokenURI), "MakerBadges/badge-not-minted");
+        if (!_mintWithTokenURI(_msgSender(), _tokenId, tokenURI)) revert AlreadyClaimed();
 
         emit BadgeActivated(_tokenId, templateId);
         return true;
@@ -152,7 +157,7 @@ contract MakerBadges is BadgeRoles, ERC721URIStorage {
     /// @param tokenId Token Id of the Badge
     /// @return redeemer Redeemer address associated with the tokenId
     function getBadgeRedeemer(uint256 tokenId) external view returns (address redeemer) {
-        require(_exists(tokenId), "MakerBadges/invalid-token-id");
+        if (!_exists(tokenId)) revert InvalidTokenId();
         (redeemer, ) = _unpackTokenId(tokenId);
     }
 
@@ -161,7 +166,7 @@ contract MakerBadges is BadgeRoles, ERC721URIStorage {
     /// @param tokenId Token Id of the Badge
     /// @return templateId Template Id associated with the tokenId
     function getBadgeTemplate(uint256 tokenId) external view returns (uint256 templateId) {
-        require(_exists(tokenId), "MakerBadges/invalid-token-id");
+        if (!_exists(tokenId)) revert InvalidTokenId();
         (, templateId) = _unpackTokenId(tokenId);
     }
 
@@ -172,9 +177,9 @@ contract MakerBadges is BadgeRoles, ERC721URIStorage {
     /// @param templateId Template Id
     /// @return tokenId Token Id associated with the redeemer and templateId
     function getTokenId(address redeemer, uint256 templateId) external view returns (uint256 tokenId) {
-        require(templateIds > templateId, "MakerBadges/invalid-template-id");
+        if (templateIds <= templateId) revert InvalidTemplateId();
         tokenId = _getTokenId(redeemer, templateId);
-        require(_exists(tokenId), "MakerBadges/invalid-token-id");
+        if (!_exists(tokenId)) revert InvalidTokenId();
     }
 
     /// @notice ERC721 _transfer() Disabled
@@ -185,7 +190,7 @@ contract MakerBadges is BadgeRoles, ERC721URIStorage {
         address,
         uint256
     ) internal pure override {
-        revert("MakerBadges/token-transfer-disabled");
+        revert TransferDisabled();
     }
 
     /// @notice Generate tokenId

diff --git a/contracts/test/MakerBadgesEchidnaTest.sol b/contracts/test/MakerBadgesEchidnaTest.sol
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: AGPL-3.0-or-later
-pragma solidity 0.8.4;
+pragma solidity 0.8.9;
 
 import "../MakerBadges.sol";
 
@@ -25,8 +25,4 @@ contract MakerBadgesEchidnaTest is MakerBadges {
     function echidna_templater_constant() public view returns (bool) {
         return hasRole(TEMPLATER_ROLE, multisig);
     }
-
-    function echidna_pauser_constant() public view returns (bool) {
-        return hasRole(PAUSER_ROLE, multisig);
-    }
 }
diff --git a/contracts/test/TokenId.sol b/contracts/test/TokenId.sol
@@ -1,6 +1,6 @@
 // SPDX-License-Identifier: AGPL-3.0-or-later
 
-pragma solidity 0.8.4;
+pragma solidity 0.8.9;
 
 contract TokenId {
     constructor() {}

diff --git a/hardhat.config.ts b/hardhat.config.ts
@@ -53,7 +53,7 @@ const config: HardhatUserConfig = {
     xdai: createNetworkConfig("xdai"),
   },
   solidity: {
-    version: "0.8.4",
+    version: "0.8.9",
     settings: {
       optimizer: {
         enabled: false,