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

`NFTDealers::buy()` violates CEI pattern via `_safeTransfer()` enabling reentrancy

Author Revealed upon completion

NFTDealers::buy() violates CEI pattern via _safeTransfer() enabling reentrancy

Description

`buy()` violates CEI pattern, `isActive` set after `_safeTransfer()` creating a state inconsistency window, where a malicious ERC721 receiver contract can observe stale state during `onERC721Received()` callback

function buy(uint256 _listingId) external payable {
// ...
usdc.transferFrom(msg.sender, address(this), listing.price);
_safeTransfer(listing.seller, msg.sender, listing.tokenId, ""); // @> callback here
​
s_listings[_listingId].isActive = false; // @> state updated too late
}

Risk

A malicious buyer contract can re-enter `NFTDealers::buy()` during `onERC721Received()` callback. However, since the NFT has already transferred to the attacker on the first call, the second `_safeTransfer()` will revert as the seller is no longer the owner. The attacker ends up paying double for the same NFT — so the practical impact is self-harm rather than protocol drain. Severity is reduced to Medium due to low likelihood of successful exploitation.

Likelihood: Low

  • Attacker contract must be whitelisted

Impact: Low

  • Nothing violates on protocol

Proof of Concept

contract BuyAttacker {
NFTDealers target;
IERC20 usdc;
uint256 targetListingId;
bool attacked;
​
constructor(address _target, address _usdc) {
target = NFTDealers(_target);
usdc = IERC20(_usdc);
}
​
function attack(uint256 _listingId) external {
targetListingId = _listingId;
// approve enough USDC for 2x price (double payment scenario)
usdc.approve(address(target), type(uint256).max);
target.buy(_listingId);
}
​
function onERC721Received(
address operator,
address from,
uint256 tokenId,
bytes calldata data
) external returns (bytes4) {
// isActive is still true here — re-enter buy()
if (!attacked) {
attacked = true;
target.buy(targetListingId); // re-enter with same listingId
// ⚠️ this will revert on _safeTransfer
// because seller no longer owns the NFT
// attacker has paid listing.price twice
}
return this.onERC721Received.selector;
}
}
​
// Test
function test_buyReentrant() public whitelisted revealed {
BuyAttacker attacker = new BuyAttacker(address(nftDealers), address(usdc));
​
usdc.mint(address(attacker), 40e6);
​
// User mint and listing
vm.startPrank(userWithCash);
usdc.approve(address(nftDealers), type(uint256).max);
​
nftDealers.mintNft();
​
nftDealers.list(1, 1e6);
vm.stopPrank();
​
vm.expectRevert();
attacker.attack(1);
}

Attack flow:

  1. Deploy BuyAttacker, fund with 2x listing.price USDC

  2. Call BuyAttacker::attack(listingId)

  3. First buy() USDC transferred, NFT transferred to BuyAttacker, callback triggered

  4. Inside onERC721Received isActive still true, re-enter buy()

  5. Second `buy()` USDC transferred again, `_safeTransfer` reverts (seller no longer owner)

  6. Entire second re-entry reverts, but first call already succeeded

  7. Net result: attacker paid listing.price once, owns NFT no gain, no protocol loss

Recommended Mitigation

Follow CEI pattern update `isActive` before any external calls:

function buy(uint256 _listingId) external {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
​
// Effects first
+ s_listings[_listingId].isActive = false;
+ activeListingsCounter--;
​
// Interactions last
+ usdc.transferFrom(msg.sender, address(this), listing.price);
+ _safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
​
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!