Root + Impact
Description
Calaculating the minimum Bid increese must me exact 5% from the previous.
Currently there is loss of precision issue due to not correct calculations of requiredAmount in placeBid function
function placeBid(uint256 tokenId) external payable isListed(tokenId) {
More code...
if (previousBidAmount == 0) {
requiredAmount = listing.minPrice;
require(msg.value > requiredAmount, "First bid must be > min price");
listing.auctionEnd = block.timestamp + S_AUCTION_EXTENSION_DURATION;
emit AuctionExtended(tokenId, listing.auctionEnd);
} else {
@> requiredAmount = (previousBidAmount / 100) * (100 + S_MIN_BID_INCREMENT_PERCENTAGE);
require(msg.value >= requiredAmount, "Bid not high enough");
uint256 timeLeft = 0;
if (listing.auctionEnd > block.timestamp) {
timeLeft = listing.auctionEnd - block.timestamp;
}
if (timeLeft < S_AUCTION_EXTENSION_DURATION) {
listing.auctionEnd = listing.auctionEnd + S_AUCTION_EXTENSION_DURATION;
emit AuctionExtended(tokenId, listing.auctionEnd);
}
}
More code...
}
Risk
Likelihood:
It will happen every time there is when there is decimal that solidity cannot handle
In the lifetime of a Auction it can stack multiple times
Impact:
consistent loss of percision in some cases making the bidding possible with less that exactly 5% increment.
Proof of Concept
The test shows how the wrong calculation affect the price of the bid
The problematic way of calculating compared to the correct way.
Added comments so it can be easier to read and understand.
function test_takeHighestBidPrecisionPoC() public {
uint256 minBidIncrementPercentage = market.S_MIN_BID_INCREMENT_PERCENTAGE();
_mintNFT();
_listNFT();
uint256 newBid = MIN_PRICE + 199999999999999999;
vm.prank(BIDDER_1);
market.placeBid{value: newBid}(TOKEN_ID);
uint256 secondBidAmount = (newBid * (100 + minBidIncrementPercentage)) / 100;
vm.prank(BIDDER_2);
market.placeBid{value: secondBidAmount}(TOKEN_ID);
CALCULATIONS
BidBeastsNFTMarket.Bid memory highestBid = market.getHighestBid(TOKEN_ID);
uint256 wrongCalculation = (highestBid.amount / 100) * (100 + minBidIncrementPercentage);
uint256 correctCalculation = (highestBid.amount * (100 + minBidIncrementPercentage)) / 100;
console.log("Wrong Calculation:", wrongCalculation);
console.log("Correct Calculation:", correctCalculation);
console.log("Difference:", correctCalculation - wrongCalculation);
CALCULATIONS
uint256 thirdBidAmount = (secondBidAmount * (100 + minBidIncrementPercentage) / 100) - 102 wei;
vm.prank(BIDDER_3);
market.placeBid{value: thirdBidAmount}(TOKEN_ID);
}
Recommended Mitigation
Multiplication must happent before division.
After replacing the line the PoC test should fail! And it needs to remove the - 102 wei part to run correctly just like the new calculations.
function placeBid(uint256 tokenId) external payable isListed(tokenId) {
More code...
if (previousBidAmount == 0) {
requiredAmount = listing.minPrice;
require(msg.value > requiredAmount, "First bid must be > min price");
listing.auctionEnd = block.timestamp + S_AUCTION_EXTENSION_DURATION;
emit AuctionExtended(tokenId, listing.auctionEnd);
} else {
- requiredAmount = (previousBidAmount / 100) * (100 + S_MIN_BID_INCREMENT_PERCENTAGE);
+ requiredAmount = (previousBidAmount * (100 + S_MIN_BID_INCREMENT_PERCENTAGE)) / 100;
require(msg.value >= requiredAmount, "Bid not high enough");
uint256 timeLeft = 0;
if (listing.auctionEnd > block.timestamp) {
timeLeft = listing.auctionEnd - block.timestamp;
}
if (timeLeft < S_AUCTION_EXTENSION_DURATION) {
listing.auctionEnd = listing.auctionEnd + S_AUCTION_EXTENSION_DURATION;
emit AuctionExtended(tokenId, listing.auctionEnd);
}
}
More code...
}
