c4-submissions commented 7 months ago

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/AuctionDemo.sol#L104

Vulnerability details

Impact

In the claimAuction function used to claim bids after a successful auction, it is seen that the looping of function used to return asset to user and refund other bidders is not well done. As a situation arises that if the highest bidder has done many bids, the claimAuction will always fail, leaving all bidders including other bidders funds stuck.

Proof of Concept

We're walking through this function here

for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
            //@audit-check: users cannot claimauction as this function will always revert, due to multiple attempt to send same token to user if bidder bids more than once: high
            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}("");
                emit ClaimAuction(owner(), _tokenid, success, highestBid);
            } else if (auctionInfoData[_tokenid][i].status == true) {
                //@audit-check: with enough bidders this might cause DOS due to not enough gas to process transaction; allow them to request for refund: high
                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
                emit Refund(auctionInfoData[_tokenid][i].bidder, _tokenid, success, highestBid);
                //@audit-check: low: no need for else
            } else {}
        }

The auctionInfoData[_tokenid] is used for looping
There is a check to see if the caller is the highestBidder if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
owner of token was originally determined outside the loop and then there is a function to send the NFT to the highestBidder IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid); inside the loop meaning, there will be multiple attempts to send the NFT from originalOwner to highestBidder, the first attempt will be successful but on further attempt due to multiple bidding, the new ownerOfToken should be the highestBidder but still assumed to be the former owner, the function will still attempt to resend same token and cause a revert
This would revert all calls and NFT will be stuck in contract, user will never get NFT and almost other bidders will not be refunded also.

Tools Used

Manual Review

Recommended Mitigation Steps

Check to see if transfer of NFT have been done already by checking the owner of the NFT each time before attempting to transfer token

Assessed type

ERC721

c4-pre-sort commented 7 months ago

141345 marked the issue as insufficient quality report

141345 commented 7 months ago

the if check has multiple conditions, other bids won't == true

alex-ppg commented 7 months ago

The core rationale is a duplicate of #1984 albeit describing that the NFT transfer would fail rather than the contract would be drained.

c4-judge commented 7 months ago

alex-ppg marked the issue as duplicate of #1984