Pieces Protocol

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

[H-1] Lost of ERC721 tokens from collision in mapping

Summary

The contract allows multiple NFTs from the same collection to be fractionalized into fungible ERC20 tokens. However, it only uses the NFT collection address (not the token ID) as a mapping key. This design causes subsequent fractionalizations to overwrite previous ones, resulting in a Denial of Service for earlier fractionalized NFTs and potential loss of access to tokens.

Vulnerability Details

The core issue lies in how the contract’s nftToErc20Info mapping is designed. The mapping uses the NFT collection’s address nftAddress as the key and associates it with the most recently fractionalized token’s information. This approach creates a mapping collision when multiple token IDs from the same collection are fractionalized.

When divideNft(address nftAddress, uint256 tokenId, uint256 amount) is called, the contract updates the mapping:

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

If an NFT with a new token ID from the same collection is subsequently fractionalized, it overwrites the existing entry in the mapping for that collection’s address. As a result, the data for the previously fractionalized NFT is lost. When the original owner of the first fractionalized NFT attempts to reclaim it by providing the correct amount of the ERC20 tokens, the contract no longer references their token ID’s data.
This prevents the owner from accessing their fractional tokens or reclaiming their NFT, creating a Denial of Service and the potential for permanent token loss.

Impact

  1. Denial of Service for Legitimate NFT Owners:

    • The original NFT owner is unable to reclaim their NFT because the fraction data now references another token ID.

  2. Potential NFT Token Loss:

    • Fractionalized tokens from previous NFT token Ids are no longer claimable, effectively rendering

Tools Used

  • Foundry & forge:

    • Wrote tests to simulate overwriting the mapping.

POC

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.28;
import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol";
import {ERC20Mock} from "@openzeppelin/contracts/mocks/token/ERC20Mock.sol";
import {ERC721} from "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import {Test, console} from "forge-std/Test.sol";
import {DeployTokenDivider} from "script/DeployTokenDivider.s.sol";
import {TokenDivider} from "src/TokenDivider.sol";
/**
* @dev A simple ERC721 mock contract for testing.
*/
contract ERC721Mock is ERC721, Ownable {
uint256 private _tokenIdCounter;
constructor() ERC721("MockToken", "MTK") Ownable(msg.sender) {}
function mint(address to) public onlyOwner {
_safeMint(to, _tokenIdCounter);
_tokenIdCounter++;
}
}
/**
* @dev This test contract demonstrates how fractionalizing two different NFTs
* from the same ERC721 collection by different legitimate users leads to overwrites,
* causing a Denial of Service for the first user's NFT.
*/
contract DDoSNFT is Test {
DeployTokenDivider deployer;
TokenDivider tokenDivider;
ERC721Mock erc721Mock;
address userA = makeAddr("userA");
address userB = makeAddr("userB");
function setUp() public {
// Deploy main contract
deployer = new DeployTokenDivider();
tokenDivider = deployer.run();
// Deploy mock ERC721
erc721Mock = new ERC721Mock();
// Mint NFT #0 -> userA
erc721Mock.mint(userA);
// Mint NFT #1 -> userB
erc721Mock.mint(userB);
// Give them some Ether
vm.deal(userA, 10 ether);
vm.deal(userB, 10 ether);
console.log("Setup complete. userA has tokenId=0, userB has tokenId=1.");
}
/**
* @notice Demonstrates how overwriting the mapping with a second fractionalization
* breaks the first user's ability to `claimNft`.
*/
function test_divideNft_issue() public {
// Step 1: userA fractionalizes NFT #0
vm.startPrank(userA);
erc721Mock.approve(address(tokenDivider), 0);
// For demonstration, mint 1,000,000 fraction tokens
tokenDivider.divideNft(address(erc721Mock), 0, 1_000_000 ether);
vm.stopPrank();
address userA_Erc20 = tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address;
console.log("userA fraction token:", userA_Erc20);
// Step 2: userB fractionalizes a different NFT #1 from the same collection
vm.startPrank(userB);
erc721Mock.approve(address(tokenDivider), 1);
tokenDivider.divideNft(address(erc721Mock), 1, 1_000_000 ether);
vm.stopPrank();
// The call above sets nftToErc20Info[erc721Mock] to point to userB's token ID #1
address userB_Erc20 = tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address;
console.log("userB fraction token:", userB_Erc20);
// Step 3: userA tries to claim back NFT #0 but `claimNft(erc721Mock)` references tokenId=1 info
vm.startPrank(userA);
console.log("Attempting to claim NFT #0, but it's overwritten by tokenId=1 data...");
vm.expectRevert();
tokenDivider.claimNft(address(erc721Mock));
vm.stopPrank();
// This revert happens because the fraction data references userB's token #1,
// so userA's fraction ownership doesn't match the new minted amount.
// => userA lost his NFT #0
}
}
% forge test --match-contract DDoSNFT -vv
[⠊] Compiling...
[⠒] Compiling 1 files with Solc 0.8.28
[⠑] Solc 0.8.28 finished in 542.56ms
Compiler run successful!
Ran 1 test for test/unit/AccessLostToNft.t.sol:DDoSNFT
[PASS] test_divideNft_issue() (gas: 2200161)
Logs:
Setup complete. userA has tokenId=0, userB has tokenId=1.
userA fraction token: 0xFEfC6BAF87cF3684058D62Da40Ff3A795946Ab06
userB fraction token: 0x2a9e8fa175F45b235efDdD97d2727741EF4Eee63
Attempting to claim NFT #0, but it's overwritten by tokenId=1 data...
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 7.72ms (1.31ms CPU time)
Ran 1 test suite in 135.71ms (7.72ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommendations

Recommendations

  1. Delete the ERC20Info struct and the erc20ToNft mapping in order to use the a more granular per tokenId mapping:

mapping(address collection => mapping(uint256 tokenId => address erc20TokenOfCollection)) collectionToTokenIdToErc20Address;
  1. Update the divideNft function to use the new mapping:

function divideNft(address nftAddress,uint256 tokenId,uint256 amount) external onlyNftOwner(nftAddress, tokenId)
{
// this check can be removed since if nftAddress is 0, the onlyNftOwner modifier will revert the execution
// if (nftAddress == address(0)) revert TokenDivider__NftAddressIsZero();
if (amount == 0) revert TokenDivider__AmountCantBeZero();
ERC20ToGenerateNftFraccion erc20Contract = new ERC20ToGenerateNftFraccion(
string(abi.encodePacked(ERC721(nftAddress).name(), "Fraccion")),
string(abi.encodePacked("F", ERC721(nftAddress).symbol()))
);
erc20Contract.mint(address(this), amount);
address erc20 = address(erc20Contract);
IERC721(nftAddress).safeTransferFrom(msg.sender, address(this), tokenId, "");
if (IERC721(nftAddress).ownerOf(tokenId) == msg.sender) revert TokenDivider__NftTransferFailed();
collectionToTokenIdToErc20Address[nftAddress][tokenId] = erc20;
balances[msg.sender][erc20] = amount;
erc20ToMintedAmount[erc20] = amount;
emit NftDivided(nftAddress, tokenId, amount, erc20);
bool transferSuccess = IERC20(erc20).transfer(msg.sender, amount);
if (!transferSuccess) revert TokenDivider__TransferFailed();
}
  1. Update the claimNft function to use the new tokenId granular strategy:

function claimNft(address nftAddress, uint256 tokenId) external {
// this check can be removed since if nftAddress is 0, the safeTransferFrom will revert the execution
// if (nftAddress == address(0)) revert TokenDivider__NftAddressIsZero();
address erc20Address = collectionToTokenIdToErc20Address[nftAddress][tokenId];
/// @audit combined vulenerability, move checks to be strict !==, since it's not supposed
/// to have more erc20 tokens than minted;
if (balances[msg.sender][erc20Address] !== erc20ToMintedAmount[erc20Address]) {
revert TokenDivider__NotEnoughErc20Balance();
}
ERC20ToGenerateNftFraccion(erc20Address).burnFrom(msg.sender, erc20ToMintedAmount[erc20Address]);
balances[msg.sender][erc20Address] = 0;
erc20ToMintedAmount[erc20Address] = 0;
emit NftClaimed(nftAddress, tokenId, erc20Address);
IERC721(nftAddress).safeTransferFrom(address(this), msg.sender, tokenId);
}
  1. Update the transferErcTokens function to use the new tokenId granular strategy:

function transferErcTokens(address nftAddress, uint256 tokenId, address to, uint256 amount) external {
// unnecesary check, if nft address is 0, balances[msg.sender][erc20Address] will be always 0 and though, < amount,
// so the condition `if (balances[msg.sender][erc20Address] < amount) revert TokenDivider__NotEnoughErc20Balance();` will always revert
// if (nftAddress == address(0)) revert TokenDivider__NftAddressIsZero();
// unnecessary check, if to is 0x0 the transferFrom will revert the execution:
// see: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ea736bd45bd844d7968a64c5707d97710fe1c077/contracts/token/ERC20/ERC20.sol#L166
// if (to == address(0)) revert TokenDivider__CantTransferToAddressZero();
if (amount == 0) revert TokenDivider__AmountCantBeZero();
address erc20Address = collectionToTokenIdToErc20Address[nftAddress][tokenId];
// unnecessary check, if to is 0x0 the transferFrom will revert the execution:
// see: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/ea736bd45bd844d7968a64c5707d97710fe1c077/contracts/token/ERC20/ERC20.sol#L166
// if (to == address(0)) revert TokenDivider__CantTransferToAddressZero();
if (balances[msg.sender][erc20Address] < amount) revert TokenDivider__NotEnoughErc20Balance();
balances[msg.sender][erc20Address] -= amount;
balances[to][erc20Address] += amount;
emit TokensTransfered(amount, erc20Address);
IERC20(erc20Address).transferFrom(msg.sender, to, amount);
  1. Update the sellErc20 function to use the new tokenId granular strategy:

function sellErc20(address nftPegged, uint256 tokenId, uint256 price, uint256 amount) external {
// unnecesary check, if nft address is 0, balances[msg.sender][erc20Address] will be always 0 and though, < amount,
// so the condition `if (balances[msg.sender][erc20Address] < amount) revert TokenDivider__NotEnoughErc20Balance();` will always revert
// if (nftPegged == address(0)) revert TokenDivider__NftAddressIsZero();
if (amount == 0) revert TokenDivider__AmountCantBeZero();
address erc20Address = collectionToTokenIdToErc20Address[nftPegged][tokenId];
if (balances[msg.sender][erc20Address] < amount) revert TokenDivider__InsuficientBalance();
balances[msg.sender][erc20Address] -= amount;
s_userToSellOrders[msg.sender].push(
SellOrder({ seller: msg.sender, erc20Address: erc20Address, price: price, amount: amount })
);
emit OrderPublished(amount, msg.sender, nftPegged);
IERC20(erc20Address).transferFrom(msg.sender, address(this), amount);
}
  1. Update the getErc20InfoFromNft function to use the new tokenId granular strategy:

function getErc20InfoFromNft(address nft, uint256 tokenId) public view returns (address) {
return collectionToTokenIdToErc20Address[nft][tokenId];
}
  1. Update the events to use the new tokenId granular strategy, note also the indexed parameters are updated:

event NftDivided(address indexed nftAddress, uint256 indexed tokenId, uint256 amountErc20Minted, address erc20Minted);
event NftClaimed(address indexed nftAddress, uint256 indexed tokenId, address erc20Address);
Updates

Lead Judging Commences

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