`mint()` will always revert if `maxTokenId` is less than previous `maxTokenId`

[hats-bug-reporter[bot]]

Github username: @0xRizwan Submission hash (on-chain): 0x8cee2ebed7ff10dd08823f35467e7ce7c57ef877f215adb34020e81c6bae8c33 Severity: low

Description: Description\

MembershipNFT.mint() is used for minting the membership NFT and it can only be accessed by Membership Manager contract. The issue is in MembershipNFT.setMaxTokenId() which is used to set max TokenId of membership NFT.

    function setMaxTokenId(uint32 _maxTokenId) external onlyAdmin() {
        maxTokenId = _maxTokenId;
    }

This function does not check _maxTokenId is greater than maxTokenId and the function also allows to set the max tokenID to 0.

To be noted, the contract has hardcoded maxTokenId as 1000 which can be checked at L-58.

        maxTokenId = 1000;

Consider a scenario, 1) Membership NFT had achieved 1000 tokenId and the admin plans to increase the max tokenId beyond 1000. 2) Consider, the admin sets the maxTokenId as 500 or any value less than previously set as it is believed it will take time to reach 1000 to have this scenario in consideration also humans are prone to errors which can not be denied. 3) when Membership Manager contract call the mint(), it will always revert with error MintingIsPaused. This is due to following check at L-70,

        if (mintingPaused || nextMintTokenId > maxTokenId) revert MintingIsPaused();

Since, here nextMintTokenId will be equal to 1001 and the admin in our scenario has set the maxTokenId as 500 therefore, 1001>500 so function will always revert unless corrected by admin.

This issue will cause deniel of service when the mint function will be called for few day, days or month until the admin takes corrective action or someone tells the protocol team about the bug.

Since the function setMaxTokenId() can only be called by admin and the possibility of issue is very much possible therefore, the issue is identified as low severity.

Attack Scenario\

As described above.

Attachments

1. Proof of Concept (PoC) File

https://github.com/hats-finance/ether-fi-0x36c3b77853dec9c4a237a692623293223d4b9bc4/blob/180c708dc7cb3214d68ea9726f1999f67c3551c9/src/MembershipNFT.sol#L70

https://github.com/hats-finance/ether-fi-0x36c3b77853dec9c4a237a692623293223d4b9bc4/blob/180c708dc7cb3214d68ea9726f1999f67c3551c9/src/MembershipNFT.sol#L104

2. Revised Code File (Optional)

This issue can be resolved by following mitigation.

    function setMaxTokenId(uint32 _maxTokenId) external onlyAdmin() {
+      require(_maxTokenId > maxTokenId, "must be greater than previous value");
        maxTokenId = _maxTokenId;
    }

[seongyun-ko]

admin wont do that :D

[0xRizwan]

@seongyun-ko

The issue is identified as low severity and the possibility of this issue is also possible. Nowhere, it mentioned admin assumptions/restrictions to particular functions.

Further, the issue can not be made invalid per hats rules and it does not fall under following rules,

A require validation can resolve this issue as indicated in recommendation above. It wont add any complexity to contract and not it affect adversely to function logic. It is simply resolving the issue which can surely happen, Therefore, this should be considered as low severity.

[0xRizwan]

@bunbuntigery Why wont it be fixed, The issue possibility can not be denied.