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

[L-2] Division before multiplication in the calculation of the `requiredAmount` in the `BidBeastsNFTMarketPlace::placeBid` function leads to precision loss

cyberethdev

[L-2] Division before multiplication in the calculation of the requiredAmount in the BidBeastsNFTMarketPlace::placeBid function leads to precision loss

Description

  • Normal behaviour: In the NFT auction marketplace, every bid has to be higher compared to the previous bid by BidBeastsNFTMarketPlace::S_MIN_BID_INCREMENT_PERCENTAGE.

  • Problematic behaviour: To calculate the minimum new bid required, the previousBidAmount is first divided by 100 and then multiplied by (100 + S_MIN_BID_INCREMENT_PERCENTAGE). The division by 100 leads to precision loss as the remainder of integer division is truncated in Solidity.

Root cause:

...
@> requiredAmount = (previousBidAmount / 100) * (100 + S_MIN_BID_INCREMENT_PERCENTAGE);
require(msg.value >= requiredAmount, "Bid not high enough");
...
​

Risk

Likelihood: High

  • Precision loss occurs whenever previousBidAmount % 100 != 0.

Impact: Low

  • The impact is low as the maximum precision loss in this case is max division remainder * (100 + S_MIN_BID_INCREMENT_PERCENTAGE) = 99 * 105 = 104 wei, which is 0.00000000000104 % of the minimum allowed bid amount.

Proof of Concept

As a PoC add the following Fuzz test to the Foundry test suite and run with forge test --fuzz-runs 10000 --mt testFuzz_requiredAmountLosesPrecision_FuzzedPreviousAmount:

function testFuzz_requiredAmountLosesPrecision_FuzzedPreviousAmount(uint256 previousBidAmount) public view {
uint256 minPrice = 0.01 ether;
uint256 bidIncrementPercentage = market.S_MIN_BID_INCREMENT_PERCENTAGE();
// Assume previous bid on the NFT exists and is higher than the min price
vm.assume(previousBidAmount > minPrice);
// Prevent overflow
previousBidAmount = bound(previousBidAmount, minPrice, type(uint256).max / (100 + bidIncrementPercentage));
// Assume previousBidAmount is *not* evenly divisible by 100
vm.assume(previousBidAmount % 100 != 0);
​
// Calculate the requiredAmount to place a new bid with the current formula (division before multiplication)
uint256 requiredAmountCurrent = (previousBidAmount / 100) * (100 + bidIncrementPercentage);
// Calculate the requiredAmount to place a new bid with multiplication before division
uint256 requiredAmountNew = previousBidAmount * (100 + bidIncrementPercentage) / 100;
assertTrue(requiredAmountCurrent != requiredAmountNew, "There should be differences due to precision loss");
assertTrue(requiredAmountCurrent < requiredAmountNew, "Current required amount value should lose precision");
}

Recommended Mitigation

To mitigate the vulnerability multiply first before dividing:

...
-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");
...
Updates

Lead Judging Commences

cryptoghost Lead Judge 2 months ago
Submission Judgement Published
Validated
Assigned finding tags:

BidBeasts Marketplace: Integer Division Precision Loss

Integer division in requiredAmount truncates fractions, allowing bids slightly lower than intended.

Support

FAQs

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

Give us feedback!