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

artist *Signature* does not check if max TotalSupply is correct allow artist payment address be *changed* after artist signing #855

Closed
c4-submissions opened this issue Nov 10, 2023 · 2 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%)

Comments

@c4-submissions
Copy link
Contributor

Lines of code

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

Vulnerability details

Impact

There are 2 issues related to each other:

  • NextGenCore.artistSignature() only check 1 out of 2 immutable variables of a collection: collectionArtistAddress and maxTotalSupply.
  • collectionArtistAddress or artist payment address can be changed after artist signing.

Missing artist signature check for maxTotalSupply can accidentally lead to situtaion where more NFT minted than artist intended.
Longterm devalued due to oversupply or simply NFT uri not support that many token.

Changing collectionArtistAddress again can only be done by Collection Admin.
Assuming Collection Admin is not NextGen project owner but by malicious actor.
Right after artist sign and propose payment addresses, malicious actor can propose their own payment address, override original artist payment address.

Proof of Concept

For collectionAdditonalDataStructure (NFT info collection), there is only 2 things cannot be changed after it was created.
Which are collectionArtistAddress and collectionTotalSupply, both admin and artist cannot change these variable as trust mechanism. (Admin can change sale time and price but not supply)
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L147-L166

collectionArtistAddress is used as EOA to propose payment share for NFT developer.
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/MinterContract.sol#L380-L390

Because payment address can be updated before finalization by admin, so anyone can change artist address for a specific collection would be quite alarming
Malicous actor can change payment address right after artist propose their own payment address. Effectively stealing artist payment before they are aware of it.

Project do verify if collection artist payment address is correct. By artist calling NextGenCore.artistSignature() function.
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L257-L262

collectionArtistAddress suppose to only set once, be immutable. But this can be easily bypass if CollectionAdmin who have access to setCollectionData() function set original collection supply to 0.
Allowing collection admin to update collectionArtistAddress again.
https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenCore.sol#L149-L153

This also reveal the problem where artistSignature() never check for collectionTotalSupply is correct amount as artist intended.
The problem can be easily fixed by allowing artistSignature to check for collectionTotalSupply as well.

Tools Used

Manual

Recommended Mitigation Steps

NextGenCore.artistSignature() should allow artist to verify for collectionAdditionalData[_collectionID].collectionTotalSupply value as well.

Assessed type

Invalid Validation

@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 10, 2023
c4-submissions added a commit that referenced this issue Nov 10, 2023
@c4-pre-sort
Copy link

141345 marked the issue as duplicate of #478

@c4-judge
Copy link

c4-judge commented Dec 8, 2023

alex-ppg marked the issue as partial-50

@c4-judge c4-judge added the partial-50 Incomplete articulation of vulnerability; eligible for partial credit only (50%) label Dec 8, 2023
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%)
Projects
None yet
Development

No branches or pull requests

3 participants