Fix/english auction fee on transfer#63
Open
Rav1Chauhan wants to merge 2 commits intoStabilityNexus:mainfrom
Open
Fix/english auction fee on transfer#63Rav1Chauhan wants to merge 2 commits intoStabilityNexus:mainfrom
Rav1Chauhan wants to merge 2 commits intoStabilityNexus:mainfrom
Conversation
WalkthroughImplements fee-on-transfer token support across all auction contracts using balance-difference accounting. Adds bidding invariant enforcement in EnglishAuction (minimumBid > 0, minBidDelta > 0), refunds previous bidders on new bids, and improves reentrancy safety in VickreyAuction by updating state before external transfers. Changes
Sequence DiagramsequenceDiagram
actor Bidder
participant EnglishAuction
participant ERC20Token
participant PreviousBidder
Bidder->>EnglishAuction: bid(auctionId, amount)
EnglishAuction->>ERC20Token: balanceOf(address(this))
ERC20Token-->>EnglishAuction: balanceBefore
Bidder->>ERC20Token: approve(EnglishAuction, amount)
ERC20Token-->>Bidder: approval confirmed
EnglishAuction->>ERC20Token: safeTransferFrom(Bidder, address(this), amount)
ERC20Token-->>EnglishAuction: transfer executed (may apply fee)
EnglishAuction->>ERC20Token: balanceOf(address(this))
ERC20Token-->>EnglishAuction: balanceAfter
note over EnglishAuction: actualReceived = balanceAfter - balanceBefore
note over EnglishAuction: Validate: actualReceived >= minimumBid or minBidDelta
alt Higher Bid Received
note over EnglishAuction: Store previousHighest bidder
EnglishAuction->>EnglishAuction: Update highestBid to actualReceived
EnglishAuction->>EnglishAuction: Update winner to Bidder
EnglishAuction->>PreviousBidder: Refund previousHighest amount
end
note over EnglishAuction: Extend deadline
EnglishAuction-->>Bidder: bidPlaced event emitted
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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
♻️ Duplicate comments (2)
contracts/LinearReverseDutchAuction.sol (1)
125-133:⚠️ Potential issue | 🟠 MajorSame underbid risk as in ExponentialReverseDutchAuction.bid.
Please apply the same invariant check here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/LinearReverseDutchAuction.sol` around lines 125 - 133, The bid function may record a winner and settlePrice even if receiveERC20 transfers fewer tokens than the expected currentPrice, creating an underbid invariant; add the same check used in ExponentialReverseDutchAuction.bid after calling receiveERC20 to ensure actualReceived >= currentPrice and revert if not, so that AuctionData (winner, availableFunds, settlePrice) and the subsequent claim(auctionId) only execute when the received amount meets or exceeds getCurrentPrice(auctionId).contracts/LogarithmicReverseDutchAuction.sol (1)
209-217:⚠️ Potential issue | 🟠 MajorSame underbid risk as in ExponentialReverseDutchAuction.bid.
Please apply the same invariant check here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@contracts/LogarithmicReverseDutchAuction.sol` around lines 209 - 217, In bid() add the same invariant check used in ExponentialReverseDutchAuction.bid to ensure the amount returned by receiveERC20 meets the current price: after computing currentPrice via getCurrentPrice(auctionId) and calling receiveERC20(auction.biddingToken, msg.sender, currentPrice), require(actualReceived >= currentPrice, "Insufficient bid"); then set auction.availableFunds and auction.settlePrice appropriately (use currentPrice as the settlePrice and handle/refund any excess if actualReceived > currentPrice) before calling claim(auctionId); reference symbols: bid, getCurrentPrice, receiveERC20, auction.availableFunds, auction.settlePrice, claim.
🤖 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/abstract/Auction.sol`:
- Around line 92-108: The require in receiveFunds unconditionally rejects
fee‑on‑transfer ERC20s; change receiveFunds to accept a boolean flag (e.g.,
allowFeeOnTransfer) or provide a separate receiveFundsAllowFeeOnTransfer variant
so callers can opt in; update the implementation to skip the strict equality
check when allowFeeOnTransfer is true (keep current behavior when false), and
update all call sites that create auctions to pass the appropriate flag (or call
the new variant) so deflationary auction tokens are allowed only when explicitly
requested; reference functions receiveFunds and receiveERC20 when locating and
modifying the logic.
In `@contracts/ExponentialReverseDutchAuction.sol`:
- Around line 211-219: The bid flow currently sets auction.winner and uses
actualReceived from receiveERC20 without ensuring actualReceived meets the
required price, allowing fee-on-transfer tokens to underpay; update the
bid(uint256 auctionId) logic (around functions bid, getCurrentPrice,
receiveERC20, and the fields auction.availableFunds/auction.settlePrice) to
verify after the transfer that actualReceived >= currentPrice and revert with a
clear error if not (or alternatively accept overpayment by reading
msg.value/extra param and ensuring the post-transfer receipts cover
currentPrice), then only set availableFunds/settlePrice and call
claim(auctionId) when the invariant holds.
In `@contracts/mocks/FeeOnTransferToken.sol`:
- Around line 6-22: The 10% fee in FeeOnTransferToken::_update is a magic
number; introduce a named constant (e.g., uint256 constant FEE_RATE = 10 and
optionally uint256 constant FEE_DENOMINATOR = 100 or use BASIS_POINTS = 10000)
and replace the literal 10 and 100 in the fee calculation with those constants
so the fee logic is self-documenting and easier to change; update any comments
to reference the constant and keep the computation as fee = (value * FEE_RATE) /
FEE_DENOMINATOR and use amountAfterFee as before.
---
Duplicate comments:
In `@contracts/LinearReverseDutchAuction.sol`:
- Around line 125-133: The bid function may record a winner and settlePrice even
if receiveERC20 transfers fewer tokens than the expected currentPrice, creating
an underbid invariant; add the same check used in
ExponentialReverseDutchAuction.bid after calling receiveERC20 to ensure
actualReceived >= currentPrice and revert if not, so that AuctionData (winner,
availableFunds, settlePrice) and the subsequent claim(auctionId) only execute
when the received amount meets or exceeds getCurrentPrice(auctionId).
In `@contracts/LogarithmicReverseDutchAuction.sol`:
- Around line 209-217: In bid() add the same invariant check used in
ExponentialReverseDutchAuction.bid to ensure the amount returned by receiveERC20
meets the current price: after computing currentPrice via
getCurrentPrice(auctionId) and calling receiveERC20(auction.biddingToken,
msg.sender, currentPrice), require(actualReceived >= currentPrice, "Insufficient
bid"); then set auction.availableFunds and auction.settlePrice appropriately
(use currentPrice as the settlePrice and handle/refund any excess if
actualReceived > currentPrice) before calling claim(auctionId); reference
symbols: bid, getCurrentPrice, receiveERC20, auction.availableFunds,
auction.settlePrice, claim.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (8)
contracts/EnglishAuction.solcontracts/ExponentialReverseDutchAuction.solcontracts/LinearReverseDutchAuction.solcontracts/LogarithmicReverseDutchAuction.solcontracts/VickreyAuction.solcontracts/abstract/Auction.solcontracts/mocks/FeeOnTransferToken.soltest/EnglishAuction.test.ts
Comment on lines
+92
to
+108
| function receiveFunds( | ||
| bool isNFT, | ||
| address token, | ||
| address from, | ||
| uint256 tokenIdOrAmount | ||
| ) internal returns (uint256 actualReceived) { | ||
| if (isNFT) { | ||
| receiveNFT(token, from, tokenIdOrAmount); | ||
| actualReceived = tokenIdOrAmount; | ||
| } else { | ||
| receiveERC20(token, from, tokenIdOrAmount); | ||
| actualReceived = receiveERC20(token, from, tokenIdOrAmount); | ||
|
|
||
| // Strict mode: reject deflationary tokens | ||
| require( | ||
| actualReceived == tokenIdOrAmount, | ||
| "Auction: fee-on-transfer tokens not supported" | ||
| ); |
There was a problem hiding this comment.
Strict equality check blocks fee‑on‑transfer auctioned tokens.
receiveFunds always reverts when actualReceived != tokenIdOrAmount, which prevents creating auctions where the auctioned ERC20 is deflationary. If fee‑on‑transfer support is intended for auctioned assets too, make strictness explicit per call site (or remove the check and enforce only where required).
💡 One way to make strictness configurable
- function receiveFunds(
- bool isNFT,
- address token,
- address from,
- uint256 tokenIdOrAmount
- ) internal returns (uint256 actualReceived) {
+ function receiveFunds(
+ bool isNFT,
+ address token,
+ address from,
+ uint256 tokenIdOrAmount,
+ bool strict
+ ) internal returns (uint256 actualReceived) {
if (isNFT) {
receiveNFT(token, from, tokenIdOrAmount);
actualReceived = tokenIdOrAmount;
} else {
actualReceived = receiveERC20(token, from, tokenIdOrAmount);
- // Strict mode: reject deflationary tokens
- require(
- actualReceived == tokenIdOrAmount,
- "Auction: fee-on-transfer tokens not supported"
- );
+ if (strict) {
+ require(
+ actualReceived == tokenIdOrAmount,
+ "Auction: fee-on-transfer tokens not supported"
+ );
+ }
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/abstract/Auction.sol` around lines 92 - 108, The require in
receiveFunds unconditionally rejects fee‑on‑transfer ERC20s; change receiveFunds
to accept a boolean flag (e.g., allowFeeOnTransfer) or provide a separate
receiveFundsAllowFeeOnTransfer variant so callers can opt in; update the
implementation to skip the strict equality check when allowFeeOnTransfer is true
(keep current behavior when false), and update all call sites that create
auctions to pass the appropriate flag (or call the new variant) so deflationary
auction tokens are allowed only when explicitly requested; reference functions
receiveFunds and receiveERC20 when locating and modifying the logic.
Comment on lines
211
to
219
| function bid(uint256 auctionId) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) notClaimed(auctions[auctionId].isClaimed) { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
| auction.winner = msg.sender; | ||
| uint256 currentPrice = getCurrentPrice(auctionId); | ||
| receiveERC20(auction.biddingToken, msg.sender, currentPrice); | ||
| auction.availableFunds = currentPrice; | ||
| auction.settlePrice = currentPrice; | ||
| uint256 actualReceived = receiveERC20(auction.biddingToken, msg.sender, currentPrice); | ||
|
|
||
| auction.availableFunds = actualReceived; | ||
| auction.settlePrice = actualReceived; | ||
| claim(auctionId); |
There was a problem hiding this comment.
Price invariant not enforced with fee‑on‑transfer bids.
actualReceived can be lower than currentPrice, so a bidder can win below the advertised price curve. If the price must be honored, add a post‑transfer check (or extend the API to allow overpayment to cover transfer fees).
🛠️ Suggested invariant check
uint256 currentPrice = getCurrentPrice(auctionId);
uint256 actualReceived = receiveERC20(auction.biddingToken, msg.sender, currentPrice);
+ require(actualReceived >= currentPrice, "Auction: bid below current price");
auction.availableFunds = actualReceived;
auction.settlePrice = actualReceived;📝 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
| function bid(uint256 auctionId) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) notClaimed(auctions[auctionId].isClaimed) { | |
| AuctionData storage auction = auctions[auctionId]; | |
| auction.winner = msg.sender; | |
| uint256 currentPrice = getCurrentPrice(auctionId); | |
| receiveERC20(auction.biddingToken, msg.sender, currentPrice); | |
| auction.availableFunds = currentPrice; | |
| auction.settlePrice = currentPrice; | |
| uint256 actualReceived = receiveERC20(auction.biddingToken, msg.sender, currentPrice); | |
| auction.availableFunds = actualReceived; | |
| auction.settlePrice = actualReceived; | |
| claim(auctionId); | |
| function bid(uint256 auctionId) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) notClaimed(auctions[auctionId].isClaimed) { | |
| AuctionData storage auction = auctions[auctionId]; | |
| auction.winner = msg.sender; | |
| uint256 currentPrice = getCurrentPrice(auctionId); | |
| uint256 actualReceived = receiveERC20(auction.biddingToken, msg.sender, currentPrice); | |
| require(actualReceived >= currentPrice, "Auction: bid below current price"); | |
| auction.availableFunds = actualReceived; | |
| auction.settlePrice = actualReceived; | |
| claim(auctionId); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/ExponentialReverseDutchAuction.sol` around lines 211 - 219, The bid
flow currently sets auction.winner and uses actualReceived from receiveERC20
without ensuring actualReceived meets the required price, allowing
fee-on-transfer tokens to underpay; update the bid(uint256 auctionId) logic
(around functions bid, getCurrentPrice, receiveERC20, and the fields
auction.availableFunds/auction.settlePrice) to verify after the transfer that
actualReceived >= currentPrice and revert with a clear error if not (or
alternatively accept overpayment by reading msg.value/extra param and ensuring
the post-transfer receipts cover currentPrice), then only set
availableFunds/settlePrice and call claim(auctionId) when the invariant holds.
Comment on lines
+6
to
+22
| contract FeeOnTransferToken is ERC20 { | ||
|
|
||
| constructor() ERC20("FeeToken", "FEE") { | ||
| _mint(msg.sender, 1_000_000 ether); | ||
| } | ||
|
|
||
| function _update(address from, address to, uint256 value) internal override { | ||
| if (from != address(0) && to != address(0)) { | ||
| // Apply 10% fee on transfers (not mint/burn) | ||
| uint256 fee = (value * 10) / 100; | ||
| uint256 amountAfterFee = value - fee; | ||
|
|
||
| super._update(from, to, amountAfterFee); | ||
| super._update(from, address(0xdead), fee); | ||
| } else { | ||
| // Minting or burning | ||
| super._update(from, to, value); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract the fee rate into a constant to avoid magic numbers.
♻️ Proposed refactor
contract FeeOnTransferToken is ERC20 {
+ uint256 private constant FEE_BPS = 1000; // 10%
+ uint256 private constant BPS_DENOMINATOR = 10_000;
constructor() ERC20("FeeToken", "FEE") {
_mint(msg.sender, 1_000_000 ether);
}
function _update(address from, address to, uint256 value) internal override {
if (from != address(0) && to != address(0)) {
// Apply 10% fee on transfers (not mint/burn)
- uint256 fee = (value * 10) / 100;
+ uint256 fee = (value * FEE_BPS) / BPS_DENOMINATOR;
uint256 amountAfterFee = value - fee;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/mocks/FeeOnTransferToken.sol` around lines 6 - 22, The 10% fee in
FeeOnTransferToken::_update is a magic number; introduce a named constant (e.g.,
uint256 constant FEE_RATE = 10 and optionally uint256 constant FEE_DENOMINATOR =
100 or use BASIS_POINTS = 10000) and replace the literal 10 and 100 in the fee
calculation with those constants so the fee logic is self-documenting and easier
to change; update any comments to reference the constant and keep the
computation as fee = (value * FEE_RATE) / FEE_DENOMINATOR and use amountAfterFee
as before.
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 #62
Summary
This PR fixes accounting and validation issues related to ERC20 fee-on-transfer (deflationary) tokens in EnglishAuction.
Previously:
bid() validated using bidAmount
But state updates used actualReceived
This allowed underbidding if tokens deducted transfer fees
Now:
Validation is performed using actualReceived
State updates occur only after successful validation
Refund logic uses stored previousHighest
receiveFunds now propagates actual received amount
Strict rejection of non-exact token deposits added
Screenshots/Recordings:
Additional Notes:
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
Release Notes
New Features
Bug Fixes
Improvements