Incorrect Error Emission in `_updateMintV1Status` Function

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0xae43b4c9059290e7566ec156a39ef7049f52e4ac11bc31ac8524e9fb2b10cf1e Severity: low

Description: Description\ In the Hub contract, the _updateMintV1Status function is responsible for updating the mint status of a human avatar when transitioning from Circles v1 to Circles v2. The function ensures that the avatar's minting status is properly updated, provided that the avatar is already registered as a human.

The current implementation, however, throws a generic error:

revert CirclesLogicAssertion(0);

This error is a general-purpose assertion failure, and it does not accurately represent the reason for the failure in the context of this function. Specifically, the logic in _updateMintV1Status is dependent on whether the avatar is registered as a human, and if this check fails, a more appropriate error should be emitted.

The key part of the function is the check to ensure that the avatar’s lastMintTime is non-zero, which signifies that the avatar is registered as a human. If this check fails, the function should indicate that the avatar is not properly registered rather than using a generic assertion error.

Impact\ The use of a generic error message like CirclesLogicAssertion in this context creates ambiguity and makes debugging more difficult. Without a clear error message, it may take more time for developers to diagnose why the function reverted.

Developers might be misled to believe that the error is caused by an unrelated logic assertion, rather than the avatar not being registered as a human.

Recommendation\ Replace the generic CirclesLogicAssertion(0) error with the more specific and contextually appropriate CirclesAvatarMustBeRegistered error. This will align the error handling in _updateMintV1Status with other parts of the Hub contract, improving both code clarity and developer experience.

[benjaminbollen]

Thank you for your report on the error emission in the _updateMintV1Status function. After review, we've determined this is not an issue. The CirclesLogicAssertion error is intentionally used as an explicit assert for a condition that should never be met unless there's a bug in the code. This guard is crucial to prevent potential misclassification of a group as a human. We appreciate your attention to error handling in our contract. However, in this case, the error usage is deliberate and serves an important purpose. Thank you for your participation in this security review.

For your reference, please read the comment above the error again

        // precautionary check to ensure that the last mint time is already set
        // as this marks whether an avatar is registered as human or not
        if (mintTime.lastMintTime == 0) {
            // Avatar must already be registered as human before we call update
            revert CirclesLogicAssertion(0);
        }