Fix/english auction bid invariants#61
Open
Rav1Chauhan wants to merge 3 commits intoStabilityNexus:mainfrom
Open
Fix/english auction bid invariants#61Rav1Chauhan wants to merge 3 commits intoStabilityNexus:mainfrom
Rav1Chauhan wants to merge 3 commits intoStabilityNexus:mainfrom
Conversation
WalkthroughEnforces positive Changes
Sequence Diagram(s)sequenceDiagram
participant Bidder
participant ERC20
participant EnglishAuction
participant PrevBidder
Bidder->>ERC20: approve(spender=EnglishAuction, amount)
Bidder->>EnglishAuction: createAuction(...) / (seller deploys)
Bidder->>EnglishAuction: bid(amount)
EnglishAuction->>ERC20: transferFrom(Bidder, contract, amount)
alt previous highest exists
EnglishAuction->>PrevBidder: refund(previousAmount)
EnglishAuction->>EnglishAuction: set availableFunds = amount
EnglishAuction->>EnglishAuction: update highestBid, highestBidder
else first bid
EnglishAuction->>EnglishAuction: require amount >= minimumBid
EnglishAuction->>EnglishAuction: set availableFunds = amount
EnglishAuction->>EnglishAuction: set highestBid, highestBidder
end
EnglishAuction-->>Bidder: emit BidPlaced / revert if validations fail
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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: 3
🤖 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 67-68: There are two identical validation lines checking duration:
the duplicate require(duration > 0, 'Duration must be greater than zero
seconds'); removes unnecessary gas; delete the redundant require and keep a
single instance of require(duration > 0, 'Duration must be greater than zero
seconds') in the function/constructor that validates the auction duration (where
that require appears) so only one check remains.
- Around line 113-119: Replace the fragile "first bid" sentinel that checks
auction.highestBid == 0 with an explicit auctioneer sentinel (check
auction.auctioneer == address(0)) so a zero-value first bid doesn't keep
treating the auction as bidless; specifically, in the bidding logic (where
auction.highestBid, auction.minimumBid and auction.minBidDelta are used) change
the branch to require(bidAmount >= auction.minimumBid) when auction.auctioneer
== address(0), otherwise require(bidAmount >= auction.highestBid +
auction.minBidDelta) for subsequent bids and then update auction.auctioneer
appropriately.
In `@test/EnglishAuction.bidding.test.js`:
- Around line 38-82: Update the test helper createAuction to accept a
configurable minimumBid (currently hardcoded to ethers.parseEther('10')) in
addition to minBidDelta, then add a new test that creates an auction with
minimumBid == 0 and verifies first bids are subject to minBidDelta enforcement:
call createAuction( /* minBidDelta */, /* minimumBid = 0 */ ) and assert that a
first bid lower than minBidDelta reverts (or is rejected) while a bid >=
minBidDelta succeeds; reference the helper createAuction and the
auction.bid(...) calls to locate where to change tests and assertions.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
contracts/EnglishAuction.solcontracts/mocks/ERC20Mock.soltest/EnglishAuction.bidding.test.js
Comment on lines
+113
to
+119
| // 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'); | ||
| } |
There was a problem hiding this comment.
First-bid detection can be bypassed when minimumBid is 0.
With highestBid == 0 as the sole sentinel, a zero first bid leaves highestBid at 0, so subsequent bids are still treated as “first bids” and can ignore minBidDelta. That reintroduces equal-bid replacement and spam risk when minimumBid is zero.
Proposed fix (use auctioneer sentinel)
- if (auction.highestBid == 0) {
+ if (auction.highestBid == 0 && auction.winner == auction.auctioneer) {
require(bidAmount >= auction.minimumBid, 'Bid below minimum');
} else {
// Enforce strict increment
require(bidAmount >= auction.highestBid + auction.minBidDelta, 'Bid increment too low');
}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 113 - 119, Replace the fragile
"first bid" sentinel that checks auction.highestBid == 0 with an explicit
auctioneer sentinel (check auction.auctioneer == address(0)) so a zero-value
first bid doesn't keep treating the auction as bidless; specifically, in the
bidding logic (where auction.highestBid, auction.minimumBid and
auction.minBidDelta are used) change the branch to require(bidAmount >=
auction.minimumBid) when auction.auctioneer == address(0), otherwise
require(bidAmount >= auction.highestBid + auction.minBidDelta) for subsequent
bids and then update auction.auctioneer appropriately.
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 113-121: Remove the redundant runtime check
require(auction.minBidDelta > 0, 'Invalid minBidDelta') from the bid handling
logic because minBidDelta is already validated during createAuction and never
mutated afterwards; simply delete that require in the function that processes
bids (the block referencing auction.highestBid, auction.minimumBid,
auction.minBidDelta) to save gas while leaving the earlier validation in
createAuction intact.
In `@test/EnglishAuction.bidding.test.js`:
- Around line 72-78: The test currently asserts a generic revert for
auction.connect(bidder2).bid(0, ethers.parseEther('10')); update this to assert
the explicit revert reason from the contract (e.g., replace .to.be.reverted with
.to.be.revertedWith('<REVERT_REASON>')) so the failure proves the bid-increment
rule triggered; locate the assertion in the '3️⃣ Equal bid should revert' test
that follows createAuction(...) and the prior auction.connect(bidder1).bid(...)
call and substitute the correct revert string from the auction contract.
Comment on lines
+113
to
+121
| // 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.
🧹 Nitpick | 🔵 Trivial
Remove redundant minBidDelta runtime check to save gas.
minBidDelta is already validated in createAuction and is not mutated afterward, so this per-bid require adds cost without improving safety.
♻️ Suggested change
- require(auction.minBidDelta > 0, 'Invalid minBidDelta');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/EnglishAuction.sol` around lines 113 - 121, Remove the redundant
runtime check require(auction.minBidDelta > 0, 'Invalid minBidDelta') from the
bid handling logic because minBidDelta is already validated during createAuction
and never mutated afterwards; simply delete that require in the function that
processes bids (the block referencing auction.highestBid, auction.minimumBid,
auction.minBidDelta) to save gas while leaving the earlier validation in
createAuction intact.
Comment on lines
+72
to
+78
| it('3️⃣ Equal bid should revert', async function () { | ||
| await createAuction(ethers.parseEther('1')); | ||
|
|
||
| await auction.connect(bidder1).bid(0, ethers.parseEther('10')); | ||
|
|
||
| await expect(auction.connect(bidder2).bid(0, ethers.parseEther('10'))).to.be.reverted; | ||
| }); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Make the revert reason explicit for clarity.
This ensures the failure is due to the bid-increment rule, not another precondition.
✅ Suggested change
- await expect(auction.connect(bidder2).bid(0, ethers.parseEther('10'))).to.be.reverted;
+ await expect(auction.connect(bidder2).bid(0, ethers.parseEther('10'))).to.be.revertedWith('Bid increment too low');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/EnglishAuction.bidding.test.js` around lines 72 - 78, The test currently
asserts a generic revert for auction.connect(bidder2).bid(0,
ethers.parseEther('10')); update this to assert the explicit revert reason from
the contract (e.g., replace .to.be.reverted with
.to.be.revertedWith('<REVERT_REASON>')) so the failure proves the bid-increment
rule triggered; locate the assertion in the '3️⃣ Equal bid should revert' test
that follows createAuction(...) and the prior auction.connect(bidder1).bid(...)
call and substitute the correct revert string from the auction contract.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addressed Issues:
Fixes #50
This PR strengthens the bidding logic of
EnglishAuctionby enforcing strictminimum bid increment rules and adds comprehensive invariant tests.
The change prevents equal-value bid replacement, griefing, and unnecessary
refund churn.
Problem
Ascending auctions allowed potential bid replacement without meaningful
economic increase if misconfigured.
This could enable:
Solution
Contract Changes
minimumBidhighestBid + minBidDeltarequire(minBidDelta > 0, "minBidDelta must be > 0");
Tests Added
New file:
test/EnglishAuction.bidding.test.js
Covers the following invariants:
All tests pass successfully.
Impact
"Verify that any modification to contract logic includes corresponding updates to automated tests."
Testing
npx hardhat test
All tests passing.
No new warnings or errors.
Screenshots/Recordings
N/A — Smart contract logic update only.
Additional Notes
This PR affects only
EnglishAuctionlogic and related tests.No changes were made to other auction types.
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
Bug Fixes
Tests