Seller cannot collect USDC from selling if buyer immediately relists
Description
After selling an NFT, the seller calls NFTDealers::collectUsdcFromSellingto receive their USDC. This function requires that the caller be the seller listed in the listing, using the onlySellermodifier.
Because the _listingIdfield is simply the NFT's id, if the new holder of the NFT immediately relists it, the original listing is overwritten. This means that the sellerfield is set to the new holder, and the original seller cannot receive their money.
function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
require(_price >= MIN_PRICE, "Price must be at least 1 USDC");
require(ownerOf(_tokenId) == msg.sender, "Not owner of NFT");
require(s_listings[_tokenId].isActive == false, "NFT is already listed");
require(_price > 0, "Price must be greater than 0");
listingsCounter++;
activeListingsCounter++;
@> s_listings[_tokenId] =
Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});
emit NFT_Dealers_Listed(msg.sender, listingsCounter);
}
@> function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) { ... }
Risk
Likelihood:
Impact:
Proof of Concept
The following test illustrates the issue.
function testCannotCollectUsdcFromSellingIfBuyerImmediatelyRelists() public revealed {
uint256 tokenId = 1;
uint32 nftPrice = 1000e6;
vm.prank(owner);
nftDealers.whitelistWallet(userWithEvenMoreCash);
mintAndListNFTForTesting(tokenId, nftPrice);
vm.startBroadcast(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(1);
nftDealers.list(1, nftPrice);
vm.stopBroadcast();
vm.prank(userWithCash);
vm.expectRevert("Only seller can call this function");
nftDealers.collectUsdcFromSelling(tokenId);
}
Recommended Mitigation
The _listingId should be unique. Since a single NFT should be listed more than once, as long as it is listing only once at a time, tokenIdis insufficient.
+ mapping(uint256 => bool) public s_nftHasActiveListing;
function list(uint256 _tokenId, uint32 _price) external onlyWhitelisted {
require(_price >= MIN_PRICE, "Price must be at least 1 USDC");
require(ownerOf(_tokenId) == msg.sender, "Not owner of NFT");
- require(s_listings[_tokenId].isActive == false, "NFT is already listed");
+ require(s_nftHasActiveListing[_tokenId] == false, "NFT is already listed");
require_price > 0, "Price must be greater than 0");
listingsCounter++;
activeListingsCounter++;
+ s_nftHasActiveListing[_tokenId] = true;
+ s_listings[listingsCounter] =
+ Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});
- s_listings[_tokenId] =
- Listing({seller: msg.sender, price: _price, nft: address(this), tokenId: _tokenId, isActive: true});
emit NFT_Dealers_Listed(msg.sender, listingsCounter);
}
