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

NFTs get stuck if more than one NFT of a same collection is concurrently divided

0xalexsr

Summary

NFTs are only tracked per collection address and not per tokenId. Dividing an NFT of a same collection where another NFT is already dividing will cause TokenDivider::nftToErc20Info to erase previously dividing NFT data to only keep the latest. Previous NFT will remain unclaimable and it won't be possible to create sell orders for the associated ERC20 tokens

Vulnerability Details

TokenDivider::nftToErc20Info

mapping(address nft => ERC20Info) nftToErc20Info;
​

is set when an NFT is divided

nftToErc20Info[nftAddress] = ERC20Info({
erc20Address: erc20,
tokenId: tokenId
});

But the mapping tracks only the nftAddress so a previously divided NFT could be overwritten if another NFT of the same nftAddress gets divided

The mapping is used in TokenDivider::claimNft , TokenDivider::transferErcTokens and TokenDivider::sellErc20 causing these functions to not work with the erased information for previously divided NFTs.

Impact

  • NFT will be stucked for ever in the TokenDivider contract with erased nftToErc20Info

  • sellOrders will not be possible with erased nftToErc20Info

POC

function testNftToErc20InfoGetsErasedForAnotherTokenId() public nftDivided {
address initialErc20Address = tokenDivider
.getErc20InfoFromNft(address(erc721Mock))
.erc20Address;
uint256 initialTokenId = tokenDivider
.getErc20InfoFromNft(address(erc721Mock))
.tokenId;
​
erc721Mock.mint(USER);
​
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID + 1);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID + 1, AMOUNT);
vm.stopPrank();
​
address newErc20Address = tokenDivider
.getErc20InfoFromNft(address(erc721Mock))
.erc20Address;
uint256 newTokenId = tokenDivider
.getErc20InfoFromNft(address(erc721Mock))
.tokenId;
​
// nftToErc20Info has been updated and previously divided nft has been erased
assertNotEq(initialErc20Address, newErc20Address);
assertNotEq(initialTokenId, newTokenId);
}

Recommendations

  • similar to s_userToSellOrders, an array of ERC20Info could be used

    - mapping(address nft => ERC20Info) nftToErc20Info;
    + mapping(address nft => ERC20Info[]) nftToErc20Info;
    ​

  • a double mapping adding data per tokenId

    - mapping(address nft => ERC20Info) nftToErc20Info;
    + mapping(address nftAddress => mapping(uint256 nftId => ERC20Info)) nftToErc20Info;
    ​
Updates

Lead Judging Commences

fishy Lead Judge 6 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.