Submission Details
Severity:
Critical NFT Loss Due to Single-Key Mapping Overwrite in divideNft Function
## Critical NFT Loss Due to Single-Key Mapping Overwrite in divideNft Function
The bug occurs because the contract uses a single NFT address as the mapping key, causing any subsequent NFTs from the same collection to overwrite the previous NFT's data.
**Lines of code**
https://github.com/Cyfrin/2025-01-pieces-protocol/blob/4ef5e96fced27334f2a62e388a8a377f97a7f8cb/src/TokenDivider.sol#L59
https://github.com/Cyfrin/2025-01-pieces-protocol/blob/4ef5e96fced27334f2a62e388a8a377f97a7f8cb/src/TokenDivider.sol#L127
```solidity
// Vulnerable Code Section
mapping(address nft => ERC20Info) nftToErc20Info; // ❌ Incorrect implementation
// In divideNft function:
nftToErc20Info[nftAddress] = ERC20Info({erc20Address: erc20, tokenId: tokenId}); // ❌ Overwrites previous NFT data
```
## Vulnerability Details
**Here's the sequence that demonstrates the problem:**
**First NFT Division:**
```solidity
divider.divideNft(address(mfnft), 0, 200);
```
Creates a new ERC20 token (let's call it ERC20_A)
Stores in mapping: nftToErc20Info[address(mfnft)] = {erc20Address: ERC20_A, tokenId: 0}
Mints 200 tokens of ERC20_A
**Second NFT Division:**
```solidity
divider.divideNft(address(mfnft), 1, 100);
```
Creates another new ERC20 token (let's call it ERC20_B)
Overwrites the previous mapping: nftToErc20Info[address(mfnft)] = {erc20Address: ERC20_B, tokenId: 1}
Mints 100 tokens of ERC20_B
When querying:
```solidity
divider.getErc20InfoFromNft(address(mfnft))
```
Returns only the latest entry: {erc20Address: ERC20_B, tokenId: 1}
**Coded POC**
- create a file in the test folder in the project repo, paste this code and run the test.
```solidity
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.18;
import {Test, console} from "forge-std/Test.sol";
import {ERC721Mock} from "./mocks/ERC721Mock.sol";
import {TokenDivider} from "src/TokenDivider.sol";
import {ERC20ToGenerateNftFraccion} from "src/token/ERC20ToGenerateNftFraccion.sol";
contract TokenDividerBugPOC is Test {
ERC721Mock nft;
TokenDivider divider;
address alice = makeAddr("alice");
address bob = makeAddr("bob");
uint256 constant FRACTION_AMOUNT_1 = 200e18;
uint256 constant FRACTION_AMOUNT_2 = 100e18;
uint256 constant PRICE = 1 ether;
TokenDivider.ERC20Info info1;
TokenDivider.ERC20Info info2;
address firstErc20;
address secondErc20;
function setUp() public {
divider = new TokenDivider();
nft = new ERC721Mock();
// Mint 2 NFTs to Alice
nft.mint(alice); // tokenId 0
nft.mint(alice); // tokenId 1
vm.startPrank(alice);
nft.approve(address(divider), 0);
nft.approve(address(divider), 1);
vm.stopPrank();
// Setup initial state by dividing both NFTs
vm.startPrank(alice);
divider.divideNft(address(nft), 0, FRACTION_AMOUNT_1);
info1 = divider.getErc20InfoFromNft(address(nft));
firstErc20 = info1.erc20Address;
divider.divideNft(address(nft), 1, FRACTION_AMOUNT_2);
info2 = divider.getErc20InfoFromNft(address(nft));
secondErc20 = info2.erc20Address;
vm.stopPrank();
}
function testMappingOverwrite() public {
// Test Bug 1: Mapping Overwrite
TokenDivider.ERC20Info memory latestInfo = divider.getErc20InfoFromNft(address(nft));
assertEq(latestInfo.tokenId, 1, "Latest tokenId should be 1 (mapping was overwritten)");
assertEq(latestInfo.erc20Address, secondErc20, "Latest ERC20 address should be from second NFT");
assertTrue(latestInfo.erc20Address != firstErc20, "First ERC20 mapping was lost");
}
function testLostNFTRecovery() public {
vm.startPrank(alice);
// Verify initial NFT ownership
assertEq(nft.ownerOf(0), address(divider), "TokenId 0 should be owned by contract");
assertEq(nft.ownerOf(1), address(divider), "TokenId 1 should be owned by contract");
// Try to claim second NFT (tokenId 1)
ERC20ToGenerateNftFraccion(secondErc20).approve(address(divider), FRACTION_AMOUNT_2);
divider.claimNft(address(nft));
// Verify final state
assertEq(nft.ownerOf(0), address(divider), "TokenId 0 remains locked in contract");
assertEq(nft.ownerOf(1), alice, "TokenId 1 should be claimed by Alice");
vm.stopPrank();
}
function testSellingImplications() public {
vm.startPrank(alice);
// attempt to sell all of the first NFT's fractions will rever TokenDivider__InsuficientBalance() because the contract has no balance of the first ERC20 token
// balance of the first ERC20 token is greater than balance of the second ERC20 token, so it will revert.
ERC20ToGenerateNftFraccion(firstErc20).approve(address(divider), FRACTION_AMOUNT_1);
vm.expectRevert(TokenDivider.TokenDivider__InsuficientBalance.selector);
divider.sellErc20(address(nft), PRICE, FRACTION_AMOUNT_1);
//approve the second ERC20 token to the contract
ERC20ToGenerateNftFraccion(secondErc20).approve(address(divider), FRACTION_AMOUNT_2);
divider.sellErc20(address(nft), PRICE, FRACTION_AMOUNT_2);
// The sell order will be created with the second ERC20 token instead of the first
TokenDivider.ERC20Info memory currentInfo = divider.getErc20InfoFromNft(address(nft));
assertEq(currentInfo.erc20Address, secondErc20, "Sell order uses wrong ERC20 token");
vm.stopPrank();
}
function testTransferIssues() public {
vm.startPrank(alice);
// Transfer fail if approve is called on the first ERC20 token because the contract uses the second ERC20 token
//ERC20ToGenerateNftFraccion(firstErc20).approve(address(divider), FRACTION_AMOUNT_1);
ERC20ToGenerateNftFraccion(secondErc20).approve(address(divider), FRACTION_AMOUNT_2);
// Transfer will use wrong ERC20 token due to overwritten mapping
divider.transferErcTokens(address(nft), bob, 50e18);
// Check balances
uint256 bobBalance = divider.getBalanceOf(bob, secondErc20);
assertEq(bobBalance, 50e18, "Transfer used wrong ERC20 token");
vm.stopPrank();
}
function testFinalState() public {
// Verify final state of both NFTs
assertEq(nft.ownerOf(0), address(divider), "First NFT should be locked in contract");
assertEq(nft.ownerOf(1), address(divider), "Second NFT should be locked in contract");
// Verify mapping only contains latest NFT info
TokenDivider.ERC20Info memory finalInfo = divider.getErc20InfoFromNft(address(nft));
assertEq(finalInfo.tokenId, 1, "Only tokenId 1 info is preserved");
assertEq(finalInfo.erc20Address, secondErc20, "Only second ERC20 mapping is preserved");
}
}
```
This leads to:
## Impact:
**NFT Loss:** The information about the first NFT fractionalization (token ID 0) is completely lost
This makes it impossible to track or claim back the original NFT with token ID 0 so it becomes permanently locked in the contract.
The ```claimNft``` function will only work with the latest tokenId stored in the mapping.
**Incorrect Token Association:** The ```nftToErc20Info``` mapping only stores the latest NFT-to-ERC20 association, leading to confusion about which ERC20 tokens correspond to which NFT.
Impact on Other Functions:
**claimNft:** Can only claim the latest NFT divided, earlier NFTs are lost
**sellErc20:** Orders may be created with incorrect token associations
**transferErcTokens:** Transfers might use wrong token information
**getErc20InfoFromNft:** Returns incorrect/outdated information for previously divided NFTs
## Tools Used
Manual Review
## Fix Recommendation:
The fix would require changing the mapping to include the tokenId:
Should be:
```solidity
// Correct Implementation
mapping(address nft => mapping(uint256 tokenId => ERC20Info)) nftToErc20Info; // ✅ Proper implementation
// In divideNft function:
nftToErc20Info[nftAddress][tokenId] = ERC20Info({erc20Address: erc20, tokenId: tokenId}); // ✅ Maintains data for each tokenId
```
And updating all related functions to use both the NFT address and tokenId as keys:
``` solidity
function getErc20InfoFromNft(address nft, uint256 tokenId) public view returns(ERC20Info memory) {
return nftToErc20Info[nft][tokenId];
}
```
Updates
Lead Judging Commences
fishy 9 months ago
Submission Judgement Published Assigned finding tags:
Wrong nft collection handling
Rewards breakdown
nSLOC
181This contest has a flat reward structure with the following payouts:
100 EXP
20 EXP
2 EXP
Live
The contest is live. Earn rewards by submitting a finding.
Appeals
This is your time to appeal against judgements on your submissions.
Appeals Review
Appeals are being carefully reviewed by our judges.