The owner of the auctioned token does not receive the funds after an auction ends

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Funds are transfered to a different address than they should.

When an auction ends, the owner() of the AuctionDemo contract receives the funds from the auction, instead of the token owner.

This breaks one of the main invarians of the protocol:

Properties that should NEVER be broken under any circumstance:

• The highest bidder will receive the token after an auction finishes, the owner of the token will receive the funds and all other participants will get refunded.

Evaluating the severity as Medium, since it breaks a main invariant of the protocol involving assets transfers (regardless of any trusted roles, as the contract performs an action that it shouldn't).

Proof of Concept

Tokens put into auction are first airdropped to a specific _recipient address.

    function mintAndAuction(address _recipient, ...)
        /// ...
        gencore.airDropTokens(mintIndex, _recipient, _tokenData, _saltfun_o, _collectionID);

MinterContract.sol#L282

Then when an auction ends, the earnings are paid to the AuctionDemo contract owner(), which is the owner of the contract defined by the inherited OpenZeppelin Ownable contract.

    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 {}
        }
    }

AuctionDemo.sol#L113

Tools Used

Manual Review

Recommended Mitigation Steps

Pay the earnings to the corresponding receiver, instead of the contract owner.

    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}("");
+               (bool success, ) = payable(ownerOfToken).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 {}
        }
    }

Assessed type

ETH-Transfer

[c4-pre-sort]

141345 marked the issue as duplicate of #245

[c4-judge]

alex-ppg marked the issue as selected for report

[alex-ppg]

The Warden specifies that the highest bid of a sale is sent to the contract's owner rather than the token holder.

The Sponsor initially declined this claim (#245), however, they appear to have retracted their initial assessment as they have confirmed this submission.

I consider the submission valid as the owner of the AuctionDemo may be unrelated to the intended auction proceed recipient which will be holding the token until the auction ends and I judge it to be a medium severity issue as the funds would be sent to a trusted party (NextGen Administrator) which could then "unblock" the situation by transmitting the funds to the correct party, meaning that no loss of funds occurs but the protocol is impacted.

This submission was judged as the best due to its correct severity categorization, invariant voidance citation, and correct mitigation steps.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-judge]

alex-ppg marked issue #971 as primary and marked this issue as a duplicate of 971