code-423n4 / 2023-10-nextgen-findings

5 stars 3 forks source link

[M-01] Data in **collectionAdditionalData** can be changed even after setting the data by calling **setCollectionData** function #1972

Closed c4-submissions closed 7 months ago

c4-submissions commented 7 months ago

Lines of code

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

Vulnerability details

Impact

The function setCollectionData can be called more than once till the collectionTotalSupply isnt changed. This can give rise to many issues like :

1) reservedMaxTokensIndex can be changed even after setting the details of the collection. This can be done by first calling the setCollectionData function with _collectionTotalSupply as 0 and then after sometime calling this function again , but this time setting the correct values. These occurences can distrupt the protocol.

2) The values like collectionArtistAddress can also be changed even after the artist has signed the collection.

Proof of Concept

Here is a scenario :

1) A Collection is created by the functional admin and correspoding to the same collectionID the collection data is filled but with _collectionTotalSupply as 0 and with _collectionArtistAddress as the address of the artist of the collection

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;

2) The artist not questioning anything signs the collectionID by calling the artistSignature

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

3) But once this is done , the collection admin can once again call the function this time setting a valid totalSupply and with different _collectionArtistAddress address.

Tools Used

Manual Review

Recommended Mitigation Steps

Consider adding a check check in function setCollectionData for parameter _collectionTotalSupply to be not equal to 0.

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");
        require(_collectionTotalSupply != 0 , "Total Supply cant be 0"); // this check 
        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;
        }
    }

Assessed type

Governance

c4-pre-sort commented 7 months ago

141345 marked the issue as duplicate of #478

c4-judge commented 7 months ago

alex-ppg marked the issue as not a duplicate

c4-judge commented 7 months ago

alex-ppg marked the issue as primary issue

c4-judge commented 7 months ago

alex-ppg marked the issue as duplicate of #478

c4-judge commented 7 months ago

alex-ppg marked the issue as partial-50