Bidder can be changed when bid is not the biggest

[c4-bot-8]

Lines of code

https://github.com/code-423n4/2023-12-revolutionprotocol/blob/main/packages/revolution/src/AuctionHouse.sol#L180-L183

Vulnerability details

Proof of Concept

AuctionHouse is configured with reservePrice and minBidIncrementPercentage params adn both of them can be 0. While reservePrice is unlikely will be 0, minBidIncrementPercentage can be for sure, as minBidIncrementPercentage == 0 means that auction owner doesn't care what is the min price increment, he only wants each bid to be bigger than previous.

createBid function is used to provide your bid. Several criterias should be met. Bid should be not smaller than reservePrice and bid should be not less than increment previous bid.

In case if minBidIncrementPercentage is set to 0, then it will be enough for next bidder to just send same amount of funds that was sent by previous bidder and he will get the nft.

Impact

Bidder can provide same amount and get the nft.

Tools Used

VsCode

Recommended Mitigation Steps

Make sure that current bid is bigger than previous one.

Assessed type

Error

[c4-pre-sort]

raymondfam marked the issue as sufficient quality report

[c4-pre-sort]

raymondfam marked the issue as primary issue

[c4-pre-sort]

raymondfam marked the issue as insufficient quality report

[c4-pre-sort]

raymondfam marked the issue as duplicate of #197

[c4-judge]

MarioPoneder changed the severity to QA (Quality Assurance)

[c4-judge]

MarioPoneder marked the issue as grade-c

[rvierdiiev]

I would like the judge to review this issue one more time. While root issue #197 is talking about malicious owner, this report doesn't talk about any malicious owner, but it says about using minBidIncrementPercentage == 0 as normal param for the auction. In the report i have described why it's possible that minBidIncrementPercentage == 0 will be used and i think it's realistic that such situation will occur.

[MarioPoneder]

Thank you for your comment!

Although I appreciate the validity of this concern, it requires the owner/DAO to misconfigure the parameter to minBidIncrementPercentage = 0. There is no incentive to do that, but it's a valid QA suggestion to enforce limits in the setter method to avoid accidental misconfiguration.

[c4-judge]

MarioPoneder marked the issue as grade-b