In AllPayAuction.sol and EnglishAuction.sol, the withdraw functions are external so must be secured

[aniket866]

In AllPayAuction.sol and EnglishAuction.sol, the withdraw functions are external but do not verify that the msg.sender is the auctioneer.

Issue: Anyone can trigger the withdrawal of funds to the auctioneer's address.

Risk: While the funds go to the correct person (the auctioneer), this allows external parties to force financial realizations for the auctioneer, which might have tax or accounting implications, or interfere with a manager's planned strategy.

Fix: Add require(msg.sender == auction.auctioneer) to the withdraw functions.

Closes #21

Summary by CodeRabbit

Bug Fixes

• Added authorization verification to withdrawal operations in auction contracts to restrict fund withdrawals to authorized parties.

[coderabbitai]

📝 Walkthrough

Walkthrough

Access control checks have been added to the withdraw functions in both the AllPayAuction and EnglishAuction contracts. These checks ensure that only the auctioneer associated with a specific auction can execute withdrawals, reverting unauthorized attempts.

Changes

Cohort / File(s)	Summary
Auctioneer-Only Withdrawal Access Control contracts/AllPayAuction.sol, contracts/EnglishAuction.sol	Added runtime authorization check requiring msg.sender to match the auction's auctioneer before allowing fund withdrawals. Existing withdrawal logic remains unchanged.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related issues

• Lack of Access Control on Withdrawals #21: Directly addresses the same security requirement to add require(msg.sender == auction.auctioneer) access checks to the withdraw functions in both contracts.

Possibly related PRs

• added min-bid in vickrey,retrieval by auctioneer in dutch auctions #7: Implements auctioneer-restricted access controls on withdrawal flows, either through inline require statements or access control modifiers applied to withdrawal functions.

Suggested reviewers

• ceilican

Poem

🐰 A hop, a check, a secure withdraw,
Only auctioneers may take what's raw,
No unauthorized paws on the funds,
Access control shines when it runs! ✨

🚥 Pre-merge checks | ✅ 3

✅ Passed checks (3 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title directly and clearly summarizes the main security fix: adding access control checks to external withdraw functions in two auction contracts to restrict them to the auctioneer.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

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)

contracts/AllPayAuction.sol (1)

107-117: ⚠️ Potential issue | 🟠 Major

Add onlyAfterDeadline modifier to prevent mid-auction fund withdrawals.

The withdraw function lacks the onlyAfterDeadline guard, allowing the auctioneer to drain accumulated bid funds before the auction deadline. In an all-pay auction where all bids are non-refundable, this enables the auctioneer to extract funds while bidders continue to pay in, breaking the auction's security model. EnglishAuction.withdraw (line 110) and VickreyAuction.withdraw (line 146) both enforce this check.

Proposed fix

-    function withdraw(uint256 auctionId) external exists(auctionId) {
+    function withdraw(uint256 auctionId) external exists(auctionId) onlyAfterDeadline(auctions[auctionId].deadline) {

🧹 Nitpick comments (2)

contracts/AllPayAuction.sol (1)

110-116: Consider guarding against zero-amount withdrawals.

If availableFunds is already 0 (e.g., already withdrawn or no bids placed), the function proceeds with two zero-value sendERC20 calls and emits Withdrawn with amount 0, wasting gas and producing misleading events.

Proposed fix

+        require(withdrawAmount > 0, "Nothing to withdraw");
         auction.availableFunds = 0;

contracts/EnglishAuction.sol (1)

113-119: Same zero-amount withdrawal concern as in AllPayAuction.

If no bids were placed, availableFunds is 0 and the function will execute two no-op token transfers and emit a misleading Withdrawn(auctionId, 0) event. A require(withdrawAmount > 0, "Nothing to withdraw") guard would prevent this.