fix: enforce strict minimum bid increment in EnglishAuction#60
fix: enforce strict minimum bid increment in EnglishAuction#60Rav1Chauhan wants to merge 1 commit intoStabilityNexus:mainfrom
Conversation
WalkthroughThe bid function in EnglishAuction.sol has been restructured to enforce minimum bid increments through explicit branching, preventing equal-value bids by requiring either minimumBid for initial bids or highestBid plus minBidDelta for subsequent bids, while validating minBidDelta is non-zero. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@contracts/EnglishAuction.sol`:
- Around line 93-132: Add unit tests targeting the bid(uint256 auctionId,
uint256 bidAmount) flow to cover the new invariants: create an auction fixture
and write tests that (1) submit the first bid < minimumBid and assert it
reverts, then submit first bid ≥ minimumBid and assert success and bidPlaced
emitted; (2) after a successful first bid, attempt a subsequent bid < highestBid
+ minBidDelta and assert revert; (3) attempt a subsequent bid equal to
highestBid (or equal to highestBid + minBidDelta - 1) and assert revert to
verify equal bids are rejected; and (4) set auction.minBidDelta == 0 (or
deploy/create an auction with minBidDelta zero) and assert any bid reverts with
"Invalid minBidDelta". Use the contract function name bid and the AuctionData
fields minimumBid, highestBid, minBidDelta, and the bidPlaced event to locate
assertions; ensure tests also simulate ERC20 transfers (mock
receiveERC20/sendERC20 behavior) and check deadline extension side-effects where
relevant.
- Around line 100-115: Validate auction parameters inside the createAuction
function: add require checks that auction.minimumBid > 0 and auction.minBidDelta
> 0 (or the equivalent constructor/initializer arguments) to prevent creating
auctions with minimumBid == 0 or minBidDelta == 0; keep using auction.highestBid
as the sentinel for first bid but ensure these invariants are enforced at
creation so the bid logic in the bid function (comparisons against
auction.minimumBid, auction.highestBid, and auction.minBidDelta) cannot be
bypassed or create dead auctions.
| function bid(uint256 auctionId, uint256 bidAmount) | ||
| external | ||
| exists(auctionId) | ||
| beforeDeadline(auctions[auctionId].deadline) | ||
| { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
|
|
||
| // First bid must meet minimumBid | ||
| if (auction.highestBid == 0) { | ||
| require( | ||
| bidAmount >= auction.minimumBid, | ||
| "Bid below minimum" | ||
| ); | ||
| } else { | ||
| // Enforce strict increment | ||
| require( | ||
| bidAmount >= auction.highestBid + auction.minBidDelta, | ||
| "Bid increment too low" | ||
| ); | ||
| } | ||
|
|
||
| require(auction.minBidDelta > 0, "Invalid minBidDelta"); | ||
|
|
||
| receiveERC20(auction.biddingToken, msg.sender, bidAmount); | ||
|
|
||
| uint256 refund = auction.highestBid; | ||
| address previousWinner = auction.winner; | ||
|
|
||
| auction.winner = msg.sender; | ||
| auction.highestBid = bidAmount; | ||
| auction.availableFunds = bidAmount; | ||
|
|
||
| if (refund != 0) { | ||
| sendERC20(auction.biddingToken, previousWinner, refund); | ||
| } | ||
|
|
||
| auction.deadline += auction.deadlineExtension; | ||
|
|
||
| emit bidPlaced(auctionId, msg.sender, bidAmount); | ||
| } |
There was a problem hiding this comment.
Add tests for the new bidding invariants (first‑bid minimum, strict increment, and invalid minBidDelta).
This change alters core auction correctness and griefing protection but the PR adds no tests. Please add unit tests covering: (1) first bid ≥ minimumBid, (2) subsequent bid ≥ highestBid + minBidDelta, (3) equal bids revert, and (4) minBidDelta == 0 reverts.
As per coding guidelines: "Verify that any modification to contract logic includes corresponding updates to automated tests."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/EnglishAuction.sol` around lines 93 - 132, Add unit tests targeting
the bid(uint256 auctionId, uint256 bidAmount) flow to cover the new invariants:
create an auction fixture and write tests that (1) submit the first bid <
minimumBid and assert it reverts, then submit first bid ≥ minimumBid and assert
success and bidPlaced emitted; (2) after a successful first bid, attempt a
subsequent bid < highestBid + minBidDelta and assert revert; (3) attempt a
subsequent bid equal to highestBid (or equal to highestBid + minBidDelta - 1)
and assert revert to verify equal bids are rejected; and (4) set
auction.minBidDelta == 0 (or deploy/create an auction with minBidDelta zero) and
assert any bid reverts with "Invalid minBidDelta". Use the contract function
name bid and the AuctionData fields minimumBid, highestBid, minBidDelta, and the
bidPlaced event to locate assertions; ensure tests also simulate ERC20 transfers
(mock receiveERC20/sendERC20 behavior) and check deadline extension side-effects
where relevant.
| // First bid must meet minimumBid | ||
| if (auction.highestBid == 0) { | ||
| require( | ||
| bidAmount >= auction.minimumBid, | ||
| "Bid below minimum" | ||
| ); | ||
| } else { | ||
| // Enforce strict increment | ||
| require( | ||
| bidAmount >= auction.highestBid + auction.minBidDelta, | ||
| "Bid increment too low" | ||
| ); | ||
| } | ||
|
|
||
| require(auction.minBidDelta > 0, "Invalid minBidDelta"); | ||
|
|
There was a problem hiding this comment.
Validate minimumBid and minBidDelta at auction creation to avoid zero‑bid loopholes and dead auctions.
With the current sentinel (highestBid == 0), a zero bid (possible if minimumBid == 0) keeps highestBid at 0, so later bids never reach the increment path. Also, minBidDelta == 0 lets auctions be created that can never accept bids. Validate both parameters in createAuction so these invalid states can’t be instantiated.
✅ Suggested fix
function createAuction(
@@
) external nonEmptyString(name) nonZeroAddress(auctionedToken) nonZeroAddress(biddingToken) {
require(duration > 0, 'Duration must be greater than zero seconds');
+ require(minimumBid > 0, "Minimum bid must be > 0");
+ require(minBidDelta > 0, "Invalid minBidDelta");
receiveFunds(auctionType == AuctionType.NFT, auctionedToken, msg.sender, auctionedTokenIdOrAmount);As per coding guidelines: "Review for common smart contract vulnerabilities, including but not limited to: Improper input validation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/EnglishAuction.sol` around lines 100 - 115, Validate auction
parameters inside the createAuction function: add require checks that
auction.minimumBid > 0 and auction.minBidDelta > 0 (or the equivalent
constructor/initializer arguments) to prevent creating auctions with minimumBid
== 0 or minBidDelta == 0; keep using auction.highestBid as the sentinel for
first bid but ensure these invariants are enforced at creation so the bid logic
in the bid function (comparisons against auction.minimumBid, auction.highestBid,
and auction.minBidDelta) cannot be bypassed or create dead auctions.
1 participant
Addressed Issues:
Fixes #50
Screenshots/Recordings:
N/A — Smart contract logic update (no UI changes)
Additional Notes:
This PR strengthens the bidding logic in EnglishAuction by enforcing a strict minimum bid increment.
Problem
Previously, bidders could potentially submit a bid equal to the current highest bid (depending on configuration), which could:
Enable griefing behavior
Cause unnecessary refund churn
Create gas inefficiencies
Allow spam bidding patterns
Solution
Updated the bid() function to enforce:
First bid must be ≥ minimumBid
Subsequent bids must be ≥ highestBid + minBidDelta
Added validation to ensure minBidDelta > 0
This ensures strict ascending bid enforcement and prevents equal-value takeover bids.
Impact
Prevents DOS-like bidding behavior
Eliminates equal-bid replacement
Improves economic fairness
Reduces unnecessary refund operations
Testing
Contracts compile successfully with Hardhat
No new warnings or errors generated
Checklist
AI Usage Disclosure
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
We encourage contributors to use AI tools responsibly when creating Pull Requests. While AI can be a valuable aid, it is essential to ensure that your contributions meet the task requirements, build successfully, include relevant tests, and pass all linters. Submissions that do not meet these standards may be closed without warning to maintain the quality and integrity of the project. Please take the time to understand the changes you are proposing and their impact.
Summary by CodeRabbit