From 7bd8488534e1ab1162137aa374a75d8d407b31d2 Mon Sep 17 00:00:00 2001 From: steven2308 Date: Tue, 9 Jan 2024 10:56:17 -0500 Subject: [PATCH 1/5] Adds reference to test cases and implementation. --- ERCS/erc-7590.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/ERCS/erc-7590.md b/ERCS/erc-7590.md index dcd8f74f22..7f62167a0e 100644 --- a/ERCS/erc-7590.md +++ b/ERCS/erc-7590.md @@ -157,11 +157,19 @@ No backward compatibility issues found. ## Test Cases -Will be added. +Tests are included in [`erc7590.ts`](../assets/eip-7590/test/erc7590.ts). + +To run them in terminal, you can use the following commands: + +``` +cd ../assets/eip-erc7590 +npm install +npx hardhat test +``` ## Reference Implementation -Will be added. +See [`ERC7590Mock.sol`](../assets/erc-7590/contracts/ERC7590Mock.sol). ## Security Considerations From 25c7eb9eb4b7f3bb1c04f89332c4f3f6613bd4fb Mon Sep 17 00:00:00 2001 From: steven2308 Date: Tue, 9 Jan 2024 15:02:11 -0500 Subject: [PATCH 2/5] Updates internal reference. --- ERCS/erc-7590.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ERCS/erc-7590.md b/ERCS/erc-7590.md index 7f62167a0e..cddadcb57f 100644 --- a/ERCS/erc-7590.md +++ b/ERCS/erc-7590.md @@ -169,7 +169,7 @@ npx hardhat test ## Reference Implementation -See [`ERC7590Mock.sol`](../assets/erc-7590/contracts/ERC7590Mock.sol). +See [`ERC7590Mock.sol`](../assets/eip-7590/contracts/ERC7590Mock.sol). ## Security Considerations From 537e76fce5eed871f225b03c1f0f2476ba590de6 Mon Sep 17 00:00:00 2001 From: steven2308 Date: Thu, 29 Feb 2024 12:10:10 -0500 Subject: [PATCH 3/5] Handles erc20s with transfer fees. --- ERCS/erc-7590.md | 4 ++++ assets/erc-7590/contracts/AbstractERC7590.sol | 19 +++++++++++++++++-- 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/ERCS/erc-7590.md b/ERCS/erc-7590.md index cddadcb57f..249c143265 100644 --- a/ERCS/erc-7590.md +++ b/ERCS/erc-7590.md @@ -179,6 +179,10 @@ Caution is advised when dealing with non-audited contracts. Implementations MUST use the message sender as from parameter when they are transferring tokens into an NFT. Otherwise, since the current contract needs approval, it could potentially pull the external tokens into a different NFT. +When transferring ERC20 tokens in or out of an NFT, it could be the case that the amount transferred is not the same as the amount requested. This could happen if the ERC20 contract has a fee on transfer. This could cause a bug on your Token Holder contract if you do not manage it properly. There are 2 ways to do it, both of which are valid: +1. Use the `IERC20` interface to check the balance of the contract before and after the transfer, and revert if the balance is not the expected one, hence not supporting tokens with fees on transfer. +2. Use the `IERC20` interface to check the balance of the contract before and after the transfer, and use the difference to calculate the amount of tokens that were actually transferred. + To prevent a seller from front running the sale of an NFT holding [ERC-20](./eip-20.md) tokens to transfer out such tokens before a sale is executed, marketplaces MUST beware of the `erc20TransferOutNonce` and revert if it has changed since listed. [ERC-20](./eip-20.md) tokens that are transferred directly to the NFT contract will be lost. diff --git a/assets/erc-7590/contracts/AbstractERC7590.sol b/assets/erc-7590/contracts/AbstractERC7590.sol index 6efad08d48..1c9ae22903 100644 --- a/assets/erc-7590/contracts/AbstractERC7590.sol +++ b/assets/erc-7590/contracts/AbstractERC7590.sol @@ -8,6 +8,7 @@ import "@openzeppelin/contracts/token/ERC20/IERC20.sol"; error InvalidValue(); error InvalidAddress(); error InsufficientBalance(); +error InvalidAmountTransferred(); abstract contract AbstractERC7590 is IERC7590 { mapping(uint256 tokenId => mapping(address erc20Address => uint256 balance)) @@ -57,10 +58,17 @@ abstract contract AbstractERC7590 is IERC7590 { amount, data ); + IERC20 erc20 = IERC20(erc20Contract); + uint256 initBalance = erc20.balanceOf(address(this)); _balances[tokenId][erc20Contract] -= amount; _erc20TransferOutNonce[tokenId]++; - IERC20(erc20Contract).transfer(to, amount); + erc20.transfer(to, amount); + uint256 newBalance = erc20.balanceOf(address(this)); + // Here you can either use the difference as the amount, or revert if the difference is not equal to the amount and you don't want to support transfer fees + if (newBalance + amount != initBalance) { + revert InvalidAmountTransferred(); + } emit TransferredERC20(erc20Contract, tokenId, to, amount); _afterTransferHeldERC20FromToken( @@ -94,7 +102,14 @@ abstract contract AbstractERC7590 is IERC7590 { amount, data ); - IERC20(erc20Contract).transferFrom(msg.sender, address(this), amount); + IERC20 erc20 = IERC20(erc20Contract); + uint256 initBalance = erc20.balanceOf(address(this)); + erc20.transferFrom(msg.sender, address(this), amount); + uint256 newBalance = erc20.balanceOf(address(this)); + // Here you can either use the difference as the amount, or revert if the difference is not equal to the amount and you don't want to support transfer fees + if (initBalance + amount != newBalance) { + revert InvalidAmountTransferred(); + } _balances[tokenId][erc20Contract] += amount; emit ReceivedERC20(erc20Contract, tokenId, msg.sender, amount); From 44a9717bb89d576e85401c36f3a1cd30c20f9f73 Mon Sep 17 00:00:00 2001 From: steven2308 Date: Thu, 29 Feb 2024 12:13:14 -0500 Subject: [PATCH 4/5] Adds proper links for ERC20 mentions. --- ERCS/erc-7590.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ERCS/erc-7590.md b/ERCS/erc-7590.md index 249c143265..0d517e75d2 100644 --- a/ERCS/erc-7590.md +++ b/ERCS/erc-7590.md @@ -179,7 +179,7 @@ Caution is advised when dealing with non-audited contracts. Implementations MUST use the message sender as from parameter when they are transferring tokens into an NFT. Otherwise, since the current contract needs approval, it could potentially pull the external tokens into a different NFT. -When transferring ERC20 tokens in or out of an NFT, it could be the case that the amount transferred is not the same as the amount requested. This could happen if the ERC20 contract has a fee on transfer. This could cause a bug on your Token Holder contract if you do not manage it properly. There are 2 ways to do it, both of which are valid: +When transferring [ERC-20](./eip-20.md) tokens in or out of an NFT, it could be the case that the amount transferred is not the same as the amount requested. This could happen if the [ERC-20](./eip-20.md) contract has a fee on transfer. This could cause a bug on your Token Holder contract if you do not manage it properly. There are 2 ways to do it, both of which are valid: 1. Use the `IERC20` interface to check the balance of the contract before and after the transfer, and revert if the balance is not the expected one, hence not supporting tokens with fees on transfer. 2. Use the `IERC20` interface to check the balance of the contract before and after the transfer, and use the difference to calculate the amount of tokens that were actually transferred. From 72589a81d660a4a92e1a9f7bf7e540074a892c25 Mon Sep 17 00:00:00 2001 From: steven2308 Date: Thu, 29 Feb 2024 12:43:30 -0500 Subject: [PATCH 5/5] Moves 7590 to review. --- ERCS/erc-7590.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ERCS/erc-7590.md b/ERCS/erc-7590.md index 0d517e75d2..50023a449f 100644 --- a/ERCS/erc-7590.md +++ b/ERCS/erc-7590.md @@ -4,7 +4,7 @@ title: ERC-20 Holder Extension for NFTs description: Extension to allow NFTs to receive and transfer ERC-20 tokens. author: Steven Pineda (@steven2308), Jan Turk (@ThunderDeliverer) discussions-to: https://ethereum-magicians.org/t/token-holder-extension-for-nfts/16260 -status: Draft +status: Review type: Standards Track category: ERC created: 2024-01-05