`returnHighestBidder` of `Auction.sol` will return wrong bidder rather than highest due to missing logic in for loop.

[c4-submissions]

Lines of code

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

Vulnerability details

Summary

When returnHighestBidder is called. To find highest bidder for _tokenid loop runs on auctionInfoStru[] array for that _tokenid. But in for loop one line is missing to update highBid o each iteration. Because of it highBid remains 0 for all loop iterations. And each bid value will show higher than highBid so it will return last positive bid's bidder which status is true rather than returning highest bid bidder.

Vulnerability Details

In this function's for loop highBid updating line is missing so it will remain 0 for all iterations. Because of it this function will not return highest bid bidder. Since each time condition auctionInfoData[_tokenid][i].bid > highBid will be true even if bid is very little. And index will be updated even if it bid is lesser than previous with true status. it will update the index even if bid is lesser than previous it just should be above 0 with true status and index will be updated. Resulting in wrong bidder returning for that index.

/hardhat/smart-contracts/AuctionDemo.sol#L87-L100

87:    function returnHighestBidder(uint256 _tokenid) public view returns (address) {
        uint256 highBid = 0;
        uint256 index;
        for (uint256 i=0; i< auctionInfoData[_tokenid].length; i++) {
91:            if (auctionInfoData[_tokenid][i].bid > highBid && auctionInfoData[_tokenid][i].status == true) {
92:                index = i;
              //@audit highBid = auctionInfoData[_tokenid][i].bid  line is missing in this loop so highBid always remains 0
93:            }
        }
        if (auctionInfoData[_tokenid][index].status == true) {
                return auctionInfoData[_tokenid][index].bidder;
            } else {
                revert("No Active Bidder");
        }
    }

Impact

Due to returning wrong bidder. Bidder will not match with the highest bid because returnHighestBid function is working fine and returning highest bid amount. returnHighestBidder returning wrong bidder. Due to this mismatch claimAuction can never transfer NFT to highest bidder. Also the bid value for all these bidders will be transferred to them. It makes the whole AuctionDemo.sol contract useless.

In claimAuction function if highestBid is correct and highestBidder is wrong than both will mismatches. And due to this both won't be found simultaneously at same index.

Then highestBid will be of different bidder. And current bidder's bid is not highest due to this mismatch , both condition at line 111 in if(auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid will never be simultaneously true at same index resulting in that if condition at line 111 will always be false.

They both will be from different index in array. To pass this condition both should be on same index. For that both highestBid and highestBidder should be correct which is not correct currently.

So it will trigger only second else if(){} block(line 115) of this function and all the ethers of all the bidders including highest bidder will be transferred to the bidders addresses. And makes the whole Auction.Demo contract useless.

hardhat/smart-contracts/AuctionDemo.sol##L104-L120

104:  function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
            ...
107:        uint256 highestBid = returnHighestBid(_tokenid);
108:        address ownerOfToken = IERC721(gencore).ownerOf(_tokenid);
109:        address highestBidder = returnHighestBidder(_tokenid);
110:        for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
111:            if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && auctionInfoData[_tokenid][i].status == true) {
112:                IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
113:                (bool success, ) = payable(owner()).call{value: highestBid}("");
114:               emit ClaimAuction(owner(), _tokenid, success, highestBid);
115:            } else if (auctionInfoData[_tokenid][i].status == true) {
116:                (bool success, ) = payable(auctionInfoData[_tokenid][i].bidder).call{value: auctionInfoData[_tokenid][i].bid}("");
              ...

Tools Used

Manual Review

Recommended Mitigation Steps

Add the missing line into returnHighestBidder function so that it can return right highest bidder. And so it can match with it's highest bid and work fine as intended.

File:  hardhat/smart-contracts/AuctionDemo.sol##L104-L120

87:    function returnHighestBidder(uint256 _tokenid) public view returns (address) {
        uint256 highBid = 0;
        uint256 index;
        for (uint256 i=0; i< auctionInfoData[_tokenid].length; i++) {
91:            if (auctionInfoData[_tokenid][i].bid > highBid && auctionInfoData[_tokenid][i].status == true) {
+          highBid = auctionInfoData[_tokenid][i].bid;
92:                index = i;
93:            }
        }
        if (auctionInfoData[_tokenid][index].status == true) {
                return auctionInfoData[_tokenid][index].bidder;
            } else {
                revert("No Active Bidder");
        }
    }

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #586

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid