Division-first calculation in placeBid causes precision loss due to truncation, enabling bids that violate the 5% minimum increment rule

Description

• This contract makes sure that each new bid must be at least 5% higher than the previous one. The logic for this is implemented in the BidBeastsNFTMarketPlace::placeBid function (lines 156-157):

  uint256 constant public S_MIN_BID_INCREMENT_PERCENTAGE = 5;

  ...

  ​

  // lines 156-157 in placeBid

  @> requiredAmount = (previousBidAmount / 100) * (100 + S_MIN_BID_INCREMENT_PERCENTAGE);

  require(msg.value >= requiredAmount, "Bid not high enough");
  
  

• The issue here is that the calculation (previousBidAmount / 100) * (100 + S_MIN_BID_INCREMENT_PERCENTAGE) uses division first, which truncates (integer division) and loses precision. This means that for certain bid amounts, the calculated requiredAmount can be lower than the intended 5% increase.

  
  

• There's a simple rule the calculations should follow - Always Multiply Before Dividing. Although many developers follow this rule, "Hidden Precision Loss" can still occur, resulting from complex calculations across different functions or contracts. But that's not the case here, as the calculation is straightforward.

  // Bad

  uint256 result = (a / b) * c; // Division first, can lose precision

  // Better

  uint256 result = (a * c) / b; // Multiplication first, preserves precision
  
  

  • When we divide first, the fractional part is discarded completely. For example, if a = 5, b = 3, and c = 6, the bad approach gives (5/3) * 6 = 1 * 6 = 6 (lost precision)

  • The good approach maintains precision longer (correct (5 * 6) / 3 = 30 / 3 = 10 result).

  
  

• Hence, the correct way to calculate the minimum required bid should be:

  requiredAmount = (previousBidAmount * (100 + S_MIN_BID_INCREMENT_PERCENTAGE)) / 100;

Risk

Likelihood: Medium

• Occurs with certain bid amounts (not all)

• More likely with smaller bids or awkward decimal amounts (e.g., 1.200000000000099999 ETH)

Impact: Medium

• Minor Financial Loss: Financial loss is typically small (a few wei to small amounts).

• Cumulative Effect: Small losses may accumulate over multiple auctions.

• Rule Violation: Undermines the contract’s 5% bid increment requirement.

Proof of Concept

• Add the following test test_bidIncrementPrecisionLoss in the test file:

  function test_bidIncrementPrecisionLoss() public {

  // Mint and list NFT

  _mintNFT();

  _listNFT(); // MIN_PRICE = 1 ether

  ​

  // Setting up an awkward bid amount that can cause precision loss

  uint256 bid_amount = 1.200000000000099999 ether;

  ​

  // Place first bid

  vm.prank(BIDDER_1);

  market.placeBid{value: bid_amount}(TOKEN_ID);

  ​

  (, uint256 amount) = market.bids(TOKEN_ID);

  console.log("First bid placed:", amount);

  ​

  // Calculating next minimum bids

  uint256 incorrectNextBidAmount = (amount / 100) * (100 + market.S_MIN_BID_INCREMENT_PERCENTAGE()); // S_MIN_BID_INCREMENT_PERCENTAGE = 5

  console.log("Incorrect next min bid (due to precision loss):", incorrectNextBidAmount);

  ​

  uint256 correctNextBidAmount = (amount * (100 + market.S_MIN_BID_INCREMENT_PERCENTAGE())) / 100;

  console.log("Correct next min bid (with actual 5% increase):", correctNextBidAmount);

  ​

  // Calculating difference between the two

  uint256 difference = correctNextBidAmount - incorrectNextBidAmount;

  console.log("Difference due to precision loss:", difference, "wei"); // Should be small but non-zero

  ​

  // 2nd bidder can easily place a bid with less than 5% increase due to this precision loss

  vm.prank(BIDDER_2);

  market.placeBid{value: incorrectNextBidAmount}(TOKEN_ID);

  ​

  (, amount) = market.bids(TOKEN_ID);

  console.log(); // just to give a space in console

  console.log("Placing second bid with incorrect min bid amount...");

  console.log("Second bid placed:", amount);

  assertLt(incorrectNextBidAmount, correctNextBidAmount, "Second bid is less than required 5% increase");

  }
  
  

• Run the above test using the command:

  forge test --mt test_bidIncrementPrecisionLoss -vv
  
  

• The output we get:

  Ran 1 test for test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest

  [PASS] test_bidIncrementPrecisionLoss() (gas: 326643)

  Logs:

  First bid placed: 1200000000000099999

  Incorrect next min bid (due to precision loss): 1260000000000104895

  Correct next min bid (with actual 5% increase): 1260000000000104998

  Difference due to precision loss: 103 wei

  ​

  Placing second bid with incorrect min bid amount...

  Second bid placed: 1260000000000104895

  ​

  Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.02ms (244.80µs CPU time)

Recommended Mitigation

Simply change the calculation to multiply first, then divide: