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

Cross-Function Reentrancy Across `buy`, `cancelListing`, and `collectUsdcFromSelling`

Author Revealed upon completion

Root + Impact

Multiple functions perform external calls before finalising state updates, violating the Checks-Effects-Interactions (CEI) pattern. A malicious token with transfer hooks can re-enter the contract and drain funds or manipulate the listing state.

Description

  • buy() calls _safeTransfer before setting s_listings[_listingId].isActive = false, allowing a reentrant call to purchase the same listing again

  • cancelListing() calls usdc.safeTransfer before zeroing collateralForMinting, allowing a reentrant call to drain collateral multiple times

  • collectUsdcFromSelling() has no reentrancy guard and makes two consecutive external safeTransfer calls with no state finalization in between

  • No ReentrancyGuard is applied anywhere in the contract

// buy() — isActive is set AFTER the external NFT transfer
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
// @audit reentrant call can re-enter buy() here before isActive = false
s_listings[_listingId].isActive = false;

Risk

Likelihood:

  • Requires a malicious or hook-enabled ERC20 token, or a buyer contract that implements onERC721Received to re-enter

  • More likely in permissionless or upgradeable token deployments

Impact:

  • Attacker can buy the same NFT listing multiple times before isActive is set to false

  • Attacker can drain collateral from cancelListing by re-entering before collateralForMinting is zeroed

  • Contract USDC balance can be fully drained in a single transaction

Proof of Concept

Since _safeTransfer calls onERC721Received on the recipient before isActive is set to false, an attacker can deploy a contract that re-enters buy() inside that callback. Each reentrant call sees the listing as still active and successfully purchases the NFT again, draining USDC from the contract on every iteration until the balance is exhausted.

// Attacker contract implementing onERC721Received
function onERC721Received(address, address, uint256, bytes calldata) external returns (bytes4) {
// Re-enter buy() before isActive is set to false
nftDealers.buy(listingId);
return this.onERC721Received.selector;
}

Recommended Mitigation

Two complementary fixes should be applied together. First, inherit OpenZeppelin's ReentrancyGuard and apply nonReentrant to all functions that make external calls. Second, restructure each affected function to follow the CEI pattern — all state changes must complete before any external call is made. This ensures that even if a reentrant call is attempted, the state will already reflect the completed operation and the call will revert.

+ import {ReentrancyGuard} from "@openzeppelin/contracts/security/ReentrancyGuard.sol";
​
- contract NFTDealers is ERC721 {
+ contract NFTDealers is ERC721, ReentrancyGuard {
​
- function buy(uint256 _listingId) external payable {
+ function buy(uint256 _listingId) external payable nonReentrant {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
+ s_listings[_listingId].isActive = false;
+ activeListingsCounter--;
usdc.safeTransferFrom(msg.sender, address(this), listing.price);
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
- s_listings[_listingId].isActive = false;
- activeListingsCounter--;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}

Support

FAQs

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

Give us feedback!