NextGen - Bauchibred's results

Lines of code

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

Vulnerability details

Impact

• All bidders funds provided in participateToAuction() are locked in the contract, including the highest bidder's.
• Token to be sent to the highest bidder is also stuck in the contract
• Lastly, the owner would never receive the funds from the winning bid since the auction can never get claimed, all the above cover more than one case under the requested Attack ideas (Where to look for bugs) regarding auctions.

• Consider ways in which the token is not transferred to the final winning bidder of an Auction after the auction finishes (token approval to the AuctionDemo contract is needed) and the funds are not refunded to other bidders.
• Consider ways in which a cancelled auction bid does not return the funds back to the bidder.
• Consider ways in which the owner of the token will not receive the funds of the highest bid after an Auction is claimed.

Proof of Concept

Take a look at AuctionDemo.sol#L102-L120

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        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) {
              //@audit
                IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

As seen, this function is used to claim the auction after the end time of that particular tokenId's auction has reached/passed.

Do note that there are two transfer methods that are available to ERC721s as can be seen here, i.e transferFrom() & safeTransferFrom(). Now a popular approach to these two functions in the web3 development space is to adopt the latter as it routes the call via a _safeTransfer() that ensures that the receiving contract is aware of the ERC721 protocol so as to prevent tokens from being forever locked.

    function safeTransferFrom(address from, address to, uint256 tokenId) public virtual override {
        safeTransferFrom(from, to, tokenId, "");
    }

    function _safeTransfer(address from, address to, uint256 tokenId, bytes memory data) internal virtual {
        _transfer(from, to, tokenId);
        require(_checkOnERC721Received(from, to, tokenId, data), "ERC721: transfer to non ERC721Receiver implementer");
    }

Where as this is a great work around to ensure safety of tokens, context matters and in this instance implementing this could lead to all funds sent by every participator in the auction to get stuck in the AuctionDemo.sol contract.

Step by Step POC

• A new bidder or previous bidder that wants to place a higher bid calls on participateToAuction() to place a new bid.

• A check is made that the ether value passed is more than the current highest bid.

• Two more checks are applied:

  • One to ensure the auction has not ended, and

  • The auction status for the tokenId is still true.

  NB: In all these, no hints/checks/documentations are attached to this function to require participators be able to interact with ERC721 tokens before placing a bid.

• Auction ends, i.e block.timestamp >= minter.getAuctionEndTime(_tokenid), making both the winner or admin have access to call the claimAuction().

• ⚠️ Winner (highest bidder) ends up being an address that can't deal with ERC721 tokens.

• Winner/Admin calls claimAuction().

• While attempting to claim the auction, the transfer of the tokenID is done using the IERC721.safeTransferFrom() and as previously explained in the report, this would revert in our case since winner is an address that can't deal with ERC721 tokens.

NB: The reversion would not only affect the transfer of the NFT tokenId to the winner but also all other bids placed by every other participant getss effectively stuck in the contract, key to note that there is no method to refund these tokens since an attempt to cancelBid() would revert due to this line require(block.timestamp <= minter.getAuctionEndTime(_tokenid), "Auction ended");

Argument for Severity

I'm submitting this as a high, cause even if this happens just once for the lifetime of the project, every participators funds are effectively lost to the contract and based on Code4rena's Severity Categorization, this should be classified as high.. quoting below.

3 — High: Assets can be stolen/lost/compromised directly (or indirectly if there is a valid attack path that does not have hand-wavy hypotheticals).

Additionally note that there are valid arguments that this could happen multiple times, since some popular NFTs are actually implemented as ERC1155s and this fact might lead people who own these type of NFTS or even know about them to think that a contract that can work with ERC1155 tokens would have no problem integrating with NextGen Auctions and placing/winning bids.

Tool used

Manual Review

Recommended Mitigation Steps

Using safeTransferFrom() in this instance should be dropped and instead transferFrom() should be used, but at the same time, clearly document and inform participators that while placing bids they should ensure that their contracts can interact with ERC 721s

Note that both recommendations must be implemented to effectively mitigate this as only adding a documentation around this does not solve this cause, a user could just intentionally/unintentionally provide an address that can't receive ERC721 tokens via safeTransfer() and still block every other person's funds

Code changes to implement:

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
        auctionClaim[_tokenid] = true;
        uint256 highestBid = returnHighestBid(_tokenid);
        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
        address highestBidder = returnHighestBidder(_tokenid);
        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);
+                IERC721(gencore).transferFrom(ownerOfToken, highestBidder, _tokenid);
                (bool success, ) = payable(owner()).call{value: highestBid}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
            } else {}
        }
    }

         // participate to auction
+   // callers SHOULD be able to receive ERC721 or their NFTs would be locked in their contract if they win the bid.
    function participateToAuction(uint256 _tokenid) public payable {
        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
        auctionInfoStru memory newBid = auctionInfoStru(msg.sender, msg.value, true);
        auctionInfoData[_tokenid].push(newBid);
    }

Additional Note

Unlike other contracts that employ a emergencyWithdraw() function, this is not available in the AuctionDemo.sol, so introducing the emergency withdrawal function would've helped as the owner can just call this to get the funds stuck and then refund it to all participators, but this is also just a work around and does not solve the issue, cause the root cause is how the transfer of the NFT is being made to the highest bidder after the auction ends.

Assessed type

ERC721