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

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

thecil

Root + Impact

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

Description

The BidBeastsNFTMarket::listNFT function is exposed to reentrancy because it calls an external contract (BBERC721.transferFrom) before updating internal state, 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 function, we can see that the transferFrom call is made before updating the internal state.

function listNFT(
uint256 tokenId,
uint256 _minPrice,
uint256 _buyNowPrice
) external {
require(BBERC721.ownerOf(tokenId) == msg.sender, "Not the owner");
require(_minPrice >= S_MIN_NFT_PRICE, "Min price too low");
if (_buyNowPrice > 0) {
require(
_minPrice <= _buyNowPrice,
"Min price cannot exceed buy now price"
);
}
​
@> BBERC721.transferFrom(msg.sender, address(this), tokenId);
​
@> listings[tokenId] = Listing({
seller: msg.sender,
minPrice: _minPrice,
buyNowPrice: _buyNowPrice,
auctionEnd: 0, // Timer starts only after the first valid bid.
listed: true
});
​
emit NftListed(tokenId, msg.sender, _minPrice, _buyNowPrice);
}

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 listNFT(
uint256 tokenId,
uint256 _minPrice,
uint256 _buyNowPrice
) external {
// checks
require(BBERC721.ownerOf(tokenId) == msg.sender, "Not the owner");
require(_minPrice >= S_MIN_NFT_PRICE, "Min price too low");
if (_buyNowPrice > 0) {
require(
_minPrice <= _buyNowPrice,
"Min price cannot exceed buy now price"
);
}
// effects
+ listings[tokenId] = Listing({
+ seller: msg.sender,
+ minPrice: _minPrice,
+ buyNowPrice: _buyNowPrice,
+ auctionEnd: 0, // Timer starts only after the first valid bid.
+ listed: true
+ });
​
+ emit NftListed(tokenId, msg.sender, _minPrice, _buyNowPrice);
​
- BBERC721.transferFrom(msg.sender, address(this), tokenId);
// interactions
- listings[tokenId] = Listing({
- seller: msg.sender,
- minPrice: _minPrice,
- buyNowPrice: _buyNowPrice,
- auctionEnd: 0, // Timer starts only after the first valid bid.
- listed: true
- });
​
- emit NftListed(tokenId, msg.sender, _minPrice, _buyNowPrice);
​
+ BBERC721.transferFrom(msg.sender, address(this), tokenId);
}

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: Incorrect statement

Support

FAQs

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

Give us feedback!