Insufficient Validation in updateArtSettings Allows Setting Invalid maxSupply, Potentially Bricking Minting

[c4-bot-9]

Lines of code

https://github.com/code-423n4/2024-08-phi/blob/8c0985f7a10b231f916a51af5d506dd6b0c54120/src/PhiFactory.sol#L215

Vulnerability details

Summary

The updateArtSettings function in the PhiFactory contract allows the artist to modify various parameters of an existing art piece, including the maxSupply. However, there's currently no validation to prevent the maxSupply from being set to a value lower than the current numberMinted (the number of NFTs already minted for that art piece).

Code Snippets

PhiFactory.updateArtSettings:

function updateArtSettings(
    // ... other parameters
    uint256 maxSupply_,
    // ...
) external onlyArtCreator(artId_) {
    // ... (Input validations - no check to ensure maxSupply_ >= art.numberMinted)

    PhiArt storage art = arts[artId_];

    // ...

    if (art.numberMinted > maxSupply_) { // This check is insufficient
        revert ExceedMaxSupply();
    }

    art.maxSupply = maxSupply_; 

    // ...
}

PhiFactory._validateAndUpdateClaimState:

function _validateAndUpdateClaimState(uint256 artId_, address minter_, uint256 quantity_) private {
    // ...

    PhiArt storage art = arts[artId_];

    // ...

    if (art.numberMinted + quantity_ > art.maxSupply) revert OverMaxAllowedToMint(); 

    // ...

    art.numberMinted += quantity_;
}

Impact

• Unmintable Art: If an artist sets maxSupply_ lower than art.numberMinted, any subsequent minting attempts will fail due to the OverMaxAllowedToMint revert in _validateAndUpdateClaimState. This could happen even if there are legitimate pending claims or if the artist mistakenly set the maxSupply too low.

• User Frustration: Users who attempt to mint after such an update will encounter unexpected errors, leading to confusion and frustration.

Scenario

1. An artist creates an art piece with a maxSupply of 100.
2. 80 NFTs are minted (numberMinted becomes 80).
3. The artist decides to update the settings, mistakenly setting maxSupply_ to 75.
4. Now, even though there are 20 more NFTs allowed within the original maxSupply, no further minting is possible.

Fix

Add a validation in updateArtSettings to explicitly check if the new maxSupply_ is greater than or equal to the current numberMinted:

if (maxSupply_ < art.numberMinted) {
    revert MaxSupplyLessThanNumberMinted();
}

This simple check will prevent the maxSupply from being set to an invalid value, ensuring that the art piece remains mintable as long as it's within the original intended supply limit.

Assessed type

Invalid Validation

[anthonyshiks]

Using the output of ChatGPT, GPT-3, or automated tools for audit submissions is highly discouraged as it leads to a high ratio of nonsense submissions. Wardens are responsible for verifying the validity and clarity of their own submissions. Sending multiple poor quality submissions in a single audit will result in all of your audit submissions being ruled invalidated. Additional penalties may also be applied at the discretion of judges and C4 staff.

We are aware this privileges native English speakers as online translation services can result in unclear wordings; therefore, a submission should not be marked as unsatisfactory purely based on grammar and spelling which does not interfere with a judge's ability to understand the submission.

Judges must make the best decision they can regarding quality and understandability of findings.

Note to judges and c4 team as a whole:

This section of the documentation is not quite clear and needs to be changed... either outright ban gpt reports and change documentation to strictly not allow gpt content...I agree gpt content has a lot of false positives but that's why I take a great deal to discard all issues I believe to be false positives before submitting my reports..mind you all my reports and roughly about 50 percent of other warden reports are ai...most of the time it's really what you ask the AI just like doc wolf ..ai is vast well of knowledge and it doesn't know what you want till you tell it ...so if you in the dark it doesn't necessarily take you out of that darkness you need to know the issues that would likely present themselves in such a function then guide the Ai...this skill as you see is really dependant on wardens skills and knowledge which goes on to reduce false positives better as you can discern which reports are valid. I brought this issue up a while back after in the c4 questions discord channel and other wardens and c4 team assured me that what matters is really validity of the report first...in that case I don't believe gpt should be solely grounds for not judging a report..all my findings are gpt even the two that are accepted. Some clarity on this will be much appreciated as I feel I'm walking on a tight rope...an announcement of the same is also crucial so wardens , sponsors, judges and c4 team can all be in the same team. Thanks.

Ps: yes I still believe issue above is valid

[fatherGoose1]

@anthonyshiks This is by far the worst submission I've judged. You state that this check is insufficient:

if (art.numberMinted > maxSupply_) { // This check is insufficient
    revert ExceedMaxSupply();
}

But think that this is:

if (maxSupply_ < art.numberMinted) {
    revert MaxSupplyLessThanNumberMinted();
}

[anthonyshiks]

@fatherGoose1 Apologies for the inaccuracy in this report ...I didn't take enough time to review this one ,I don't know how it slipped through...the issue here is that the check allow setting maxSupply_ equal to numberMinted, which could immediately cap the supply and prevent further minting.

The check should ensure that the new maxSupply_ is strictly greater than the number of tokens already minted.

if (maxSupply_ <= art.numberMinted) {
    revert MaxSupplyTooLow();
}

So yes the check is insufficient. Though I understand contest is over.

This is by far the worst submission I've judged. You state that this check is insufficient.

Glad to make your hall of fame.😁