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

`TokenDivider::nftToErc20Info` mapping does not handle dividing different NFTs from the same collection, causing overriding and loss of information

marcoroccon

Description:
The TokenDivider::nftToErc20Info mapping keeps track of the address of the NFT contract and the corresponding ERC20Info struct created when calling TokenDivider::divideNft. This way, if a user wants to divide multiple NFTs from the same colleciton, the address will remain the same, and therefore the mapping will override the already stored information, causing a loss of all the details regarding the previous NFT divided.

Impact:
This vulnerability will severly break the protocol, as there there will be no track of the information regarding the NFTs from the same collection that have been divided before the last NFT. Information such as contract address and tokenId will be no longer available on-chain and will cause collision when trying to sell the corresponding ERC20 via the TokenDivider::sellErc20 function.

Tools Used:
Manual review

Proof of Concept:
Add the following test in TokenDividerTest.t.sol. This test demostrates that when multiple users divide NFTs from the same collection, hence with the same contract address, the TokenDivider::getErc20InfoFromNft function, which will retrieve information from TokenDivider::nftToErc20Info mapping, will return a different value for erc20Address before and after the division of the second NFT.

uint256 constant public STARTING_USER_BALANCE = 10e18;
uint256 constant public AMOUNT = 2e18;
uint256 constant public TOKEN_ID_USER = 0;
uint256 constant public TOKEN_ID_USER_2 = 1;
​
function setUp() public {
deployer = new DeployTokenDivider();
tokenDivider = deployer.run();
erc721Mock = new ERC721Mock();
erc721Mock.mint(USER);
erc721Mock.mint(USER2);
vm.deal(USER2, STARTING_USER_BALANCE);
}
​
function testMappingOverrideWhenDividingSameCollectionNFTs() public {
// USER approves their NFT
vm.prank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID_USER);
​
// USER2 approves their NFT
vm.prank(USER2);
erc721Mock.approve(address(tokenDivider), TOKEN_ID_USER_2);
​
// USER divides their NFT for an AMOUNT of ERC20 tokens
vm.prank(USER);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID_USER, AMOUNT);
​
// Saving the ERC20 contract address retrieved from `nftToErc20Info` mapping
address erc20AddressUSER = tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address;
​
// USER2 divides their NFT for an AMOUNT of ERC20 tokens
vm.prank(USER2);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID_USER_2, AMOUNT);
​
// Saving the ERC20 contract address retrieved from `nftToErc20Info` mapping
address erc20AddressUSER2 = tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address;
​
// Checking if these two addresses are different
assert(erc20AddressUSER != erc20AddressUSER2);
}

Recommended Mitigation:
It is recommended to modify the TokenDivider::nftToErc20Info mapping in order to keep track of the tokenId for the NFT, along with its contract address.

- mapping(address nft => ERC20Info) nftToErc20Info;
+ mapping(address nft => mapping(uint256 tokenId => ERC20Info)) nftToErc20Info;

ATTENTION: this change will need further integration with all the other functions in TokenDivider. Updates to the mapping will now have to consider the tokenId, along with the contract address for the NFT. As an example, TokenDivider::divideNft will be modified as follows:

- nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId});
+ nftToErc20Info[nftAddress][tokenId] = ERC20Info({erc20Address: erc20, tokenId: tokenId});
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.