diff --git a/.github/workflows/slither.yml b/.github/workflows/slither.yml new file mode 100644 index 0000000..ca45c69 --- /dev/null +++ b/.github/workflows/slither.yml @@ -0,0 +1,32 @@ +name: Slither Analysis + +on: + push: + branches: [ development ] + paths: + - '**.sol' + pull_request: + paths: + - '**.sol' + +jobs: + analyze: + runs-on: ubuntu-latest + permissions: + contents: read + security-events: write + steps: + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Run Slither + uses: crytic/slither-action@v0.3.1 + id: slither + with: + node-version: 16 + sarif: results.sarif + + - name: Upload SARIF file + uses: github/codeql-action/upload-sarif@v3 + with: + sarif_file: ${{ steps.slither.outputs.sarif }} \ No newline at end of file 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/script/Deploy001_NftReward.s.sol b/script/Deploy001_NftReward.s.sol index b35906b..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.13; +pragma solidity ^0.8.18; import {Script} from "forge-std/Script.sol"; import {NftReward} from "../src/NftReward.sol"; diff --git a/slither.config.json b/slither.config.json new file mode 100644 index 0000000..eff61f6 --- /dev/null +++ b/slither.config.json @@ -0,0 +1,4 @@ +{ + "filter_paths": "openzeppelin", + "detectors_to_exclude": "timestamp,solc-version" +} \ No newline at end of file diff --git a/src/NftReward.sol b/src/NftReward.sol index 9176a92..3114aaf 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.18; import "@openzeppelin/contracts-upgradeable/utils/cryptography/EIP712Upgradeable.sol"; import "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol"; @@ -60,26 +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 { - __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; } //================== @@ -88,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)) ))); } @@ -118,59 +120,59 @@ 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(block.timestamp < mintRequest.deadline, "Signature expired"); + 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++); } //================= @@ -186,18 +188,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 { - minter = _newMinter; + function setMinter(address newMinter) external onlyOwner { + require(newMinter != address(0), "Minter cannot be zero address"); + minter = newMinter; } /** @@ -215,11 +218,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; } //==================== diff --git a/test/NftReward.t.sol b/test/NftReward.t.sol index 82b9042..7df1b03 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.18; import {Test, console} from "forge-std/Test.sol"; import {NftReward} from "../src/NftReward.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 //================== @@ -528,6 +558,12 @@ contract NftRewardTest is Test { assertFalse(nftReward.paused()); } + function testSetMinter_ShouldRevert_IfMinterIsZeroAddress() public { + vm.prank(owner); + vm.expectRevert("Minter cannot be zero address"); + nftReward.setMinter(address(0)); + } + function testInvalidateNonce_ShouldInvalidateNonce() public { // prepare arbitrary data keys bytes32[] memory keys = new bytes32[](1);