NFTDealers::buy does not follow CEI pattern allowing reentrancy to buy NFT multiple times
Description
-
buy allows any user to purchase a listed NFT by paying the listing price in USDC. The function transfers USDC from the buyer, sends the NFT to the buyer, and then marks the listing as inactive.
-
The function marks listing.isActive = false AFTER calling _safeTransfer. Since _safeTransfer triggers onERC721Received on the buyer if it is a contract, an attacker can reenter buy during this callback while isActive is still true, draining USDC from the contract by purchasing the same listing multiple times.
function buy(uint256 _listingId) external payable {
Listing memory listing = s_listings[_listingId];
if (!listing.isActive) revert ListingNotActive(_listingId);
require(listing.seller != msg.sender, "Seller cannot buy their own NFT");
activeListingsCounter--;
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
Risk
Likelihood: Medium
Any buyer deploying a contract with a malicious onERC721Received
hook can trigger this — no special access or whitelist required
The attack is straightforward to execute and profitable whenever
the contract holds USDC from other listings or collateral
Impact: High
Attacker drains USDC from the contract by triggering multiple purchases of the same listing in a single transaction
activeListingsCounter is decremented on every reentrant call causing underflow and permanently corrupting protocol accounting
Other sellers lose their funds as contract balance is drained
Proof of Concept
The following attacker contract reads isActive inside onERC721Received— if CEI was followed it would be false at this point. The assert reverts proving isActive is still true during the callback, confirming the vulnerability.
contract BuyReentrancyAttacker is IERC721Receiver {
NFTDealers nftDealers;
MockUSDC usdc;
uint256 listingId;
bool attacked = false;
constructor(address _nftDealers, address _usdc) {
nftDealers = NFTDealers(_nftDealers);
usdc = MockUSDC(_usdc);
}
function attack(uint256 _listingId, uint256 _price) external {
listingId = _listingId;
usdc.approve(address(nftDealers), type(uint256).max);
nftDealers.buy(_listingId);
}
function onERC721Received(
address,
address,
uint256,
bytes calldata
) external returns (bytes4) {
(,,,, bool isActive) = nftDealers.s_listings(listingId);
assert(!isActive);
return IERC721Receiver.onERC721Received.selector;
}
}
function test_BuyReentrancy() public revealed whitelisted() {
BuyReentrancyAttacker buyAttacker = new BuyReentrancyAttacker(
address(nftDealers),
address(usdc)
);
uint256 tokenId = 1;
uint256 nftPrice = 1000e6;
mintAndListNFTForTesting(tokenId, nftPrice);
usdc.mint(address(buyAttacker), nftPrice);
vm.expectRevert();
buyAttacker.attack(tokenId, nftPrice);
console.log("isActive was still true during _safeTransfer callback");
console.log("s_listings[1].isActive = false comes AFTER the external call");
}
Recommended Mitigation
Move isActive = false above _safeTransfer to follow CEI, and add
OpenZeppelin ReentrancyGuard as a second layer of protection:
+import {ReentrancyGuard} from "@openzeppelin/contracts/utils/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");
+ // EFFECTS first
+ s_listings[_listingId].isActive = false;
activeListingsCounter--;
+ // INTERACTIONS last
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
- s_listings[_listingId].isActive = false;
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
