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

Some users will not be able to Claim their NFTs Due to Incorrect `nftToErc20Info` Mapping

victortheoracle

Summary

The claimNft function fails when multiple NFTs from the same collection (nftAddress) are divided because the nftToErc20Info mapping is overwritten. This results in incorrect ERC20Info being retrieved, which causes the function to revert when users attempt to claim their NFTs.

Vulnerability Details

The nftToErc20Info mapping stores information using only the NFT collection address (nftAddress) as the key. This means it can't differentiate between multiple tokens in the same collection, leading to overwriting. This allows the mapping to be overwritten when a new token from the same NFT collection is divided. As a result, the ERC20Info for the NFT being claimed may no longer be valid:

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

mapping(address nft => ERC20Info) nftToErc20Info;

The claimNft function relies on the nftToErc20Info mapping to retrieve the corresponding ERC20Info. If the mapping has been overwritten, the erc20Address and tokenId values will be incorrect, leading to a revert in this line as the user's balance will be less than the amount of that token that was minted since that is not the token associated with the NFT the user wished to claim:

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

if (balances[msg.sender][tokenInfo.erc20Address] < erc20ToMintedAmount[tokenInfo.erc20Address]) {
revert TokenDivider__NotEnoughErc20Balance();
}

Example Scenario

  1. User A divides an NFT from nftAddress with tokenId = 1.

    • The mapping is updated: nftToErc20Info[nftAddress] = ERC20Info({erc20Address: <erc20Address1>, tokenId: 1}).

  2. User B divides another NFT from the same nftAddress but with tokenId = 2.

    • The mapping is overwritten: nftToErc20Info[nftAddress] = ERC20Info({erc20Address: <erc20Address2>, tokenId: 2}).

  3. User A tries to claim their NFT with tokenId = 1. The claimNft function retrieves the incorrect ERC20Info for tokenId = 2.

  4. The function reverts because the balances and erc20ToMintedAmount checks are based on the wrong ERC20Info.

Proof of Concept

In the TokenDividerTest.t.sol file, do the following:

  • Replace your setup function with this:

function setUp() public {
deployer = new DeployTokenDivider();
tokenDivider = deployer.run();
​
erc721Mock = new ERC721Mock();
​
erc721Mock.mint(USER);
erc721Mock.mint(USER3);
vm.deal(USER2, STARTING_USER_BALANCE);
}

  • Add two new variables at the top of the test contract

+ address public USER3 = makeAddr("user3");
+ uint256 public constant TOKEN_ID_2 = 1;

  • Now copy this test and add it to your file:

function testClaimNftRevertsAfterSecondUserDividesNftFromSameCollection() public nftDivided {
​
// This user will override the mapping
vm.startPrank(USER3);
erc721Mock.approve(address(tokenDivider), TOKEN_ID_2);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID_2, AMOUNT);
vm.stopPrank();
​
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
​
vm.startPrank(USER);
erc20Mock.approve(address(tokenDivider), AMOUNT);
// This user will not be able to claim because the check for the token balance is used on wrong information fetched from the nftToErc20Info mapping
tokenDivider.claimNft(address(erc721Mock));
vm.stopPrank();
}

  • Run this command in your terminal:

forge test --mt testClaimNftRevertsAfterSecondUserDividesNftFromSameCollection -vvvv

This test will fail with the error TokenDivider__NotEnoughErc20Balance() just like I said it would.

Encountered 1 failing test in test/unit/TokenDividerTest.t.sol:TokenDiverTest
[FAIL. Reason: TokenDivider__NotEnoughErc20Balance()] testClaimNftRevertsAfterSecondUserDividesNftFromSameCollection() (gas: 1507361)

Impact

  • Users cannot claim their NFTs because the mapping points to an incorrect ERC20Info.

  • This can lead to a loss of user trust, as they are unable to retrieve their assets due to the contract's faulty design.

Tools Used

  • Manual code review

  • Foundry

Recommendations

To resolve this issue,

  • Modify the nftToErc20Info mapping in line 59 to account for both nftAddress and tokenId.

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

  • Update the divideNft function in line 127 to correctly store the ERC20Info for each unique token:

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

  • Update the claimNft, transferErcTokens, sellErc20, and getErc20InfoFromNft functions to take in the correct arguments:

- function claimNft(address nftAddress) external {
+ function claimNft(address nftAddress, uint256 tokenId) external {
- function transferErcTokens(address nftAddress,address to, uint256 amount) external {
+ function transferErcTokens(address nftAddress, uint256 tokenId, address to, uint256 amount) external {
- function sellErc20(address nftPegged, uint256 price,uint256 amount) external {
+ function sellErc20(address nftPegged, uint256 tokenId, uint256 price, uint256 amount) external {
- function getErc20InfoFromNft(address nft) public view returns(ERC20Info memory) {
+ function getErc20InfoFromNft(address nft, uint256 tokenId) public view returns(ERC20Info memory) {

  • Finally, correct all instances where the mapping is called to fetch the correct ERC20Info:

- ERC20Info storage tokenInfo = nftToErc20Info[nftAddress];
+ ERC20Info storage tokenInfo = nftToErc20Info[nftAddress][tokenId];
- ERC20Info memory tokenInfo = nftToErc20Info[nftAddress];
+ ERC20Info memory tokenInfo = nftToErc20Info[nftAddress][tokenId];
- ERC20Info memory tokenInfo = nftToErc20Info[nftPegged];
+ ERC20Info memory tokenInfo = nftToErc20Info[nftPegged][tokenId];
- return nftToErc20Info[nft];
+ return nftToErc20Info[nft][tokenId];
Updates

Lead Judging Commences

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