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

Calling TokenDivider::divideNft for the same NFT address overwrites nftToErc20Info, erasing previous data. This risks losing critical information of previous NFTs from the same collection.

riceee

Description

The divideNft function has a critical issue where calling it multiple times with the same nftAddress but different tokenIds causes the nftToErc20Info mapping to be overwritten each time. This results in the loss of previously stored ERC20Info structs for the same NFT collection. As a consequence, users lose the ability to reclaim or interact with the previously divided NFT tokens of the same NFT collection, as the state of the old divisions is permanently erased. This behavior creates a significant risk of losing access to fractionalized tokens and undermines the protocol's functionality.

Vulnerability Details

  1. Suppose a USER calls TokenDivider::divideNft with an nftAddress = address(1) and tokenId = 0

  2. The nftToErc20Info stores ERC20Info({ erc20Address: address(1), tokenId: 0 })

  3. USER2 calls TokenDivider::divideNft with the same nftAddress but tokenId = 5

  4. The nftToErc20Info stores overwrites it data to ERC20Info({ erc20Address: address(1), tokenId: 5 })

This results in loss of critical data to reclaim the previous token / transfer / place sell orders for the erc20tokens for the previous NFTs of a same collection.

Impact

All previous NFTs of a collection using this protocol will get locked in the protocol forever.

TokenDivider:claimNft
TokenDivider:transferErcTokens
TokenDivider:sellErc20
are all deemed unusable for any previous NFTs of a collection that is currently in the protocol.

Tools Used

Manual Review
Foundry

POC

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.18;
​
​
import {Test, console} from 'forge-std/Test.sol';
import {DeployTokenDivider} from 'script/DeployTokenDivider.s.sol';
import {TokenDivider} from 'src/TokenDivider.sol';
import {ERC721Mock} from '../mocks/ERC721Mock.sol';
import {ERC20Mock} from '@openzeppelin/contracts/mocks/token/ERC20Mock.sol';
import {Ownable} from '@openzeppelin/contracts/access/Ownable.sol';
​
​
contract TokenDiverTest is Test {
DeployTokenDivider deployer;
TokenDivider tokenDivider;
ERC721Mock erc721Mock;
​
address public USER = makeAddr("user");
address public USER2 = makeAddr("user2");
uint256 constant public STARTING_USER_BALANCE = 10e18;
uint256 constant public AMOUNT = 2e18;
uint256 constant public TOKEN_ID = 0;
​
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 test_divideNft_overwrites_nftToErc20Info() public {
//An user deposits an NFT
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, AMOUNT);
vm.stopPrank();
​
​
//USER2 deposita another NFT from the same collection
vm.startPrank(USER2);
erc721Mock.approve(address(tokenDivider), TOKEN_ID + 1);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID + 1, AMOUNT);
vm.stopPrank();
​
vm.startPrank(USER);
//USER wants to claim their NFT, which they should be able to as they have all the tokens but it reverts
vm.expectRevert(TokenDivider.TokenDivider__NotEnoughErc20Balance.selector);
tokenDivider.claimNft(address(erc721Mock));
//USER wants to transfer their tokens, but that reverts too
vm.expectRevert(TokenDivider.TokenDivider__NotEnoughErc20Balance.selector);
tokenDivider.transferErcTokens(address(erc721Mock), USER2, AMOUNT);
//USER wants to place a sell order
vm.expectRevert(TokenDivider.TokenDivider__InsuficientBalance.selector);
tokenDivider.sellErc20(address(erc721Mock), 1e18, AMOUNT);
vm.stopPrank();
}
}

If we run the test we see that even if the USER has the NFT deposited they are not able to access it anymore

Recommendations

We need to modify the nftToErc20Info mapping such that it stores a collection of the erc20 Info for each tokenId of a given NFT address.
that can be done by

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

this makes the ERC20Info struct useless so we can just delete that too.

- struct ERC20Info {
- address erc20Address;
- uint256 tokenId;
- }

Now, we need to change the divideNft, claimNft, transferErcTokens, sellErc20 functionality to properly claim NFT. transfer erc20 tokens and publish sell orders based on the tokenId of the NFT we are dealing with from a NFT address.

divideNft()

function divideNft(address nftAddress, uint256 tokenId, uint256 amount)
- nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId});
+ nftToErc20Info[nftAddress][tokenId] = erc20;

claimNft()

- function claimNft(address nftAddress)
+ function claimNft(address nftAddress, uint256 tokenId)
​
- ERC20Info storage tokenInfo = nftToErc20Info[nftAddress];
+ address erc20 = nftToErc20Info[nftAddress][tokenId];

transferErcTokens()

- function transferErcTokens(address nftAddress, address to, uint256 amount)
+ function transferErcTokens(address nftAddress, uint256 tokenId, address to, uint256 amount)
​
- ERC20Info memory tokenInfo = nftToErc20Info[nftAddress];
+ address erc20 = nftToErc20Info[nftAddress][tokenId];

sellERC20()

- function sellErc20(address nftPegged, uint256 price, uint256 amount)
+ function sellErc20(address nftPegged, uint256 tokenId, uint256 price, uint256 amount)
​
- ERC20Info memory tokenInfo = nftToErc20Info[nftPegged];
+ address erc20 = nftToErc20Info[nftPegged][tokenId];

getter function change

- function getErc20InfoFromNft(address nft) public view returns(ERC20Info memory)
+ function getErc20InfoFromNft(address nft, uint256 tokenId) public view returns(address)

Summary of Key Changes

  • Data Structure: Removed the ERC20Info struct in favor of a direct mapping from NFT address and token ID to ERC20 address

  • Function Parameters: Added tokenId parameter to several functions where it was previously implicit in the ERC20Info struct

  • Storage Clearing: Added explicit clearing of NFT to ERC20 mapping when claiming NFTs

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.