Highest bid is sent to wrong owner in claimAuction

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

In Attack Ideas there is:

"Consider ways in which the owner of the token will not receive the funds of the highest bid after an Auction is claimed."

This makes it clear that expected behavior of the AuctionDemo.claimAuction function is to send highest bid to the owner of the auctioned token.

However, highest bid is sent to the owner of the AuctionDemo contract instead (AuctionDemo inherits from Ownable).

So if the owner of the contract is different from the owner of the token, then highest bid is sent to wrong address.

Proof of Concept

Full code to and exact steps to reproduce the issue are in this secret gist.

Here is only test that demonstrates the issue (without setup):

    function testWrongOwner() public {
        // get id of of the auctioned token
        uint256 tokenId = gencore.viewTokensIndexMin(collectionId);

        // check initial state
        assertEq(gencore.ownerOf(tokenId), nftOwner);
        assertEq(auction.owner(), auctionOwner);

        // Ownabele owner of the contract
        // and owner of the auction tokenId
        // are two different addresses
        assertTrue((auctionOwner != nftOwner));
        assertEq(auctionOwner.balance, 0);
        assertEq(nftOwner.balance, 0);

        // required approval so that claimAuction won't revert
        vm.prank(nftOwner);
        gencore.approve(address(auction), tokenId);

        // user1 participates in the acution
        vm.prank(user1);
        auction.participateToAuction{value: 1 ether}(tokenId);

        // user2 wins the auction
        vm.prank(user2);
        auction.participateToAuction{value: 2 ether}(tokenId);

        // wait until claimAuction available
        vm.warp(minter.getAuctionEndTime(tokenId) + 12);

        // call claimAuction
        vm.prank(user2);
        auction.claimAuction(tokenId);

        // check final state
        assertEq(gencore.ownerOf(tokenId), user2);
        assertEq(auction.owner(), auctionOwner);

        // Verify that ETH was sent to auctionOwner, not nftOwner
        assertTrue((auctionOwner != nftOwner));
        assertEq(auctionOwner.balance, 2 ether);
        assertEq(nftOwner.balance, 0);
    }

Tools Used

Foundry, manual analysis

Recommended Mitigation Steps

Change owner() to ownerOfToken in AuctionDemo.claimAuction function.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #245

[c4-judge]

alex-ppg marked the issue as satisfactory