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

collateralForMinting not reset after collectUsdcFromSelling()

Author Revealed upon completion

Description

  • Under the intended flow, minting locks lockAmount USDC into the contract and records it in collateralForMinting[tokenId]. When a sale is later settled through collectUsdcFromSelling(), that collateral should be returned once as part of the seller’s payout and then cleared, because the collateral for that sale has already been consumed. The README also describes the protocol as “collecting base price/collateral on minting,” which indicates this value is part of the token’s economic accounting rather than a reusable bonus for every resale.

  • The issue is that collectUsdcFromSelling() reads collateralForMinting[listing.tokenId] and adds it into amountToSeller, but it never resets that mapping entry afterward. As a result, after the first successful sale claim, the same lockAmount remains recorded forever. On the next resale of the same NFT, the new seller receives that same collateral again even though they never deposited it, creating a phantom collateral payout. Depending on surrounding balances, this either overpays the secondary seller or causes later accounting/withdrawal failures because the contract has effectively paid out funds that were not meant for that seller.

function mintNft() external payable onlyWhenRevealed onlyWhitelisted {
...
require(usdc.transferFrom(msg.sender, address(this), lockAmount), "USDC transfer failed");
tokenIdCounter++;
collateralForMinting[tokenIdCounter] = lockAmount; // @> collateral recorded on mint
_safeMint(msg.sender, tokenIdCounter);
}
​
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; // @> collateral is paid out here
usdc.safeTransfer(address(this), fees);
usdc.safeTransfer(msg.sender, amountToSeller);
​
// @> BUG: missing collateralForMinting[listing.tokenId] = 0;
}
​
function cancelListing(uint256 _listingId) external {
...
usdc.safeTransfer(listing.seller, collateralForMint

Risk

Likelihood: High

  • This occurs whenever an NFT is sold once, the seller calls collectUsdcFromSelling(), and the same NFT is later resold, because collateralForMinting[tokenId] remains unchanged after the first claim.

  • This occurs during normal marketplace usage because resale is a core intended flow, while nothing in the contract requires the next seller to post fresh collateral before they receive the old token’s recorded lockAmount.

Impact: High

  • Secondary sellers can receive a phantom 20 USDC collateral payout even though they never locked that collateral themselves, leading to overpayment and draining balances that should remain as fees or reserves.

  • Because totalFeesCollected continues to increase while phantom collateral is paid out, later fee withdrawals or seller settlements can fail due to insufficient contract balance, leaving accounting inconsistent and potentially blocking honest participants.

Proof of Concept

  • Add import {console2} from "forge-std/console2.sol"; at the top of NFTDealersTest.t.sol.

  • Copy the code below to NFTDealersTest contract.

  • Run command forge test --mt testCollateralForMintingNotResetAfterCollectUsdcFromSelling -vv --via-ir.

function testCollateralForMintingNotResetAfterCollectUsdcFromSelling() public revealed whitelisted {
uint256 tokenId = 1;
uint32 firstSalePrice = 1000e6;
uint32 secondSalePrice = 1000e6;
​
address secondBuyer = makeAddr("secondBuyer");
usdc.mint(secondBuyer, 200_000e6);
​
// Whitelist userWithEvenMoreCash so they can relist after becoming the owner.
// This is only needed because the current implementation incorrectly gates list() by onlyWhitelisted.
vm.prank(owner);
nftDealers.whitelistWallet(userWithEvenMoreCash);
​
uint256 feeFirstSale = nftDealers.calculateFees(firstSalePrice);
uint256 feeSecondSale = nftDealers.calculateFees(secondSalePrice);
uint256 expectedFirstSellerPayout = uint256(firstSalePrice) + nftDealers.lockAmount() - feeFirstSale;
uint256 expectedSecondSellerPayoutWithPhantomCollateral =
uint256(secondSalePrice) + nftDealers.lockAmount() - feeSecondSale;
uint256 expectedSecondSellerPayoutWithoutPhantomCollateral = uint256(secondSalePrice) - feeSecondSale;
​
// ------------------------------------------------------------
// Phase 1: original seller mints and sells token #1
// ------------------------------------------------------------
mintAndListNFTForTesting(tokenId, firstSalePrice);
​
vm.startPrank(userWithEvenMoreCash);
usdc.approve(address(nftDealers), firstSalePrice);
nftDealers.buy(tokenId);
vm.stopPrank();
​
vm.prank(userWithCash);
nftDealers.collectUsdcFromSelling(tokenId);
​
uint256 collateralAfterFirstCollect = nftDealers.collateralForMinting(tokenId);
uint256 firstSellerBalanceAfterCollect = usdc.balanceOf(userWithCash);
uint256 contractBalanceAfterFirstCollect = usdc.balanceOf(address(nftDealers));
​
console2.log("feeFirstSale:", feeFirstSale);
console2.log("expectedFirstSellerPayout:", expectedFirstSellerPayout);
console2.log("firstSellerBalanceAfterCollect:", firstSellerBalanceAfterCollect);
console2.log("contractBalanceAfterFirstCollect:", contractBalanceAfterFirstCollect);
console2.log("collateralAfterFirstCollect:", collateralAfterFirstCollect);
​
// Core bug precondition:
// collateral should have been consumed by the first claim, but it remains recorded.
assertEq(firstSellerBalanceAfterCollect, expectedFirstSellerPayout, "sanity: first seller collected once");
assertEq(
contractBalanceAfterFirstCollect, feeFirstSale, "sanity: only first-sale fee remains after first collect"
);
assertEq(
collateralAfterFirstCollect,
nftDealers.lockAmount(),
"BUG: collateralForMinting[tokenId] was not cleared after successful collect"
);
​
// ------------------------------------------------------------
// Phase 2: the buyer becomes seller #2 and relists the SAME token #1
// ------------------------------------------------------------
vm.prank(userWithEvenMoreCash);
nftDealers.list(tokenId, secondSalePrice);
​
(address relistedSeller, uint32 relistedPrice,, uint256 relistedTokenId, bool relistedIsActive) =
nftDealers.s_listings(tokenId);
​
console2.log("relistedSeller:", relistedSeller);
console2.log("relistedPrice:", uint256(relistedPrice));
console2.log("relistedTokenId:", relistedTokenId);
console2.log("relistedIsActive:", relistedIsActive ? uint256(1) : uint256(0));
​
assertEq(relistedSeller, userWithEvenMoreCash, "sanity: buyer is now the relisting seller");
assertEq(relistedPrice, secondSalePrice, "sanity: second listing price stored");
assertTrue(relistedIsActive, "sanity: second listing is active");
​
// ------------------------------------------------------------
// Phase 3: a fresh buyer purchases token #1 from seller #2
// ------------------------------------------------------------
vm.startPrank(secondBuyer);
usdc.approve(address(nftDealers), secondSalePrice);
nftDealers.buy(tokenId);
vm.stopPrank();
​
uint256 secondSellerBalanceBeforeCollect = usdc.balanceOf(userWithEvenMoreCash);
uint256 contractBalanceBeforeSecondCollect = usdc.balanceOf(address(nftDealers));
​
console2.log("feeSecondSale:", feeSecondSale);
console2.log("secondSellerBalanceBeforeCollect:", secondSellerBalanceBeforeCollect);
console2.log("contractBalanceBeforeSecondCollect:", contractBalanceBeforeSecondCollect);
console2.log(
"expectedSecondSellerPayoutWithPhantomCollateral:", expectedSecondSellerPayoutWithPhantomCollateral
);
console2.log(
"expectedSecondSellerPayoutWithoutPhantomCollateral:", expectedSecondSellerPayoutWithoutPhantomCollateral
);
​
// After the second buy, the contract holds:
// old fee from first sale + second buyer's payment = 10e6 + 1000e6 = 1010e6
assertEq(
contractBalanceBeforeSecondCollect,
feeFirstSale + uint256(secondSalePrice),
"sanity: second collect is funded only by pooled balance, not by fresh collateral"
);
​
// ------------------------------------------------------------
// Phase 4: seller #2 collects and improperly receives the old collateral again
// ------------------------------------------------------------
vm.prank(userWithEvenMoreCash);
nftDealers.collectUsdcFromSelling(tokenId);
​
uint256 secondSellerBalanceAfterCollect = usdc.balanceOf(userWithEvenMoreCash);
uint256 contractBalanceAfterSecondCollect = usdc.balanceOf(address(nftDealers));
uint256 collateralAfterSecondCollect = nftDealers.collateralForMinting(tokenId);
​
console2.log("secondSellerBalanceAfterCollect:", secondSellerBalanceAfterCollect);
console2.log("contractBalanceAfterSecondCollect:", contractBalanceAfterSecondCollect);
console2.log("collateralAfterSecondCollect:", collateralAfterSecondCollect);
​
// Strong exploit signal:
// seller #2 receives 1010e6 even though they never locked 20e6 collateral for token #1.
assertEq(
secondSellerBalanceAfterCollect,
secondSellerBalanceBeforeCollect + expectedSecondSellerPayoutWithPhantomCollateral,
"BUG: secondary seller received phantom collateral on resale"
);
​
// For comparison, without the bug they should only receive price - fee.
assertTrue(
secondSellerBalanceAfterCollect
!= secondSellerBalanceBeforeCollect + expectedSecondSellerPayoutWithoutPhantomCollateral,
"BUG: payout should not include old collateral on secondary sale"
);
​
// The phantom payout drains the entire contract balance in this scenario.
assertEq(
contractBalanceAfterSecondCollect,
0,
"BUG: phantom collateral payout consumed balances that should not belong to the secondary seller"
);
​
// The stale collateral entry is STILL not cleared even after the second collect.
assertEq(
collateralAfterSecondCollect,
nftDealers.lockAmount(),
"BUG: collateral mapping remains stale even after repeated sale settlements"
);
}

Output:

[⠊] Compiling...
No files changed, compilation skipped
​
Ran 1 test for test/NFTDealersTest.t.sol:NFTDealersTest
[PASS] testCollateralForMintingNotResetAfterCollectUsdcFromSelling() (gas: 650153)
Logs:
feeFirstSale: 10000000
expectedFirstSellerPayout: 1010000000
firstSellerBalanceAfterCollect: 1010000000
contractBalanceAfterFirstCollect: 10000000
collateralAfterFirstCollect: 20000000
relistedSeller: 0x533575789af8F38A73C7747E36C17C1835FDF44a
relistedPrice: 1000000000
relistedTokenId: 1
relistedIsActive: 1
feeSecondSale: 10000000
secondSellerBalanceBeforeCollect: 199000000000
contractBalanceBeforeSecondCollect: 1010000000
expectedSecondSellerPayoutWithPhantomCollateral: 1010000000
expectedSecondSellerPayoutWithoutPhantomCollateral: 990000000
secondSellerBalanceAfterCollect: 200010000000
contractBalanceAfterSecondCollect: 0
collateralAfterSecondCollect: 20000000
​
Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 4.68ms (1.79ms CPU time)
​
Ran 1 test suite in 13.88ms (4.68ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Recommended Mitigation

  • Clear the collateral mapping in the successful sale-collection path, exactly as the contract already does in cancelListing().

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];
​
+ collateralForMinting[listing.tokenId] = 0;
totalFeesCollected += fees;
amountToSeller += collateralToReturn;
​
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!