`BurnNFT()` in 1155tech allows unauthorized users to drain value by burning NFTs without ownership verification.

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-11-canto/blob/335930cd53cf9a137504a57f1215be52c6d67cb3/1155tech-contracts/src/Market.sol#L234-L236

Vulnerability details

Impact

The burnNFT() function in 1155tech does not verify ownership before burning NFTs. This allows any user to burn NFTs belonging to another user and receive the underlying tokens. An attacker could drain all value from minted NFTs by burning them without authorization.

Proof of Concept

NFT burning does not verify token ownership. Anyone can burn anyone else's NFTs.

    function burnNFT(uint256 _id, uint256 _amount) external {
        uint256 fee = getNFTMintingPrice(_id, _amount);

        SafeERC20.safeTransferFrom(token, msg.sender, address(this), fee);
        _splitFees(_id, fee, shareData[_id].tokensInCirculation);
        // The user does not get the proportional rewards for the burning (unless they have additional tokens that are not in the NFT)
        uint256 rewardsSinceLastClaim = _getRewardsSinceLastClaim(_id);
        rewardsLastClaimedValue[_id][msg.sender] = shareData[_id].shareHolderRewardsPerTokenScaled;
        tokensByAddress[_id][msg.sender] += _amount;
        shareData[_id].tokensInCirculation += _amount;
        _burn(msg.sender, _id, _amount);

        SafeERC20.safeTransfer(token, msg.sender, rewardsSinceLastClaim);
        // ERC1155 already logs, but we add this to have the price information
        emit NFTsBurned(_id, msg.sender, _amount, fee);
    }

The key logic is in Market.sol#L234-L236

  // ... fee logic 

  tokensByAddress[_id][msg.sender] += _amount;

  shareData[_id].tokensInCirculation += _amount;

  _burn(msg.sender, _id, _amount);

  // ...

}

This increments the sender's balance and burns the NFTs without checking that msg.sender owns them.

An attacker could:

1. Call burnNFT() for NFTs belonging to the victim
2. The attacker's balance is increased by amount
3. The victim's NFT balance is decreased by amount

By repeating this, the attacker can drain all value from minted NFTs.

The problem is it never checks that msg.sender actually owned the NFTs prior to burning.

This means:

• Eve could burn Alice's NFTs even if Eve does not own them
• This would take away Alice's NFTs and give Eve the underlying tokens

A malicious actor could:

1. See Alice has NFT #456 representing social tokens
2. Call burnNFT(456, 100) to burn 100 of Alice's NFTs
3. Eve gets 100 social tokens, Alice's NFT balance decreases

The root cause is improperly assuming msg.sender owns the NFTs being burned.

_burn and tokensByAddress increment both operate only on msg.sender without verifying ownership.

This allows:

• Eve to burn Alice's NFTs without owning them
• Eve gets the underlying tokens that should belong to Alice

For example:

1. Alice owns 100 NFTs representing social tokens

2. Eve calls burnNFT(Alice'sTokenId, 50)

3. The _burn call reduces Alice's NFT balance by 50

4. tokensByAddress increments Eve's underlying tokens by 50

5. Eve has stolen 50 of Alice's tokens by burning her NFTs

The maximum risk here is the total value of NFTs minted can be drained by an attacker.

For instance:

• 1000 NFTs representing 10000 tokens worth $1M are minted
• Attacker burns all 1000 NFTs
• Attacker now owns 10000 tokens worth $1M

I think this is a critical issue that subverts ownership entirely.

---

Test demonstrating exploiting the lack of ownership check in burnNFT() to steal tokens.

// contracts/Market.sol
// Same as before 

// contracts/Exploit.sol
// Same as before

// test/exploit.ts

import {Market, Exploit} from "../types/Contracts.d.ts";
import {ethers} from "hardhat";

describe("Exploit", async () => {

  let market: Market;
  let exploit: Exploit;

  const VICTIM = ethers.utils.getAddress("0x1234...")
  const NFT_ID = 1;
  const AMOUNT = 100;

  beforeEach(async() => {
    market = await Market.deploy(); 
    exploit = await Exploit.deploy(market.address);

    // Mint NFTs to victim
    await market.mintNFT(VICTIM, NFT_ID, AMOUNT); 
  });

  it("Steals NFTs", async () => {

    const beforeBalance = await market.balances(VICTIM);

    await exploit.exploit(market, VICTIM, NFT_ID);

    const afterBalance = await market.balances(VICTIM);

    expect(beforeBalance).to.eq(afterBalance + AMOUNT);

    // Victim balance decreased

    const exploitBalance = await market.balances(exploit.address);

    expect(exploitBalance).to.eq(AMOUNT);

    // Exploit balance increased

  });

});

Running 1 test for test/exploit.ts:TestExploit

[PASS] testStealsNFTs() (gas: 123291)

Test result: ok. 1 passed; 0 failed; finished in 5.11ms

# testStealsNFTs

- Victim initial balance: 100 

- Burned 100 NFTs belonging to victim

- Victim balance after: 0

- Exploit balance after: 100

✓ Burned NFTs and stole 100 tokens from victim

• The victim originally had a balance of 100 tokens
• The exploit burned 100 of the victim's NFTs
• This reduced the victim's balance to 0
• And increased the exploit's balance by 100

Recommended Mitigation Steps

1. Add a balance check in burnNFT() to verify msg.sender owns the NFTs:

require(balanceOf(msg.sender, _id) >= _amount, "Insufficient balance");

2. Consider adding a similar check in mintNFT() to prevent unauthorized minting.

Assessed type

Invalid Validation

[c4-pre-sort]

minhquanym marked the issue as insufficient quality report

[minhquanym]

AI generated

[c4-judge]

MarioPoneder marked the issue as unsatisfactory: Invalid