Pieces Protocol
First Flight #32
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

`nftToErc20Info` is override when different NFTs from the same ERC721 contract are divided, will break `claimNft` function

vincent71399

Description:

The tokenDivider contract uses a mapping to store the ERC20 contract address for each
NFT contract. but with only the ERC721 contract address as key
If multiple NFTs from the same ERC721 contract are divided, the last NFT divided will override the previous ones,
Unfortunately, the claimNft function uses the nftToErc20Info[nftAddress] to retrieve the ERC20 contract address for the NFT
if a buyer bought all the fraction tokens of the first NFT, then nftToErc20Info[nftAddress] got override by the second NFT,
the buyer will not be able to claim the NFT

Impact:

If override happens it can break claimNft logic, also it is impossible to track the ERC20 fraction contract address for the previous NFTs.

Proof of Concept:

add the following in test/unit/TokenDividerTest.t.sol

function testBreakClaimNft() public nftDivided nftSell(1e18, AMOUNT) {
uint256 price = 1e18;
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
// 1. mint another NFT from the same ERC721 contract
erc721Mock.mint(USER);
assertEq(erc721Mock.ownerOf(0), address(tokenDivider));
assertEq(erc721Mock.ownerOf(1), USER);
// 2. user2 buy the first NFT
uint256 fee = price / 100;
uint256 sellerFee = fee / 2;
uint256 sentValue = price + sellerFee;
vm.prank(USER2);
tokenDivider.buyOrder{value: sentValue}(0, USER);
assertEq(tokenDivider.getBalanceOf(USER2, address(erc20Mock)), AMOUNT);
// 3. divide the second NFT
uint256 secondTokenId = 1;
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), secondTokenId);
tokenDivider.divideNft(address(erc721Mock), secondTokenId, AMOUNT);
vm.stopPrank();
// 4. user2 claim the first NFT, claim will fail
vm.startPrank(USER2);
erc20Mock.approve(address(tokenDivider), AMOUNT);
vm.expectRevert(TokenDivider.TokenDivider__NotEnoughErc20Balance.selector);
tokenDivider.claimNft(address(erc721Mock));
vm.stopPrank();
}

and run the test forge test --mt testBreakClaimNft, tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address will be different after dividing the second NFT.

Recommended Mitigation:

change mapping(address nft => ERC20Info) nftToErc20Info to mapping(address nft -> mapping(uint256 tokenId => address)) nftToErc20Info to store the ERC20 contract address for each NFT tokenId.
User need to provide both NFT contract address and tokenId to get the ERC20 contract address.

Updates

Lead Judging Commences

fishy Lead Judge 5 months ago
Submission Judgement Published
Validated
Assigned finding tags:

Wrong nft collection handling

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.