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

All protocol functionalities become ineffective for a previously divided nft, if another nft token of the same nft contract family is divided.

rohanjacin

Summary

All TokenDivider.sol functionalities become ineffective for a previously divided nft, if aleast one more nft(ERC721 token) with the same token address nftAddress, but different tokenId is divided.

Vulnerability Details

When a nft token is divided by divideNft() function, the function creates an ERC20 contract as follows

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);

The above code creates a instance of the ERC20 contract using the new keyword which in turn calls the CREATE opcode and calcualtes the address of the ERC20 contract using senders address (i.e TokenAddress contract address) and sender's nonce (transaction nonce which increments post). This results in a new unique ERC20 instance even though nftAddress name and symbol stay the same, so well this is not a problem if we try to divide two nft tokens of the same nft token (nftAddress) contract.

The issue arises in the code below

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

here the pegged ERC20 token contract address is stored in the mapping nftToErc20Info where the nftAddress maps to the ERC20Info struct and indirectly to erc20Address the address of the ERC20 pegged token contract instance.

You see the an nft token is unique and is classified by its nft contract address (where the token resides) and its token id (the id of the token in the token contract).

The mapping nftToErc20Info lacks the tokenId in it key to unqiuely represent its pegged ERC20 token contract. Thus if another token from the same nftAddress is divided, it will add the ERC20Info and the henceforth the erc20Address for that tokenId to the same key in the nftToErc20Info mapping.

This is overwrite the pegged ERC20 address and other information of the previous divided nft token from the nftAddress NFT contract family.

All references to nftToErc20Info which is critical in determining the pegged ERC20 contract in function like claimNft(), transferErcTokens(), sellErc20() and getErc20InfoFromNft() will point to the pegged ERC20 contract of the most recent divided nft token from the nftAddress family and hence these funtionality are lost for the previously divided nft token from the same family and the nft and its value is lost.

The issue can be verified by running the following PoC.
Run forge test --match-test testClaimNft2 -vvv

Here in testClaimNft2() two nft token of the same family are divided
successfully, but when you claimNft() only the second token TOKEN_ID+1 is claimed successfully.
Also lines

assertEq(erc721Mock.ownerOf(TOKEN_ID+1), USER);
assertEq(erc721Mock.ownerOf(TOKEN_ID), address(tokenDivider));

passes which shows that the first token TOKEN_ID is remains unclaimed, where as the most recent token TOKEN_ID is claimed instead on the first call to claimNft()

// 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(USER);
vm.deal(USER2, STARTING_USER_BALANCE);
}
​
function testDivideNft() public {
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
​
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, AMOUNT);
vm.stopPrank();
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
​
assertEq(tokenDivider.getErc20TotalMintedAmount(address(erc20Mock)), AMOUNT);
assertEq(erc721Mock.ownerOf(TOKEN_ID), address(tokenDivider));
assertEq(tokenDivider.getBalanceOf(USER, address(erc20Mock)), AMOUNT);
+ assertEq(erc20Mock.balanceOf(USER), AMOUNT);
+
+ // Try to divide same nft different token
+ vm.startPrank(USER);
+ erc721Mock.approve(address(tokenDivider), TOKEN_ID+1);
+ tokenDivider.divideNft(address(erc721Mock), TOKEN_ID+1, AMOUNT);
+ assertEq(erc721Mock.ownerOf(TOKEN_ID+1), address(tokenDivider));
+ vm.stopPrank();
}
​
​
modifier nftDivided() {
vm.startPrank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, AMOUNT);
vm.stopPrank();
​
_;
​
}
​
+ modifier nftDivided2(uint256 id) {
+ vm.startPrank(USER);
+ erc721Mock.approve(address(tokenDivider), id);
+ tokenDivider.divideNft(address(erc721Mock), id, AMOUNT);
+ erc721Mock.approve(address(tokenDivider), id+1);
+ tokenDivider.divideNft(address(erc721Mock), id+1, AMOUNT);
+ vm.stopPrank();
+
+ _;
+
+ }
​
function testDivideNftFailsIsSenderIsNotNftOwner() public {
vm.prank(USER);
erc721Mock.approve(address(tokenDivider), TOKEN_ID);
​
vm.startPrank(USER2);
vm.expectRevert(TokenDivider.TokenDivider__NotFromNftOwner.selector);
tokenDivider.divideNft(address(erc721Mock), TOKEN_ID, AMOUNT);
vm.stopPrank();
}
​
function testTransferErcTokensAndClaimNftFailsIfDontHaveAllTheErc20() public nftDivided() {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
vm.startPrank(USER);
// Arrange
​
erc20Mock.approve(address(tokenDivider), AMOUNT);
​
// Act / Assert
tokenDivider.transferErcTokens(address(erc721Mock),USER2, AMOUNT);
assertEq(tokenDivider.getBalanceOf(USER2, address(erc20Mock)), AMOUNT);
assertEq(tokenDivider.getBalanceOf(USER, address(erc20Mock)), 0);
​
vm.expectRevert(TokenDivider.TokenDivider__NotEnoughErc20Balance.selector);
tokenDivider.claimNft(address(erc721Mock));
vm.stopPrank();
​
}
​
function testClaimNft() public nftDivided() {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
vm.startPrank(USER);
erc20Mock.approve(address(tokenDivider), AMOUNT);
tokenDivider.claimNft(address(erc721Mock));
vm.stopPrank();
​
assertEq(erc20Mock.totalSupply(), 0);
assertEq(tokenDivider.getBalanceOf(USER, address(erc20Mock)), 0);
assertEq(erc721Mock.ownerOf(TOKEN_ID), USER);
}
​
+ function testClaimNft2() public nftDivided2(TOKEN_ID) {
+ ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
+ vm.startPrank(USER);
+ erc20Mock.approve(address(tokenDivider), AMOUNT);
+ tokenDivider.claimNft(address(erc721Mock));
+ vm.stopPrank();
+
+ assertEq(erc20Mock.totalSupply(), 0);
+ assertEq(tokenDivider.getBalanceOf(USER, address(erc20Mock)), 0);
+ assertEq(erc721Mock.ownerOf(TOKEN_ID+1), USER);
+ assertEq(erc721Mock.ownerOf(TOKEN_ID), address(tokenDivider));
+
+ vm.startPrank(USER);
+ erc20Mock.approve(address(tokenDivider), AMOUNT);
+ tokenDivider.claimNft(address(erc721Mock));
+ vm.stopPrank();
+ }
​
function testSellErc20() public nftDivided(){
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
​
vm.startPrank(USER);
erc20Mock.approve(address(tokenDivider), AMOUNT);
tokenDivider.sellErc20(address(erc721Mock), 1e18, AMOUNT);
vm.stopPrank();
assertEq(tokenDivider.getBalanceOf(USER, address(erc20Mock)), 0);
assertEq(erc20Mock.balanceOf(address(tokenDivider)), AMOUNT);
}
​
function testBuyErc20() public nftDivided() {
ERC20Mock erc20Mock = ERC20Mock(tokenDivider.getErc20InfoFromNft(address(erc721Mock)).erc20Address);
​
uint256 ownerBalanceBefore = address(tokenDivider.owner()).balance;
uint256 userBalanceBefore = address(USER).balance;
uint256 user2TokenBalanceBefore = tokenDivider.getBalanceOf(USER2, address(erc20Mock));
​
vm.startPrank(USER);
​
erc20Mock.approve(address(tokenDivider), AMOUNT);
tokenDivider.sellErc20(address(erc721Mock), AMOUNT, 1e18); // Creamos una orden de venta por 1 ETH
​
uint256 fees = AMOUNT / 100;
vm.stopPrank();
​
​
vm.prank(USER2);
tokenDivider.buyOrder{value: (3e18)}(0, USER);
​
uint256 ownerBalanceAfter = address(tokenDivider.owner()).balance;
uint256 userBalanceAfter = address(USER).balance;
uint256 user2TokenBalanceAfter = tokenDivider.getBalanceOf(USER2, address(erc20Mock));
​
assertEq(user2TokenBalanceAfter - 1e18, user2TokenBalanceBefore);
assertEq(ownerBalanceAfter - fees, ownerBalanceBefore);
​
if(block.chainid != 31337) {
assertEq(userBalanceAfter - AMOUNT + fees / 2, userBalanceBefore);
} else {
assertEq(user2TokenBalanceAfter, 1e18);
}
}
}

Impact

The user will not be able to claim the previously divided nft, nor will the user be able to transfer the pegged Erc20 Tokens, the user will not be able to even sell the pegged ERC20 tokens, also the user will not be able to create a buy order for the nft shares.
Thus the nft shares are lost with any access to the buyer and seller.

Tools Used

Manual Review

Recommended Mitigation

There are 2 simple mitigations

  1. Restrict the entire protocol to work on only one token id of a given family of nft (nftAddress)
    by converting the mapping(address nft => ERC20Info) nftToErc20Info mapping to
    a simple struct instance of ERC20Info like this ERC20Info nftToErc20Info

  2. Extend the protocol to support multiple token ids of the same nft family
    by converting the mapping(address nft => ERC20Info) nftToErc20Info mapping to
    a mapping(address nft => mapping(uint256 tokenId => ERC20Info)) nftToErc20Info;
    and referencing the pegged ERC20 info by using tokenId the second key.

Updates

Lead Judging Commences

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