Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 37 additions & 6 deletions contracts/EnglishAuction.sol
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';
* @notice Auction contract for NFT and token auctions, where the highest bidder wins the auction and rest of the bidders get their bid refunded.
*/
contract EnglishAuction is Auction {
constructor (address _protocolParametersAddress) Auction(_protocolParametersAddress){}
constructor(address _protocolParametersAddress) Auction(_protocolParametersAddress) {}
mapping(uint256 => AuctionData) public auctions;
struct AuctionData {
uint256 id;
Expand Down Expand Up @@ -65,6 +65,8 @@ contract EnglishAuction is Auction {
uint256 deadlineExtension
) external nonEmptyString(name) nonZeroAddress(auctionedToken) nonZeroAddress(biddingToken) {
require(duration > 0, 'Duration must be greater than zero seconds');
require(minimumBid > 0, "minimumBid must be > 0");
require(minBidDelta > 0, 'minBidDelta must be > 0');
receiveFunds(auctionType == AuctionType.NFT, auctionedToken, msg.sender, auctionedTokenIdOrAmount);
uint256 deadline = block.timestamp + duration;
auctions[auctionCounter] = AuctionData({
Expand All @@ -87,23 +89,52 @@ contract EnglishAuction is Auction {
isClaimed: false,
protocolFee: protocolParameters.fee()
});
emit AuctionCreated(auctionCounter++, name, description, imgUrl, msg.sender, auctionType, auctionedToken, auctionedTokenIdOrAmount, biddingToken, minimumBid, minBidDelta, deadline, deadlineExtension, protocolParameters.fee());
emit AuctionCreated(
auctionCounter++,
name,
description,
imgUrl,
msg.sender,
auctionType,
auctionedToken,
auctionedTokenIdOrAmount,
biddingToken,
minimumBid,
minBidDelta,
deadline,
deadlineExtension,
protocolParameters.fee()
);
}

function bid(uint256 auctionId, uint256 bidAmount) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) {
AuctionData storage auction = auctions[auctionId];
require(auction.highestBid != 0 || bidAmount >= auction.minimumBid, 'First bid should be greater than starting bid');
require(auction.highestBid == 0 || bidAmount >= auction.highestBid + auction.minBidDelta, 'Bid amount should exceed current bid by atleast minBidDelta');

// 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');
}
Comment on lines +113 to +119
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.


require(auction.minBidDelta > 0, 'Invalid minBidDelta');
Comment on lines +113 to +121
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


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.availableFunds = bidAmount;

auction.deadline += auction.deadlineExtension;

emit bidPlaced(auctionId, msg.sender, bidAmount);
}

Expand All @@ -114,7 +145,7 @@ contract EnglishAuction is Auction {
uint256 fees = (auction.protocolFee * withdrawAmount) / 10000;
address feeRecipient = protocolParameters.treasury();
sendERC20(auction.biddingToken, auction.auctioneer, withdrawAmount - fees);
sendERC20(auction.biddingToken,feeRecipient,fees);
sendERC20(auction.biddingToken, feeRecipient, fees);
emit Withdrawn(auctionId, withdrawAmount);
}

Expand Down
15 changes: 15 additions & 0 deletions contracts/mocks/ERC20Mock.sol
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.28;

import "@openzeppelin/contracts/token/ERC20/ERC20.sol";

contract ERC20Mock is ERC20 {
constructor(
string memory name,
string memory symbol,
address initialAccount,
uint256 initialBalance
) ERC20(name, symbol) {
_mint(initialAccount, initialBalance);
}
}
100 changes: 100 additions & 0 deletions test/EnglishAuction.bidding.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
const { expect } = require('chai');
const { ethers } = require('hardhat');

describe('EnglishAuction - Bidding Invariants', function () {
let auction;
let owner, seller, bidder1, bidder2;
let token;
let protocol;

beforeEach(async function () {
[owner, seller, bidder1, bidder2] = await ethers.getSigners();

const MockERC20 = await ethers.getContractFactory('ERC20Mock');
token = await MockERC20.deploy('MockToken', 'MTK', owner.address, ethers.parseEther('1000000'));

const ProtocolMock = await ethers.getContractFactory('ProtocolParameters');
protocol = await ProtocolMock.deploy(owner.address, owner.address, 500);

const EnglishAuction = await ethers.getContractFactory('EnglishAuction');
auction = await EnglishAuction.deploy(protocol.target);

// Give seller tokens
await token.transfer(seller.address, ethers.parseEther('1000'));

// Give bidders tokens
await token.transfer(bidder1.address, ethers.parseEther('1000'));
await token.transfer(bidder2.address, ethers.parseEther('1000'));

// Seller must approve for escrow
await token.connect(seller).approve(auction.target, ethers.parseEther('1000'));

// Bidders approve for bidding
await token.connect(bidder1).approve(auction.target, ethers.parseEther('1000'));

await token.connect(bidder2).approve(auction.target, ethers.parseEther('1000'));
});

async function createAuction(minBidDelta = ethers.parseEther('1')) {
await auction.connect(seller).createAuction(
'Test',
'Test Desc',
'img',
1, // Token auction
token.target,
ethers.parseEther('100'), // amount escrowed (mock)
token.target,
ethers.parseEther('10'), // minimumBid
minBidDelta,
3600,
0,
);
}

it('1️⃣ First bid must be >= minimumBid', async function () {
await createAuction();

await expect(auction.connect(bidder1).bid(0, ethers.parseEther('5'))).to.be.revertedWith('Bid below minimum');

await expect(auction.connect(bidder1).bid(0, ethers.parseEther('10'))).to.not.be.reverted;
});

it('2️⃣ Subsequent bid must be >= highestBid + minBidDelta', async function () {
await createAuction(ethers.parseEther('2'));

await auction.connect(bidder1).bid(0, ethers.parseEther('10'));

await expect(auction.connect(bidder2).bid(0, ethers.parseEther('11'))).to.be.revertedWith('Bid increment too low');

await expect(auction.connect(bidder2).bid(0, ethers.parseEther('12'))).to.not.be.reverted;
});

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;
});
Comment on lines +72 to +78
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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.


it('4️⃣ minBidDelta == 0 should revert', async function () {
await expect(createAuction(0)).to.be.revertedWith('minBidDelta must be > 0');
});
it("5️⃣ minimumBid == 0 should revert", async function () {
await expect(
auction.connect(seller).createAuction(
"Test",
"Test Desc",
"img",
1,
token.target,
ethers.parseEther("100"),
token.target,
0, // minimumBid = 0
ethers.parseEther("1"),
3600,
0
)
).to.be.revertedWith("minimumBid must be > 0");
});
});