Auction payout goes to AuctionDemo contract owner, not the token owner

[c4-submissions]

Lines of code

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

Vulnerability details

Summary

At the end of an auction in AuctionDemo, the highest bidder claims the token, this transfers the token from the token owner to the auction winner. In the same transaction, the token owner should receive the auction payout.

However, the function AuctionDemo::claimAuction() sends the payout to the AuctionDemo contract owner. This behavior deviates from the listed invariant 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.

1. Auction is started, alice deployed the AuctionDemo contract and cecilia approved the AuctionDemo contract to transfer her token to the winning bidder.
2. Auction is completed. The highest bid was bob.
3. bob claims his winnings. The token is transfered from cecilia to bob. The bid from bob is sent to alice. cecilia gets nothing.

Impact

Any auction executed through AuctionDemo will have proceeds sent to the AuctionDemo contract owner, not the token owner. The token owner is left without auction proceeds.

PoC

context("Auction Sends proceeds to owner of auctiondemo, not the token owner", () => {
    it.only("should execute", async () => {
    const tokenOwner = signers.addr2;
    const nextGenOwner = signers.owner;
    const highestBidder = signers.addr3;

    // Auction contract and collections Setup
    const AuctionDemo = await ethers.getContractFactory("auctionDemo");
    let auctionDemo = await AuctionDemo.deploy(
        await contracts.hhMinter.getAddress(),
        await contracts.hhCore.getAddress(),
        await contracts.hhAdmin.getAddress()
    );
    timestamp = (await ethers.provider.getBlock(await ethers.provider.getBlockNumber())).timestamp;

    await contracts.hhCore.addMinterContract(contracts.hhMinter.getAddress());
    await contracts.hhCore.createCollection(
        "Test Collection 1",
        "Artist 1",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
    );
    await contracts.hhCore.addRandomizer(1, contracts.hhRandomizer.getAddress());
    await contracts.hhCore.connect(signers.owner).setCollectionData(1, signers.addr1.getAddress(), 100, 200, timestamp + 250);

    await contracts.hhMinter.setCollectionCosts(1, ethers.parseEther('.1'), ethers.parseEther('.1'), 0, 100, 0, signers.addr1.getAddress())
    await contracts.hhMinter.setCollectionPhases(1, timestamp + 25, timestamp + 50, timestamp + 100, timestamp + 150, '0x0000000000000000000000000000000000000000000000000000000000000000')

    await ethers.provider.send("evm_increaseTime", [20]);
    await contracts.hhMinter.connect(nextGenOwner).mintAndAuction(tokenOwner.getAddress(), "Test Auction 1", 10, 1, timestamp + 100);

    id1 = 10000000000;
    await contracts.hhCore.connect(tokenOwner).approve(auctionDemo.getAddress(), id1);

    // Winning auction bid
    await auctionDemo.connect(highestBidder).participateToAuction(id1, { value: ethers.parseEther('2') });

    // Move past auction end time and claim token
    await ethers.provider.send("evm_setNextBlockTimestamp", [timestamp + 101]);
    const transaction = auctionDemo.connect(highestBidder).claimAuction(id1);

    // get owner of auditDemo contract and make sure it's not the token owner for this usecase
    let owner = await auctionDemo.connect(nextGenOwner).owner();
    expect(owner).to.not.equal(tokenOwner.getAddress());

    // NextGen owner receives proceeds, token owner receives nothing.
    await expect(() => transaction).to.changeEtherBalance(nextGenOwner, ethers.parseEther('2'));
    await expect(() => transaction).to.changeEtherBalance(tokenOwner, 0);
    expect(await contracts.hhCore.ownerOf(id1)).to.eq(await highestBidder.getAddress());
    });
});

Tools Used

Manual Review

Recommendations

1. Send the auction proceeds to ownerOfToken instead of owner.

AuctionDemo L113-L114

@@ -110,8 +110,8 @@ contract auctionDemo is Ownable {
for (uint256 i=0; i< auctionInfoData[_tokenid].length; i ++) {
    if (auctionInfoData[_tokenid][i].bidder == highestBidder && auctionInfoData[_tokenid][i].bid == highestBid && 112| auctionInfoData[_tokenid][i].status == true) {
        IERC721(gencore).safeTransferFrom(ownerOfToken, highestBidder, _tokenid);
-       (bool success, ) = payable(owner()).call{value: highestBid}("");
-       emit ClaimAuction(owner(), _tokenid, success, highestBid);
+       (bool success, ) = payable(ownerOfToken).call{value: highestBid}("");
+       emit ClaimAuction(ownerOfToken, _tokenid, success, highestBid);

Assessed type

ETH-Transfer

[c4-pre-sort]

141345 marked the issue as duplicate of #245

[c4-judge]

alex-ppg marked the issue as satisfactory

[alex-ppg]

After re-visiting, I consider this submission to be better than #738 because it also correctly specifies that the event should be fixed rather than just the statement. While I cannot penalize submissions for not including the event in their proposed remediations, I can mark a submission that cites it and is of equivalent quality as "best".

[c4-judge]

alex-ppg marked the issue as selected for report

[mcgrathcoutinho]

Hi @alex-ppg , here is why I believe this issue is QA at most:

1. As per the sponsors comments here, the owner of the auction contract and the owner of the token are trusted team's address. This means that whether the funds are sent to either, they are not lost, just that the team needs to make an additional fund transfer on their end (if needed).
2. Although the invariant is broken, it has no impact on the functioning of the protocol.
3. Due to this, I believe the severity should be QA at most.

Thank you for taking the time to read this.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for contributing! The code goes against its specification and breaks an invariant of the protocol. Regardless of severity, an invariant being broken will always be considered a medium-risk issue given that it relates to pivotal functionality in the system being incorrect.

In this case, funds are sent to a NextGen address rather than a collection-affiliated address or secondary smart contract meant to facilitate fund disbursements. This has implications tax-wise, implications about trust (i.e. if a 10m auction is held, the stakes of trust are increased significantly), and other such problems. Logistically, it is also a heavy burden to manage multiple auction payments at once, prove which source sent which, and so on.

Combining the above with the fact that a clear invariant of the protocol is broken, I will maintain the medium-risk rating.