fix: support fee-on-transfer ERC20 tokens and enforce auction invariants#62
Open
Rav1Chauhan wants to merge 1 commit intoStabilityNexus:mainfrom
Open
fix: support fee-on-transfer ERC20 tokens and enforce auction invariants#62Rav1Chauhan wants to merge 1 commit intoStabilityNexus:mainfrom
Rav1Chauhan wants to merge 1 commit intoStabilityNexus:mainfrom
Conversation
WalkthroughThis pull request implements balance-based token reception tracking across multiple auction contracts to handle fee-on-transfer and deflationary ERC20 tokens. The Changes
Sequence Diagram(s)sequenceDiagram
participant Bidder
participant AuctionContract
participant ERC20Token
participant PreviousBidder
Bidder->>AuctionContract: bid(amount)
AuctionContract->>ERC20Token: transferFrom(bidder, contract, amount)
ERC20Token-->>AuctionContract: (may deduct fees)
AuctionContract->>AuctionContract: balanceAfter - balanceBefore = actualReceived
AuctionContract->>AuctionContract: validate actualReceived > 0
AuctionContract->>AuctionContract: store previousHighest & previousWinner
alt New Highest Bid
AuctionContract->>AuctionContract: update highestBid = actualReceived
AuctionContract->>AuctionContract: update winner
AuctionContract->>PreviousBidder: refund previousHighest (if previousHighest != 0)
PreviousBidder-->>AuctionContract: ✓
end
AuctionContract->>AuctionContract: availableFunds = actualReceived
AuctionContract->>AuctionContract: emit BidPlaced(actualReceived)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 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
🤖 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 90-99: receiveERC20 can return less than expected for
fee‑on‑transfer tokens, but callers (receiveFunds / token auction creation
paths) still record the expectedAmount into auctionedTokenIdOrAmount causing
claim to later revert; update the callers to use the actualReceived value
returned by receiveERC20 (or alternatively reject non‑exact receipts).
Specifically, change the receiveFunds / createTokenAuction flow so after calling
receiveERC20(...) you capture its return value and write that actualReceived
into the auction state field auctionedTokenIdOrAmount (or require(actualReceived
== expectedAmount) if you choose to reject shortfalls), and ensure claim uses
that stored amount when transferring tokens. Make sure functions involved
(receiveFunds, any createTokenAuction wrapper, and claim) reference
auctionedTokenIdOrAmount consistently and add a clear revert message when
rejecting non‑exact receipts.
In `@contracts/EnglishAuction.sol`:
- Around line 110-139: Validation currently compares bidAmount but the contract
records actualReceived from receiveERC20 (so fee‑on‑transfer tokens can
underbid). In function bid, move or duplicate the checks to validate using
actualReceived (the value returned by receiveERC20) against auction.minimumBid
and auction.highestBid + auction.minBidDelta (and against auction.highestBid ==
0 case) and revert if actualReceived is too low; keep receiveERC20 call
(function receiveERC20) and then perform these require checks, update
auction.winner/auction.highestBid/auction.availableFunds only after successful
validation, and ensure refund logic (previousHighest / previousWinner) still
uses the stored previousHighest.
- Around line 67-69: Add unit tests for the constructor parameter checks and
fee-on-transfer bidding behavior: write tests that assert the constructor or
initializer reverts when minimumBid == 0 and when minBidDelta == 0 (referencing
the minimumBid and minBidDelta require checks), and add tests for bid() that
simulate a fee-on-transfer/deflationary ERC20 where transferred amount is less
than the specified bid (use a test token that deducts fees and verify
actualReceived < bidAmount), confirming the contract handles actualReceived
correctly (e.g., rejects bids that don't meet minimums or updates highestBid
using actualReceived as implemented in bid()).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
contracts/EnglishAuction.solcontracts/ExponentialReverseDutchAuction.solcontracts/LinearReverseDutchAuction.solcontracts/LogarithmicReverseDutchAuction.solcontracts/VickreyAuction.solcontracts/abstract/Auction.sol
Comment on lines
+90
to
+99
| function receiveERC20(address token, address from, uint256 expectedAmount) internal returns (uint256 actualReceived) { | ||
| uint256 balanceBefore = IERC20(token).balanceOf(address(this)); | ||
|
|
||
| SafeERC20.safeTransferFrom(IERC20(token), from, address(this), expectedAmount); | ||
|
|
||
| uint256 balanceAfter = IERC20(token).balanceOf(address(this)); | ||
|
|
||
| actualReceived = balanceAfter - balanceBefore; | ||
|
|
||
| require(actualReceived > 0, 'No tokens received'); |
There was a problem hiding this comment.
Prevent underfunded token auctions when the auctioned asset is fee-on-transfer.
receiveERC20 now measures actualReceived, but receiveFunds callers still record the expected amount for auctionedTokenIdOrAmount. If the auctioned asset is a fee‑on‑transfer ERC20, the contract can hold fewer tokens than recorded and claim can revert when trying to transfer the full amount. Plumb the actual received amount into token-auction state (or explicitly reject non‑exact receipts for auctioned assets).
🛠️ Example direction (plumb actualReceived to callers)
-function receiveFunds(bool isNFT, address token, address from, uint256 tokenIdOrAmount) internal {
+function receiveFunds(bool isNFT, address token, address from, uint256 tokenIdOrAmount) internal returns (uint256 actualReceived) {
if (isNFT) {
receiveNFT(token, from, tokenIdOrAmount);
+ return tokenIdOrAmount;
} else {
- receiveERC20(token, from, tokenIdOrAmount);
+ actualReceived = receiveERC20(token, from, tokenIdOrAmount);
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/abstract/Auction.sol` around lines 90 - 99, receiveERC20 can return
less than expected for fee‑on‑transfer tokens, but callers (receiveFunds / token
auction creation paths) still record the expectedAmount into
auctionedTokenIdOrAmount causing claim to later revert; update the callers to
use the actualReceived value returned by receiveERC20 (or alternatively reject
non‑exact receipts). Specifically, change the receiveFunds / createTokenAuction
flow so after calling receiveERC20(...) you capture its return value and write
that actualReceived into the auction state field auctionedTokenIdOrAmount (or
require(actualReceived == expectedAmount) if you choose to reject shortfalls),
and ensure claim uses that stored amount when transferring tokens. Make sure
functions involved (receiveFunds, any createTokenAuction wrapper, and claim)
reference auctionedTokenIdOrAmount consistently and add a clear revert message
when rejecting non‑exact receipts.
Comment on lines
67
to
+69
| require(duration > 0, 'Duration must be greater than zero seconds'); | ||
| require(minimumBid > 0, 'minimumBid must be > 0'); | ||
| require(minBidDelta > 0, 'minBidDelta must be > 0'); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the EnglishAuction.sol contract
find . -name "EnglishAuction.sol" -type fRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Search for test files related to EnglishAuction
find . -type f \( -name "*test*" -o -name "*spec*" \) | head -20Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 307
🏁 Script executed:
#!/bin/bash
# Search for EnglishAuction references in test files
rg -l "EnglishAuction" --type sol --type ts --type jsRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Read the EnglishAuction.sol contract, focusing on the area around lines 67-69
cat -n ./contracts/EnglishAuction.sol | sed -n '50,100p'Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 2314
🏁 Script executed:
#!/bin/bash
# Check the full test file for EnglishAuction
wc -l ./test/EnglishAuction.test.tsRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 119
🏁 Script executed:
#!/bin/bash
# Search for test cases related to minimumBid, minBidDelta, and fee-on-transfer
rg -n "minimumBid|minBidDelta|fee.*transfer|deflationary|actualReceived" ./test/EnglishAuction.test.tsRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 145
🏁 Script executed:
#!/bin/bash
# Read the EnglishAuction test file to see what tests exist
cat -n ./test/EnglishAuction.test.tsRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 14981
🏁 Script executed:
#!/bin/bash
# Read more of the EnglishAuction.sol contract to see the bid function
wc -l ./contracts/EnglishAuction.solRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Look at the bid function in EnglishAuction
cat -n ./contracts/EnglishAuction.sol | sed -n '110,200p'Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 2863
Add tests for minimumBid/minBidDelta constraints and fee-on-transfer token handling.
The contract properly implements fee-on-transfer support (via actualReceived tracking in bid()), but tests are missing for:
- Boundary conditions rejecting zero values for
minimumBidandminBidDelta - Fee-on-transfer/deflationary token scenarios where
actualReceived < bidAmount
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/EnglishAuction.sol` around lines 67 - 69, Add unit tests for the
constructor parameter checks and fee-on-transfer bidding behavior: write tests
that assert the constructor or initializer reverts when minimumBid == 0 and when
minBidDelta == 0 (referencing the minimumBid and minBidDelta require checks),
and add tests for bid() that simulate a fee-on-transfer/deflationary ERC20 where
transferred amount is less than the specified bid (use a test token that deducts
fees and verify actualReceived < bidAmount), confirming the contract handles
actualReceived correctly (e.g., rejects bids that don't meet minimums or updates
highestBid using actualReceived as implemented in bid()).
Comment on lines
110
to
+139
| function bid(uint256 auctionId, uint256 bidAmount) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
|
|
||
| // First bid validation | ||
| require(auction.highestBid != 0 || bidAmount >= auction.minimumBid, 'First bid should be greater than starting bid'); | ||
|
|
||
| // Increment validation | ||
| require(auction.highestBid == 0 || bidAmount >= auction.highestBid + auction.minBidDelta, 'Bid amount should exceed current bid by atleast minBidDelta'); | ||
| receiveERC20(auction.biddingToken, msg.sender, bidAmount); | ||
| uint256 refund = auction.highestBid; | ||
|
|
||
| // Capture actual tokens received (supports fee-on-transfer tokens) | ||
| uint256 actualReceived = receiveERC20(auction.biddingToken, msg.sender, bidAmount); | ||
|
|
||
| // Refund previous highest bidder | ||
| uint256 previousHighest = auction.highestBid; | ||
| address previousWinner = auction.winner; | ||
|
|
||
| auction.winner = msg.sender; | ||
| auction.highestBid = bidAmount; | ||
| if (refund != 0) { | ||
| sendERC20(auction.biddingToken, previousWinner, refund); | ||
| auction.highestBid = actualReceived; | ||
|
|
||
| if (previousHighest != 0) { | ||
| sendERC20(auction.biddingToken, previousWinner, previousHighest); | ||
| } | ||
| auction.availableFunds = bidAmount; | ||
|
|
||
| // Track only what contract actually received | ||
| auction.availableFunds = actualReceived; | ||
|
|
||
| // Extend deadline | ||
| auction.deadline += auction.deadlineExtension; | ||
| emit bidPlaced(auctionId, msg.sender, bidAmount); | ||
|
|
||
| emit bidPlaced(auctionId, msg.sender, actualReceived); |
There was a problem hiding this comment.
Validate bids using actualReceived to preserve auction invariants.
The checks use bidAmount (requested transfer) while state uses actualReceived. With fee‑on‑transfer tokens, a bidder can pass the checks but still deposit less than minimumBid or less than previousHighest + minBidDelta, effectively underbidding and displacing the current winner. Gate the invariants on actualReceived.
✅ Suggested fix (use actualReceived for validation)
- // First bid validation
- require(auction.highestBid != 0 || bidAmount >= auction.minimumBid, 'First bid should be greater than starting bid');
-
- // Increment validation
- require(auction.highestBid == 0 || bidAmount >= auction.highestBid + auction.minBidDelta, 'Bid amount should exceed current bid by atleast minBidDelta');
-
- // Capture actual tokens received (supports fee-on-transfer tokens)
- uint256 actualReceived = receiveERC20(auction.biddingToken, msg.sender, bidAmount);
+ uint256 previousHighest = auction.highestBid;
+
+ // Capture actual tokens received (supports fee-on-transfer tokens)
+ uint256 actualReceived = receiveERC20(auction.biddingToken, msg.sender, bidAmount);
+
+ // Validate against actual received amount
+ if (previousHighest == 0) {
+ require(actualReceived >= auction.minimumBid, 'First bid should be greater than starting bid');
+ } else {
+ require(actualReceived >= previousHighest + auction.minBidDelta, 'Bid amount should exceed current bid by atleast minBidDelta');
+ }
- // Refund previous highest bidder
- uint256 previousHighest = auction.highestBid;
address previousWinner = auction.winner;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/EnglishAuction.sol` around lines 110 - 139, Validation currently
compares bidAmount but the contract records actualReceived from receiveERC20 (so
fee‑on‑transfer tokens can underbid). In function bid, move or duplicate the
checks to validate using actualReceived (the value returned by receiveERC20)
against auction.minimumBid and auction.highestBid + auction.minBidDelta (and
against auction.highestBid == 0 case) and revert if actualReceived is too low;
keep receiveERC20 call (function receiveERC20) and then perform these require
checks, update auction.winner/auction.highestBid/auction.availableFunds only
after successful validation, and ensure refund logic (previousHighest /
previousWinner) still uses the stored previousHighest.
1 task
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 #46
Addressed Issues:
Fixes #
Summary
This PR improves auction safety and correctness by:
All existing tests pass with no regressions.
Problem
ERC20 Accounting Assumption
Auction contracts assumed that
transferFrom()transfers the full bid amount.This is incorrect for:
This could result in:
Bidding Invariant Weaknesses
EnglishAuction allowed:
Solution
Balance-Difference Accounting
Updated all ERC20 bid transfers to use: