Bid Beasts
First Flight #49
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Impact: low
Likelihood: low
Invalid

[L-4] - `BidBeastsNFTMarket::_executeSale` function should follow CEI Pattern to Avoid Reentrancy Risk.

thecil

Root + Impact

[L-4] - BidBeastsNFTMarket::_executeSale function should follow CEI Pattern to Avoid Reentrancy Risk.

Description

The BidBeastsNFTMarket::_executeSale function is exposed to reentrancy because it calls an external contract (BBERC721.transferFrom) before emitting the AuctionSettled event and call _payout to transfer the funds to the seller, allowing an attacker to execute malicious code and re-enter your contract while it’s in an inconsistent state.

Risk

Likelihood: Low.

Impact: Low.

Proof of Concept

This is the actual codebase of the _executeSale function:

function _executeSale(uint256 tokenId) internal {
Listing storage listing = listings[tokenId];
Bid memory bid = bids[tokenId];
​
listing.listed = false;
delete bids[tokenId];
​
@> BBERC721.transferFrom(address(this), bid.bidder, tokenId);
​
uint256 fee = (bid.amount * S_FEE_PERCENTAGE) / 100;
s_totalFee += fee;
uint256 sellerProceeds = bid.amount - fee;
@> _payout(listing.seller, sellerProceeds);
​
@> emit AuctionSettled(tokenId, bid.bidder, listing.seller, bid.amount);
}

Recommended Mitigation

To mitigate potential reentrancy risks, adhere to the CEI pattern by updating state variables (effects) before making any external calls (interactions). For instance:

function _executeSale(uint256 tokenId) internal {
Listing storage listing = listings[tokenId];
Bid memory bid = bids[tokenId];
​
listing.listed = false;
delete bids[tokenId];
+ emit AuctionSettled(tokenId, bid.bidder, listing.seller, bid.amount);
- BBERC721.transferFrom(address(this), bid.bidder, tokenId);
uint256 fee = (bid.amount * S_FEE_PERCENTAGE) / 100;
s_totalFee += fee;
uint256 sellerProceeds = bid.amount - fee;
_payout(listing.seller, sellerProceeds);
+ BBERC721.transferFrom(address(this), bid.bidder, tokenId);
- emit AuctionSettled(tokenId, bid.bidder, listing.seller, bid.amount);
}

Rearranging the code to follow the CEI pattern ensures that all relevant state changes are made before any interactions with external contracts, reducing the risk of reentrancy attacks and enhancing the overall security of the contract.

Updates

Lead Judging Commences

cryptoghost Lead Judge 2 months ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity

Support

FAQs

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

Give us feedback!