Skip to content

fix: enforce NFT escrow ownership verification before claim #58

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Rav1Chauhan wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: enforce NFT escrow ownership verification before claim #58

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 154 additions & 23 deletions contracts/AllPayAuction.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,15 +6,22 @@ import './ProtocolParameters.sol';
import '@openzeppelin/contracts/token/ERC721/IERC721.sol';
import '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';
import '@openzeppelin/contracts/token/ERC20/IERC20.sol';
import "@openzeppelin/contracts/access/Ownable.sol";
import "@openzeppelin/contracts/utils/Pausable.sol";
Comment on lines +9 to +10
Copy link

@coderabbitai coderabbitai bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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).


/**
* @title AllPayAuction
* @notice Auction contract for NFT and token auctions,where all bidders pay their bid amount but only the highest bidder wins the auction.
*/
contract AllPayAuction is Auction {
constructor (address _protocolParametersAddress) Auction(_protocolParametersAddress){}
mapping(uint256 => AuctionData) public auctions; // auctionId => AuctionData
mapping(uint256 => mapping(address => uint256)) public bids; // auctionId => (bidder => bidAmount)
contract AllPayAuction is Auction, Ownable, Pausable {

constructor(address _protocolParametersAddress)
Auction(_protocolParametersAddress)
Ownable(msg.sender)
{}

mapping(uint256 => AuctionData) public auctions;
mapping(uint256 => mapping(address => uint256)) public bids;

struct AuctionData {
uint256 id;
string name;
Expand All @@ -35,6 +42,7 @@ contract AllPayAuction is Auction {
bool isClaimed;
uint256 protocolFee;
}

event AuctionCreated(
uint256 indexed Id,
string name,
Expand All @@ -52,6 +60,22 @@ contract AllPayAuction is Auction {
uint256 protocolFee
);

// -----------------------
// PAUSE CONTROL
// -----------------------

function pause() external onlyOwner {
_pause();
}

function unpause() external onlyOwner {
_unpause();
}

// -----------------------
// CREATE AUCTION
// -----------------------

function createAuction(
string memory name,
string memory description,
Expand All @@ -64,10 +88,24 @@ contract AllPayAuction is Auction {
uint256 minBidDelta,
uint256 duration,
uint256 deadlineExtension
) external nonEmptyString(name) nonZeroAddress(auctionedToken) nonZeroAddress(biddingToken) {
require(duration > 0, 'Duration should be greater than 0');
receiveFunds(auctionType == AuctionType.NFT, auctionedToken, msg.sender, auctionedTokenIdOrAmount);
)
external
whenNotPaused
nonEmptyString(name)
nonZeroAddress(auctionedToken)
nonZeroAddress(biddingToken)
{
require(duration > 0, "Duration must be > 0");

receiveFunds(
auctionType == AuctionType.NFT,
auctionedToken,
msg.sender,
auctionedTokenIdOrAmount
);

uint256 deadline = block.timestamp + duration;

auctions[auctionCounter] = AuctionData({
id: auctionCounter,
name: name,
Expand All @@ -82,45 +120,138 @@ contract AllPayAuction is Auction {
availableFunds: 0,
minBidDelta: minBidDelta,
highestBid: 0,
winner: msg.sender, //Initially set to auctioneer,to ensure that auctioneer can withdraw funds in case of no bids
winner: msg.sender,
deadline: deadline,
deadlineExtension: deadlineExtension,
isClaimed: false,
protocolFee: protocolParameters.fee()
});
emit AuctionCreated(auctionCounter++, name, description, imgUrl, msg.sender, auctionType, auctionedToken, auctionedTokenIdOrAmount, biddingToken, minimumBid, minBidDelta, deadline, deadlineExtension, protocolParameters.fee());

emit AuctionCreated(
auctionCounter++,
name,
description,
imgUrl,
msg.sender,
auctionType,
auctionedToken,
auctionedTokenIdOrAmount,
biddingToken,
minimumBid,
minBidDelta,
deadline,
deadlineExtension,
protocolParameters.fee()
);
Comment on lines +130 to +145
Copy link

@coderabbitai coderabbitai bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 bid(uint256 auctionId, uint256 bidIncrement) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) {
// -----------------------
// BID
// -----------------------

function bid(uint256 auctionId, uint256 bidIncrement)
external
whenNotPaused
exists(auctionId)
beforeDeadline(auctions[auctionId].deadline)
{
AuctionData storage auction = auctions[auctionId];
require(auction.highestBid != 0 || bids[auctionId][msg.sender] + bidIncrement >= auction.minimumBid, 'First bid should be greater than starting bid');
require(auction.highestBid == 0 || bids[auctionId][msg.sender] + bidIncrement >= auction.highestBid + auction.minBidDelta, 'Bid amount should exceed current bid by atleast minBidDelta');

require(
auction.highestBid != 0 ||
bids[auctionId][msg.sender] + bidIncrement >= auction.minimumBid,
"Bid too low"
);

require(
auction.highestBid == 0 ||
bids[auctionId][msg.sender] + bidIncrement >=
auction.highestBid + auction.minBidDelta,
"Bid below min increment"
);

bids[auctionId][msg.sender] += bidIncrement;
auction.highestBid = bids[auctionId][msg.sender];
auction.winner = msg.sender;
auction.availableFunds += bidIncrement;
auction.deadline += auction.deadlineExtension;

receiveERC20(auction.biddingToken, msg.sender, bidIncrement);

emit bidPlaced(auctionId, msg.sender, bids[auctionId][msg.sender]);
}

function withdraw(uint256 auctionId) external exists(auctionId) {
// -----------------------
// WITHDRAW
// -----------------------

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);
}
Comment on lines +188 to 216
Copy link

@coderabbitai coderabbitai bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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 exists(auctionId) onlyAfterDeadline(auctions[auctionId].deadline) notClaimed(auctions[auctionId].isClaimed) {
// -----------------------
// CLAIM (WITH ESCROW CHECK)
// -----------------------

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
);
}
Comment on lines +222 to +256
Copy link

@coderabbitai coderabbitai bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 -5

Repository: StabilityNexus/HammerAuctionHouse-Solidity

Length of output: 115

🏁 Script executed:

# Check the contract inheritance and imports in AllPayAuction
head -50 contracts/AllPayAuction.sol

Repository: StabilityNexus/HammerAuctionHouse-Solidity

Length of output: 1509

🏁 Script executed:

# Find and examine the withdraw() function in AllPayAuction
rg -A 30 "function withdraw\(" contracts/AllPayAuction.sol

Repository: StabilityNexus/HammerAuctionHouse-Solidity

Length of output: 826

🏁 Script executed:

# Check VickreyAuction for ReentrancyGuard to confirm the reviewer's claim
head -50 contracts/VickreyAuction.sol

Repository: 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.sol

Repository: 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 -20

Repository: 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.sol

Repository: 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.

}
29 changes: 26 additions & 3 deletions contracts/EnglishAuction.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import '@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol';
* @notice Auction contract for NFT and token auctions, where the highest bidder wins the auction and rest of the bidders get their bid refunded.
*/
contract EnglishAuction is Auction {
constructor (address _protocolParametersAddress) Auction(_protocolParametersAddress){}
constructor(address _protocolParametersAddress) Auction(_protocolParametersAddress) {}
mapping(uint256 => AuctionData) public auctions;
struct AuctionData {
uint256 id;
Expand Down Expand Up @@ -87,7 +87,22 @@ contract EnglishAuction is Auction {
isClaimed: false,
protocolFee: protocolParameters.fee()
});
emit AuctionCreated(auctionCounter++, name, description, imgUrl, msg.sender, auctionType, auctionedToken, auctionedTokenIdOrAmount, biddingToken, minimumBid, minBidDelta, deadline, deadlineExtension, protocolParameters.fee());
emit AuctionCreated(
auctionCounter++,
name,
description,
imgUrl,
msg.sender,
auctionType,
auctionedToken,
auctionedTokenIdOrAmount,
biddingToken,
minimumBid,
minBidDelta,
deadline,
deadlineExtension,
protocolParameters.fee()
);
Comment on lines +90 to +105
Copy link

@coderabbitai coderabbitai bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 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 bid(uint256 auctionId, uint256 bidAmount) external exists(auctionId) beforeDeadline(auctions[auctionId].deadline) {
Expand All @@ -114,14 +129,22 @@ contract EnglishAuction is Auction {
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, feeRecipient, fees);
emit Withdrawn(auctionId, withdrawAmount);
}

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);
}
Comment on lines 136 to 149
Copy link

@coderabbitai coderabbitai bot Feb 24, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 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 -n

Repository: 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 -n

Repository: 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 -n

Repository: 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.

}
Loading