Added auction cancellation functionality and related tests across auctions#39
Open
VishwajeetTulse wants to merge 11 commits intoStabilityNexus:mainfrom
Open
Added auction cancellation functionality and related tests across auctions#39VishwajeetTulse wants to merge 11 commits intoStabilityNexus:mainfrom
VishwajeetTulse wants to merge 11 commits intoStabilityNexus:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds an AuctionCancelled event and cancelAuction(auctionId) API across multiple auction contracts; cancellation is restricted to the auctioneer when there are no bids/commits, marks auction claimed, sets deadline to now, returns assets/funds via existing send logic, and blocks further bids. Changes
Sequence Diagram(s)sequenceDiagram
participant Auctioneer as Auctioneer
participant Contract as AuctionContract
participant Asset as ERC20/ERC721
Auctioneer->>Contract: cancelAuction(auctionId)
Note right of Contract: validate exists(auctionId)\ncaller == auctioneer\nno bids/commits
Contract->>Contract: set isClaimed = true\nset deadline = block.timestamp
Contract->>Asset: sendFunds / transfer NFT back to auctioneer
Asset-->>Auctioneer: funds / NFT returned
Contract-->>Auctioneer: emit AuctionCancelled(auctionId, auctioneer)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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: 1
🤖 Fix all issues with AI agents
In `@contracts/VickreyAuction.sol`:
- Around line 147-154: The cancelAuction function allows the auctioneer to
cancel even after bidders have committed, which can strand commit fees; update
the AuctionData (or add a new commit counter/flag) to track whether any commits
have been made (e.g., totalCommits or hasCommits) and modify cancelAuction to
require that no commits exist before cancelling (use auction.totalCommits == 0
or !auction.hasCommits), or alternatively implement logic to iterate committed
bidders and refund their commit fees before returning the asset; ensure the
check or refund logic references cancelAuction, AuctionData, and whatever
commit-tracking storage (e.g., commit mapping or counter) so cancellations only
happen safely.
🧹 Nitpick comments (6)
test/AllPayAuction.test.ts (1)
474-474: Consider specifying the expected revert message for consistency.Other tests in this suite use
.to.be.revertedWith('...')to assert specific error messages. Using the generic.to.be.revertedhere makes the test less precise and could mask unexpected revert reasons.🔧 Suggested fix
- await expect(allPayAuction.connect(bidder1).bid(0, bidAmount)).to.be.reverted; + await expect(allPayAuction.connect(bidder1).bid(0, bidAmount)).to.be.revertedWith( + 'Auctioned asset has already been claimed', + );test/LogarithmicReverseDutchAuction.test.ts (1)
134-239: Missing test cases for full cancellation coverage.This test suite is missing several scenarios that are covered in
AllPayAuction.test.tsandEnglishAuction.test.ts:
- Token auction cancellation (verifying token refunds)
- Preventing double-cancellation (already cancelled auction)
- Preventing bids on cancelled auctions
Adding these would ensure consistent coverage across all auction types.
🧪 Suggested additional test cases
it('should allow auctioneer to cancel token auction before any bids', async function () { const amount = ethers.parseEther('10'); await mockToken.connect(auctioneer).approve(await logarithmicReverseDutchAuction.getAddress(), amount); await logarithmicReverseDutchAuction .connect(auctioneer) .createAuction( 'Token Auction', 'Test Description', 'https://example.com/test.jpg', 1, await mockToken.getAddress(), amount, await biddingToken.getAddress(), ethers.parseEther('10'), ethers.parseEther('1'), 20000, 10, ); const balanceBefore = await mockToken.balanceOf(await auctioneer.getAddress()); await logarithmicReverseDutchAuction.connect(auctioneer).cancelAuction(0); const balanceAfter = await mockToken.balanceOf(await auctioneer.getAddress()); expect(balanceAfter).to.equal(balanceBefore + amount); }); it('should not allow cancellation of already cancelled auction', async function () { await mockNFT.connect(auctioneer).approve(await logarithmicReverseDutchAuction.getAddress(), 1); await logarithmicReverseDutchAuction .connect(auctioneer) .createAuction( 'Test Auction', 'Test Description', 'https://example.com/test.jpg', 0, await mockNFT.getAddress(), 1, await biddingToken.getAddress(), ethers.parseEther('10'), ethers.parseEther('1'), 20000, 10, ); await logarithmicReverseDutchAuction.connect(auctioneer).cancelAuction(0); await expect(logarithmicReverseDutchAuction.connect(auctioneer).cancelAuction(0)).to.be.revertedWith( 'Auctioned asset has already been claimed', ); }); it('should not allow bidding on cancelled auction', async function () { await mockNFT.connect(auctioneer).approve(await logarithmicReverseDutchAuction.getAddress(), 1); await logarithmicReverseDutchAuction .connect(auctioneer) .createAuction( 'Test Auction', 'Test Description', 'https://example.com/test.jpg', 0, await mockNFT.getAddress(), 1, await biddingToken.getAddress(), ethers.parseEther('10'), ethers.parseEther('1'), 20000, 10, ); await logarithmicReverseDutchAuction.connect(auctioneer).cancelAuction(0); const currentPrice = await logarithmicReverseDutchAuction.getCurrentPrice(0); await biddingToken.connect(bidder1).approve(await logarithmicReverseDutchAuction.getAddress(), currentPrice); await expect(logarithmicReverseDutchAuction.connect(bidder1).bid(0)).to.be.revertedWith( 'Auctioned asset has already been claimed', ); });test/EnglishAuction.test.ts (1)
490-490: Consider specifying the expected revert message for consistency.Similar to the pattern used elsewhere in this file, specify the expected revert reason to make the test more precise.
🔧 Suggested fix
- await expect(englishAuction.connect(bidder1).bid(0, bidAmount)).to.be.reverted; + await expect(englishAuction.connect(bidder1).bid(0, bidAmount)).to.be.revertedWith( + 'Auctioned asset has already been claimed', + );test/ExponentialReverseDutchAuction.test.ts (1)
125-230: Missing test cases for full cancellation coverage.This test suite is missing several scenarios that are covered in other auction test files:
- Token auction cancellation (verifying token refunds)
- Preventing double-cancellation (already cancelled auction)
- Preventing bids on cancelled auctions
Adding these would ensure consistent coverage across all auction types.
🧪 Suggested additional test cases
it('should allow auctioneer to cancel token auction before any bids', async function () { const amount = ethers.parseEther('10'); await mockToken.connect(auctioneer).approve(await exponentialReverseDutchAuction.getAddress(), amount); await exponentialReverseDutchAuction .connect(auctioneer) .createAuction( 'Token Auction', 'Test Description', 'https://example.com/test.jpg', 1, await mockToken.getAddress(), amount, await biddingToken.getAddress(), ethers.parseEther('10'), ethers.parseEther('1'), 20000, 10, ); const balanceBefore = await mockToken.balanceOf(await auctioneer.getAddress()); await exponentialReverseDutchAuction.connect(auctioneer).cancelAuction(0); const balanceAfter = await mockToken.balanceOf(await auctioneer.getAddress()); expect(balanceAfter).to.equal(balanceBefore + amount); }); it('should not allow cancellation of already cancelled auction', async function () { await mockNFT.connect(auctioneer).approve(await exponentialReverseDutchAuction.getAddress(), 1); await exponentialReverseDutchAuction .connect(auctioneer) .createAuction( 'Test Auction', 'Test Description', 'https://example.com/test.jpg', 0, await mockNFT.getAddress(), 1, await biddingToken.getAddress(), ethers.parseEther('10'), ethers.parseEther('1'), 20000, 10, ); await exponentialReverseDutchAuction.connect(auctioneer).cancelAuction(0); await expect(exponentialReverseDutchAuction.connect(auctioneer).cancelAuction(0)).to.be.revertedWith( 'Auctioned asset has already been claimed', ); }); it('should not allow bidding on cancelled auction', async function () { await mockNFT.connect(auctioneer).approve(await exponentialReverseDutchAuction.getAddress(), 1); await exponentialReverseDutchAuction .connect(auctioneer) .createAuction( 'Test Auction', 'Test Description', 'https://example.com/test.jpg', 0, await mockNFT.getAddress(), 1, await biddingToken.getAddress(), ethers.parseEther('10'), ethers.parseEther('1'), 20000, 10, ); await exponentialReverseDutchAuction.connect(auctioneer).cancelAuction(0); const currentPrice = await exponentialReverseDutchAuction.getCurrentPrice(0); await biddingToken.connect(bidder1).approve(await exponentialReverseDutchAuction.getAddress(), currentPrice); await expect(exponentialReverseDutchAuction.connect(bidder1).bid(0)).to.be.revertedWith( 'Auctioned asset has already been claimed', ); });test/VickreyAuction.test.ts (2)
299-324: Test description doesn't match implementation.The test is named "should not allow cancellation after commit phase has started" but it fast-forwards 1100 seconds (past the 1000-second
bidCommitDuration), which tests the scenario after the commit phase has ended. The revert message "Cannot cancel: commit phase started or ended" suggests both scenarios should be blocked.Consider either:
- Renaming this test to clarify it tests post-commit-phase cancellation
- Adding a separate test that verifies cancellation is blocked once a commit is made (during the commit phase)
🧪 Suggested clarification and additional test
- it('should not allow cancellation after commit phase has started', async function () { + it('should not allow cancellation after commit phase has ended', async function () { await mockNFT.connect(auctioneer).approve(await vickreyAuction.getAddress(), 1); // ... rest of test }); + + it('should not allow cancellation once a commit is placed', async function () { + await mockNFT.connect(auctioneer).approve(await vickreyAuction.getAddress(), 1); + await vickreyAuction + .connect(auctioneer) + .createAuction( + 'Test Auction', + 'Test Description', + 'https://example.com/test.jpg', + 0, + await mockNFT.getAddress(), + 1, + await biddingToken.getAddress(), + ethers.parseEther('1'), + 1000, + 90000, + ethers.parseEther('0.001'), + ); + + // Place a commit during the commit phase + const bid = ethers.parseEther('5'); + const salt = ethers.encodeBytes32String('secret'); + const commitment = ethers.keccak256(ethers.AbiCoder.defaultAbiCoder().encode(['uint256', 'bytes32'], [bid, salt])); + await vickreyAuction.connect(bidder1).commitBid(0, commitment, { value: ethers.parseEther('0.001') }); + + await expect(vickreyAuction.connect(auctioneer).cancelAuction(0)).to.be.revertedWith( + 'Cannot cancel: commits exist', + ); + });
376-378: Consider specifying the expected revert message for consistency.Using the generic
.to.be.revertedmakes the test less precise. Since the auction is cancelled andisClaimedis set to true, the expected revert reason should be verifiable.🔧 Suggested fix
- await expect( - vickreyAuction.connect(bidder1).commitBid(0, commitment, { value: ethers.parseEther('0.001') }), - ).to.be.reverted; + await expect( + vickreyAuction.connect(bidder1).commitBid(0, commitment, { value: ethers.parseEther('0.001') }), + ).to.be.revertedWith('Auctioned asset has already been claimed');
kaneki003
reviewed
Feb 5, 2026
contracts/AllPayAuction.sol
Outdated
| } | ||
|
|
||
| function bid(uint256 auctionId, uint256 bidIncrement) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) { | ||
| function bid(uint256 auctionId, uint256 bidIncrement) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) notClaimed(auctions[auctionId].isClaimed) { |
Contributor
There was a problem hiding this comment.
not necessary since in AllPay asset can only be claimed after deadline so before deadline modifier is sufficient here.
kaneki003
reviewed
Feb 5, 2026
contracts/AllPayAuction.sol
Outdated
| emit Withdrawn(auctionId, withdrawAmount); | ||
| } | ||
|
|
||
| function cancelAuction(uint256 auctionId) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) notClaimed(auctions[auctionId].isClaimed) { |
Contributor
There was a problem hiding this comment.
here also notClaimed is unnecessary
Contributor
There was a problem hiding this comment.
Also i just realized checking beforeDeadline is also redundant here as and our only condition is no one has made a bid yet.
kaneki003
reviewed
Feb 5, 2026
contracts/AllPayAuction.sol
Outdated
| uint256 deadlineExtension, | ||
| uint256 protocolFee | ||
| ); | ||
| event AuctionCancelled(uint256 indexed auctionId, address indexed auctioneer); |
Contributor
There was a problem hiding this comment.
This event should be placed in abstract contract since it's same in all contracts.
kaneki003
reviewed
Feb 5, 2026
contracts/EnglishAuction.sol
Outdated
| } | ||
|
|
||
| function bid(uint256 auctionId, uint256 bidAmount) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) { | ||
| function bid(uint256 auctionId, uint256 bidAmount) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) notClaimed(auctions[auctionId].isClaimed) { |
Contributor
There was a problem hiding this comment.
not necessary here as well.
kaneki003
reviewed
Feb 5, 2026
contracts/EnglishAuction.sol
Outdated
| emit Withdrawn(auctionId, withdrawAmount); | ||
| } | ||
|
|
||
| function cancelAuction(uint256 auctionId) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) notClaimed(auctions[auctionId].isClaimed) { |
Contributor
There was a problem hiding this comment.
here also not necessary of using notClaimed
kaneki003
reviewed
Feb 5, 2026
contracts/VickreyAuction.sol
Outdated
| } | ||
|
|
||
| function commitBid(uint256 auctionId, bytes32 commitment) external payable exists(auctionId) beforeDeadline(auctions[auctionId].bidCommitEnd) { | ||
| function commitBid(uint256 auctionId, bytes32 commitment) external payable exists(auctionId) beforeDeadline(auctions[auctionId].bidCommitEnd) notClaimed(auctions[auctionId].isClaimed) { |
Contributor
There was a problem hiding this comment.
not necessary here as well since item cannot be claimed before commit period ends.
kaneki003
reviewed
Feb 5, 2026
contracts/VickreyAuction.sol
Outdated
| function cancelAuction(uint256 auctionId) external exists(auctionId) notClaimed(auctions[auctionId].isClaimed) { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
| require(msg.sender == auction.auctioneer, "Only auctioneer can cancel"); | ||
| require(block.timestamp < auction.bidCommitEnd, "Cannot cancel: commit phase started or ended"); |
Contributor
There was a problem hiding this comment.
here instead of checking time you should check that there are no commits in auction,otherwise someone has initiated the participation and in that case auction shall not be cancelled.
Contributor
|
@VishwajeetTulse replace beforeDeadline everywhere and our basic condition is to check that no one has initiated the participation in auction. |
… notClaimed modifier to prevent bids for cancelled auctions
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/VickreyAuction.test.ts`:
- Around line 299-324: The test titled "should not allow cancellation after
commit phase has started" is misnamed and missing a critical scenario; update
the existing case to reflect it fast-forwards past bidCommitEnd (rename to
"...after commit phase ends") and add a new test that creates an auction, has a
bidder call vickreyAuction.connect(bidder).commitBid(auctionId, commitment, {
value }), then assert that
vickreyAuction.connect(auctioneer).cancelAuction(auctionId) is reverted (expect
revert message matching the contract, e.g., 'Cannot cancel after commits');
reference vickreyAuction, commitBid, and cancelAuction to locate where to add
the new test and reuse the existing auction creation helper code and time window
handling.
🧹 Nitpick comments (2)
contracts/LinearReverseDutchAuction.sol (1)
112-121: Consider using thenotClaimedmodifier for cross-contract consistency.
EnglishAuction,AllPayAuction, andVickreyAuctionuse thenotClaimedmodifier oncancelAuction, while this contract (andLogarithmicReverseDutchAuction) use an inlinerequire. This causes inconsistent user-facing error messages: double-cancelling here reverts with "Deadline of auction reached" (frombeforeDeadline) while elsewhere it reverts with "Auctioned asset has already been claimed" (fromnotClaimed). Using the modifier uniformly improves predictability for integrators and frontend error handling.Suggested diff
- function cancelAuction(uint256 auctionId) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) { + function cancelAuction(uint256 auctionId) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) notClaimed(auctions[auctionId].isClaimed) { AuctionData storage auction = auctions[auctionId]; require(msg.sender == auction.auctioneer, "Only auctioneer can cancel"); require(auction.winner == auction.auctioneer, "Cannot cancel auction with bids"); - require(!auction.isClaimed, "Auctioned asset has already been claimed"); auction.isClaimed = true;contracts/LogarithmicReverseDutchAuction.sol (1)
172-181: Same modifier inconsistency asLinearReverseDutchAuction.This
cancelAuctionuses inlinerequire(!auction.isClaimed, ...)while other auction contracts use thenotClaimedmodifier. Consider unifying for consistent error messages and maintainability (see the comment onLinearReverseDutchAuction.solfor details).
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/VickreyAuction.test.ts (1)
492-569:⚠️ Potential issue | 🟠 MajorRemove duplicate
advanceTimeand "Reentrancy Regression Tests" block.Lines 414–491 already define
advanceTimeand the identical "Reentrancy Regression Tests" suite. This block is a verbatim copy-paste that redeclares the helper (flagged by Biome'snoRedeclarelint) and doubles the reentrancy test execution for no benefit.Proposed fix
-async function advanceTime(seconds: number) { - await ethers.provider.send('evm_increaseTime', [seconds]); - await ethers.provider.send('evm_mine', []); -} - - describe('Reentrancy Regression Tests', function () { - const BID_COMMIT_DURATION = 1000; - const BID_REVEAL_DURATION = 90000; - const COMMIT_FEE = ethers.parseEther('0.1'); - const BID_AMOUNT_1 = ethers.parseEther('10'); - const BID_AMOUNT_2 = ethers.parseEther('20'); - - async function createAuction() { - await mockNFT.connect(auctioneer).approve(vickreyAuction.getAddress(), 1); - await vickreyAuction.connect(auctioneer).createAuction( - 'Test NFT', - 'Test Description', - 'https://example.com/image.jpg', - 0, - await mockNFT.getAddress(), - 1, - await biddingToken.getAddress(), - ethers.parseEther('5'), - BID_COMMIT_DURATION, - BID_REVEAL_DURATION, - COMMIT_FEE - ); - return 0; - } - - async function commitBids(auctionId: number) { - const salt1 = ethers.id('salt1'); - const salt2 = ethers.id('salt2'); - - const commitment1 = ethers.solidityPackedKeccak256(['uint256', 'bytes32'], [BID_AMOUNT_1, salt1]); - const commitment2 = ethers.solidityPackedKeccak256(['uint256', 'bytes32'], [BID_AMOUNT_2, salt2]); - - await vickreyAuction.connect(bidder1).commitBid(auctionId, commitment1, { value: COMMIT_FEE }); - await vickreyAuction.connect(bidder2).commitBid(auctionId, commitment2, { value: COMMIT_FEE }); - - return { salt1, salt2 }; - } - - it('should update accumulatedCommitFee before external call in revealBid', async function () { - const auctionId = await createAuction(); - const { salt1 } = await commitBids(auctionId); - - await advanceTime(BID_COMMIT_DURATION + 1); - - const before = await vickreyAuction.auctions(auctionId); - expect(before.accumulatedCommitFee).to.equal(COMMIT_FEE * 2n); - await biddingToken.connect(bidder1).approve(vickreyAuction.getAddress(), BID_AMOUNT_1); - await vickreyAuction.connect(bidder1).revealBid(auctionId, BID_AMOUNT_1, salt1); - - const after = await vickreyAuction.auctions(auctionId); - expect(after.accumulatedCommitFee).to.equal(COMMIT_FEE); - }); - - it('should update accumulatedCommitFee to 0 before external call in withdraw', async function () { - const auctionId = await createAuction(); - const { salt1, salt2 } = await commitBids(auctionId); - - await advanceTime(BID_COMMIT_DURATION + 1); - - await biddingToken.connect(bidder1).approve(vickreyAuction.getAddress(), BID_AMOUNT_1); - await biddingToken.connect(bidder2).approve(vickreyAuction.getAddress(), BID_AMOUNT_2); - - await vickreyAuction.connect(bidder1).revealBid(auctionId, BID_AMOUNT_1, salt1); - await vickreyAuction.connect(bidder2).revealBid(auctionId, BID_AMOUNT_2, salt2); - - await advanceTime(BID_REVEAL_DURATION + 1); - - await vickreyAuction.connect(auctioneer).withdraw(auctionId); - - const after = await vickreyAuction.auctions(auctionId); - expect(after.accumulatedCommitFee).to.equal(0); - }); - });
🧹 Nitpick comments (1)
test/VickreyAuction.test.ts (1)
383-411: Test relies on an implementation-specific side-effect rather than an explicit cancellation guard.The test asserts that committing on a cancelled auction reverts with
"Deadline of auction reached", which means the contract blocks bids by manipulatingbidCommitEndrather than checking a dedicated cancellation flag. This is fragile — if the deadline mechanism changes, this test would silently stop covering the cancellation path.Consider also asserting against a more explicit revert reason (e.g.,
"Auction is cancelled") if/when the contract adds a dedicated guard, or at minimum add a comment in the test clarifying this coupling.
kaneki003
reviewed
Feb 8, 2026
contracts/VickreyAuction.sol
Outdated
| @@ -148,6 +148,7 @@ contract VickreyAuction is Auction { | |||
| AuctionData storage auction = auctions[auctionId]; | |||
| require(msg.sender == auction.auctioneer, "Only auctioneer can cancel"); | |||
| require(block.timestamp < auction.bidCommitEnd, "Cannot cancel: commit phase started or ended"); | |||
Contributor
There was a problem hiding this comment.
This is not required then,after checking commitFee is zero.So this line could be removed.
Contributor
|
@VishwajeetTulse not all the comments above are addressed.Could you look again into them. |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In `@contracts/ExponentialReverseDutchAuction.sol`:
- Around line 134-142: The cancelAuction function is missing the notClaimed
guard and can be called after claim(), enabling double-spend for token auctions;
update cancelAuction (in ExponentialReverseDutchAuction) to enforce the same
protection as LinearReverseDutchAuction by requiring the auction is not already
claimed (use the existing notClaimed modifier or add require(!auction.isClaimed,
...) at the top of cancelAuction) so that sendFunds cannot be invoked twice for
the same auction/winner; keep the rest of the logic (setting isClaimed,
deadline, sendFunds, emit AuctionCancelled) unchanged.
In `@contracts/LinearReverseDutchAuction.sol`:
- Around line 111-119: The cancelAuction function lacks a notClaimed guard
allowing double-spend; fix by preventing cancellation if auction.isClaimed is
true — either add the existing notClaimed modifier to cancelAuction's signature
or insert require(!auction.isClaimed, "Already claimed") near the start of
cancelAuction (after loading AuctionData and confirming auctioneer), so that
sendFunds cannot be executed twice; reference cancelAuction,
AuctionData.isClaimed, notClaimed modifier, exists modifier, and sendFunds when
making the change.
In `@contracts/LogarithmicReverseDutchAuction.sol`:
- Around line 171-179: The cancelAuction function is missing the notClaimed
guard, allowing a double-spend on ERC-20 auctions; update the function signature
for cancelAuction(uint256 auctionId) to include the notClaimed modifier (i.e.,
cancelAuction(uint256 auctionId) external exists(auctionId) notClaimed) so it
checks AuctionData.isClaimed before proceeding, ensuring AuctionData in
auctions[auctionId] cannot be cancelled/claimed twice; keep the existing body
(setting isClaimed, deadline, sendFunds, emitting AuctionCancelled).
In `@test/AllPayAuction.test.ts`:
- Around line 302-454: Add two tests exercising the notClaimed guard: (1)
"double-cancel (token auction)" — create a token auction via createAuction with
mockToken/ERC20, cancelAuction(0) once, then assert a second cancelAuction(0)
call reverts; (2) "claim then cancel" — create a token auction, advance time
past deadline, call claim(0) as the rightful claimer, then assert a subsequent
cancelAuction(0) call reverts. Reference cancelAuction, claim, createAuction,
and the auction index 0 (and mockToken approvals/balance checks) to locate where
to add these specs. Ensure approvals and time travel (evm_increaseTime/evm_mine)
mirror existing token-auction tests.
In `@test/ExponentialReverseDutchAuction.test.ts`:
- Around line 125-230: Add tests covering missing cancellation/edge cases: 1)
create a token-backed auction via createAuction (use biddingToken and token
contract address similar to NFT flow) and assert cancelAuction emits
AuctionCancelled and returns token to auctioneer; 2) ensure double-cancel
protection by calling cancelAuction twice on the same auction id and expecting
the second call to revert (match message used in contract); 3) simulate deadline
passing then call claim(auctionId) and afterwards assert
cancelAuction(auctionId) reverts (or behaves per contract) to cover
claim-then-cancel path; 4) after cancelling an auction, attempt bid(auctionId)
(after approving biddingToken and using getCurrentPrice) and assert bid reverts
to verify bids are blocked on cancelled auctions; mirror patterns/assertions
used in existing tests (e.g., AuctionCancelled event, auctions(0).isClaimed) for
consistency.
In `@test/LinearReverseDutchAuction.test.ts`:
- Around line 237-364: Add three unit tests in the "Auction Cancellation"
describe block to cover double-cancel, claim-then-cancel, and bid-after-cancel
flows: (1) Double-cancel: create a token auction via createAuction, call
cancelAuction once (assert AuctionCancelled emitted and token returned), then
call cancelAuction a second time and assert the call reverts (use
.to.be.reverted) to guard the notClaimed behavior; (2) Claim-then-cancel: create
a token auction, fast-forward past deadline, call claim() (assert
success/claimed state via auctions(0).isClaimed or event), then call
cancelAuction and assert it reverts (.to.be.reverted); (3) Bid-after-cancel:
create an auction, cancel it, then attempt bid(0) (after approving biddingToken)
and assert bid reverts (.to.be.reverted). Use the existing helpers and contracts
(createAuction, cancelAuction, claim, bid, auctions, mockToken/mockNFT,
biddingToken, ethers.provider.send('evm_increaseTime')) and follow the same
setup patterns as the surrounding tests.
In `@test/VickreyAuction.test.ts`:
- Around line 222-412: Add a test that verifies double-cancel is rejected by
creating an auction, calling vickreyAuction.connect(auctioneer).cancelAuction(0)
once, then asserting a second cancelAuction(0) call reverts (matching the
contract revert message) and checking auctions(0).isClaimed remains true; and
add a test that creates an auction, has a bidder call commitBid(0, commitment, {
value: commitFee }), advances time past the commit deadline
(evm_increaseTime/evm_mine) and then asserts that
vickreyAuction.connect(auctioneer).cancelAuction(0) is reverted (same guard as
when commitments exist), referencing cancelAuction, commitBid, and the
auctions(...).isClaimed flag to locate the logic.
🧹 Nitpick comments (5)
contracts/LinearReverseDutchAuction.sol (1)
121-121: Consider restoringnotClaimedonbid()for defense-in-depth.The
beforeDeadlinecheck effectively blocks bids after cancellation (sincedeadlineis set toblock.timestamp), andclaim()internally still hasnotClaimed. However, keepingnotClaimedonbid()provides an explicit, self-documenting safety net against future refactors that might alter the deadline logic.♻️ Suggested change
- function bid(uint256 auctionId) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) { + function bid(uint256 auctionId) external exists(auctionId) notClaimed(auctions[auctionId].isClaimed) beforeDeadline(auctions[auctionId].deadline) {contracts/ExponentialReverseDutchAuction.sol (1)
144-144: Consider restoringnotClaimedonbid()for defense-in-depth.Same reasoning as the
LinearReverseDutchAuctioncomment —beforeDeadlineis sufficient today, butnotClaimedadds an explicit guard against future regressions.♻️ Suggested change
- function bid(uint256 auctionId) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) { + function bid(uint256 auctionId) external exists(auctionId) notClaimed(auctions[auctionId].isClaimed) beforeDeadline(auctions[auctionId].deadline) {contracts/LogarithmicReverseDutchAuction.sol (1)
181-181: Consider restoringnotClaimedonbid()for defense-in-depth.Same reasoning as the other reverse Dutch auction contracts.
♻️ Suggested change
- function bid(uint256 auctionId) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) { + function bid(uint256 auctionId) external exists(auctionId) notClaimed(auctions[auctionId].isClaimed) beforeDeadline(auctions[auctionId].deadline) {test/VickreyAuction.test.ts (2)
448-449: Inconsistent commitment hash encoding across test sections.The main test suite (line 77) uses
ethers.keccak256(AbiCoder.defaultAbiCoder().encode(...))while the reentrancy tests useethers.solidityPackedKeccak256(...). These happen to produce the same output here because bothuint256andbytes32are exactly 32 bytes, but the divergent style is confusing and fragile — if a type changes (e.g., touint128), the two approaches would silently diverge.Pick one encoding style and use it consistently.
414-417:advanceTimehelper is underutilized.This helper duplicates the inline
evm_increaseTime+evm_minepattern used throughout the cancellation tests (e.g., lines 318-319, 345-346). Consider moving it above the cancellationdescribeblock and using it there too to reduce boilerplate.
Comment on lines
+134
to
+142
| function cancelAuction(uint256 auctionId) external exists(auctionId) { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
| require(msg.sender == auction.auctioneer, "Only auctioneer can cancel"); | ||
| require(auction.winner == auction.auctioneer, "Cannot cancel auction with bids"); | ||
| auction.isClaimed = true; | ||
| auction.deadline = block.timestamp; // Set deadline to now, preventing future bids via beforeDeadline modifier | ||
| sendFunds(auction.auctionType == AuctionType.NFT, auction.auctionedToken, auction.auctioneer, auction.auctionedTokenIdOrAmount); | ||
| emit AuctionCancelled(auctionId, auction.auctioneer); | ||
| } |
There was a problem hiding this comment.
Critical: Same notClaimed guard missing as in LinearReverseDutchAuction — double-spend on ERC-20 token auctions.
The identical exploit path applies: after a no-bid auction expires, claim() followed by cancelAuction() (or two cancelAuction calls for token auctions) drains tokens from the contract's pooled balance.
🐛 Proposed fix — add `notClaimed` modifier
- function cancelAuction(uint256 auctionId) external exists(auctionId) {
+ function cancelAuction(uint256 auctionId) external exists(auctionId) notClaimed(auctions[auctionId].isClaimed) {🤖 Prompt for AI Agents
In `@contracts/ExponentialReverseDutchAuction.sol` around lines 134 - 142, The
cancelAuction function is missing the notClaimed guard and can be called after
claim(), enabling double-spend for token auctions; update cancelAuction (in
ExponentialReverseDutchAuction) to enforce the same protection as
LinearReverseDutchAuction by requiring the auction is not already claimed (use
the existing notClaimed modifier or add require(!auction.isClaimed, ...) at the
top of cancelAuction) so that sendFunds cannot be invoked twice for the same
auction/winner; keep the rest of the logic (setting isClaimed, deadline,
sendFunds, emit AuctionCancelled) unchanged.
Comment on lines
+111
to
+119
| function cancelAuction(uint256 auctionId) external exists(auctionId) { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
| require(msg.sender == auction.auctioneer, "Only auctioneer can cancel"); | ||
| require(auction.winner == auction.auctioneer, "Cannot cancel auction with bids"); | ||
| auction.isClaimed = true; | ||
| auction.deadline = block.timestamp; // Set deadline to now, preventing future bids via beforeDeadline modifier | ||
| sendFunds(auction.auctionType == AuctionType.NFT, auction.auctionedToken, auction.auctioneer, auction.auctionedTokenIdOrAmount); | ||
| emit AuctionCancelled(auctionId, auction.auctioneer); | ||
| } |
There was a problem hiding this comment.
Critical: cancelAuction is missing a notClaimed guard — enables double-spend on ERC-20 token auctions.
Because cancelAuction never checks isClaimed, an auctioneer can drain tokens belonging to other auctions stored in the same contract:
- Create a token auction (ERC-20 transferred into the contract).
- Let the deadline pass with no bids (
winner == auctioneerremains true). - Call
claim(auctionId)→isClaimed = true, tokens sent to auctioneer. - Call
cancelAuction(auctionId)→winner == auctioneerstill passes,sendFundssends the same amount again from the contract's pooled balance.
The same vector applies to calling cancelAuction twice in a row for token auctions.
🐛 Proposed fix — add `notClaimed` modifier
- function cancelAuction(uint256 auctionId) external exists(auctionId) {
+ function cancelAuction(uint256 auctionId) external exists(auctionId) notClaimed(auctions[auctionId].isClaimed) {🤖 Prompt for AI Agents
In `@contracts/LinearReverseDutchAuction.sol` around lines 111 - 119, The
cancelAuction function lacks a notClaimed guard allowing double-spend; fix by
preventing cancellation if auction.isClaimed is true — either add the existing
notClaimed modifier to cancelAuction's signature or insert
require(!auction.isClaimed, "Already claimed") near the start of cancelAuction
(after loading AuctionData and confirming auctioneer), so that sendFunds cannot
be executed twice; reference cancelAuction, AuctionData.isClaimed, notClaimed
modifier, exists modifier, and sendFunds when making the change.
Comment on lines
+171
to
+179
| function cancelAuction(uint256 auctionId) external exists(auctionId) { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
| require(msg.sender == auction.auctioneer, "Only auctioneer can cancel"); | ||
| require(auction.winner == auction.auctioneer, "Cannot cancel auction with bids"); | ||
| auction.isClaimed = true; | ||
| auction.deadline = block.timestamp; // Set deadline to now, preventing future bids via beforeDeadline modifier | ||
| sendFunds(auction.auctionType == AuctionType.NFT, auction.auctionedToken, auction.auctioneer, auction.auctionedTokenIdOrAmount); | ||
| emit AuctionCancelled(auctionId, auction.auctioneer); | ||
| } |
There was a problem hiding this comment.
Critical: Same notClaimed guard missing — double-spend on ERC-20 token auctions.
Same exploit path as LinearReverseDutchAuction and ExponentialReverseDutchAuction. Please add the notClaimed modifier.
🐛 Proposed fix — add `notClaimed` modifier
- function cancelAuction(uint256 auctionId) external exists(auctionId) {
+ function cancelAuction(uint256 auctionId) external exists(auctionId) notClaimed(auctions[auctionId].isClaimed) {🤖 Prompt for AI Agents
In `@contracts/LogarithmicReverseDutchAuction.sol` around lines 171 - 179, The
cancelAuction function is missing the notClaimed guard, allowing a double-spend
on ERC-20 auctions; update the function signature for cancelAuction(uint256
auctionId) to include the notClaimed modifier (i.e., cancelAuction(uint256
auctionId) external exists(auctionId) notClaimed) so it checks
AuctionData.isClaimed before proceeding, ensuring AuctionData in
auctions[auctionId] cannot be cancelled/claimed twice; keep the existing body
(setting isClaimed, deadline, sendFunds, emitting AuctionCancelled).
Comment on lines
+302
to
+454
| describe('Auction Cancellation', function () { | ||
| it('should allow auctioneer to cancel auction before any bids', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await allPayAuction.getAddress(), 1); | ||
| await allPayAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('1'), | ||
| ethers.parseEther('0.1'), | ||
| 5, | ||
| 10, | ||
| ); | ||
|
|
||
| expect(await mockNFT.ownerOf(1)).to.equal(await allPayAuction.getAddress()); | ||
|
|
||
| await expect(allPayAuction.connect(auctioneer).cancelAuction(0)) | ||
| .to.emit(allPayAuction, 'AuctionCancelled') | ||
| .withArgs(0, await auctioneer.getAddress()); | ||
|
|
||
| expect(await mockNFT.ownerOf(1)).to.equal(await auctioneer.getAddress()); | ||
| const auction = await allPayAuction.auctions(0); | ||
| expect(auction.isClaimed).to.be.true; | ||
| }); | ||
|
|
||
| it('should allow auctioneer to cancel token auction before any bids', async function () { | ||
| const amount = ethers.parseEther('10'); | ||
| await mockToken.connect(auctioneer).approve(await allPayAuction.getAddress(), amount); | ||
|
|
||
| await allPayAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Token Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 1, | ||
| await mockToken.getAddress(), | ||
| amount, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('1'), | ||
| ethers.parseEther('0.1'), | ||
| 5, | ||
| 10, | ||
| ); | ||
|
|
||
| const balanceBefore = await mockToken.balanceOf(await auctioneer.getAddress()); | ||
| await allPayAuction.connect(auctioneer).cancelAuction(0); | ||
| const balanceAfter = await mockToken.balanceOf(await auctioneer.getAddress()); | ||
| expect(balanceAfter).to.equal(balanceBefore + amount); | ||
| }); | ||
|
|
||
| it('should not allow non-auctioneer to cancel auction', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await allPayAuction.getAddress(), 1); | ||
| await allPayAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('1'), | ||
| ethers.parseEther('0.1'), | ||
| 5, | ||
| 10, | ||
| ); | ||
|
|
||
| await expect(allPayAuction.connect(bidder1).cancelAuction(0)).to.be.revertedWith('Only auctioneer can cancel'); | ||
| }); | ||
|
|
||
| it('should not allow cancellation after bid is placed', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await allPayAuction.getAddress(), 1); | ||
| await allPayAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('1'), | ||
| ethers.parseEther('0.1'), | ||
| 5, | ||
| 10, | ||
| ); | ||
|
|
||
| const bidAmount = ethers.parseEther('1.5'); | ||
| await biddingToken.connect(bidder1).approve(await allPayAuction.getAddress(), bidAmount); | ||
| await allPayAuction.connect(bidder1).bid(0, bidAmount); | ||
|
|
||
| await expect(allPayAuction.connect(auctioneer).cancelAuction(0)).to.be.revertedWith('Cannot cancel auction with bids'); | ||
| }); | ||
|
|
||
| it('should allow cancellation after deadline if no bids', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await allPayAuction.getAddress(), 1); | ||
| await allPayAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('1'), | ||
| ethers.parseEther('0.1'), | ||
| 5, | ||
| 10, | ||
| ); | ||
|
|
||
| await ethers.provider.send('evm_increaseTime', [10]); | ||
| await ethers.provider.send('evm_mine', []); | ||
|
|
||
| await expect(allPayAuction.connect(auctioneer).cancelAuction(0)) | ||
| .to.emit(allPayAuction, 'AuctionCancelled') | ||
| .withArgs(0, await auctioneer.getAddress()); | ||
| }); | ||
|
|
||
| it('should not allow bidding on cancelled auction', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await allPayAuction.getAddress(), 1); | ||
| await allPayAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('1'), | ||
| ethers.parseEther('0.1'), | ||
| 5, | ||
| 10, | ||
| ); | ||
|
|
||
| await allPayAuction.connect(auctioneer).cancelAuction(0); | ||
|
|
||
| const bidAmount = ethers.parseEther('1.5'); | ||
| await biddingToken.connect(bidder1).approve(await allPayAuction.getAddress(), bidAmount); | ||
| await expect(allPayAuction.connect(bidder1).bid(0, bidAmount)).to.be.revertedWith('Deadline of auction reached'); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Add test cases for double-cancel and claim-then-cancel to exercise the notClaimed guard.
The current suite validates the happy path and basic rejections well. However, two important adversarial scenarios are missing:
- Double cancel (token auction): Cancel once, then attempt
cancelAuctionagain — should revert (once thenotClaimedfix is applied to the contract). - Claim then cancel: Create a token auction, let the deadline pass with no bids, call
claim(), then callcancelAuction()— should revert. This path currently exploits the missingnotClaimedguard oncancelAuction.
These tests would serve as regression guards for the critical double-spend vulnerability identified in the contract reviews.
🤖 Prompt for AI Agents
In `@test/AllPayAuction.test.ts` around lines 302 - 454, Add two tests exercising
the notClaimed guard: (1) "double-cancel (token auction)" — create a token
auction via createAuction with mockToken/ERC20, cancelAuction(0) once, then
assert a second cancelAuction(0) call reverts; (2) "claim then cancel" — create
a token auction, advance time past deadline, call claim(0) as the rightful
claimer, then assert a subsequent cancelAuction(0) call reverts. Reference
cancelAuction, claim, createAuction, and the auction index 0 (and mockToken
approvals/balance checks) to locate where to add these specs. Ensure approvals
and time travel (evm_increaseTime/evm_mine) mirror existing token-auction tests.
Comment on lines
+125
to
+230
| describe('Auction Cancellation', function () { | ||
| it('should allow auctioneer to cancel auction before any bids', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await exponentialReverseDutchAuction.getAddress(), 1); | ||
| await exponentialReverseDutchAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('10'), | ||
| ethers.parseEther('1'), | ||
| 20000, | ||
| 10, | ||
| ); | ||
|
|
||
| expect(await mockNFT.ownerOf(1)).to.equal(await exponentialReverseDutchAuction.getAddress()); | ||
|
|
||
| await expect(exponentialReverseDutchAuction.connect(auctioneer).cancelAuction(0)) | ||
| .to.emit(exponentialReverseDutchAuction, 'AuctionCancelled') | ||
| .withArgs(0, await auctioneer.getAddress()); | ||
|
|
||
| expect(await mockNFT.ownerOf(1)).to.equal(await auctioneer.getAddress()); | ||
| const auction = await exponentialReverseDutchAuction.auctions(0); | ||
| expect(auction.isClaimed).to.be.true; | ||
| }); | ||
|
|
||
| it('should not allow non-auctioneer to cancel auction', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await exponentialReverseDutchAuction.getAddress(), 1); | ||
| await exponentialReverseDutchAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('10'), | ||
| ethers.parseEther('1'), | ||
| 20000, | ||
| 10, | ||
| ); | ||
|
|
||
| await expect(exponentialReverseDutchAuction.connect(bidder1).cancelAuction(0)).to.be.revertedWith( | ||
| 'Only auctioneer can cancel', | ||
| ); | ||
| }); | ||
|
|
||
| it('should not allow cancellation after bid is placed', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await exponentialReverseDutchAuction.getAddress(), 1); | ||
| await exponentialReverseDutchAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('10'), | ||
| ethers.parseEther('1'), | ||
| 20000, | ||
| 10, | ||
| ); | ||
|
|
||
| const currentPrice = await exponentialReverseDutchAuction.getCurrentPrice(0); | ||
| await biddingToken.connect(bidder1).approve(await exponentialReverseDutchAuction.getAddress(), currentPrice); | ||
| await exponentialReverseDutchAuction.connect(bidder1).bid(0); | ||
|
|
||
| await expect(exponentialReverseDutchAuction.connect(auctioneer).cancelAuction(0)).to.be.revertedWith( | ||
| 'Cannot cancel auction with bids', | ||
| ); | ||
| }); | ||
|
|
||
| it('should allow cancellation after deadline if no bids', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await exponentialReverseDutchAuction.getAddress(), 1); | ||
| await exponentialReverseDutchAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('10'), | ||
| ethers.parseEther('1'), | ||
| 20000, | ||
| 10, | ||
| ); | ||
|
|
||
| await ethers.provider.send('evm_increaseTime', [15]); | ||
| await ethers.provider.send('evm_mine', []); | ||
|
|
||
| await expect(exponentialReverseDutchAuction.connect(auctioneer).cancelAuction(0)) | ||
| .to.emit(exponentialReverseDutchAuction, 'AuctionCancelled') | ||
| .withArgs(0, await auctioneer.getAddress()); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
Test coverage gaps: missing token-auction cancel, double-cancel, claim-then-cancel, and bid-after-cancel scenarios.
Compared to the AllPayAuction test suite, this file is missing:
- Token auction cancellation (only NFT is tested).
- Double-cancel protection (cancel → cancel again).
- Claim-then-cancel path (deadline passes →
claim()→cancelAuction()). - Bidding on a cancelled auction (verifies the deadline mechanism blocks bids).
These are important given the notClaimed vulnerability in the contract.
🤖 Prompt for AI Agents
In `@test/ExponentialReverseDutchAuction.test.ts` around lines 125 - 230, Add
tests covering missing cancellation/edge cases: 1) create a token-backed auction
via createAuction (use biddingToken and token contract address similar to NFT
flow) and assert cancelAuction emits AuctionCancelled and returns token to
auctioneer; 2) ensure double-cancel protection by calling cancelAuction twice on
the same auction id and expecting the second call to revert (match message used
in contract); 3) simulate deadline passing then call claim(auctionId) and
afterwards assert cancelAuction(auctionId) reverts (or behaves per contract) to
cover claim-then-cancel path; 4) after cancelling an auction, attempt
bid(auctionId) (after approving biddingToken and using getCurrentPrice) and
assert bid reverts to verify bids are blocked on cancelled auctions; mirror
patterns/assertions used in existing tests (e.g., AuctionCancelled event,
auctions(0).isClaimed) for consistency.
Comment on lines
+237
to
+364
| describe('Auction Cancellation', function () { | ||
| it('should allow auctioneer to cancel auction before any bids', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await linearReverseDutchAuction.getAddress(), 1); | ||
| await linearReverseDutchAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('10'), | ||
| ethers.parseEther('1'), | ||
| 10, | ||
| ); | ||
|
|
||
| expect(await mockNFT.ownerOf(1)).to.equal(await linearReverseDutchAuction.getAddress()); | ||
|
|
||
| await expect(linearReverseDutchAuction.connect(auctioneer).cancelAuction(0)) | ||
| .to.emit(linearReverseDutchAuction, 'AuctionCancelled') | ||
| .withArgs(0, await auctioneer.getAddress()); | ||
|
|
||
| expect(await mockNFT.ownerOf(1)).to.equal(await auctioneer.getAddress()); | ||
| const auction = await linearReverseDutchAuction.auctions(0); | ||
| expect(auction.isClaimed).to.be.true; | ||
| }); | ||
|
|
||
| it('should allow auctioneer to cancel token auction before any bids', async function () { | ||
| const amount = ethers.parseEther('10'); | ||
| await mockToken.connect(auctioneer).approve(await linearReverseDutchAuction.getAddress(), amount); | ||
|
|
||
| await linearReverseDutchAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Token Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 1, | ||
| await mockToken.getAddress(), | ||
| amount, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('10'), | ||
| ethers.parseEther('1'), | ||
| 10, | ||
| ); | ||
|
|
||
| const balanceBefore = await mockToken.balanceOf(await auctioneer.getAddress()); | ||
| await linearReverseDutchAuction.connect(auctioneer).cancelAuction(0); | ||
| const balanceAfter = await mockToken.balanceOf(await auctioneer.getAddress()); | ||
| expect(balanceAfter).to.equal(balanceBefore + amount); | ||
| }); | ||
|
|
||
| it('should not allow non-auctioneer to cancel auction', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await linearReverseDutchAuction.getAddress(), 1); | ||
| await linearReverseDutchAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('10'), | ||
| ethers.parseEther('1'), | ||
| 10, | ||
| ); | ||
|
|
||
| await expect(linearReverseDutchAuction.connect(bidder1).cancelAuction(0)).to.be.revertedWith( | ||
| 'Only auctioneer can cancel', | ||
| ); | ||
| }); | ||
|
|
||
| it('should not allow cancellation after bid is placed', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await linearReverseDutchAuction.getAddress(), 1); | ||
| await linearReverseDutchAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('10'), | ||
| ethers.parseEther('1'), | ||
| 10, | ||
| ); | ||
|
|
||
| const bidAmount = ethers.parseEther('10'); | ||
| await biddingToken.connect(bidder1).approve(await linearReverseDutchAuction.getAddress(), bidAmount); | ||
| await linearReverseDutchAuction.connect(bidder1).bid(0); | ||
|
|
||
| await expect(linearReverseDutchAuction.connect(auctioneer).cancelAuction(0)).to.be.revertedWith( | ||
| 'Cannot cancel auction with bids', | ||
| ); | ||
| }); | ||
|
|
||
| it('should allow cancellation after deadline if no bids', async function () { | ||
| await mockNFT.connect(auctioneer).approve(await linearReverseDutchAuction.getAddress(), 1); | ||
| await linearReverseDutchAuction | ||
| .connect(auctioneer) | ||
| .createAuction( | ||
| 'Test Auction', | ||
| 'Test Description', | ||
| 'https://example.com/test.jpg', | ||
| 0, | ||
| await mockNFT.getAddress(), | ||
| 1, | ||
| await biddingToken.getAddress(), | ||
| ethers.parseEther('10'), | ||
| ethers.parseEther('1'), | ||
| 10, | ||
| ); | ||
|
|
||
| await ethers.provider.send('evm_increaseTime', [15]); | ||
| await ethers.provider.send('evm_mine', []); | ||
|
|
||
| await expect(linearReverseDutchAuction.connect(auctioneer).cancelAuction(0)) | ||
| .to.emit(linearReverseDutchAuction, 'AuctionCancelled') | ||
| .withArgs(0, await auctioneer.getAddress()); | ||
| }); | ||
|
|
||
| }); |
There was a problem hiding this comment.
Good coverage for basic flows; add double-cancel and claim-then-cancel tests.
NFT and token cancellation, authorization, and post-bid rejection are well covered. Please also add:
- Double cancel: Cancel a token auction twice — should revert after the
notClaimedfix. - Claim then cancel: Let a no-bid token auction expire, call
claim(), thencancelAuction()— should revert. - Bid after cancel: Verify
bid()reverts on a cancelled auction.
These mirror the gaps identified across the other test suites and guard against the notClaimed vulnerability.
🤖 Prompt for AI Agents
In `@test/LinearReverseDutchAuction.test.ts` around lines 237 - 364, Add three
unit tests in the "Auction Cancellation" describe block to cover double-cancel,
claim-then-cancel, and bid-after-cancel flows: (1) Double-cancel: create a token
auction via createAuction, call cancelAuction once (assert AuctionCancelled
emitted and token returned), then call cancelAuction a second time and assert
the call reverts (use .to.be.reverted) to guard the notClaimed behavior; (2)
Claim-then-cancel: create a token auction, fast-forward past deadline, call
claim() (assert success/claimed state via auctions(0).isClaimed or event), then
call cancelAuction and assert it reverts (.to.be.reverted); (3)
Bid-after-cancel: create an auction, cancel it, then attempt bid(0) (after
approving biddingToken) and assert bid reverts (.to.be.reverted). Use the
existing helpers and contracts (createAuction, cancelAuction, claim, bid,
auctions, mockToken/mockNFT, biddingToken,
ethers.provider.send('evm_increaseTime')) and follow the same setup patterns as
the surrounding tests.
…nd fix the test gaps
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/VickreyAuction.test.ts`:
- Around line 503-514: The main auction tests use
AbiCoder.defaultAbiCoder().encode(...) to build commitment hashes which pads
values, but the contract's VickreyAuction.revealBid() expects
keccak256(abi.encodePacked(bidAmount, salt)) (tight packing); change the test
code that builds commitments (where AbiCoder.defaultAbiCoder().encode is used)
to use ethers.solidityPackedKeccak256(['uint256','bytes32'], [bidAmount, salt])
(same types/order as used in commit/reveal flow) so the generated commitment
values match the contract; ensure the same salt and BID_AMOUNT variables are
passed and that commitBid/revealBid calls remain unchanged.
🧹 Nitpick comments (2)
test/VickreyAuction.test.ts (2)
431-436: Strengthen the double-cancel assertion with a specific revert reason.Using
.to.be.revertedwithout a reason string is fragile — the transaction could revert for an unrelated reason and the test would still pass. Assert the specific revert message to ensure thenotClaimed/isClaimedguard is what blocks the second call.♻️ Suggested tightening
- await expect(vickreyAuction.connect(auctioneer).cancelAuction(0)).to.be.reverted; + await expect(vickreyAuction.connect(auctioneer).cancelAuction(0)).to.be.revertedWith( + 'Auction already claimed', // adjust to match the actual contract revert string + );
473-476:advanceTimehelper is defined but only used inReentrancy Regression Tests.Consider moving it to a shared test utilities file or using it consistently in the cancellation tests above (which inline
evm_increaseTime/evm_minecalls instead).
…itments in Vickrey auction tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@test/VickreyAuction.test.ts`:
- Around line 433-435: The test currently checks double-cancel with a generic
revert; update the assertion to assert the exact revert reason from the contract
(use .to.be.revertedWith(...)) so the failure is tied to the not-claimed
condition. Locate the test line using
vickreyAuction.connect(auctioneer).cancelAuction(0) and replace the generic
.to.be.reverted with .to.be.revertedWith("...") using the exact revert string
emitted by the contract's notClaimed modifier (or the contract's cancelAuction
revert message).
🧹 Nitpick comments (2)
test/VickreyAuction.test.ts (2)
222-471: Extract shared auction-creation helper to reduce duplication.Every cancellation test repeats the same ~15-line
createAuctionblock. The "Reentrancy Regression Tests" section (line 485) already defines acreateAuction()helper. Consider hoisting a similar helper to the outerdescribescope (or a shared utility) so both suites and all cancellation tests can reuse it.
473-476: Consider usingadvanceTimein the cancellation tests too.This helper is defined at the outer scope but only called in the reentrancy suite. The cancellation tests (lines 318-319, 345-346, 463-464) inline the same
evm_increaseTime/evm_minepattern. UsingadvanceTimeeverywhere would be more consistent.
Comment on lines
+433
to
+435
|
|
||
| // Try to cancel again - should revert because asset already claimed | ||
| await expect(vickreyAuction.connect(auctioneer).cancelAuction(0)).to.be.reverted; |
There was a problem hiding this comment.
Assert the specific revert reason for double-cancel.
Every other revert assertion in this file uses .to.be.revertedWith(...), but this one only uses .to.be.reverted. This is weaker — it would pass even if the transaction reverts for an unrelated reason. Pin it to the expected message (likely from the notClaimed modifier) for consistency and correctness.
Proposed fix
- await expect(vickreyAuction.connect(auctioneer).cancelAuction(0)).to.be.reverted;
+ await expect(vickreyAuction.connect(auctioneer).cancelAuction(0)).to.be.revertedWith('Asset already claimed');Adjust the string to match the actual revert message in the contract.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Try to cancel again - should revert because asset already claimed | |
| await expect(vickreyAuction.connect(auctioneer).cancelAuction(0)).to.be.reverted; | |
| // Try to cancel again - should revert because asset already claimed | |
| await expect(vickreyAuction.connect(auctioneer).cancelAuction(0)).to.be.revertedWith('Asset already claimed'); |
🤖 Prompt for AI Agents
In `@test/VickreyAuction.test.ts` around lines 433 - 435, The test currently
checks double-cancel with a generic revert; update the assertion to assert the
exact revert reason from the contract (use .to.be.revertedWith(...)) so the
failure is tied to the not-claimed condition. Locate the test line using
vickreyAuction.connect(auctioneer).cancelAuction(0) and replace the generic
.to.be.reverted with .to.be.revertedWith("...") using the exact revert string
emitted by the contract's notClaimed modifier (or the contract's cancelAuction
revert message).
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
3 participants
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.
Feature: Auction Cancellation Functionality
Summary
Implements auction cancellation functionality across all 6 auction types, allowing auctioneers to cancel their auctions before any bids are placed. This addresses the issue of locked capital and poor UX when auctioneers make mistakes or market conditions change.
Problem Statement
Issue: Auctioneers cannot cancel auctions once created, even with zero bids
Impact:
claim()Changes
Smart Contracts Modified (6 files)
Core Functionality Added to Each Auction Type:
cancelAuction()function - Allows auctioneers to cancel their auctionsAuctionCancelledevent - Emits transparent on-chain record of cancellationnotClaimedmodifier to bid/commit functions where missingEnglishAuction.solhighestBid == 0deadlineAllPayAuction.solhighestBid == 0deadlineVickreyAuction.solbidCommitEndLinearReverseDutchAuction.solwinner == auctioneerdeadlineExponentialReverseDutchAuction.solwinner == auctioneerdeadlineLogarithmicReverseDutchAuction.solwinner == auctioneerdeadlineTests Added (6 files)
Comprehensive test coverage for each auction type:
Total Tests Added: 34 new test cases
Test Status: ✅ All 71 tests passing
Security Considerations
Access Control
auctioneer) can cancelmsg.sender == auction.auctioneerState Management
isClaimed = trueto prevent further interactionssendFunds()helperIssue Fixed
#28 Implement Auction Cancellation for Zero-Bid Auctions to Prevent Locked Assets
Summary by CodeRabbit
New Features
Bug Fixes
Tests