Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

artistSignature can be replaced in setCollectionData function with zero collectionTotalSupply #478

Closed
c4-submissions opened this issue Nov 7, 2023 · 6 comments
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-741 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality

Comments

@c4-submissions
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact
Collection admin can overwrite the collectionArtistAddress when the collectionTotalSupply is zero.
This can lead to confusion, incorrect attribution of artwork ownership, and potential disputes between artists and collection administrators.
Proof of Concept
The vulnerability exists in the setCollectionData function, allowing the collection admin to replace the collectionArtistAddress when the current collectionTotalSupply is zero.
This inconsistency occurs as the collection admin can update the collectionArtistAddress.
Consequently, this prevents the intended artist from registering their signature for the selected collectionId.

  1. collection admin calls the setCollectionData function for the first time and the _collectionTotalSupply is zero, and the _collectionArtistAddress is A
    https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L147
  2. artist A calls the artistSignature function to register his signature.
    https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L257
  3. collection admin calls setCollectionData again with another artist B, it can be updated successfully because the collectionTotalSupply is zero.
    https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L149
  4. However, artist B cannot update the artistSignature because it has already been registered by artist A.
    https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L257

Tools Used
Manual Review
Recommended Mitigation Steps
Provide function to update artistSignature

Assessed type

Access Control

@c4-submissions c4-submissions added 2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working labels Nov 7, 2023
c4-submissions added a commit that referenced this issue Nov 7, 2023
@c4-pre-sort
Copy link

141345 marked the issue as sufficient quality report

@c4-pre-sort c4-pre-sort added sufficient quality report This report is of sufficient quality primary issue Highest quality submission among a set of duplicates labels Nov 15, 2023
@c4-pre-sort
Copy link

141345 marked the issue as primary issue

This was referenced Nov 16, 2023
@c4-sponsor
Copy link

a2rocket (sponsor) disputed

@a2rocket
Copy link

once the collection supply is not set the artist can change. When the collectionSupply is set but artist did not sign yet, the address can change. When the artists signed it cannot change. This is the intended design.

@c4-judge
Copy link

c4-judge commented Dec 6, 2023

alex-ppg marked issue #741 as primary and marked this issue as a duplicate of 741

@c4-judge c4-judge added the partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) label Dec 8, 2023
@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as partial-50

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2 (Med Risk) Assets not at direct risk, but function/availability of the protocol could be impacted or leak value bug Something isn't working duplicate-741 partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) sponsor disputed Sponsor cannot duplicate the issue, or otherwise disagrees this is an issue sufficient quality report This report is of sufficient quality
Projects
None yet
Development

No branches or pull requests

5 participants