Withdrawal Vulnerability across multiple contracts#64
Conversation
WalkthroughAdds deadline and non-zero preconditions to withdrawals across auction contracts, caches Vickrey commit-fee before state changes, and updates tests to assert withdrawal idempotency and require advancing time past deadlines. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 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 Tip You can customize the tone of the review comments and chat replies.Configure the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/VickreyAuction.sol (1)
111-111: 🧹 Nitpick | 🔵 TrivialConsider clarifying the error message for exact fee requirement.
The check
msg.value == auction.commitFeecorrectly enforces exact payment (as recommended to prevent locked funds), but the error message "Insufficient commit fee" is slightly misleading when users send too much ETH. A clearer message would be "Commit fee must match exactly".Suggested improvement
- require(msg.value == auction.commitFee, 'Insufficient commit fee'); // require exact fee + require(msg.value == auction.commitFee, 'Commit fee must match exactly');Based on learnings: "In Vickrey auction contracts, accepting arbitrary ETH amounts in commitBid while only refunding a fixed amount creates a vulnerability where attackers can lock funds forever by sending excess ETH and never revealing their bids. Always enforce exact payment amounts or track individual fees paid per bidder."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/VickreyAuction.sol` at line 111, The require in commitBid currently reads require(msg.value == auction.commitFee, 'Insufficient commit fee'); — change the error string to explicitly state the exact-match requirement (e.g., "Commit fee must match exactly") so callers who send too much ETH aren’t misled; update the require in commitBid that compares msg.value to auction.commitFee to use the clearer message referencing the exact fee requirement.contracts/AllPayAuction.sol (1)
141-141: 🧹 Nitpick | 🔵 TrivialRemove or clarify the dangling TODO comment.
This bare
// TODOlacks context. Either complete the intended task, add a description of what needs to be done, or remove it if no longer applicable.Suggested fix
-// TODO🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/AllPayAuction.sol` at line 141, There is a bare "// TODO" comment inside the AllPayAuction contract that provides no context; either remove that dangling comment or replace it with a meaningful note describing the exact work to be done (e.g., intended behavior, edge-case to handle, or a reference to an issue/PR) so future readers know the intent; locate the lonely TODO in the AllPayAuction contract and either delete it or expand it into a specific actionable comment referencing the affected function or state (e.g., constructor, placeBid, settleAuction) and the required change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/AllPayAuction.sol`:
- Line 141: There is a bare "// TODO" comment inside the AllPayAuction contract
that provides no context; either remove that dangling comment or replace it with
a meaningful note describing the exact work to be done (e.g., intended behavior,
edge-case to handle, or a reference to an issue/PR) so future readers know the
intent; locate the lonely TODO in the AllPayAuction contract and either delete
it or expand it into a specific actionable comment referencing the affected
function or state (e.g., constructor, placeBid, settleAuction) and the required
change.
In `@contracts/VickreyAuction.sol`:
- Line 111: The require in commitBid currently reads require(msg.value ==
auction.commitFee, 'Insufficient commit fee'); — change the error string to
explicitly state the exact-match requirement (e.g., "Commit fee must match
exactly") so callers who send too much ETH aren’t misled; update the require in
commitBid that compares msg.value to auction.commitFee to use the clearer
message referencing the exact fee requirement.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9fa15d45-cef8-4f08-87ed-8ee3bec0d107
📒 Files selected for processing (6)
contracts/AllPayAuction.solcontracts/EnglishAuction.solcontracts/VickreyAuction.soltest/AllPayAuction.test.tstest/EnglishAuction.test.tstest/VickreyAuction.test.ts
There was a problem hiding this comment.
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)
137-137: 🧹 Nitpick | 🔵 TrivialRemove orphaned TODO comment.
There's an incomplete
// TODOcomment at the end of the file with no description. Either add the intended task description or remove it.🧹 Suggested fix
- -// TODO🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/AllPayAuction.sol` at line 137, The file contains an orphaned "// TODO" comment at the end of the AllPayAuction contract; remove the stray comment or replace it with a concrete task description relevant to the AllPayAuction contract (e.g., document known limitation or TODO for a specific function like placeBid or settleAuction) so no ambiguous TODO remains—locate the trailing comment after the AllPayAuction contract closing brace and delete it or replace it with a clear, actionable comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@contracts/AllPayAuction.sol`:
- Line 137: The file contains an orphaned "// TODO" comment at the end of the
AllPayAuction contract; remove the stray comment or replace it with a concrete
task description relevant to the AllPayAuction contract (e.g., document known
limitation or TODO for a specific function like placeBid or settleAuction) so no
ambiguous TODO remains—locate the trailing comment after the AllPayAuction
contract closing brace and delete it or replace it with a clear, actionable
comment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3fa0c6a8-ebc6-47fc-81db-5901da5246f7
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
contracts/AllPayAuction.solcontracts/EnglishAuction.solcontracts/VickreyAuction.soltest/AllPayAuction.test.tstest/EnglishAuction.test.tstest/VickreyAuction.test.ts
aniket866
changed the title
2 participants
Addressed Issues:
Fix: Withdrawal Vulnerability
Fix: Prevent Multiple Withdrawals
Problem
SafeERC20.safeTransferwith amount0does not revert, so repeated calls towithdraw()would silently succeed and emit falseWithdrawnevents.Fix
Added
require(withdrawAmount > 0, 'No funds to withdraw')inwithdraw()across all three auction contracts. ForVickreyAuction, the guard also checkscommitFeeToTransfersince both ERC20 and ETH commit fees can be withdrawn independently.AI Usage Disclosure:
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. AI slop is strongly discouraged and may lead to banning and blocking. Do not spam our repos with AI slop.
Check one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
@DengreSarthak
Summary by CodeRabbit
Bug Fixes
Tests