Bid Beasts
First Flight #49
Beginner FriendlyFoundrySolidityNFT
100 EXP
View results
Previous Next
Submission Details
Severity: low
Valid

CEI pattern not followed, the event is emitted in wrong situations

farnad

Root + Impact

Description

  • The BidBeastsNFTMarket::AuctionSettledExplain event is not emitted when the bidder has paid more than or equal to the BidBeastsNFTMarket::Listing::buyNowPrice. In fact, it is located at a wrong position in the function.

if (listing.buyNowPrice > 0 && msg.value >= listing.buyNowPrice) {
uint256 salePrice = listing.buyNowPrice;
uint256 overpay = msg.value - salePrice;
​
// EFFECT: set winner bid to exact sale price (keep consistent)
bids[tokenId] = Bid(msg.sender, salePrice);
listing.listed = false;
if (previousBidder != address(0)) {
_payout(previousBidder, previousBidAmount);
}
​
// NOTE: using internal finalize to do transfer/payouts. _executeSale will assume bids[tokenId] is the final winner.
_executeSale(tokenId);
​
// Refund overpay (if any) to buyer
if (overpay > 0) {
_payout(msg.sender, overpay);
}
​
return;
}
require(msg.sender != previousBidder, "Already highest bidder");
@> emit AuctionSettled(tokenId, msg.sender, listing.seller, msg.value);
​

Risk

Likelihood:

  • It happens when the user pays more than or equal to the BidBeastsNFTMarket::Listing::buyNowPrice

Impact:

  • It may impact the external (off-chain) services which rely on the events for bookkeeping, etc.

Proof of Concept

Please copy the code into the test file.

The following test function call is supposed to emit the BidBeastsNFTMarket::AuctionSettledExplain event. However, due to the aforementioned issue, it does not.

contract BidBeastsNFTMarketTest is Test {
+ event AuctionSettled(uint256 tokenId, address winner, address seller, uint256 price);
.
.
.
​
+ function test_PayBuyNowPriceDoesNotEmitEvent() public {
+ _mintNFT();
+ _listNFT();
+ vm.startPrank(BIDDER_1);
+ emit AuctionSettled(TOKEN_ID, msg.sender, SELLER, BUY_NOW_PRICE);
+ vm.expectEmit(false, false, false, false);
+ market.placeBid{value: BUY_NOW_PRICE}(TOKEN_ID);
+ vm.stopPrank();
+ }
​

Recommended Mitigation

You should move the emit instruction to the proper location (inside the if statement, and before the return).

if (listing.buyNowPrice > 0 && msg.value >= listing.buyNowPrice) {
uint256 salePrice = listing.buyNowPrice;
uint256 overpay = msg.value - salePrice;
​
// EFFECT: set winner bid to exact sale price (keep consistent)
bids[tokenId] = Bid(msg.sender, salePrice);
listing.listed = false;
if (previousBidder != address(0)) {
_payout(previousBidder, previousBidAmount);
}
​
// NOTE: using internal finalize to do transfer/payouts. _executeSale will assume bids[tokenId] is the final winner.
_executeSale(tokenId);
​
// Refund overpay (if any) to buyer
if (overpay > 0) {
_payout(msg.sender, overpay);
}
+ emit AuctionSettled(tokenId, msg.sender, listing.seller, msg.value);
return;
}
require(msg.sender != previousBidder, "Already highest bidder");
- emit AuctionSettled(tokenId, msg.sender, listing.seller, msg.value);
​
Updates

Lead Judging Commences

cryptoghost Lead Judge about 1 month ago
Submission Judgement Published
Validated
Assigned finding tags:

BidBeasts Marketplace: Incorrect Event Emission

placeBid emits AuctionSettled even though the auction hasn’t ended, causing misleading event logs.

Support

FAQs

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