In AuctionDemo, a user can win the auctionned token and get his funds back

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Because of combination of multiples issues in the AuctionDemo contract, a malicious user can make a sandwich attack on another user to take the auctionned token/NFT and recover his funds.

Proof of Concept

In the AuctionDemo contract, we have multiples problems.
Firstly, I think for a more safe auction process, when an auction is ended, we mustn't be able to re-bid.
And we can see the functions participateToAuction(), cancelBid() and cancelAllBids() makes a require check for time that block.timestamp <= auctionEndTime:

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

And function claimAuction() make a require check for time that block.timestamp >= auctionEndTime.

    function claimAuction(uint256 _tokenid) public WinnerOrAdminRequired(_tokenid,this.claimAuction.selector){
        require(block.timestamp >= minter.getAuctionEndTime(_tokenid) && auctionClaim[_tokenid] == false && minter.getAuctionStatus(_tokenid) == true);
    }

As the result when block.timestamp == auctionEndTime, users can potentially claim, participate to the auction and cancel a bid.

There's also the following problem. The status of an auctionInfoData (auctionInfoData[_tokenid][index].status) is set to true inside participateToAuction() and set to false in cancelBid() and cancelAllBids(). This permit to verify if for an index of the array, the bid is claimable or not. The problem inside the claimAuction() is the status is not set to false when ETH are send to the user, this problem turn to a vulnerability in the scenario below.

Because of the time checking problem in the contract functions, check this potential scenario of a sandwich attack:

1. Alice and Bob decide to participate in the auction of a tokenID, in using participateToAuction()
2. Sarah try to be the highest bidder by sending her transaction in the last accepted block.timestamp
3. Bob see that in the mempool and decide to frontrun alice by calling claimAuction(). Bob claim the tokens because it's the highest bidder and all the bids are send back to all bidders.
4. The Alice transaction is executed and she becomes the highest bidder and send ETH to the contract.
5. Now Bob backrun the Alice transaction in using cancelBid() for his bid. Because the contract doesn't set the auctionInfoData[_tokenid][index].status to false in claimAuction() and now the contract have the Sarah funds (which is of course more than the Bob amount because Sarah send a transaction to be the highest bidder), Bob retrieve his funds by stealing Alice's deposit.

Bob won the Token/NFT for free and he steal the funds of a user.

Now Alice can't withdraw her deposited ETH because the AuctionDemo balance is less than her bid and all call to claimAuction() or cancelBid() wil revert.
And if the same contract is used to auction other tokenId, Alice can withdraw her ETH but impossibility to use claimAuction() for other tokenId because there are an offset between the internal balance of the contract and values saved in all the auctionInfoData[_tokenid][i].bid.

Logs

You have the image with the logs of the PoC

You can check this Gist to setup foundry with the smart contracts for an executable PoC:
https://gist.github.com/AxelAramburu/c10597b5ff60616b8a15d091f88de8da

And execute this command: forge test --mt testClaimAuctionForFree -vvv

Tools Used

Manuall Review, Foundry

Recommended Mitigation Steps

You can fix this issue by change the time check in the claimAuction() function, you have also to set the auctionInfoData[_tokenid][i].status to false when the ETH are redeemed:

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

Assessed type

Timing

[c4-pre-sort]

141345 marked the issue as duplicate of #1172

[c4-judge]

alex-ppg marked the issue as duplicate of #1323

[c4-judge]

alex-ppg marked the issue as satisfactory