search

code-423n4 / 2023-10-nextgen-findings

5 stars 3 forks source link

NFT Claiming Issue Due to Lacking Ownership #1987

Closed c4-submissions closed 7 months ago

c4-submissions commented 7 months ago

Lines of code

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

Vulnerability details

Impact

The vulnerability in the AuctionDemo contract has a significant impact on the ability of auction winners to claim their NFTs. The root cause of the vulnerability is that the AuctionDemo contract fails to check whether it owns or has approval to transfer the NFT token being auctioned before open participation. If the contract doesn't own the token or have the necessary approval, the winner will be unable to claim the auctioned NFT.

Proof of Concept

To demonstrate this vulnerability, consider a scenario in which the admin starts an auction in the AuctionDemo contract via minter.mintAndAuction call but sets the recipient for the NFT to an address that is not equal to address(AuctionDemo). When a participant becomes the winner and attempts to claim the NFT, the contract fails to transfer it due to the lacking ownership.

  1. An admin calls minter.mintAndAuction to start an auction, but the recipient for the NFT token is set to an address that doesn't belong to the AuctionDemo contract.
  2. Participants are allowed to join the auction via the participateToAuction function, and one of them becomes the winner.
  3. When the winner attempts to claim the NFT, the AuctionDemo contract fails to transfer the NFT to the winner, as it doesn't own the NFT or have approval to transfer it.

Tools Used

Manual Review

Recommended Mitigation Steps

To mitigate this issue, it is recommended to implement a check to ensure that the AuctionDemo contract either owns the NFT or has the necessary approval to transfer the token before allowing participants to join the auction. 

// File: smart-contracts/AuctionDemo.sol
57:    function participateToAuction(uint256 _tokenid) public payable {
58:        require(msg.value > returnHighestBid(_tokenid) && block.timestamp <= minter.getAuctionEndTime(_tokenid) && minter.getAuctionStatus(_tokenid) == true);
+          require(IERC721(gencore).ownerOf(_tokenid) == address(this), "NFT not owned");
61:    }

Assessed type

Access Control

c4-sponsor commented 7 months ago

a2rocket (sponsor) disputed

a2rocket commented 7 months ago

mintAndAuction mints a token to a trusted wallet, that wallet will setApprovalForAll also.

c4-pre-sort commented 7 months ago

141345 marked the issue as duplicate of #245

c4-pre-sort commented 7 months ago

141345 marked the issue as not a duplicate

c4-pre-sort commented 7 months ago

141345 marked the issue as duplicate of #364

c4-judge commented 7 months ago

alex-ppg marked the issue as unsatisfactory: Overinflated severity