Bug Bounty Report: superuser burn() still in four shipped mocks (incomplete fix of #216/#217/#218; regression shipped in 2.5.0)

Start with the four mocks named in the report: src/contracts/mocks/nf-token-enumerable-mock.sol, nf-token-metadata-mock.sol, nf-token-metadata-enumerable-mock.sol and nf-token-metadata-base-uri-mock.sol. Compare each with nf-token-mock.sol, where the onlyOwner burn() was already removed in #217. Run the existing test suite (npx hardhat test) before and after the change to see whether any test depends on burn() in these mocks. Done when none of the four exposes an owner-only burn(), the suite still passes, and maintainers confirm whether any test-only mock should keep it.

Bug Bounty Report — Superuser burn() remains in four shipped mocks (incomplete fix of #216/#217/#218; regression shipped in 2.5.0–2.6.1)

Title: Superuser burn() removed from one mock but still present in four shipped mocks — incomplete fix of #216/#217/#218, with a regression (new mock added with the function after the fix)
CWE: CWE-269 — Improper Privilege Management (excessive owner capability retained in default template code)
Affected: @nibbstack/erc721 2.6.1 — src/contracts/mocks/nf-token-enumerable-mock.sol (L34), src/contracts/mocks/nf-token-metadata-mock.sol (L51), src/contracts/mocks/nf-token-metadata-enumerable-mock.sol (L54), src/contracts/mocks/nf-token-metadata-base-uri-mock.sol (L56)
Severity: Medium (claimed conservatively; the program's matrix and its own “steal a token” example can justify higher, but we claim the safe floor and defer final adjudication to the maintainers).

---

Summary

In February–March 2019 the project accepted issues #216 / #218 (#216: 2019-02-21, #218: 2019-03-02) and merged PR #217 (2019-03-15), "Breaking dangerous superuser burn into mock and test mocks". The project policy is stated in its own words:

"Mocks" and "examples" should not expose dangerous features by default. For example, superuser burning. — #216 (straight quotes as in the original; the original is two paragraphs, merged here)

"The baseline nf-token-mock in the contracts directory includes a owner-usable destroy function. This makes the contract centralized by default. (Since owner can destroy, create and transfer to somebody else.) The example contract should remove this function or the file name and contract name should be updated to show that the recommended baseline feature set." — #218

"Separately, /testing/ mocks are okay to have this additional features because they are for testing and not general use." — #218 (same issue — exactly why the fix should move the function to the test mocks, as PR #217 did for the base mock)

PR #217 actioned that in one file — src/contracts/mocks/nf-token-mock.sol — and in the README example. The identical onlyOwner burn function was left in place in every other mock in the same directory, and those copies still ship in the latest release. A further mock added in 2021 includes the function from its first commit, i.e. the fixed behavior was not carried into new code.

File (all in scope: src/contracts/**/*.sol)	At fix commit 6610571 (2019)	At 2.6.1 (latest)
nf-token-mock.sol	removed ✓	absent ✓
nf-token-enumerable-mock.sol	present	present
nf-token-metadata-mock.sol	present	present
nf-token-metadata-enumerable-mock.sol	present	present
nf-token-metadata-base-uri-mock.sol	— (did not exist)	present — added 2021-11-03, first shipped in 2.5.0

The last row is the strongest point: commit 80599f3 ("Make tokenURI overridable", 2021-11-03), first released in 2.5.0, introduced nf-token-metadata-base-uri-mock.sol with the superuser burn from day one — over two and a half years after the project accepted that such a function should not be in mocks.

Details

Each affected mock contains:

  /**
   * @dev Removes a NFT from owner.
   * @param _tokenId Which NFT we want to remove.
   */
  function burn(
    uint256 _tokenId
  )
    external
    onlyOwner
  {
    super._burn(_tokenId);
  }

_burn performs no ownership check on the caller; it burns whoever currently owns _tokenId. The only gate is onlyOwner, so the contract owner can destroy any token held by any user, at any time, without approval or knowledge of that user.

Why this matters even though the code is "just mocks":

1. The README documents this folder as the usage reference: "Mock contracts showing basic contract usage are available in the mocks folder." A developer copying nf-token-enumerable-mock.sol or a metadata mock (instead of nf-token-mock.sol) inherits the centralized destroy function along with the mock — the exact deployment pattern #216/#218 decided should not exist by default.
2. At the ERC-721 integration surface the variants are indistinguishable — the standard transferFrom / approve / balanceOf semantics are identical across the mocks — so nothing at that surface signals which variants additionally carry a destroy function; noticing it requires reading each mock's extra functions (e.g. in the ABI).
3. burn() is the only function in the codebase through which the owner can reach into an existing user's holdings (approvals and transfers are guarded by owner/operator checks; mint only creates new ids). Removing it from the base mock was deliberate; leaving it in the other three, and adding it to a fourth, defeats the intent.

The npm package (which is what users actually install) ships ABIs advertising burn on all four contracts and none on the fixed NFTokenMock — confirming this is the default exposure, not an artifact of the repository.

Impact

• Destroy + re-mint = seizure ("revert it to your control" in the program's own wording). Owner calls burn(victimTokenId); the token is destroyed; owner then calls mint(anyAddress, sameTokenId) — the token id now belongs to the attacker. The victim loses the id entirely; this implementation documents that burned ids can be re-minted.
• Metadata destruction. In the metadata variants, _burn also deletes the token URI, so the metadata linkage is destroyed with the token.
• Likelihood: Medium — the practical scenario is copy-then-deploy of the documented mock templates, which affects all token holders of such a deployment. This mirrors the reasoning the project itself accepted in #216/#218 when it required removing the same function from the base mock.
• Scope note, stated honestly: a random address cannot call burn; it is onlyOwner. The defect is centralization-by-default in template code — the same class the maintainers classified as "dangerous" and "centralized by default" in #216/#218.

Proof

Environment: tag 2.6.1 (commit 198dec9), sources unmodified; npm tarball ships every file under src/ byte-identical to the repository (tarball sha256 99cb38a16ab9ea94c1379bbc10c9e816a8c43185faaa317fffec3eeb5b92b032; registry shasum 3febedff66327a628e9c7a43f7b2f9473df85f89 matched). Reproduced both with the project's own Hardhat suite and with Foundry.

Hardhat test (the requested format)

// src/tests/tokens/bounty-superburn.js  (new file, added next to the existing tests)
const { expect } = require('chai');

describe('Bug bounty PoC: superuser burn in shipped mocks (2.6.1)', function() {
  let owner, victim, attacker;

  beforeEach(async () => {
    [owner, victim, attacker] = await ethers.getSigners();
  });

  it('NFTokenEnumerableMock: contract owner burns a token the victim owns, with zero consent', async () => {
    const Factory = await ethers.getContractFactory('NFTokenEnumerableMock');
    const tok = await Factory.deploy();
    await tok.deployed();

    await tok.connect(owner).mint(victim.address, 1234);
    expect(await tok.ownerOf(1234)).to.equal(victim.address);

    // no approval requested, victim signed nothing:
    await expect(tok.connect(owner).burn(1234)).to.emit(tok, 'Transfer')
      .withArgs(victim.address, ethers.constants.AddressZero, 1234);

    await expect(tok.ownerOf(1234)).to.be.revertedWith('003002');
    expect(await tok.totalSupply()).to.equal(0);
  });

  it('NFTokenMetadataMock (same shape) — superuser burn', async () => {
    const Factory = await ethers.getContractFactory('NFTokenMetadataMock');
    const tok = await Factory.deploy('N', 'S');
    await tok.deployed();
    await tok.connect(owner).mint(victim.address, 42, 'ipfs://meta');
    await tok.connect(owner).burn(42);
    await expect(tok.ownerOf(42)).to.be.revertedWith('003002');
  });

  it('NFTokenMetadataEnumerableMock — superuser burn', async () => {
    const Factory = await ethers.getContractFactory('NFTokenMetadataEnumerableMock');
    const tok = await Factory.deploy('N', 'S');
    await tok.deployed();
    await tok.connect(owner).mint(victim.address, 7, 'ipfs://m');
    await tok.connect(owner).burn(7);
    await expect(tok.ownerOf(7)).to.be.revertedWith('003002');
  });

  it('NFTokenMetadataBaseUriMock — regression that shipped after the #217 fix', async () => {
    const Factory = await ethers.getContractFactory('NFTokenMetadataBaseUriMock');
    const tok = await Factory.deploy('N', 'S', 'https://example.com/');
    await tok.deployed();
    await tok.connect(owner).mint(victim.address, 9);
    await tok.connect(owner).burn(9);
    await expect(tok.ownerOf(9)).to.be.revertedWith('003002');
  });

  it('CONTRAST: nf-token-mock (fixed in #217) correctly has NO burn()', async () => {
    const Factory = await ethers.getContractFactory('NFTokenMock');
    const tok = await Factory.deploy();
    await tok.deployed();
    expect(tok.burn).to.equal(undefined);
  });

  it('FULL ATTACKER CHAIN: destroy the victim token then re-mint the same id to the attacker', async () => {
    const Factory = await ethers.getContractFactory('NFTokenEnumerableMock');
    const tok = await Factory.deploy();
    await tok.deployed();

    await tok.connect(owner).mint(victim.address, 1001);
    expect(await tok.ownerOf(1001)).to.equal(victim.address);

    await tok.connect(owner).burn(1001);              // step 1: destroy
    await tok.connect(owner).mint(attacker.address, 1001); // step 2: re-mint to attacker

    expect(await tok.ownerOf(1001)).to.equal(attacker.address);
    expect(await tok.balanceOf(victim.address)).to.equal(0);
    expect(await tok.totalSupply()).to.equal(1);
  });

  it('NEGATIVE CONTROL: a random address cannot burn (documented role requirement)', async () => {
    const Factory = await ethers.getContractFactory('NFTokenEnumerableMock');
    const tok = await Factory.deploy();
    await tok.deployed();
    await tok.connect(owner).mint(victim.address, 1001);
    await expect(tok.connect(attacker).burn(1001)).to.be.revertedWith('018001'); // NOT_CURRENT_OWNER
    expect(await tok.ownerOf(1001)).to.equal(victim.address);
  });
});

Verbatim transcript (Hardhat — includes the project's full suite)

$ npx hardhat test
...
62 passing (4s)   # with this PoC file copied into src/tests/tokens/ first — 55 native tests + 7 from this PoC (a pristine checkout passes 55/55)

$ npx hardhat test src/tests/tokens/bounty-superburn.js
Bug bounty PoC: superuser burn in shipped mocks (2.6.1)
  ✓ NFTokenEnumerableMock: contract owner burns a token the victim owns, with zero consent (184ms)
  ✓ NFTokenMetadataMock (same shape) — superuser burn (89ms)
  ✓ NFTokenMetadataEnumerableMock — superuser burn (63ms)
  ✓ NFTokenMetadataBaseUriMock — regression that shipped after the #217 fix (64ms)
  ✓ CONTRAST: nf-token-mock (fixed in #217) correctly has NO burn()
  ✓ FULL ATTACKER CHAIN: destroy the victim token then re-mint the same id to the attacker (66ms)
  ✓ NEGATIVE CONTROL: a random address cannot burn (documented role requirement) (51ms)
  7 passing (1s)

Verbatim transcript (Foundry)

$ forge test --match-contract StealChain -vv
Ran 3 tests for test/StealChain.t.sol:StealChain
[PASS] test_random_address_cannot_burn() (gas: 179857)
[PASS] test_steal_chain_burn_remint() (gas: 401021)
[PASS] test_steal_chain_with_metadata() (gas: 437009)
Suite result: ok. 3 passed; 0 failed; 0 skipped; finished in 1.43ms

Foundry PoC source (sources unmodified; import paths as in-repo):

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import "forge-std/Test.sol";
import "../src/contracts/mocks/nf-token-enumerable-mock.sol";
import "../src/contracts/mocks/nf-token-metadata-enumerable-mock.sol";

contract StealChain is Test {
    NFTokenEnumerableMock tok;
    NFTokenMetadataEnumerableMock metaTok;

    address deployer;
    address victim = address(0x71C71A);
    address mallory = address(0xBAD);

    function setUp() public {
        deployer = address(this); // test contract = contract owner
        tok = new NFTokenEnumerableMock();
        metaTok = new NFTokenMetadataEnumerableMock("X", "X");
    }

    function test_steal_chain_burn_remint() public {
        tok.mint(victim, 1001);               // victim owns token #1001
        assertEq(tok.ownerOf(1001), victim);

        tok.burn(1001);                       // destroy — no approval, no consent
        tok.mint(mallory, 1001);              // re-mint the same id to attacker

        assertEq(tok.ownerOf(1001), mallory); // seized
        assertEq(tok.balanceOf(victim), 0);
    }

    function test_random_address_cannot_burn() public {
        tok.mint(victim, 1001);
        vm.prank(address(0xF00D));
        vm.expectRevert("018001");            // NOT_CURRENT_OWNER — role honesty
        tok.burn(1001);
    }

    function test_steal_chain_with_metadata() public {
        metaTok.mint(victim, 7, "ipfs://victim-art");
        metaTok.burn(7);                      // ownership + uri destroyed
        metaTok.mint(mallory, 7, "ipfs://attacker-art");
        assertEq(metaTok.ownerOf(7), mallory);
        assertEq(metaTok.tokenURI(7), "ipfs://attacker-art");
    }
}

Verification against the pristine release

$ git checkout 2.6.1 && git rev-parse HEAD
198dec92dffef9259442d132470006d4f0f68896

$ for f in nf-token-mock nf-token-enumerable-mock nf-token-metadata-mock \
           nf-token-metadata-enumerable-mock nf-token-metadata-base-uri-mock; do
    printf "%-42s: " "$f"; grep -c "function burn" src/contracts/mocks/$f.sol; done
nf-token-mock                             : 0
nf-token-enumerable-mock                  : 1
nf-token-metadata-mock                    : 1
nf-token-metadata-enumerable-mock         : 1
nf-token-metadata-base-uri-mock           : 1

Suggested fix (ready to apply)

Apply the same treatment PR #217 applied to nf-token-mock.sol: move burn() out of the contract mocks and into the test mocks under src/tests/mocks/. Verified on a clean checkout of tag 2.6.1.

Files changed

File	Action
src/contracts/mocks/nf-token-enumerable-mock.sol	remove burn()
src/contracts/mocks/nf-token-metadata-mock.sol	remove burn()
src/contracts/mocks/nf-token-metadata-enumerable-mock.sol	remove burn()
src/contracts/mocks/nf-token-metadata-base-uri-mock.sol	remove burn()
src/tests/mocks/nf-token-enumerable-test-mock.sol	add burn()
src/tests/mocks/nf-token-metadata-test-mock.sol	add burn()
src/tests/mocks/nf-token-metadata-enumerable-test-mock.sol	add burn()

(NFTokenTestMock already carries its own burn() from PR #217 — the pattern to follow.)

Patch (applies cleanly with git apply on a checkout of tag 2.6.1)

diff --git a/src/contracts/mocks/nf-token-enumerable-mock.sol b/src/contracts/mocks/nf-token-enumerable-mock.sol
index 3671502..45ecfa0 100644
--- a/src/contracts/mocks/nf-token-enumerable-mock.sol
+++ b/src/contracts/mocks/nf-token-enumerable-mock.sol
@@ -27,17 +27,4 @@ contract NFTokenEnumerableMock is
     super._mint(_to, _tokenId);
   }
 
-  /**
-   * @dev Removes a NFT from owner.
-   * @param _tokenId Which NFT we want to remove.
-   */
-  function burn(
-    uint256 _tokenId
-  )
-    external
-    onlyOwner
-  {
-    super._burn(_tokenId);
-  }
-
 }
diff --git a/src/contracts/mocks/nf-token-metadata-base-uri-mock.sol b/src/contracts/mocks/nf-token-metadata-base-uri-mock.sol
index 45f13d5..03cdd9a 100644
--- a/src/contracts/mocks/nf-token-metadata-base-uri-mock.sol
+++ b/src/contracts/mocks/nf-token-metadata-base-uri-mock.sol
@@ -49,19 +49,6 @@ contract NFTokenMetadataBaseUriMock is
     super._mint(_to, _tokenId);
   }
 
-  /**
-   * @dev Removes a NFT from owner.
-   * @param _tokenId Which NFT we want to remove.
-   */
-  function burn(
-    uint256 _tokenId
-  )
-    external
-    onlyOwner
-  {
-    super._burn(_tokenId);
-  }
-
     /**
    * @dev A distinct URI (RFC 3986) for a given NFT.
    * @param _tokenId Id for which we want uri.
diff --git a/src/contracts/mocks/nf-token-metadata-enumerable-mock.sol b/src/contracts/mocks/nf-token-metadata-enumerable-mock.sol
index 54003da..59c34a1 100644
--- a/src/contracts/mocks/nf-token-metadata-enumerable-mock.sol
+++ b/src/contracts/mocks/nf-token-metadata-enumerable-mock.sol
@@ -47,19 +47,6 @@ contract NFTokenMetadataEnumerableMock is
     super._setTokenUri(_tokenId, _uri);
   }
 
-  /**
-   * @dev Removes a NFT from owner.
-   * @param _tokenId Which NFT we want to remove.
-   */
-  function burn(
-    uint256 _tokenId
-  )
-    external
-    onlyOwner
-  {
-    super._burn(_tokenId);
-  }
-
   /**
    * @dev Mints a new NFT.
    * @notice This is an internal function which should be called from user-implemented external
diff --git a/src/contracts/mocks/nf-token-metadata-mock.sol b/src/contracts/mocks/nf-token-metadata-mock.sol
index 6a678d8..e1145b2 100644
--- a/src/contracts/mocks/nf-token-metadata-mock.sol
+++ b/src/contracts/mocks/nf-token-metadata-mock.sol
@@ -44,17 +44,4 @@ contract NFTokenMetadataMock is
     super._setTokenUri(_tokenId, _uri);
   }
 
-  /**
-   * @dev Removes a NFT from owner.
-   * @param _tokenId Which NFT we want to remove.
-   */
-  function burn(
-    uint256 _tokenId
-  )
-    external
-    onlyOwner
-  {
-    super._burn(_tokenId);
-  }
-
 }
diff --git a/src/tests/mocks/nf-token-enumerable-test-mock.sol b/src/tests/mocks/nf-token-enumerable-test-mock.sol
index 2614669..afea3a8 100644
--- a/src/tests/mocks/nf-token-enumerable-test-mock.sol
+++ b/src/tests/mocks/nf-token-enumerable-test-mock.sol
@@ -48,4 +48,17 @@ contract NFTokenEnumerableTestMock is
     return idToIndex[_tokenId];
   }
 
+ /**
+   * @dev Removes a NFT from owner.
+   * @param _tokenId Which NFT we want to remove.
+   */
+  function burn(
+    uint256 _tokenId
+  )
+    external
+    onlyOwner
+  {
+    super._burn(_tokenId);
+  }
+
 }
diff --git a/src/tests/mocks/nf-token-metadata-enumerable-test-mock.sol b/src/tests/mocks/nf-token-metadata-enumerable-test-mock.sol
index 056705f..27c5b68 100644
--- a/src/tests/mocks/nf-token-metadata-enumerable-test-mock.sol
+++ b/src/tests/mocks/nf-token-metadata-enumerable-test-mock.sol
@@ -25,4 +25,17 @@ contract NFTokenMetadataEnumerableTestMock is
     return idToUri[_tokenId];
   }
 
+ /**
+   * @dev Removes a NFT from owner.
+   * @param _tokenId Which NFT we want to remove.
+   */
+  function burn(
+    uint256 _tokenId
+  )
+    external
+    onlyOwner
+  {
+    super._burn(_tokenId);
+  }
+
 }
diff --git a/src/tests/mocks/nf-token-metadata-test-mock.sol b/src/tests/mocks/nf-token-metadata-test-mock.sol
index afd1b17..e8d2a37 100644
--- a/src/tests/mocks/nf-token-metadata-test-mock.sol
+++ b/src/tests/mocks/nf-token-metadata-test-mock.sol
@@ -25,4 +25,17 @@ contract NFTokenMetadataTestMock is
     return idToUri[_tokenId];
   }
 
+ /**
+   * @dev Removes a NFT from owner.
+   * @param _tokenId Which NFT we want to remove.
+   */
+  function burn(
+    uint256 _tokenId
+  )
+    external
+    onlyOwner
+  {
+    super._burn(_tokenId);
+  }
+
 }

Verification

$ git apply fix.patch
$ npx hardhat test
55 passing (4s)

$ npm run solhint
(clean)

$ for f in src/contracts/mocks/*.sol; do printf "%-60s: " "$f"; grep -c "function burn" "$f"; done
src/contracts/mocks/nf-token-enumerable-mock.sol          : 0
src/contracts/mocks/nf-token-metadata-base-uri-mock.sol   : 0
src/contracts/mocks/nf-token-metadata-enumerable-mock.sol : 0
src/contracts/mocks/nf-token-metadata-mock.sol            : 0
src/contracts/mocks/nf-token-mock.sol                     : 0

All five contract mocks end up without burn(); the test mocks that need it for the shipped test suite keep it. Zero test regressions, solhint clean (CI runs npm run solhint && npm test).

Scope compliance

• All referenced files are under src/contracts/**/*.sol (in scope), at the latest release 2.6.1.
• The npm 2.6.1 tarball was verified: all files under src/ are byte-identical to the repository at tag 2.6.1 (tarball sha256 99cb38a1…; registry shasum 3febedff… matched). The tarball additionally ships build artifacts (build/, cache/, coverage/, abi/), as expected.
• All work was run locally (Foundry + the project's Hardhat suite). No external systems touched.
• Prior-art check: the closest items are #216 / #217 / #218 (the fix this report shows is incomplete / was not carried into new code). Reviewing all issues and pull requests (300+), no published or open issue covers the four remaining files or the 2021 addition. The superseded round-1 items (#46, closed) do not cover this either.