fix: enforce NFT escrow ownership verification before claim#58
fix: enforce NFT escrow ownership verification before claim#58Rav1Chauhan wants to merge 1 commit intoStabilityNexus:mainfrom
Conversation
WalkthroughMultiple auction contracts are updated with ownership and pausability controls, adding owner-restricted pause/unpause functions. All contracts implement NFT escrow verification in claim functions to ensure the contract holds the NFT before processing claims. Function signatures are guarded with whenNotPaused modifiers, and event signatures are expanded with additional parameters. These changes address security concerns around preventing unauthorized NFT withdrawal during auctions. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 2❌ Failed checks (2 warnings)
✅ 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: 9
🤖 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/AllPayAuction.sol`:
- Around line 9-10: The PR adds Ownable and Pausable to AllPayAuction and
ExponentialReverseDutchAuction but not to the other auction contracts; update
EnglishAuction, VickreyAuction, LinearReverseDutchAuction, and
LogarithmicReverseDutchAuction to match: add imports for
"@openzeppelin/contracts/access/Ownable.sol" and
"@openzeppelin/contracts/utils/Pausable.sol", have each contract inherit Ownable
and Pausable (e.g., contract EnglishAuction is ... Ownable, Pausable), and apply
pausability to user-facing state-changing functions (e.g., bid, placeBid,
createAuction, settle, finalize etc.) by using whenNotPaused where appropriate
and adding onlyOwner pause/unpause entrypoints if not present; ensure
constructors remain compatible with Ownable and no-op for Pausable, and keep
this change isolated from the NFT escrow fix (or move to a separate PR).
- Around line 188-216: The withdraw function currently allows anyone to trigger
payouts at any time; update withdraw(AuctionData storage auction) to enforce the
auction deadline and restrict who can call it: add a require that
block.timestamp > auction.deadline (or use the existing onlyAfterDeadline
modifier if available) and require(msg.sender == auction.auctioneer) (or an
authorized role) before zeroing auction.availableFunds and sending funds/fees;
keep the existing fee calculation (auction.protocolFee) and sendERC20 calls but
only execute them after these checks to prevent mid-auction draining.
- Around line 130-145: The emit uses post-increment auctionCounter++ which
relies on post-increment semantics and can be confusing; to improve clarity,
capture the id in a local variable (e.g., uint256 auctionId = auctionCounter),
use that variable in the AuctionCreated emit, then increment auctionCounter in
the next statement (auctionCounter = auctionId + 1 or auctionCounter++); update
the AuctionCreated emit call to use auctionId so the code reads clearly and
behavior is unchanged.
- Around line 222-256: Add ReentrancyGuard to AllPayAuction for
defense-in-depth: import OpenZeppelin's ReentrancyGuard, have AllPayAuction
inherit ReentrancyGuard, and annotate external functions that perform external
calls (e.g., claim() and withdraw()) with the nonReentrant modifier; ensure the
contract compiles after updating the imports and the inheritance list so CEI
remains but reentrancy protection is explicit and consistent with
VickreyAuction.
In `@contracts/EnglishAuction.sol`:
- Around line 90-105: The emit uses auctionCounter++ inline which relies on
post-increment semantics; to improve clarity, capture the new auction id in a
local variable before emitting (e.g., uint256 auctionId = auctionCounter++; )
and use auctionId in the AuctionCreated emit instead of auctionCounter++; do
this consistently for the AuctionCreated emits (and other similar emits) across
the auction contracts (same pattern as in AllPayAuction) so the increment and
emitted id are explicit.
- Around line 136-149: The bid() flow is vulnerable to reentrancy because it
calls receiveERC20() before updating auction state; fix by importing/using a
ReentrancyGuard and applying nonReentrant to all external state-changing entry
points (bid(), withdraw(), claim()), and by reordering bid() so state is updated
(e.g., update auctions[auctionId] highestBid/highestBidder and any
balances/flags) before calling receiveERC20(); also review receiveERC20() and
sendFunds() to ensure they make no unsafe external calls that assume post-call
state and avoid relying on mutable contract state during those external calls.
In `@contracts/ExponentialReverseDutchAuction.sol`:
- Around line 223-238: The claim() function is currently restricted by the
whenNotPaused modifier which lets a paused contract block winners from redeeming
— remove whenNotPaused from ExponentialReverseDutchAuction.claim (and mirror the
change in AllPayAuction.claim) so winners can always call claim(), or
alternatively implement a dedicated emergency withdrawal/claim path that
bypasses pause (e.g., claimWhenPaused or emergencyClaim) or a time-limited pause
auto-unpause mechanism; update function modifiers and any access checks in
claim(), AuctionData.isClaimed handling, and sendFunds usage to ensure the new
flow still verifies escrow (IERC721.ownerOf) and prevents double-claims.
- Around line 20-82: The decayLookup storage array causes expensive cold SLOADs;
replace it by computing decay values on-the-fly in getDecayValue instead of
reading from the uint256[61] decayLookup storage variable: remove the storage
array (decayLookup) and implement getDecayValue(uint256 step) to validate step <
61 and return the base fixed-point value shifted right by step (e.g., start from
1e18 and use bit/shift or division by 2**step) so no storage reads are needed;
update any callers to rely on the new pure/view getDecayValue implementation and
adjust tests accordingly (refer to decayLookup and getDecayValue to locate
changes).
In `@contracts/VickreyAuction.sol`:
- Around line 177-196: The claim function performs external calls (sendERC20 and
sendFunds) and should be protected with reentrancy guards for defense-in-depth;
update the claim declaration to include the nonReentrant modifier (the same used
by revealBid and withdraw) and ensure the contract inherits/uses ReentrancyGuard
if not already present; keep the current isClaimed assignment but still add
nonReentrant to claim(uint256 auctionId) to prevent reentrant calls into
claim/withdraw/revealBid via malicious ERC20/ERC721 callbacks.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
contracts/AllPayAuction.solcontracts/EnglishAuction.solcontracts/ExponentialReverseDutchAuction.solcontracts/LinearReverseDutchAuction.solcontracts/LogarithmicReverseDutchAuction.solcontracts/VickreyAuction.sol
| import "@openzeppelin/contracts/access/Ownable.sol"; | ||
| import "@openzeppelin/contracts/utils/Pausable.sol"; |
There was a problem hiding this comment.
Pausable/Ownable added here but missing from EnglishAuction, VickreyAuction, LinearReverseDutchAuction, and LogarithmicReverseDutchAuction.
Only AllPayAuction and ExponentialReverseDutchAuction get Ownable + Pausable in this PR. The other four auction contracts have no pause mechanism. This inconsistency means an emergency pause would only partially halt the protocol. If pausing is deemed necessary, it should be applied uniformly. Additionally, this change is unrelated to the NFT escrow fix and arguably should be in a separate PR.
As per coding guidelines: "Flag pull requests that mix unrelated changes or multiple concerns in a single submission."
Also applies to: 15-20, 67-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/AllPayAuction.sol` around lines 9 - 10, The PR adds Ownable and
Pausable to AllPayAuction and ExponentialReverseDutchAuction but not to the
other auction contracts; update EnglishAuction, VickreyAuction,
LinearReverseDutchAuction, and LogarithmicReverseDutchAuction to match: add
imports for "@openzeppelin/contracts/access/Ownable.sol" and
"@openzeppelin/contracts/utils/Pausable.sol", have each contract inherit Ownable
and Pausable (e.g., contract EnglishAuction is ... Ownable, Pausable), and apply
pausability to user-facing state-changing functions (e.g., bid, placeBid,
createAuction, settle, finalize etc.) by using whenNotPaused where appropriate
and adding onlyOwner pause/unpause entrypoints if not present; ensure
constructors remain compatible with Ownable and no-op for Pausable, and keep
this change isolated from the NFT escrow fix (or move to a separate PR).
| emit AuctionCreated( | ||
| auctionCounter++, | ||
| name, | ||
| description, | ||
| imgUrl, | ||
| msg.sender, | ||
| auctionType, | ||
| auctionedToken, | ||
| auctionedTokenIdOrAmount, | ||
| biddingToken, | ||
| minimumBid, | ||
| minBidDelta, | ||
| deadline, | ||
| deadlineExtension, | ||
| protocolParameters.fee() | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
auctionCounter++ inside emit — subtle but correct; worth a clarity note.
The post-increment auctionCounter++ emits the current counter value (the ID just assigned to the auction) and then increments it for the next auction. This is correct but relies on post-increment semantics that can confuse readers. A minor readability improvement would be to increment separately:
Optional clarity refactor
- emit AuctionCreated(
- auctionCounter++,
+ emit AuctionCreated(
+ auctionCounter,
...
);
+ auctionCounter++;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/AllPayAuction.sol` around lines 130 - 145, The emit uses
post-increment auctionCounter++ which relies on post-increment semantics and can
be confusing; to improve clarity, capture the id in a local variable (e.g.,
uint256 auctionId = auctionCounter), use that variable in the AuctionCreated
emit, then increment auctionCounter in the next statement (auctionCounter =
auctionId + 1 or auctionCounter++); update the AuctionCreated emit call to use
auctionId so the code reads clearly and behavior is unchanged.
| function withdraw(uint256 auctionId) | ||
| external | ||
| whenNotPaused | ||
| exists(auctionId) | ||
| { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
|
|
||
| uint256 withdrawAmount = auction.availableFunds; | ||
| auction.availableFunds = 0; | ||
| uint256 fees = (auction.protocolFee * withdrawAmount) / 10000; | ||
|
|
||
| uint256 fees = | ||
| (auction.protocolFee * withdrawAmount) / 10000; | ||
|
|
||
| address feeRecipient = protocolParameters.treasury(); | ||
| sendERC20(auction.biddingToken, auction.auctioneer, withdrawAmount - fees); | ||
| sendERC20(auction.biddingToken,feeRecipient,fees); | ||
|
|
||
| sendERC20( | ||
| auction.biddingToken, | ||
| auction.auctioneer, | ||
| withdrawAmount - fees | ||
| ); | ||
|
|
||
| sendERC20( | ||
| auction.biddingToken, | ||
| feeRecipient, | ||
| fees | ||
| ); | ||
|
|
||
| emit Withdrawn(auctionId, withdrawAmount); | ||
| } |
There was a problem hiding this comment.
withdraw() lacks access control and deadline guard — funds can be drained mid-auction.
Anyone can call withdraw() at any time (even before the deadline), sending all accumulated availableFunds to the auctioneer. While this doesn't directly steal user funds, it breaks the expected auction lifecycle — the auctioneer can siphon bid proceeds while the auction is still active. Other auction types (e.g., EnglishAuction) gate withdraw with onlyAfterDeadline.
At minimum, add a deadline check. Consider restricting the caller as well.
Proposed fix
function withdraw(uint256 auctionId)
external
whenNotPaused
exists(auctionId)
+ onlyAfterDeadline(auctions[auctionId].deadline)
{As per coding guidelines: "Validate proper access control enforcement (e.g., Ownable, RBAC, role checks)."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/AllPayAuction.sol` around lines 188 - 216, The withdraw function
currently allows anyone to trigger payouts at any time; update
withdraw(AuctionData storage auction) to enforce the auction deadline and
restrict who can call it: add a require that block.timestamp > auction.deadline
(or use the existing onlyAfterDeadline modifier if available) and
require(msg.sender == auction.auctioneer) (or an authorized role) before zeroing
auction.availableFunds and sending funds/fees; keep the existing fee calculation
(auction.protocolFee) and sendERC20 calls but only execute them after these
checks to prevent mid-auction draining.
| function claim(uint256 auctionId) | ||
| external | ||
| whenNotPaused | ||
| exists(auctionId) | ||
| onlyAfterDeadline(auctions[auctionId].deadline) | ||
| notClaimed(auctions[auctionId].isClaimed) | ||
| { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
|
|
||
| // 🔐 ENSURE NFT ESCROW | ||
| if (auction.auctionType == AuctionType.NFT) { | ||
| require( | ||
| IERC721(auction.auctionedToken) | ||
| .ownerOf(auction.auctionedTokenIdOrAmount) == | ||
| address(this), | ||
| "NFT not escrowed" | ||
| ); | ||
| } | ||
|
|
||
| auction.isClaimed = true; | ||
| sendFunds(auction.auctionType == AuctionType.NFT, auction.auctionedToken, auction.winner, auction.auctionedTokenIdOrAmount); | ||
| emit Claimed(auctionId, auction.winner, auction.auctionedToken, auction.auctionedTokenIdOrAmount); | ||
| } | ||
| } | ||
|
|
||
| // TODO | ||
| sendFunds( | ||
| auction.auctionType == AuctionType.NFT, | ||
| auction.auctionedToken, | ||
| auction.winner, | ||
| auction.auctionedTokenIdOrAmount | ||
| ); | ||
|
|
||
| emit Claimed( | ||
| auctionId, | ||
| auction.winner, | ||
| auction.auctionedToken, | ||
| auction.auctionedTokenIdOrAmount | ||
| ); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
# Check ReentrancyGuard usage in AllPayAuction
rg -n "ReentrancyGuard" --type=sol contracts/Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 113
🏁 Script executed:
# Find the AllPayAuction contract file and check its declaration
find . -name "AllPayAuction.sol" -type f | head -5Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 115
🏁 Script executed:
# Check the contract inheritance and imports in AllPayAuction
head -50 contracts/AllPayAuction.solRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 1509
🏁 Script executed:
# Find and examine the withdraw() function in AllPayAuction
rg -A 30 "function withdraw\(" contracts/AllPayAuction.solRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 826
🏁 Script executed:
# Check VickreyAuction for ReentrancyGuard to confirm the reviewer's claim
head -50 contracts/VickreyAuction.solRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 2036
🏁 Script executed:
# Check where sendFunds and sendERC20 are defined (likely in Auction.sol)
rg -n "function sendFunds" contracts/Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 217
🏁 Script executed:
# Check where sendERC20 is defined
rg -n "function sendERC20" contracts/Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 201
🏁 Script executed:
# Look at Auction abstract contract
cat contracts/abstract/Auction.solRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 3482
🏁 Script executed:
# Check for other external calls in AllPayAuction that might have reentrancy issues
rg -n "IERC" contracts/AllPayAuction.sol | head -20Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 257
🏁 Script executed:
# Check if there are other functions in AllPayAuction that might be vulnerable
rg -n "safeTransferFrom\|safeTransfer\|transfer\(" contracts/AllPayAuction.solRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 68
Consider adding ReentrancyGuard for consistency and defense-in-depth, even though current code is protected by proper state ordering.
The NFT escrow verification in claim() is correct. However, AllPayAuction lacks ReentrancyGuard while VickreyAuction includes it. Both claim() and withdraw() make external calls—ownerOf() plus safeTransferFrom() / safeTransfer()—which could theoretically be exploited by malicious token contracts.
That said, both functions follow the Checks-Effects-Interactions (CEI) pattern: isClaimed is set to true before sendFunds(), and availableFunds is set to 0 before sendERC20(). This protects against reentrancy in practice. Still, adding ReentrancyGuard to the contract inheritance would align with VickreyAuction's approach and provide stronger defense-in-depth protection against future refactoring mistakes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/AllPayAuction.sol` around lines 222 - 256, Add ReentrancyGuard to
AllPayAuction for defense-in-depth: import OpenZeppelin's ReentrancyGuard, have
AllPayAuction inherit ReentrancyGuard, and annotate external functions that
perform external calls (e.g., claim() and withdraw()) with the nonReentrant
modifier; ensure the contract compiles after updating the imports and the
inheritance list so CEI remains but reentrancy protection is explicit and
consistent with VickreyAuction.
| emit AuctionCreated( | ||
| auctionCounter++, | ||
| name, | ||
| description, | ||
| imgUrl, | ||
| msg.sender, | ||
| auctionType, | ||
| auctionedToken, | ||
| auctionedTokenIdOrAmount, | ||
| biddingToken, | ||
| minimumBid, | ||
| minBidDelta, | ||
| deadline, | ||
| deadlineExtension, | ||
| protocolParameters.fee() | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
auctionCounter++ inside emit — same pattern as AllPayAuction.
Post-increment semantics are correct (emits the just-assigned ID), but this pattern is repeated across all 6 auction contracts. See the comment on AllPayAuction.sol regarding optional clarity improvement.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/EnglishAuction.sol` around lines 90 - 105, The emit uses
auctionCounter++ inline which relies on post-increment semantics; to improve
clarity, capture the new auction id in a local variable before emitting (e.g.,
uint256 auctionId = auctionCounter++; ) and use auctionId in the AuctionCreated
emit instead of auctionCounter++; do this consistently for the AuctionCreated
emits (and other similar emits) across the auction contracts (same pattern as in
AllPayAuction) so the increment and emitted id are explicit.
| function claim(uint256 auctionId) external exists(auctionId) onlyAfterDeadline(auctions[auctionId].deadline) notClaimed(auctions[auctionId].isClaimed) { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
|
|
||
| // Verify NFT is properly escrowed | ||
| if (auction.auctionType == AuctionType.NFT) { | ||
| require(IERC721(auction.auctionedToken).ownerOf(auction.auctionedTokenIdOrAmount) == address(this), 'NFT not escrowed'); | ||
| } | ||
|
|
||
| auction.isClaimed = true; | ||
|
|
||
| sendFunds(auction.auctionType == AuctionType.NFT, auction.auctionedToken, auction.winner, auction.auctionedTokenIdOrAmount); | ||
|
|
||
| emit Claimed(auctionId, auction.winner, auction.auctionedToken, auction.auctionedTokenIdOrAmount); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the Auction abstract contract and examine sendFunds
fd -t f "\.sol$" | xargs grep -l "function sendFunds"Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 116
🏁 Script executed:
# Check if ReentrancyGuard is imported or used in EnglishAuction
fd -t f "EnglishAuction\.sol" | xargs grep -E "(ReentrancyGuard|import)"Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 335
🏁 Script executed:
# Examine the complete EnglishAuction contract structure
fd -t f "EnglishAuction\.sol" | head -1 | xargs cat -nRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 6989
🏁 Script executed:
# Check withdraw() function and bid() function in EnglishAuction
fd -t f "EnglishAuction\.sol" | xargs grep -A 15 "function withdraw\|function bid"Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 1992
🏁 Script executed:
# Read the Auction abstract contract
fd -t f "Auction\.sol" -path "*/abstract/*" | head -1 | xargs cat -nRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 254
🏁 Script executed:
# Specifically search for sendFunds and receiveERC20, sendERC20 implementations
fd -t f "Auction\.sol" -path "*/abstract/*" | xargs grep -A 10 "function sendFunds\|function receiveERC20\|function sendERC20"Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 256
🏁 Script executed:
# Find and read Auction.sol
find . -name "Auction.sol" -type f | grep abstract | head -1 | xargs cat -nRepository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 4168
🏁 Script executed:
# Search for function implementations in the abstract Auction
find . -name "Auction.sol" -type f | xargs grep -A 12 "function sendFunds\|function receiveERC20\|function sendERC20"Repository: StabilityNexus/HammerAuctionHouse-Solidity
Length of output: 1347
NFT escrow verification in claim() is correctly implemented.
The check at lines 140-142 properly verifies contract ownership of the NFT before transfer, and isClaimed = true is set before the external call to sendFunds() (correct checks-effects-interactions pattern).
However, the contract lacks ReentrancyGuard entirely, and external calls exist across multiple functions. While claim() and withdraw() follow the CEI pattern correctly, bid() has a more critical reentrancy exposure: it calls receiveERC20() at line 112 before updating state (lines 113-121), allowing a malicious token to reenter and manipulate auction state. This is a pre-existing architectural issue beyond the scope of this PR's changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/EnglishAuction.sol` around lines 136 - 149, The bid() flow is
vulnerable to reentrancy because it calls receiveERC20() before updating auction
state; fix by importing/using a ReentrancyGuard and applying nonReentrant to all
external state-changing entry points (bid(), withdraw(), claim()), and by
reordering bid() so state is updated (e.g., update auctions[auctionId]
highestBid/highestBidder and any balances/flags) before calling receiveERC20();
also review receiveERC20() and sendFunds() to ensure they make no unsafe
external calls that assume post-call state and avoid relying on mutable contract
state during those external calls.
| uint256[61] private decayLookup = [ | ||
| 1000000000000000000, | ||
| 500000000000000000, | ||
| 250000000000000000, | ||
| 125000000000000000, | ||
| 62500000000000000, | ||
| 31250000000000000, | ||
| 15625000000000000, | ||
| 7812500000000000, | ||
| 3906250000000000, | ||
| 1953125000000000, | ||
| 976562500000000, | ||
| 488281250000000, | ||
| 244140625000000, | ||
| 122070312500000, | ||
| 61035156250000, | ||
| 30517578125000, | ||
| 15258789062500, | ||
| 7629394531250, | ||
| 3814697265625, | ||
| 1907348632812, | ||
| 953674316406, | ||
| 476837158203, | ||
| 238418579102, | ||
| 119209289551, | ||
| 59604644775, | ||
| 29802322388, | ||
| 14901161194, | ||
| 7450580597, | ||
| 3725290298, | ||
| 1862645149, | ||
| 931322574, | ||
| 465661287, | ||
| 232830643, | ||
| 116415322, | ||
| 58207661, | ||
| 29103831, | ||
| 14551915, | ||
| 7275958, | ||
| 3637979, | ||
| 1818989, | ||
| 909495, | ||
| 454747, | ||
| 227373, | ||
| 113687, | ||
| 56843, | ||
| 28422, | ||
| 14211, | ||
| 7105, | ||
| 3553, | ||
| 1776, | ||
| 888, | ||
| 444, | ||
| 222, | ||
| 111, | ||
| 56, | ||
| 28, | ||
| 14, | ||
| 7, | ||
| 3, | ||
| 2, | ||
| 1 | ||
| ]; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
decayLookup as a storage array — acceptable but gas-heavy on cold reads.
Each cold SLOAD costs ~2100 gas. getDecayValue reads 1-2 entries per call. Since Solidity doesn't support constant or immutable arrays, this is the standard approach. An alternative is to compute the values on-the-fly using bit shifts (since these are exact powers of 2), which would use only cheap arithmetic opcodes:
Optional: compute instead of lookup
- function getDecayValue(uint256 x) internal view returns (uint256) {
+ function getDecayValue(uint256 x) internal pure returns (uint256) {
if (x >= 61 * 1e5) return 0;
uint256 scaledPower = x / 1e5;
uint256 remainder = x % 1e5;
- if (remainder == 0) return decayLookup[scaledPower];
- uint256 higherValue = decayLookup[scaledPower];
- uint256 lowerValue = scaledPower < 60 ? decayLookup[scaledPower + 1] : 0;
+ uint256 higherValue = 1e18 >> scaledPower;
+ if (remainder == 0) return higherValue;
+ uint256 lowerValue = scaledPower < 60 ? (1e18 >> (scaledPower + 1)) : 0;
return higherValue - ((higherValue - lowerValue) * remainder) / 1e5;
}This eliminates the 61-slot storage array entirely, saving ~122,000 gas on deployment and converting SLOADs to SHR instructions (~3 gas each).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/ExponentialReverseDutchAuction.sol` around lines 20 - 82, The
decayLookup storage array causes expensive cold SLOADs; replace it by computing
decay values on-the-fly in getDecayValue instead of reading from the uint256[61]
decayLookup storage variable: remove the storage array (decayLookup) and
implement getDecayValue(uint256 step) to validate step < 61 and return the base
fixed-point value shifted right by step (e.g., start from 1e18 and use bit/shift
or division by 2**step) so no storage reads are needed; update any callers to
rely on the new pure/view getDecayValue implementation and adjust tests
accordingly (refer to decayLookup and getDecayValue to locate changes).
| function claim(uint256 auctionId) public whenNotPaused exists(auctionId) notClaimed(auctions[auctionId].isClaimed) { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
| require(block.timestamp > auction.deadline || auction.winner != auction.auctioneer,"Invalid call"); | ||
|
|
||
| require(block.timestamp > auction.deadline || auction.winner != auction.auctioneer, 'Invalid call'); | ||
|
|
||
| // 🔐 ENSURE NFT IS ESCROWED | ||
| if (auction.auctionType == AuctionType.NFT) { | ||
| require(IERC721(auction.auctionedToken).ownerOf(auction.auctionedTokenIdOrAmount) == address(this), 'NFT not escrowed'); | ||
| } | ||
|
|
||
| auction.isClaimed = true; | ||
|
|
||
| sendFunds(auction.auctionType == AuctionType.NFT, auction.auctionedToken, auction.winner, auction.auctionedTokenIdOrAmount); | ||
|
|
||
| emit Claimed(auctionId, auction.winner, auction.auctionedToken, auction.auctionedTokenIdOrAmount); | ||
| } |
There was a problem hiding this comment.
NFT escrow verification is correct. However, whenNotPaused on claim() can lock winners' assets.
The escrow check at Lines 229-231 is properly implemented. The concern is that claim() is gated by whenNotPaused — if the owner pauses the contract (e.g., in an emergency), winners cannot claim their legitimately won NFTs. In the worst case, a malicious or compromised owner could pause the contract indefinitely, effectively holding NFTs and bidder funds hostage.
Consider either:
- Removing
whenNotPausedfromclaim()so winners can always redeem, or - Adding a time-limited pause (e.g., auto-unpause after N blocks) or a separate emergency withdrawal path.
This same concern applies to AllPayAuction.claim().
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/ExponentialReverseDutchAuction.sol` around lines 223 - 238, The
claim() function is currently restricted by the whenNotPaused modifier which
lets a paused contract block winners from redeeming — remove whenNotPaused from
ExponentialReverseDutchAuction.claim (and mirror the change in
AllPayAuction.claim) so winners can always call claim(), or alternatively
implement a dedicated emergency withdrawal/claim path that bypasses pause (e.g.,
claimWhenPaused or emergencyClaim) or a time-limited pause auto-unpause
mechanism; update function modifiers and any access checks in claim(),
AuctionData.isClaimed handling, and sendFunds usage to ensure the new flow still
verifies escrow (IERC721.ownerOf) and prevents double-claims.
| function claim(uint256 auctionId) external exists(auctionId) onlyAfterDeadline(auctions[auctionId].bidRevealEnd) notClaimed(auctions[auctionId].isClaimed) { | ||
| AuctionData storage auction = auctions[auctionId]; | ||
|
|
||
| // 🔐 Ensure NFT escrow | ||
| if (auction.auctionType == AuctionType.NFT) { | ||
| require(IERC721(auction.auctionedToken).ownerOf(auction.auctionedTokenIdOrAmount) == address(this), 'NFT not escrowed'); | ||
| } | ||
|
|
||
| auction.isClaimed = true; | ||
|
|
||
| uint256 refund = bids[auctionId][auction.winner] - auction.winningBid; | ||
| if (refund != 0) sendERC20(auction.biddingToken, auction.winner, refund); | ||
|
|
||
| if (refund != 0) { | ||
| sendERC20(auction.biddingToken, auction.winner, refund); | ||
| } | ||
|
|
||
| sendFunds(auction.auctionType == AuctionType.NFT, auction.auctionedToken, auction.winner, auction.auctionedTokenIdOrAmount); | ||
|
|
||
| emit Claimed(auctionId, auction.winner, auction.auctionedToken, auction.auctionedTokenIdOrAmount); | ||
| } |
There was a problem hiding this comment.
claim() lacks nonReentrant modifier despite making multiple external calls.
revealBid and withdraw both use nonReentrant, but claim() makes two external calls — sendERC20 (Line 190) for the refund and sendFunds (Line 193) for the NFT/token transfer — without reentrancy protection. A malicious ERC721 onERC721Received callback or a malicious ERC20 token could reenter claim() before isClaimed is set (state is updated on Line 185, but sendERC20 for refund happens after, then sendFunds). Actually, isClaimed = true is set before the external calls here, which helps via the notClaimed guard, but a reentrant call to other functions (e.g., withdraw) is still possible.
Consider adding nonReentrant for defense-in-depth:
- function claim(uint256 auctionId) external exists(auctionId) onlyAfterDeadline(auctions[auctionId].bidRevealEnd) notClaimed(auctions[auctionId].isClaimed) {
+ function claim(uint256 auctionId) external nonReentrant exists(auctionId) onlyAfterDeadline(auctions[auctionId].bidRevealEnd) notClaimed(auctions[auctionId].isClaimed) {As per coding guidelines: "Review for common smart contract vulnerabilities, including but not limited to: Reentrancy."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@contracts/VickreyAuction.sol` around lines 177 - 196, The claim function
performs external calls (sendERC20 and sendFunds) and should be protected with
reentrancy guards for defense-in-depth; update the claim declaration to include
the nonReentrant modifier (the same used by revealBid and withdraw) and ensure
the contract inherits/uses ReentrancyGuard if not already present; keep the
current isClaimed assignment but still add nonReentrant to claim(uint256
auctionId) to prevent reentrant calls into claim/withdraw/revealBid via
malicious ERC20/ERC721 callbacks.
1 participant
Addressed Issues:
Fixes #52
Screenshots/Recordings:
N/A — This change introduces backend escrow validation and does not affect UI behavior.
Additional Notes:
Problem
Although NFTs are transferred into the contract during auction creation, there was no explicit on-chain verification ensuring that the contract still owns the NFT at the time of claim.
This could theoretically allow edge cases where the NFT is not properly escrowed, potentially leading to fund loss for the winning bidder.
Solution
Added an ownership verification check inside all claim() functions for NFT auctions:
require(
IERC721(auction.auctionedToken).ownerOf(auction.auctionedTokenIdOrAmount) == address(this),
"NFT not escrowed"
);
This ensures the NFT is properly escrowed before being transferred to the winner.
Impact
High — Prevents potential asset loss and strengthens escrow guarantees.
Testing
Contracts compile successfully using Hardhat
Escrow verification triggers correctly if NFT ownership is invalid
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