Bids will be stuck in the auction contract if the winner doesn't implement onERC721Received

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L112
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L125
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/AuctionDemo.sol#L135

Vulnerability details

Impact

If the winning bidder in the auction contract does not implement the IERC721Receiver#onERC721Received method, the execution of the claimAuction() function will revert because of ERC721#_checkOnERC721Received. As a result, bidders will be unable to retrieve their funds after the auction period ends.

Proof of Concept

Bidders may call 3 methods to participate in the auction or cancel their bid.

1. participateToAuction
2. cancelBid
3. cancelAllBids

When you investigate those functions they share a require block. They can be called only when the block.timestamp is lower or equal to the auction end time. Because of that the bidders can't add bids or withdraw their funds after the auction end.

function participateToAuction(uint256 _tokenid) public payable {
    require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
    // ...
}

function cancelBid(uint256 _tokenid, uint256 index) public {
    require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    // ...
}

function cancelAllBids(uint256 _tokenid) public {
    require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    // ...
}

Logic for returning ether to non-winners is in the claimAuction method. This function goes through all the bids and returns them. For the winning bid it tries to transfer the token to winner using safeTransferFrom function. Here lies the vulnerability.

function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
    // ...
    for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
        if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
            IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
            (bool success, ) = payable(owner()).call{value: highestBid}("");
            // ...
        } else if (auctionInfoData[_tokenid][i].status == true) {
            (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
            // ...
        } else {}
    }
}

In the event that the winning bidder is another smart contract, it must implement the onERC721Received method. Failure to do so results in a transaction revert, leaving the ether used for bidding on a specific token trapped in the contract because participants can't cancel their bids nor send new higher bid to change the winner.

This vulnerability poses a significant risk, as it opens the door for malicious users to launch a Denial of Service (DoS) attack on the contract, effectively locking up a substantial amount of ether. Consider a scenario in which a popular token is being auctioned, and approximately 100 participants place bids. If bids range from 10,000 to 10,099, the total value locked in the contract could reach 1,004,950. An attacker would only need to spend 10,100 to render this amount inaccessible, resulting in a potential loss of approximately 1% of the total value.

Moreover, there is the risk of unintentional contract bricking if the winning bidder forgets to implement the required onERC721Received method.

Tools Used

Manual review

Recommended Mitigation Steps

To address this vulnerability, remove the require statements in the cancelBid and cancelAllBids functions to allow bidders to cancel their bids after auction end.

function cancelBid(uint256 _tokenid, uint256 index) public {
-   require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    // ...
}

function cancelAllBids(uint256 _tokenid) public {
-   require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");
    // ...
}

Assessed type

DoS

[c4-pre-sort]

141345 marked the issue as duplicate of #843

[c4-pre-sort]

141345 marked the issue as duplicate of #486

[c4-judge]

alex-ppg marked the issue as not a duplicate

[c4-judge]

alex-ppg marked the issue as duplicate of #1759

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)