Artist signatures can be forged to impersonate the artist behind a collection

[c4-submissions]

Lines of code

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

Vulnerability details

Impact

Artist signatures can be forged, making it possible to sign any message, while "proving" that it was signed by any address.

This can be done to impersonate any address from any artist, breaking a core protocol functionality, and losing users' trust. The signature can't be deleted by anyone, nor the artist address once the collection is signed.

This also breaks one of the main invariants of the protocol:

Properties that should NEVER be broken under any circumstance:

• Only artists can sign their collections.

Evaluated as Medium since it breaks a core contract functionality, and a main invariant, regardless of trusted roles. As any collection admin can impersonate any legit address, which should be consider a way to "escalate their authority beyond their role" as mentioned on the Access Control and Permissions Attack Ideas.

Proof of Concept

setCollectionData() allows to set the artist address when the current collectionTotalSupply == 0:

• NextGenCore.sol#L149
• NextGenCore.sol#L150

The problem is that it doesn't check that the param collectionTotalSupply passed is != 0. Meaning that the function can be executed multiple times to change the artist address, as long as the total supply is zero:

• NextGenCore.sol#L148
• NextGenCore.sol#L153

So, it can be called first to set a fake artist address to call artistSignature() to sign the collection:

• NextGenCore.sol#L257

And then call setCollectionData() again to set the legit artist address.

Once the signature is set, it can't be changed, nor the artist address, as it will always fall under the else clause:

• NextGenCore.sol#L162
• NextGenCore.sol#L258

The following coded POC shows how it can be done.

Coded Proof of Concept

1. Run forge init --no-git --force to initiate Foundry on the root of the project
2. Create test/Poc.t.sol and copy the snippet below
3. Run forge test

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

import {Test, console2} from "forge-std/Test.sol";

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

contract AuctionTest is Test {
    NextGenAdmins admin;
    NextGenCore gencore;
    address collectionAdmin;

    function setUp() public {
        admin = new NextGenAdmins();
        gencore = new NextGenCore("core", "core", address(admin));

        string[] memory collectionScript = new string[](0);

        gencore.createCollection(
            "Name", "Artist", "Description", "Website", "License", "https://base-uri.com/" "Library", "Script", new string[](0)
        );

        uint256 collectionId = 1;
        collectionAdmin = makeAddr("collectionAdmin");
        admin.registerCollectionAdmin(collectionId, collectionAdmin, true);
    }

    function testArtistSignature() public {
        uint256 collectionId = 1;
        address fakeArtist = collectionAdmin;
        uint256 maxCollectionPurchases = 10;
        uint256 zeroSupply = 0; // Supply = 0 allows this attack
        uint256 setFinalSupplyTimeAfterMint = block.timestamp + 30 days;

        // The collection admin sets the initial collection data with a fake artist
        vm.prank(collectionAdmin);
        gencore.setCollectionData(
            collectionId,
            fakeArtist,
            maxCollectionPurchases,
            zeroSupply,
            setFinalSupplyTimeAfterMint
        );

        // Then uses the fake artist address to sign a message impersonating a legit artist
        string memory fakeSignature = "I am the Legit Artist Behind CryptoPunks. This is my signature.";
        vm.prank(fakeArtist);
        gencore.artistSignature(collectionId, fakeSignature);

        address legitArtist = makeAddr("legitArtist");
        uint256 realTotalSupply = 1000;

        // Finally sets the "initial" collection data again, but with the legit artist address and the real total supply
        vm.prank(collectionAdmin);
        gencore.setCollectionData(
            collectionId,
            legitArtist,
            maxCollectionPurchases,
            realTotalSupply,
            setFinalSupplyTimeAfterMint
        );

        // The result is a collection impersonating a legit address, with a fake signature as a proof
        assertEq(gencore.retrieveArtistAddress(collectionId), legitArtist);
        assertEq(gencore.artistsSignatures(collectionId), fakeSignature);
    }
}

Tools Used

Manual Review

Recommended Mitigation Steps

One way to prevent this is by checking that the collection total supply is set to a value > 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 > 0)
            (_collectionTotalSupply <= 10000000000
        ), "err/freezed");

Assessed type

Other

[c4-pre-sort]

141345 marked the issue as duplicate of #478

[c4-judge]

alex-ppg marked the issue as selected for report

[alex-ppg]

The Warden specifies a potential attack path in the artist configuration of a collection by weaponizing the absence of an input sanitization for the _collectionTotalSupply argument.

Specifically, they envision the following scenario:

• A collection is initialized with an artist but a zero _collectionTotalSupply value
• An artist signs the collection (this is permitted by artistSignature)
• A collection is re-initialized as the if structure enters the first conditional, this time with a non-zero collection supply and a different artist

At this point, the collection will have a different artist than the one that signed the collection which breaks an invariant of the protocol and is incorrect behavior.

The Sponsor disputes this submission, but I invite them to re-consider the above step-by-step scenario which is presently possible in the codebase.

I believe that a medium-risk rating for this is correct as the artist associated with a collection is crucial to the collection's value, and a collection will appear "signed" even if the artist did not sign it.

The Warden's submission was selected as the best due to pinpointing the exact problem in the codebase, outlining a step-by-step scenario via which it can be exploited, and assigning it an apt risk rating.

[c4-judge]

alex-ppg marked the issue as satisfactory

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

NextGen admins are responsible for setting up a collection. The steps are here:

https://seize-io.gitbook.io/nextgen/nextgen-smart-contracts/getting-started

Noone can sign a collection besides the address that was set during step2, so the statement that anyone can impersonate artist is totally wrong!

The collection is deemed finalized when a totalSupply is set! Once a totalsupply is set and the artist did not sign, the address of the artist can change. Once the artist signs the collection the signature cannot change. This is the process.

[mcgrathcoutinho]

Hi @alex-ppg , this issue should be marked QA at most or even invalid imo. Here is why:

1. In the followup feedback from the sponsor above, @a2rocket clearly mentions NextGen admins are responsible for setting up a collection. This means not anyone can just create a collection and launch this attack since it requires the NextGen team's trust in a set of people.
2. Even after the filtering process by the NextGen team, the signature can never be forged because on the blockchain you can clearly see who signed the transaction i.e. the call to artistSignature(), which can only be made by the current address set for the collection.
3. Forgery of signature would only make sense for legal purposes and the proof of who signed artistSignature() is present on the Ethereum blockchain.
4. If a collection turns out to be malicious, the NextGen team can just warn users and remove that specific collection from their frontend.

Due to all of these reasons, this issue should be QA or invalid.

Thank you for taking the time to read this.

[alex-ppg]

Hey @mcgrathcoutinho, thanks for providing feedback on this! The submission has been graded as medium in severity due to breaking a core invariant of the protocol, which is that when an artist is associated with a particular collection and has signed it it cannot be altered.

Point 1

The response by the Sponsor concerning NextGen administrators is invalid. You can evaluate the code yourself and see that the collection administrator can independently perform these actions with no input from the administrative team of NextGen.

Point 2 & Point 3

The signature is forced because there is no obligation to provide any form of data in the artistSignature call. Yes, anyone can go on-chain and see when the artistSignature was invoked using off-chain tracking tools but that does not correspond to the on-chain data reality. The on-chain data says that the collection has been signed by an artist who never did so, and this is an irreversible change even by administrators.

Point 4

This is invalid reasoning as the same principle could apply to a wide variety of submissions in this context.

Conclusion

A core invariant of the protocol has been demonstrated to be possible to break with no input by the NextGen administrators whatsoever. Additionally, this invariant is irreversibly broken and cannot be rescued even by administrative action.

As such, I retain my judgment on this submission as being of medium-risk severity.

[0xbtk]

Hey @alex-ppg, based on the readme:

Trusted Roles that can interact with specific functions of the smart contracts and are set on the:

Admins Contract Global Admin Collection Admin Function Admin Artist

Considering this, won't this issue be more fitting in analysis? The entire report centers around Centralization risk, aligning with the SC Verdicts:

Submissions would be separated into subcategories:

Direct misuse of privileges shall be submitted in the Analysis report Reckless admin mistakes are invalid. Assume calls are previewed. Mistakes in code only unblocked through admin mistakes are QA-level

Although it breaks a primary invariant, it's essential to note that only trusted admins can cause this, hence centralization.

[alex-ppg]

Hey @0xbtk, thanks for following up on this. Firstly, I will state that invariants as their name implies should not be broken even with reasonable administrative action. Concerning the relevant SC verdict, I am intricately familiar with it and have cited it throughout the contest. This particular submission does not fall under it because the verdict applies to roles that are directly affiliated with the Sponsor (i.e. function & global administrators).

Collection administrators and artists, for example, are trusted roles in the sense that they have more privileges than normal users but are not a part of "centralization" as they are not affiliated with NextGen. This particular vector can be executed by a collection administrator independently and thus is considered a valid medium-risk issue.