From ae8163f2953535c709a8b121e36b98fe7deadc9a Mon Sep 17 00:00:00 2001 From: Denis Date: Thu, 30 Jan 2025 19:57:23 +0000 Subject: [PATCH 1/2] Add EIP712 to KycNFT --- contracts/KycNFT.sol | 19 ++++++---- test/KycNFT.js | 86 +++++++++++++++++++++++++++----------------- 2 files changed, 66 insertions(+), 39 deletions(-) diff --git a/contracts/KycNFT.sol b/contracts/KycNFT.sol index 5bef21a..bdbbdb8 100644 --- a/contracts/KycNFT.sol +++ b/contracts/KycNFT.sol @@ -5,18 +5,22 @@ pragma solidity 0.8.23; import { Ownable } from "@openzeppelin/contracts/access/Ownable.sol"; import { ERC721 } from "@openzeppelin/contracts/token/ERC721/ERC721.sol"; import { ERC721Burnable } from "@openzeppelin/contracts/token/ERC721/extensions/ERC721Burnable.sol"; +import { EIP712 } from "@openzeppelin/contracts/utils/cryptography/EIP712.sol"; import { ECDSA } from "@1inch/solidity-utils/contracts/libraries/ECDSA.sol"; /** * @title KycNFT * @notice ERC721 token that allows only one NFT per address and includes transfer, mint and burn logic restricted to the contract owner. */ -contract KycNFT is Ownable, ERC721Burnable { +contract KycNFT is Ownable, ERC721Burnable, EIP712 { /// @notice Thrown when an address attempts to own more than one NFT. error OnlyOneNFTPerAddress(); /// @notice Thrown when signature is incorrect. error BadSignature(); + bytes32 public constant MINT_TYPEHASH = keccak256("Mint(address nft,uint256 chainId,address to,uint256 nonce,uint256 tokenId)"); + bytes32 public constant TRANSFER_FROM_TYPEHASH = keccak256("TransferFrom(address nft,uint256 chainId,address to,uint256 nonce,uint256 tokenId)"); + /// @notice Nonce for each token ID. mapping(uint256 => uint256) public nonces; @@ -25,9 +29,9 @@ contract KycNFT is Ownable, ERC721Burnable { * @param tokenId The ID of the token. * @param signature The signature to be verified. */ - modifier onlyOwnerSignature(address to, uint256 tokenId, bytes calldata signature) { - bytes memory message = abi.encodePacked(address(this), block.chainid, to, nonces[tokenId]++, tokenId); - bytes32 hash = keccak256(abi.encodePacked("\x19Ethereum Signed Message:\n136", message)); + modifier onlyOwnerSignature(address to, uint256 tokenId, bytes calldata signature, bytes32 typeHash) { + bytes32 structHash = keccak256(abi.encode(typeHash, address(this), block.chainid, to, nonces[tokenId]++, tokenId)); + bytes32 hash = _hashTypedDataV4(structHash); if (owner() != ECDSA.recover(hash, signature)) revert BadSignature(); _; } @@ -38,7 +42,8 @@ contract KycNFT is Ownable, ERC721Burnable { * @param symbol The symbol of the token. * @param owner The address of the owner of the contract. */ - constructor(string memory name, string memory symbol, address owner) ERC721(name, symbol) Ownable(owner) {} + constructor(string memory name, string memory symbol, string memory version, address owner) + ERC721(name, symbol) Ownable(owner) EIP712(name, version) {} /** * @notice Transfers a token to a specified address. Only the owner can call this function. @@ -57,7 +62,7 @@ contract KycNFT is Ownable, ERC721Burnable { * @param tokenId The ID of the token to be transferred. * @param signature The signature of the owner permitting the transfer. */ - function transferFrom(address from, address to, uint256 tokenId, bytes calldata signature) public onlyOwnerSignature(to, tokenId, signature) { + function transferFrom(address from, address to, uint256 tokenId, bytes calldata signature) public onlyOwnerSignature(to, tokenId, signature, TRANSFER_FROM_TYPEHASH) { _transfer(from, to, tokenId); } @@ -74,7 +79,7 @@ contract KycNFT is Ownable, ERC721Burnable { * @notice See {mint} method. This function using a valid owner's signature instead of only owner permission. * @param signature The signature of the owner permitting the mint. */ - function mint(address to, uint256 tokenId, bytes calldata signature) external onlyOwnerSignature(to, tokenId, signature) { + function mint(address to, uint256 tokenId, bytes calldata signature) external onlyOwnerSignature(to, tokenId, signature, MINT_TYPEHASH) { _mint(to, tokenId); } diff --git a/test/KycNFT.js b/test/KycNFT.js index 8245a6e..2c5c98a 100644 --- a/test/KycNFT.js +++ b/test/KycNFT.js @@ -1,15 +1,36 @@ -const { expect, deployContract, constants, trim0x } = require('@1inch/solidity-utils'); +const { expect, deployContract, constants } = require('@1inch/solidity-utils'); const { ethers } = require('hardhat'); const { loadFixture } = require('@nomicfoundation/hardhat-network-helpers'); const { getChainId } = require('./helpers/fixtures'); -async function signTokenId(nft, to, tokenId, signer, chainId) { - const packedData = ethers.solidityPacked( - ['address', 'uint256', 'address', 'uint256', 'uint256'], - [await nft.getAddress(), chainId, to, await nft.nonces(tokenId), tokenId], - ); - const message = Buffer.from(trim0x(packedData), 'hex'); - return await signer.signMessage(message); +const MINT = { + Mint: [ + { name: 'nft', type: 'address' }, + { name: 'chainId', type: 'uint256' }, + { name: 'to', type: 'address' }, + { name: 'nonce', type: 'uint256' }, + { name: 'tokenId', type: 'uint256' }, + ], +}; +const TRANSFER_FROM = { + TransferFrom: MINT.Mint, +}; + +async function signTokenId(types, eip712, nft, to, tokenId, signer, chainId) { + const domain = { + name: eip712.name, + version: eip712.version, + chainId, + verifyingContract: await nft.getAddress(), + }; + const values = { + nft: await nft.getAddress(), + chainId, + to, + nonce: await nft.nonces(tokenId), + tokenId, + }; + return signer.signTypedData(domain, types, values); } describe('KycNFT', function () { @@ -23,10 +44,11 @@ describe('KycNFT', function () { another: '0x03', nonexist: '0xabcdef', }; - const nft = await deployContract('KycNFT', ['KycNFT', 'KYC', owner]); + const eip712 = { name: 'KycNFT', version: '1' }; + const nft = await deployContract('KycNFT', [eip712.name, 'KYC', eip712.version, owner]); await nft.mint(owner, tokenIds.owner); await nft.mint(bob, tokenIds.bob); - return { owner, alice, bob, nft, tokenIds, chainId }; + return { owner, alice, bob, nft, tokenIds, chainId, eip712 }; } describe('transferFrom', function () { @@ -61,38 +83,38 @@ describe('KycNFT', function () { describe('transferFrom with signature', function () { it('should revert with signature by non-owner', async function () { - const { alice, bob, nft, tokenIds, chainId } = await loadFixture(initContracts); - const signature = await signTokenId(nft, alice.address, tokenIds.bob, bob, chainId); + const { alice, bob, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); + const signature = await signTokenId(TRANSFER_FROM, eip712, nft, alice.address, tokenIds.bob, bob, chainId); await expect(nft.connect(bob)['transferFrom(address,address,uint256,bytes)'](bob, alice, tokenIds.bob, signature)) .to.be.revertedWithCustomError(nft, 'BadSignature'); }); it('should work with signature by owner', async function () { - const { owner, bob, alice, nft, tokenIds, chainId } = await loadFixture(initContracts); + const { owner, bob, alice, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); const transferToken = tokenIds.bob; - const signature = await signTokenId(nft, alice.address, transferToken, owner, chainId); + const signature = await signTokenId(TRANSFER_FROM, eip712, nft, alice.address, transferToken, owner, chainId); await nft.connect(bob)['transferFrom(address,address,uint256,bytes)'](bob, alice, transferToken, signature); expect(await nft.ownerOf(tokenIds.bob)).to.equal(alice.address); }); it('should revert with signature by owner and transfer someone else\'s token', async function () { - const { owner, bob, alice, nft, tokenIds, chainId } = await loadFixture(initContracts); + const { owner, bob, alice, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); const transferToken = tokenIds.owner; - const signature = await signTokenId(nft, alice.address, transferToken, owner, chainId); + const signature = await signTokenId(TRANSFER_FROM, eip712, nft, alice.address, transferToken, owner, chainId); await expect(nft.connect(bob)['transferFrom(address,address,uint256,bytes)'](bob, alice, transferToken, signature)) .to.be.revertedWithCustomError(nft, 'ERC721IncorrectOwner'); }); it('should revert when recipient account already has 1 nft', async function () { - const { owner, bob, nft, tokenIds, chainId } = await loadFixture(initContracts); - const signature = await signTokenId(nft, owner.address, tokenIds.bob, owner, chainId); + const { owner, bob, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); + const signature = await signTokenId(TRANSFER_FROM, eip712, nft, owner.address, tokenIds.bob, owner, chainId); await expect(nft.connect(owner)['transferFrom(address,address,uint256,bytes)'](bob, owner, tokenIds.bob, signature)) .to.be.revertedWithCustomError(nft, 'OnlyOneNFTPerAddress'); }); it('should revert when send non-existen token', async function () { - const { owner, alice, nft, tokenIds, chainId } = await loadFixture(initContracts); - const signature = await signTokenId(nft, alice.address, tokenIds.nonexist, owner, chainId); + const { owner, alice, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); + const signature = await signTokenId(TRANSFER_FROM, eip712, nft, alice.address, tokenIds.nonexist, owner, chainId); await expect(nft.connect(owner)['transferFrom(address,address,uint256,bytes)'](owner, alice, tokenIds.nonexist, signature)) .to.be.revertedWithCustomError(nft, 'ERC721NonexistentToken'); }); @@ -123,45 +145,45 @@ describe('KycNFT', function () { describe('mint with signature', function () { it('should revert with invalid signature', async function () { - const { owner, alice, nft, tokenIds, chainId } = await loadFixture(initContracts); - const signature = await signTokenId(nft, alice.address, tokenIds.alice, owner, chainId); + const { owner, alice, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); + const signature = await signTokenId(MINT, eip712, nft, alice.address, tokenIds.alice, owner, chainId); const invalidSignature = signature.substring(-2) + '00'; await expect(nft.connect(owner)['mint(address,uint256,bytes)'](alice, tokenIds.alice, invalidSignature)) .to.be.revertedWithCustomError(nft, 'BadSignature'); }); it('should revert with invalid signature when frontrun and change to-address', async function () { - const { owner, bob, alice, nft, tokenIds, chainId } = await loadFixture(initContracts); - const signature = await signTokenId(nft, alice.address, tokenIds.alice, owner, chainId); + const { owner, bob, alice, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); + const signature = await signTokenId(MINT, eip712, nft, alice.address, tokenIds.alice, owner, chainId); const invalidSignature = signature.substring(-2) + '00'; await expect(nft.connect(owner)['mint(address,uint256,bytes)'](bob, tokenIds.alice, invalidSignature)) .to.be.revertedWithCustomError(nft, 'BadSignature'); }); it('should revert with non-owner signature', async function () { - const { alice, bob, nft, tokenIds, chainId } = await loadFixture(initContracts); - const signature = await signTokenId(nft, alice.address, tokenIds.alice, bob, chainId); + const { alice, bob, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); + const signature = await signTokenId(MINT, eip712, nft, alice.address, tokenIds.alice, bob, chainId); await expect(nft.connect(bob)['mint(address,uint256,bytes)'](alice, tokenIds.alice, signature)) .to.be.revertedWithCustomError(nft, 'BadSignature'); }); it('should work with owner signature', async function () { - const { owner, alice, bob, nft, tokenIds, chainId } = await loadFixture(initContracts); - const signature = await signTokenId(nft, alice.address, tokenIds.alice, owner, chainId); + const { owner, alice, bob, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); + const signature = await signTokenId(MINT, eip712, nft, alice.address, tokenIds.alice, owner, chainId); await nft.connect(bob)['mint(address,uint256,bytes)'](alice, tokenIds.alice, signature); expect(await nft.ownerOf(tokenIds.alice)).to.equal(alice.address); }); it('should revert when account already has 1 nft', async function () { - const { owner, bob, nft, tokenIds, chainId } = await loadFixture(initContracts); - const signature = await signTokenId(nft, bob.address, tokenIds.another, owner, chainId); + const { owner, bob, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); + const signature = await signTokenId(MINT, eip712, nft, bob.address, tokenIds.another, owner, chainId); await expect(nft.connect(owner)['mint(address,uint256,bytes)'](bob, tokenIds.another, signature)) .to.be.revertedWithCustomError(nft, 'OnlyOneNFTPerAddress'); }); it('should revert when tokenId already exist', async function () { - const { owner, alice, nft, tokenIds, chainId } = await loadFixture(initContracts); - const signature = await signTokenId(nft, alice.address, tokenIds.bob, owner, chainId); + const { owner, alice, nft, tokenIds, chainId, eip712 } = await loadFixture(initContracts); + const signature = await signTokenId(MINT, eip712, nft, alice.address, tokenIds.bob, owner, chainId); await expect(nft.connect(owner)['mint(address,uint256,bytes)'](alice, tokenIds.bob, signature)) .to.be.revertedWithCustomError(nft, 'ERC721InvalidSender'); }); From 73ff87a6d149d0c5bc296e0107e3aab73ed40cf4 Mon Sep 17 00:00:00 2001 From: Denis Date: Fri, 31 Jan 2025 00:25:52 +0000 Subject: [PATCH 2/2] Remove redundant parameters from structHash --- contracts/KycNFT.sol | 6 +++--- test/KycNFT.js | 4 ---- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/contracts/KycNFT.sol b/contracts/KycNFT.sol index bdbbdb8..7590f05 100644 --- a/contracts/KycNFT.sol +++ b/contracts/KycNFT.sol @@ -18,8 +18,8 @@ contract KycNFT is Ownable, ERC721Burnable, EIP712 { /// @notice Thrown when signature is incorrect. error BadSignature(); - bytes32 public constant MINT_TYPEHASH = keccak256("Mint(address nft,uint256 chainId,address to,uint256 nonce,uint256 tokenId)"); - bytes32 public constant TRANSFER_FROM_TYPEHASH = keccak256("TransferFrom(address nft,uint256 chainId,address to,uint256 nonce,uint256 tokenId)"); + bytes32 public constant MINT_TYPEHASH = keccak256("Mint(address to,uint256 nonce,uint256 tokenId)"); + bytes32 public constant TRANSFER_FROM_TYPEHASH = keccak256("TransferFrom(address to,uint256 nonce,uint256 tokenId)"); /// @notice Nonce for each token ID. mapping(uint256 => uint256) public nonces; @@ -30,7 +30,7 @@ contract KycNFT is Ownable, ERC721Burnable, EIP712 { * @param signature The signature to be verified. */ modifier onlyOwnerSignature(address to, uint256 tokenId, bytes calldata signature, bytes32 typeHash) { - bytes32 structHash = keccak256(abi.encode(typeHash, address(this), block.chainid, to, nonces[tokenId]++, tokenId)); + bytes32 structHash = keccak256(abi.encode(typeHash, to, nonces[tokenId]++, tokenId)); bytes32 hash = _hashTypedDataV4(structHash); if (owner() != ECDSA.recover(hash, signature)) revert BadSignature(); _; diff --git a/test/KycNFT.js b/test/KycNFT.js index 2c5c98a..8ab6e72 100644 --- a/test/KycNFT.js +++ b/test/KycNFT.js @@ -5,8 +5,6 @@ const { getChainId } = require('./helpers/fixtures'); const MINT = { Mint: [ - { name: 'nft', type: 'address' }, - { name: 'chainId', type: 'uint256' }, { name: 'to', type: 'address' }, { name: 'nonce', type: 'uint256' }, { name: 'tokenId', type: 'uint256' }, @@ -24,8 +22,6 @@ async function signTokenId(types, eip712, nft, to, tokenId, signer, chainId) { verifyingContract: await nft.getAddress(), }; const values = { - nft: await nft.getAddress(), - chainId, to, nonce: await nft.nonces(tokenId), tokenId,