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

`TokenDivider::divideNft` lacks verification of prior NFT division

victorzsh

Summary

TokenDivider::divideNft Overwrites previous NFT Division data due to missing pre-check.

Vulnerability Details

https://github.com/Cyfrin/2025-01-pieces-protocol/blob/4ef5e96fced27334f2a62e388a8a377f97a7f8cb/src/TokenDivider.sol#L109

In TokenDivider::divideNft, there is no verification to ensure that the NFT provided as a parameter has not been previously "divided" within the protocol.

The mapping TokenDivider::nftToErc20Info associates an NFT with a single set of information (ERC20 address and tokenId). If TokenDivider::divideNft is called repeatedly for the same NFT, the existing information will be overwritten. This would result in the loss of reference to previously created ERC20 contracts, making it difficult or impossible to correctly claim the NFT or manage fractions associated with those older contracts.

Impact

Data related to previously linked ERC20 contracts to the NFT would be overwritten. Users would lose the ability to claim NFTs and manage their fractions.

Proof of Concept:

  1. Two different users own NFTs from the same ERC721 contract, but with different IDs.

  2. User 1 calls the function TokenDivider::divideNft with their NFT of ID 0. The TokenDivider contract transfers this NFT to itself, creates a new fractional ERC20 token to represent it, and stores the information of this NFT–ERC20 pair in the internal mapping TokenDivider::nftToErc20Info.

  3. At this point, the mapping associates User 1's NFT with a specific ERC20 contract, recording details such as the address of the created ERC20 contract and the original tokenId.

  4. Later, User 2 calls TokenDivider::divideNft with their NFT of ID 1. Since the contract does not check if another NFT from the same ERC721 contract has already been divided, the TokenDivider repeats the process: it transfers the NFT of ID 1, generates a new ERC20 token, and overwrites the existing entry in TokenDivider::nftToErc20Info.

  5. As a result of this overwrite, the information previously stored for User 1's NFT (ID 0) is replaced by the data related to User 2's NFT (ID 1).

  6. This overwrite leads to data loss and potential inconsistencies in the management of fractional tokens.

Proof of Code

Place the following into TokenDividerTest.t.sol

​
function testNftInfoWillBeOverwritten() public {
uint256 TOKEN_ID_USER_1 = 0;
uint256 TOKEN_ID_USER_2 = 1;
uint256 AMOUNT_TO_DIVIDE = 1e18;
​
ERC721Mock newErc721Mock = new ERC721Mock();
newErc721Mock.mint(USER);
newErc721Mock.mint(USER2);
​
vm.startPrank(USER);
newErc721Mock.approve(address(tokenDivider), TOKEN_ID_USER_1);
tokenDivider.divideNft(address(newErc721Mock), TOKEN_ID_USER_1, AMOUNT_TO_DIVIDE);
vm.stopPrank();
​
TokenDivider.ERC20Info memory erc20InfoBeforeDivideAgain =
tokenDivider.getErc20InfoFromNft(address(newErc721Mock));
​
vm.startPrank(USER2);
newErc721Mock.approve(address(tokenDivider), TOKEN_ID_USER_2);
tokenDivider.divideNft(address(newErc721Mock), TOKEN_ID_USER_2, AMOUNT_TO_DIVIDE);
vm.stopPrank();
​
TokenDivider.ERC20Info memory erc20InfoAfterDivideAgain =
tokenDivider.getErc20InfoFromNft(address(newErc721Mock));
​
// Check that the erc20 address changed when dividing the nft again
assert(erc20InfoAfterDivideAgain.erc20Address != erc20InfoBeforeDivideAgain.erc20Address);
​
// USER should not be able to claim the nft, once the data has been overwritten
vm.startPrank(USER);
ERC20Mock(erc20InfoAfterDivideAgain.erc20Address).approve(address(tokenDivider), AMOUNT_TO_DIVIDE);
vm.expectRevert();
tokenDivider.claimNft(address(newErc721Mock));
vm.stopPrank();
}
​

Tools Used

  • Foundry for testing and verification

  • Manual code review

Recommendations

In TokenDivider::divideNft, introduce a check to see if the NFT has already been divided. For example, create a new bool field in the TokenDivider::ERC20Info struct. Then, in the TokenDivider::divideNft function, check if nftToErc20Info[nftAddress].isDidivided is true. If so, revert the transaction to prevent overwriting existing data.

...
+ error TokenDivider__NftAlreadyDivided()
...
struct ERC20Info {
address erc20Address;
uint256 tokenId;
+ bool isDivided;
}
...
function divideNft(address nftAddress, uint256 tokenId, uint256 amount)
external
onlyNftOwner(nftAddress, tokenId)
onlyNftOwner(nftAddress, tokenId)
{
+ if (nftToErc20Info[nftAddress].isDivided == true) revert TokenDivider__NftAlreadyDivided();
...
- nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId});
+ nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId, isDivided: true});

It is also recommended that in the TokenDivider::claimNft, when modifying state values, set the TokenDivider::tokenInfo.isDivided to false. This should allow the NFT to be divided again immediately after the claim is completed.

balances[msg.sender][tokenInfo.erc20Address] = 0;
erc20ToMintedAmount[tokenInfo.erc20Address] = 0;
+ tokenInfo.isDivided = false;
Updates

Lead Judging Commences

fishy Lead Judge 5 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement

Appeal created

victorzsh Submitter
5 months ago
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.