From a84371362d2e0eeb5d5141abd907470f7e8d1b49 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Thu, 15 Feb 2024 14:13:56 -0300 Subject: [PATCH 01/22] slither warnings --- script/Deploy001_NftReward.s.sol | 2 +- src/NftReward.sol | 5 ++++- test/NftReward.t.sol | 2 +- 3 files changed, 6 insertions(+), 3 deletions(-) diff --git a/script/Deploy001_NftReward.s.sol b/script/Deploy001_NftReward.s.sol index b35906b..a893a83 100644 --- a/script/Deploy001_NftReward.s.sol +++ b/script/Deploy001_NftReward.s.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.13; +pragma solidity ^0.8.20; import {Script} from "forge-std/Script.sol"; import {NftReward} from "../src/NftReward.sol"; diff --git a/src/NftReward.sol b/src/NftReward.sol index c4c54cf..e762c6c 100644 --- a/src/NftReward.sol +++ b/src/NftReward.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.13; +pragma solidity ^0.8.20; import "@openzeppelin/contracts-upgradeable/utils/cryptography/EIP712Upgradeable.sol"; import "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol"; @@ -74,6 +74,8 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus public initializer { + require(_minter != address(0), "Minter cannot be zero address"); + require(_initialOwner != address(0), "Initial owner cannot be zero address"); __ERC721_init(_tokenName, _tokenSymbol); __Ownable_init(_initialOwner); __EIP712_init("NftReward-Domain", "1"); @@ -197,6 +199,7 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus * @param _newMinter New minter address */ function setMinter(address _newMinter) external onlyOwner { + require(_newMinter != address(0), "Minter cannot be zero address"); minter = _newMinter; } diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index a3a798b..ef67db4 100644 --- a/test/NftReward.t.sol +++ b/test/NftReward.t.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.13; +pragma solidity ^0.8.20; import {Test, console} from "forge-std/Test.sol"; import {NftReward} from "../src/NftReward.sol"; From e7be2b4045d9442e736d6e3445eee3c6a494fe47 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Thu, 15 Feb 2024 14:53:50 -0300 Subject: [PATCH 02/22] test slither --- .github/workflows/slither.yml | 39 +++++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 .github/workflows/slither.yml diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml new file mode 100644 index 0000000..3674a87 --- /dev/null +++ b/.github/workflows/slither.yml @@ -0,0 +1,39 @@ +name: Slither Analysis +on: + push: + branches: + - development + paths: + - '**.sol' + pull_request: + paths: + - '**.sol' + +jobs: + slither-analysis: + name: Slither Analysis + runs-on: ubuntu-latest + + steps: + - uses: actions/checkout@v3 + + - name: Set up Python 3.10 + uses: actions/setup-python@v4 + with: + python-version: "3.10" + + - name: Install Foundry + uses: foundry-rs/foundry-toolchain@v1 + with: + version: nightly + + - name: Install Slither + run: pip3 install slither-analyzer + + - name: Install dependencies + run: | + yarn install + yarn workspace @ubiquity/contracts forge:install + + - name: Test with Slither + run: yarn workspace @ubiquity/contracts run test:slither \ No newline at end of file From b6fe3bbd355d40a319c3f7f9e5bb329a01f1170e Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Thu, 15 Feb 2024 14:58:16 -0300 Subject: [PATCH 03/22] test default --- .github/workflows/slither.yml | 49 ++++++++++++++--------------------- 1 file changed, 20 insertions(+), 29 deletions(-) diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml index 3674a87..7a0f733 100644 --- a/.github/workflows/slither.yml +++ b/.github/workflows/slither.yml @@ -1,39 +1,30 @@ name: Slither Analysis + on: push: - branches: - - development - paths: - - '**.sol' + branches: [ development ] pull_request: - paths: - - '**.sol' + branches: [ development ] jobs: - slither-analysis: - name: Slither Analysis + analyze: runs-on: ubuntu-latest - + permissions: + contents: read + security-events: write steps: - - uses: actions/checkout@v3 - - - name: Set up Python 3.10 - uses: actions/setup-python@v4 - with: - python-version: "3.10" - - - name: Install Foundry - uses: foundry-rs/foundry-toolchain@v1 - with: - version: nightly - - - name: Install Slither - run: pip3 install slither-analyzer + - name: Checkout repository + uses: actions/checkout@v4 - - name: Install dependencies - run: | - yarn install - yarn workspace @ubiquity/contracts forge:install + - name: Run Slither + uses: crytic/slither-action@v0.3.1 + id: slither + with: + node-version: 16 + sarif: results.sarif + fail-on: none - - name: Test with Slither - run: yarn workspace @ubiquity/contracts run test:slither \ No newline at end of file + - name: Upload SARIF file + uses: github/codeql-action/upload-sarif@v3 + with: + sarif_file: ${{ steps.slither.outputs.sarif }} \ No newline at end of file From 52e4ad15e83eaa959d14e67288dadc26ccc732d8 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Thu, 15 Feb 2024 15:01:47 -0300 Subject: [PATCH 04/22] test: remove requires for slither testing purposes --- src/NftReward.sol | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/NftReward.sol b/src/NftReward.sol index e762c6c..92abac6 100644 --- a/src/NftReward.sol +++ b/src/NftReward.sol @@ -74,8 +74,6 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus public initializer { - require(_minter != address(0), "Minter cannot be zero address"); - require(_initialOwner != address(0), "Initial owner cannot be zero address"); __ERC721_init(_tokenName, _tokenSymbol); __Ownable_init(_initialOwner); __EIP712_init("NftReward-Domain", "1"); From b4d43823e1a180f64e461c3617c71d0445ca2456 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Thu, 15 Feb 2024 15:06:25 -0300 Subject: [PATCH 05/22] fix: require address not be zero --- src/NftReward.sol | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/NftReward.sol b/src/NftReward.sol index 92abac6..e762c6c 100644 --- a/src/NftReward.sol +++ b/src/NftReward.sol @@ -74,6 +74,8 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus public initializer { + require(_minter != address(0), "Minter cannot be zero address"); + require(_initialOwner != address(0), "Initial owner cannot be zero address"); __ERC721_init(_tokenName, _tokenSymbol); __Ownable_init(_initialOwner); __EIP712_init("NftReward-Domain", "1"); From 5507e5d25b3b5b5d97447e5b1341b7aa80a5e91a Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Thu, 15 Feb 2024 16:02:38 -0300 Subject: [PATCH 06/22] feat: invalidate nonce --- src/NftReward.sol | 9 +++++++++ test/NftReward.t.sol | 44 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 53 insertions(+) diff --git a/src/NftReward.sol b/src/NftReward.sol index c4c54cf..f1e6017 100644 --- a/src/NftReward.sol +++ b/src/NftReward.sol @@ -214,6 +214,15 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} + /** + * @notice Invalidates nonce value to prevent mint request reusage + * @param _nonceValue Nonce value to invalidate + */ + + function invalidateNonce(uint256 _nonceValue) external onlyOwner { + nonceRedeemed[_nonceValue] = true; + } + //==================== // Internal methods //==================== diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index a3a798b..a345250 100644 --- a/test/NftReward.t.sol +++ b/test/NftReward.t.sol @@ -527,4 +527,48 @@ contract NftRewardTest is Test { // after assertFalse(nftReward.paused()); } + + function testInvalidateNonce_ShouldInvalidateNonce() public { + // prepare arbitrary data keys + bytes32[] memory keys = new bytes32[](1); + keys[0] = keccak256("GITHUB_ORGANIZATION_NAME"); + // prepare arbitrary data values + string[] memory values = new string[](1); + values[0] = "ubiquity"; + // prepare mint request + NftReward.MintRequest memory mintRequest = NftReward.MintRequest({ + beneficiary: user1, + deadline: block.timestamp + 1, + keys: keys, + nonce: 1, + values: values + }); + // get mint request digest which should be signed + bytes32 digest = nftReward.getMintRequestDigest(mintRequest); + // minter signs mint request digest + (uint8 v, bytes32 r, bytes32 s) = vm.sign(minterPrivateKey, digest); + // get minter's signature + bytes memory signature = abi.encodePacked(r, s, v); + + uint tokenId = 0; + + // before + vm.expectRevert(); + nftReward.ownerOf(tokenId); + assertEq(nftReward.nonceRedeemed(1), false); + assertEq(nftReward.tokenDataKeyExists(keccak256("GITHUB_ORGANIZATION_NAME")), false); + + // owner invalidates + vm.prank(owner); + nftReward.invalidateNonce(1); + + // user try to mint + vm.prank(user1); + vm.expectRevert("Already minted"); + nftReward.safeMint(mintRequest, signature); + + // after + assertEq(nftReward.nonceRedeemed(1), true); + assertEq(nftReward.tokenIdCounter(), 0); + } } From 75c78bad13390de2858e26b998a2f57d224d24ec Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Thu, 15 Feb 2024 16:03:17 -0300 Subject: [PATCH 07/22] fix: typo --- src/NftReward.sol | 2 -- test/NftReward.t.sol | 2 +- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/src/NftReward.sol b/src/NftReward.sol index f1e6017..8bb16ef 100644 --- a/src/NftReward.sol +++ b/src/NftReward.sol @@ -211,14 +211,12 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus * @notice Upgrades contract to new implementation * @param newImplementation New implementation address */ - function _authorizeUpgrade(address newImplementation) internal override onlyOwner {} /** * @notice Invalidates nonce value to prevent mint request reusage * @param _nonceValue Nonce value to invalidate */ - function invalidateNonce(uint256 _nonceValue) external onlyOwner { nonceRedeemed[_nonceValue] = true; } diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index a345250..79217ea 100644 --- a/test/NftReward.t.sol +++ b/test/NftReward.t.sol @@ -529,7 +529,7 @@ contract NftRewardTest is Test { } function testInvalidateNonce_ShouldInvalidateNonce() public { - // prepare arbitrary data keys + // prepare arbitrary data keys bytes32[] memory keys = new bytes32[](1); keys[0] = keccak256("GITHUB_ORGANIZATION_NAME"); // prepare arbitrary data values From 6ce539b027a8893b3691d57f6ea86985d740c1fc Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Fri, 16 Feb 2024 16:28:04 -0300 Subject: [PATCH 08/22] add require & test --- src/NftReward.sol | 1 + test/NftReward.t.sol | 48 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/NftReward.sol b/src/NftReward.sol index 8bb16ef..9176a92 100644 --- a/src/NftReward.sol +++ b/src/NftReward.sol @@ -218,6 +218,7 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus * @param _nonceValue Nonce value to invalidate */ function invalidateNonce(uint256 _nonceValue) external onlyOwner { + require(!nonceRedeemed[_nonceValue], "Already minted"); nonceRedeemed[_nonceValue] = true; } diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index 79217ea..82b9042 100644 --- a/test/NftReward.t.sol +++ b/test/NftReward.t.sol @@ -571,4 +571,52 @@ contract NftRewardTest is Test { assertEq(nftReward.nonceRedeemed(1), true); assertEq(nftReward.tokenIdCounter(), 0); } + + function testRequireInvalidateNonce_ShouldRevert_IfNonceIsRedeemed() public { + // prepare arbitrary data keys + bytes32[] memory keys = new bytes32[](1); + keys[0] = keccak256("GITHUB_ORGANIZATION_NAME"); + // prepare arbitrary data values + string[] memory values = new string[](1); + values[0] = "ubiquity"; + // prepare mint request + NftReward.MintRequest memory mintRequest = NftReward.MintRequest({ + beneficiary: user1, + deadline: block.timestamp + 1, + keys: keys, + nonce: 1, + values: values + }); + // get mint request digest which should be signed + bytes32 digest = nftReward.getMintRequestDigest(mintRequest); + // minter signs mint request digest + (uint8 v, bytes32 r, bytes32 s) = vm.sign(minterPrivateKey, digest); + // get minter's signature + bytes memory signature = abi.encodePacked(r, s, v); + + uint tokenId = 0; + + // before + vm.expectRevert(); + nftReward.ownerOf(tokenId); + assertEq(nftReward.nonceRedeemed(1), false); + assertEq(nftReward.tokenDataKeyExists(keccak256("GITHUB_ORGANIZATION_NAME")), false); + + // user1 mints + vm.prank(user1); + nftReward.safeMint(mintRequest, signature); + + // owner try to invalidate + vm.prank(owner); + vm.expectRevert("Already minted"); + nftReward.invalidateNonce(1); + + // after + assertEq(nftReward.nonceRedeemed(1), true); + assertEq(nftReward.tokenDataKeys(0), keccak256("GITHUB_ORGANIZATION_NAME")); + assertEq(nftReward.tokenDataKeyExists(keccak256("GITHUB_ORGANIZATION_NAME")), true); + assertEq(nftReward.ownerOf(tokenId), user1); + assertEq(nftReward.tokenIdCounter(), 1); + assertEq(nftReward.tokenData(0, keccak256("GITHUB_ORGANIZATION_NAME")), "ubiquity"); + } } From 042991431359951bd13c32e1299b5fc177d3c436 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Fri, 16 Feb 2024 16:40:07 -0300 Subject: [PATCH 09/22] add tests --- test/NftReward.t.sol | 38 +++++++++++++++++++++++++++++++++++++- 1 file changed, 37 insertions(+), 1 deletion(-) diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index ef67db4..4b056d8 100644 --- a/test/NftReward.t.sol +++ b/test/NftReward.t.sol @@ -16,7 +16,7 @@ contract NftRewardTest is Test { address minter = vm.addr(minterPrivateKey); bytes public data; - + function setUp() public { vm.prank(owner); @@ -34,6 +34,36 @@ contract NftRewardTest is Test { nftReward = NftReward(address(proxy)); } + function testSetup_ShouldRevert_IfMinterIsZeroAddress() public { + vm.prank(owner); + data = abi.encodeWithSignature( + "initialize(string,string,address,address)", + "NFT reward", + "RWD", + owner, + address(0) + ); + nftReward = new NftReward(); + vm.expectRevert("Minter cannot be zero address"); + ERC1967Proxy proxy = new ERC1967Proxy(address(nftReward), data); + nftReward = NftReward(address(proxy)); + } + + function testSetup_ShouldRevert_IfOwnerIsZeroAddress() public { + vm.prank(owner); + data = abi.encodeWithSignature( + "initialize(string,string,address,address)", + "NFT reward", + "RWD", + address(0), + minter + ); + nftReward = new NftReward(); + vm.expectRevert("Initial owner cannot be zero address"); + ERC1967Proxy proxy = new ERC1967Proxy(address(nftReward), data); + nftReward = NftReward(address(proxy)); + } + //================== // Public methods //================== @@ -527,4 +557,10 @@ contract NftRewardTest is Test { // after assertFalse(nftReward.paused()); } + + function testSetMinter_ShouldRevert_IfMinterIsZeroAddress() public { + vm.prank(owner); + vm.expectRevert("Minter cannot be zero address"); + nftReward.setMinter(address(0)); + } } From e67aad39f21e791dba8b499f8f3daa53fd01b08a Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Fri, 16 Feb 2024 22:05:58 -0300 Subject: [PATCH 10/22] fix duplicated functions --- test/NftReward.t.sol | 92 -------------------------------------------- 1 file changed, 92 deletions(-) diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index 3913a4a..262a3ae 100644 --- a/test/NftReward.t.sol +++ b/test/NftReward.t.sol @@ -655,96 +655,4 @@ contract NftRewardTest is Test { assertEq(nftReward.tokenIdCounter(), 1); assertEq(nftReward.tokenData(0, keccak256("GITHUB_ORGANIZATION_NAME")), "ubiquity"); } - - function testInvalidateNonce_ShouldInvalidateNonce() public { - // prepare arbitrary data keys - bytes32[] memory keys = new bytes32[](1); - keys[0] = keccak256("GITHUB_ORGANIZATION_NAME"); - // prepare arbitrary data values - string[] memory values = new string[](1); - values[0] = "ubiquity"; - // prepare mint request - NftReward.MintRequest memory mintRequest = NftReward.MintRequest({ - beneficiary: user1, - deadline: block.timestamp + 1, - keys: keys, - nonce: 1, - values: values - }); - // get mint request digest which should be signed - bytes32 digest = nftReward.getMintRequestDigest(mintRequest); - // minter signs mint request digest - (uint8 v, bytes32 r, bytes32 s) = vm.sign(minterPrivateKey, digest); - // get minter's signature - bytes memory signature = abi.encodePacked(r, s, v); - - uint tokenId = 0; - - // before - vm.expectRevert(); - nftReward.ownerOf(tokenId); - assertEq(nftReward.nonceRedeemed(1), false); - assertEq(nftReward.tokenDataKeyExists(keccak256("GITHUB_ORGANIZATION_NAME")), false); - - // owner invalidates - vm.prank(owner); - nftReward.invalidateNonce(1); - - // user try to mint - vm.prank(user1); - vm.expectRevert("Already minted"); - nftReward.safeMint(mintRequest, signature); - - // after - assertEq(nftReward.nonceRedeemed(1), true); - assertEq(nftReward.tokenIdCounter(), 0); - } - - function testRequireInvalidateNonce_ShouldRevert_IfNonceIsRedeemed() public { - // prepare arbitrary data keys - bytes32[] memory keys = new bytes32[](1); - keys[0] = keccak256("GITHUB_ORGANIZATION_NAME"); - // prepare arbitrary data values - string[] memory values = new string[](1); - values[0] = "ubiquity"; - // prepare mint request - NftReward.MintRequest memory mintRequest = NftReward.MintRequest({ - beneficiary: user1, - deadline: block.timestamp + 1, - keys: keys, - nonce: 1, - values: values - }); - // get mint request digest which should be signed - bytes32 digest = nftReward.getMintRequestDigest(mintRequest); - // minter signs mint request digest - (uint8 v, bytes32 r, bytes32 s) = vm.sign(minterPrivateKey, digest); - // get minter's signature - bytes memory signature = abi.encodePacked(r, s, v); - - uint tokenId = 0; - - // before - vm.expectRevert(); - nftReward.ownerOf(tokenId); - assertEq(nftReward.nonceRedeemed(1), false); - assertEq(nftReward.tokenDataKeyExists(keccak256("GITHUB_ORGANIZATION_NAME")), false); - - // user1 mints - vm.prank(user1); - nftReward.safeMint(mintRequest, signature); - - // owner try to invalidate - vm.prank(owner); - vm.expectRevert("Already minted"); - nftReward.invalidateNonce(1); - - // after - assertEq(nftReward.nonceRedeemed(1), true); - assertEq(nftReward.tokenDataKeys(0), keccak256("GITHUB_ORGANIZATION_NAME")); - assertEq(nftReward.tokenDataKeyExists(keccak256("GITHUB_ORGANIZATION_NAME")), true); - assertEq(nftReward.ownerOf(tokenId), user1); - assertEq(nftReward.tokenIdCounter(), 1); - assertEq(nftReward.tokenData(0, keccak256("GITHUB_ORGANIZATION_NAME")), "ubiquity"); - } } From 890d7cf52660e73e7c5d95e090b78478290a1b35 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Mon, 19 Feb 2024 16:28:03 -0300 Subject: [PATCH 11/22] refactor: mixedCase & solidity version --- .github/workflows/slither.yml | 5 +- src/NftReward.sol | 111 +++++++++++++++++----------------- 2 files changed, 59 insertions(+), 57 deletions(-) diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml index 7a0f733..82c8a59 100644 --- a/.github/workflows/slither.yml +++ b/.github/workflows/slither.yml @@ -3,8 +3,11 @@ name: Slither Analysis on: push: branches: [ development ] + paths: + - '**.sol' pull_request: - branches: [ development ] + paths: + - '**.sol' jobs: analyze: diff --git a/src/NftReward.sol b/src/NftReward.sol index d89bdfa..6a3a8ef 100644 --- a/src/NftReward.sol +++ b/src/NftReward.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.18; import "@openzeppelin/contracts-upgradeable/utils/cryptography/EIP712Upgradeable.sol"; import "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol"; @@ -60,28 +60,28 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus /** * @notice Contract initializer (replaces constructor) - * @param _tokenName NFT token name - * @param _tokenSymbol NFT token symbol - * @param _initialOwner Initial owner name - * @param _minter Minter address + * @param tokenName NFT token name + * @param tokenSymbol NFT token symbol + * @param initialOwner Initial owner name + * @param minterAddress Minter address */ function initialize( - string memory _tokenName, - string memory _tokenSymbol, - address _initialOwner, - address _minter + string memory tokenName, + string memory tokenSymbol, + address initialOwner, + address minterAddress ) public initializer { - require(_minter != address(0), "Minter cannot be zero address"); - require(_initialOwner != address(0), "Initial owner cannot be zero address"); - __ERC721_init(_tokenName, _tokenSymbol); - __Ownable_init(_initialOwner); + require(minterAddress != address(0), "Minter cannot be zero address"); + require(initialOwner != address(0), "Initial owner cannot be zero address"); + __ERC721_init(tokenName, tokenSymbol); + __Ownable_init(initialOwner); __EIP712_init("NftReward-Domain", "1"); __UUPSUpgradeable_init(); __Pausable_init(); - minter = _minter; + minter = minterAddress; } //================== @@ -90,22 +90,22 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus /** * @notice Returns mint request digest which should be signed by `minter` - * @param _mintRequest Mint request data + * @param mintRequest Mint request data * @return Mint request digest which should be signed by `minter` */ - function getMintRequestDigest(MintRequest calldata _mintRequest) public view returns (bytes32) { + function getMintRequestDigest(MintRequest calldata mintRequest) public view returns (bytes32) { // for `string[]` array type we need to hash all the array values first - bytes32[] memory valuesHashed = new bytes32[](_mintRequest.values.length); - for (uint256 i = 0; i < _mintRequest.values.length; i++) { - valuesHashed[i] = keccak256(bytes(_mintRequest.values[i])); + bytes32[] memory valuesHashed = new bytes32[](mintRequest.values.length); + for (uint256 i = 0; i < mintRequest.values.length; i++) { + valuesHashed[i] = keccak256(bytes(mintRequest.values[i])); } // return final hash return _hashTypedDataV4(keccak256(abi.encode( keccak256("MintRequest(address beneficiary,uint256 deadline,bytes32[] keys,uint256 nonce,string[] values)"), - _mintRequest.beneficiary, - _mintRequest.deadline, - keccak256(abi.encodePacked(_mintRequest.keys)), - _mintRequest.nonce, + mintRequest.beneficiary, + mintRequest.deadline, + keccak256(abi.encodePacked(mintRequest.keys)), + mintRequest.nonce, keccak256(abi.encodePacked(valuesHashed)) ))); } @@ -120,59 +120,58 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus /** * @notice Returns signer of the mint request - * @param _mintRequest Mint request data - * @param _signature Minter signature + * @param mintRequest Mint request data + * @param signature Minter signature * @return Signer of the mint request */ function recover( - MintRequest calldata _mintRequest, - bytes calldata _signature + MintRequest calldata mintRequest, + bytes calldata signature ) public view returns (address) { - bytes32 digest = getMintRequestDigest(_mintRequest); - address signer = ECDSA.recover(digest, _signature); + bytes32 digest = getMintRequestDigest(mintRequest); + address signer = ECDSA.recover(digest, signature); return signer; } /** * @notice Mints a reward NFT to beneficiary who provided a mint request with valid minter's signature - * @param _mintRequest Mint request data - * @param _signature Minter signature + * @param mintRequest Mint request data + * @param signature Minter signature */ function safeMint( - MintRequest calldata _mintRequest, - bytes calldata _signature + MintRequest calldata mintRequest, + bytes calldata signature ) public whenNotPaused { // validation - require(recover(_mintRequest, _signature) == minter, "Signed not by minter"); - require(msg.sender == _mintRequest.beneficiary, "Not eligible"); - require(block.timestamp < _mintRequest.deadline, "Signature expired"); - require(!nonceRedeemed[_mintRequest.nonce], "Already minted"); - require(_mintRequest.keys.length == _mintRequest.values.length, "Key/value length mismatch"); + require(recover(mintRequest, signature) == minter, "Signed not by minter"); + require(msg.sender == mintRequest.beneficiary, "Not eligible"); + require(!nonceRedeemed[mintRequest.nonce], "Already minted"); + require(mintRequest.keys.length == mintRequest.values.length, "Key/value length mismatch"); // mark nonce as used - nonceRedeemed[_mintRequest.nonce] = true; + nonceRedeemed[mintRequest.nonce] = true; // save arbitrary token data - uint256 keysCount = _mintRequest.keys.length; + uint256 keysCount = mintRequest.keys.length; for (uint256 i = 0; i < keysCount; i++) { // save data - tokenData[tokenIdCounter][_mintRequest.keys[i]] = _mintRequest.values[i]; + tokenData[tokenIdCounter][mintRequest.keys[i]] = mintRequest.values[i]; // save arbitrary token data key if not saved yet - if (!tokenDataKeyExists[_mintRequest.keys[i]]) { - tokenDataKeys.push(_mintRequest.keys[i]); - tokenDataKeyExists[_mintRequest.keys[i]] = true; + if (!tokenDataKeyExists[mintRequest.keys[i]]) { + tokenDataKeys.push(mintRequest.keys[i]); + tokenDataKeyExists[mintRequest.keys[i]] = true; } } // mint token to beneficiary - _safeMint(_mintRequest.beneficiary, tokenIdCounter++); + _safeMint(mintRequest.beneficiary, tokenIdCounter++); } //================= @@ -188,19 +187,19 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus /** * @notice Sets new base URI for all of the tokens - * @param _newBaseUri New base URI + * @param newBaseUri New base URI */ - function setBaseUri(string memory _newBaseUri) external onlyOwner { - baseUri = _newBaseUri; + function setBaseUri(string memory newBaseUri) external onlyOwner { + baseUri = newBaseUri; } /** * @notice Sets new minter address (who can sign off-chain mint requests) - * @param _newMinter New minter address + * @param newMinter New minter address */ - function setMinter(address _newMinter) external onlyOwner { - require(_newMinter != address(0), "Minter cannot be zero address"); - minter = _newMinter; + function setMinter(address newMinter) external onlyOwner { + require(newMinter != address(0), "Minter cannot be zero address"); + minter = newMinter; } /** @@ -218,11 +217,11 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus /** * @notice Invalidates nonce value to prevent mint request reusage - * @param _nonceValue Nonce value to invalidate + * @param nonceValue Nonce value to invalidate */ - function invalidateNonce(uint256 _nonceValue) external onlyOwner { - require(!nonceRedeemed[_nonceValue], "Already minted"); - nonceRedeemed[_nonceValue] = true; + function invalidateNonce(uint256 nonceValue) external onlyOwner { + require(!nonceRedeemed[nonceValue], "Already minted"); + nonceRedeemed[nonceValue] = true; } //==================== From 56e2100ad1bfb668de5f993d410edef1d95f3237 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Mon, 19 Feb 2024 16:28:59 -0300 Subject: [PATCH 12/22] remove timestamp test --- test/NftReward.t.sol | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index 262a3ae..d9c7a38 100644 --- a/test/NftReward.t.sol +++ b/test/NftReward.t.sol @@ -245,33 +245,6 @@ contract NftRewardTest is Test { nftReward.safeMint(mintRequest, signature); } - function testSafeMint_ShouldRevert_IfSignatureExpired() public { - // prepare arbitrary data keys - bytes32[] memory keys = new bytes32[](1); - keys[0] = keccak256("GITHUB_ORGANIZATION_NAME"); - // prepare arbitrary data values - string[] memory values = new string[](1); - values[0] = "ubiquity"; - // prepare mint request - NftReward.MintRequest memory mintRequest = NftReward.MintRequest({ - beneficiary: user1, - deadline: block.timestamp - 1, // set expired signature - keys: keys, - nonce: 1, - values: values - }); - // get mint request digest which should be signed - bytes32 digest = nftReward.getMintRequestDigest(mintRequest); - // minter signs mint request digest - (uint8 v, bytes32 r, bytes32 s) = vm.sign(minterPrivateKey, digest); - // get minter's signature - bytes memory signature = abi.encodePacked(r, s, v); - - vm.prank(user1); - vm.expectRevert('Signature expired'); - nftReward.safeMint(mintRequest, signature); - } - function testSafeMint_ShouldRevert_IfNonceAlreadyUsed() public { // prepare arbitrary data keys bytes32[] memory keys = new bytes32[](1); From 62b9b4bfa3498cdf21ea7bacb88efd53a69e37c8 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Mon, 19 Feb 2024 16:30:56 -0300 Subject: [PATCH 13/22] fix solidity version --- script/Deploy001_NftReward.s.sol | 2 +- test/NftReward.t.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/script/Deploy001_NftReward.s.sol b/script/Deploy001_NftReward.s.sol index a893a83..d9c16f2 100644 --- a/script/Deploy001_NftReward.s.sol +++ b/script/Deploy001_NftReward.s.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.18; import {Script} from "forge-std/Script.sol"; import {NftReward} from "../src/NftReward.sol"; diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index d9c7a38..a3666e8 100644 --- a/test/NftReward.t.sol +++ b/test/NftReward.t.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.18; import {Test, console} from "forge-std/Test.sol"; import {NftReward} from "../src/NftReward.sol"; From edffadbc7f2adee9ddc732ae808797769eb81e63 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Mon, 19 Feb 2024 16:35:09 -0300 Subject: [PATCH 14/22] add: slither fail-on all --- .github/workflows/slither.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml index 82c8a59..569a240 100644 --- a/.github/workflows/slither.yml +++ b/.github/workflows/slither.yml @@ -25,7 +25,7 @@ jobs: with: node-version: 16 sarif: results.sarif - fail-on: none + fail-on: all - name: Upload SARIF file uses: github/codeql-action/upload-sarif@v3 From c668e70b513865eb4b58061674c4ba3ab5f195d6 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Mon, 19 Feb 2024 16:50:23 -0300 Subject: [PATCH 15/22] fix target --- .github/workflows/slither.yml | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml index 569a240..8c48c0f 100644 --- a/.github/workflows/slither.yml +++ b/.github/workflows/slither.yml @@ -4,10 +4,10 @@ on: push: branches: [ development ] paths: - - '**.sol' + - 'src/**.sol' pull_request: paths: - - '**.sol' + - 'src/**.sol' jobs: analyze: @@ -26,6 +26,7 @@ jobs: node-version: 16 sarif: results.sarif fail-on: all + target: 'src/' - name: Upload SARIF file uses: github/codeql-action/upload-sarif@v3 From 39b4296b02b26dee54ff36db58a54ccdfaed7d1d Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Mon, 19 Feb 2024 16:57:41 -0300 Subject: [PATCH 16/22] fail on low --- .github/workflows/slither.yml | 2 +- slither.config.json | 4 ++++ src/NftReward.sol | 1 + test/NftReward.t.sol | 29 ++++++++++++++++++++++++++++- 4 files changed, 34 insertions(+), 2 deletions(-) create mode 100644 slither.config.json diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml index 8c48c0f..f6a7db2 100644 --- a/.github/workflows/slither.yml +++ b/.github/workflows/slither.yml @@ -25,7 +25,7 @@ jobs: with: node-version: 16 sarif: results.sarif - fail-on: all + fail-on: low target: 'src/' - name: Upload SARIF file diff --git a/slither.config.json b/slither.config.json new file mode 100644 index 0000000..13270fc --- /dev/null +++ b/slither.config.json @@ -0,0 +1,4 @@ +{ + "filter_paths": "openzeppelin", + "detectors_to_exclude": "timestamp" +} \ No newline at end of file diff --git a/src/NftReward.sol b/src/NftReward.sol index 6a3a8ef..3114aaf 100644 --- a/src/NftReward.sol +++ b/src/NftReward.sol @@ -152,6 +152,7 @@ contract NftReward is Initializable, ERC721Upgradeable, OwnableUpgradeable, Paus // validation require(recover(mintRequest, signature) == minter, "Signed not by minter"); require(msg.sender == mintRequest.beneficiary, "Not eligible"); + require(block.timestamp < mintRequest.deadline, "Signature expired"); require(!nonceRedeemed[mintRequest.nonce], "Already minted"); require(mintRequest.keys.length == mintRequest.values.length, "Key/value length mismatch"); diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index a3666e8..262a3ae 100644 --- a/test/NftReward.t.sol +++ b/test/NftReward.t.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.18; +pragma solidity ^0.8.20; import {Test, console} from "forge-std/Test.sol"; import {NftReward} from "../src/NftReward.sol"; @@ -245,6 +245,33 @@ contract NftRewardTest is Test { nftReward.safeMint(mintRequest, signature); } + function testSafeMint_ShouldRevert_IfSignatureExpired() public { + // prepare arbitrary data keys + bytes32[] memory keys = new bytes32[](1); + keys[0] = keccak256("GITHUB_ORGANIZATION_NAME"); + // prepare arbitrary data values + string[] memory values = new string[](1); + values[0] = "ubiquity"; + // prepare mint request + NftReward.MintRequest memory mintRequest = NftReward.MintRequest({ + beneficiary: user1, + deadline: block.timestamp - 1, // set expired signature + keys: keys, + nonce: 1, + values: values + }); + // get mint request digest which should be signed + bytes32 digest = nftReward.getMintRequestDigest(mintRequest); + // minter signs mint request digest + (uint8 v, bytes32 r, bytes32 s) = vm.sign(minterPrivateKey, digest); + // get minter's signature + bytes memory signature = abi.encodePacked(r, s, v); + + vm.prank(user1); + vm.expectRevert('Signature expired'); + nftReward.safeMint(mintRequest, signature); + } + function testSafeMint_ShouldRevert_IfNonceAlreadyUsed() public { // prepare arbitrary data keys bytes32[] memory keys = new bytes32[](1); From fe44b6543d9cc361a2412765c18cf1532c03e8b9 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Mon, 19 Feb 2024 17:01:14 -0300 Subject: [PATCH 17/22] add slither config --- .github/workflows/slither.yml | 2 -- slither.config.json | 3 ++- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml index f6a7db2..9ee0966 100644 --- a/.github/workflows/slither.yml +++ b/.github/workflows/slither.yml @@ -25,8 +25,6 @@ jobs: with: node-version: 16 sarif: results.sarif - fail-on: low - target: 'src/' - name: Upload SARIF file uses: github/codeql-action/upload-sarif@v3 diff --git a/slither.config.json b/slither.config.json index 13270fc..8432fc2 100644 --- a/slither.config.json +++ b/slither.config.json @@ -1,4 +1,5 @@ { "filter_paths": "openzeppelin", - "detectors_to_exclude": "timestamp" + "detectors_to_exclude": "timestamp", + "exclude_informational": true } \ No newline at end of file From 7c79bf286847241a2f3ab9477c5bc0a76c401400 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Mon, 19 Feb 2024 17:01:49 -0300 Subject: [PATCH 18/22] fix route --- .github/workflows/slither.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml index 9ee0966..ca45c69 100644 --- a/.github/workflows/slither.yml +++ b/.github/workflows/slither.yml @@ -4,10 +4,10 @@ on: push: branches: [ development ] paths: - - 'src/**.sol' + - '**.sol' pull_request: paths: - - 'src/**.sol' + - '**.sol' jobs: analyze: From 0db9bf72729b4ff84b1813f7cbd0c0ed338dd11b Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Mon, 19 Feb 2024 17:05:42 -0300 Subject: [PATCH 19/22] check infomational --- slither.config.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/slither.config.json b/slither.config.json index 8432fc2..13270fc 100644 --- a/slither.config.json +++ b/slither.config.json @@ -1,5 +1,4 @@ { "filter_paths": "openzeppelin", - "detectors_to_exclude": "timestamp", - "exclude_informational": true + "detectors_to_exclude": "timestamp" } \ No newline at end of file From bb0fab9373149b4f4a6c191049ef973e20447430 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Mon, 19 Feb 2024 17:07:41 -0300 Subject: [PATCH 20/22] fix exclude informational to pass slither test --- slither.config.json | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/slither.config.json b/slither.config.json index 13270fc..8432fc2 100644 --- a/slither.config.json +++ b/slither.config.json @@ -1,4 +1,5 @@ { "filter_paths": "openzeppelin", - "detectors_to_exclude": "timestamp" + "detectors_to_exclude": "timestamp", + "exclude_informational": true } \ No newline at end of file From cd9a90caa3ed266d6b3f1cf2b65b467ee1d2846a Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Tue, 20 Feb 2024 13:45:34 -0300 Subject: [PATCH 21/22] add: foundry specific solidity verison --- foundry.toml | 1 + test/NftReward.t.sol | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/foundry.toml b/foundry.toml index 84df88f..ae31441 100644 --- a/foundry.toml +++ b/foundry.toml @@ -2,6 +2,7 @@ src = "src" out = "out" libs = ["lib"] +solc_version = "0.8.20" # See more config options https://github.com/foundry-rs/foundry/blob/master/crates/config/README.md#all-options diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index 262a3ae..7df1b03 100644 --- a/test/NftReward.t.sol +++ b/test/NftReward.t.sol @@ -1,5 +1,5 @@ // SPDX-License-Identifier: MIT -pragma solidity ^0.8.20; +pragma solidity ^0.8.18; import {Test, console} from "forge-std/Test.sol"; import {NftReward} from "../src/NftReward.sol"; From b100ff8ab92c81a9f5b7bfff549426c7d19e93c3 Mon Sep 17 00:00:00 2001 From: gpylypchuk Date: Tue, 20 Feb 2024 14:05:08 -0300 Subject: [PATCH 22/22] fix: exclude detector solc-version --- slither.config.json | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/slither.config.json b/slither.config.json index 8432fc2..eff61f6 100644 --- a/slither.config.json +++ b/slither.config.json @@ -1,5 +1,4 @@ { "filter_paths": "openzeppelin", - "detectors_to_exclude": "timestamp", - "exclude_informational": true + "detectors_to_exclude": "timestamp,solc-version" } \ No newline at end of file