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

Reentrancy risk

sammy0x100

Root + Impact

Description

  • The 'buy' and 'cancelListings' functions do exactly as the function names say they

  • They however do not follow the Checks-Effects-Interactions pattern which could lead to possible reentrancy attacks. They perform external calls—specifically _safeTransfer (NFT) and usdc.safeTransfer (ERC20)—before updating the internal state variable s_listings[_listingId].isActive = false.

// Root cause in the codebase with @> marks to highlight the relevant section

Risk

Likelihood:

This occurs when a malicious contract calls the 'buy' and 'cancelListings' functions

Impact:

  • Asset theft

Proof of Concept

Preparation: An attacker deploys a MaliciousBuyer contract.
Step 1: The attacker calls buy(1) via their contract.
Step 2: Your contract calls _safeTransfer. Because it's a "safe" transfer, your contract checks if the MaliciousBuyer can receive NFTs by calling its onERC721Received function.
Step 3 (The Reentrancy): Inside onERC721Received, the attacker's contract calls buy(1) again.
Step 4: Because your code hasn't reached the line s_listings[_listingId].isActive = false yet, the second call passes the isActive check.
The Result: Depending on the logic, the attacker could potentially trigger multiple transfers, mess up the activeListingsCounter, or drain collateral if the cancelListing function is targeted similarly.
​

Recommended Mitigation

​
Always update the internal state before interacting with external addresses
'''solidity 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");
​
// 1. EFFECT: Update state FIRST
s_listings[_listingId].isActive = false;
activeListingsCounter--;
​
// 2. INTERACTION: External calls LAST
bool success = usdc.transferFrom(msg.sender, address(this), listing.price);
require(success, "USDC transfer failed");
_safeTransfer(listing.seller, msg.sender, listing.tokenId, "");
​
emit NFT_Dealers_Sold(msg.sender, listing.price);
}
'''
​
or use a Reentrancy Guard like OpenZepellin's or a lock modifier
​
Updates

Lead Judging Commences

rubik0n Lead Judge 7 months ago
Submission Judgement Published
Validated
Assigned finding tags:

reentrancy-risk

This is a real reentrancy point, but not an exploitable one. The transaction will revert on the second ERC721 transfer.

Appeal created

rubik0n Lead Judge 6 months ago
Submission Judgement Published
Invalidated
Reason: Incorrect statement
Assigned finding tags:

Invalid

Support

FAQs

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

Give us feedback!