User can reset the INDEFINITE_FUTURE mintTime

[hats-bug-reporter[bot]]

Github username: -- Twitter username: -- Submission hash (on-chain): 0x656ba747c9cf205046fac382c4ea7acfdf437e1261a20107b208517353de9e74 Severity: high

Description: The stop function is designed to prevent any future mints by setting lastMintTime to INDEFINITE_FUTURE. This action is intended to be irreversible.

    function stop() external {
        if (!isHuman(msg.sender)) {
            // Only human can call stop.
            revert CirclesHubMustBeHuman(msg.sender, 2);
        }
        MintTime storage mintTime = mintTimes[msg.sender];
        // check if already stopped
=>        if (mintTime.lastMintTime == INDEFINITE_FUTURE) {
            return;
        }
        // stop future mints of personal Circles
        // by setting the last mint time to indefinite future.
        mintTime.lastMintTime = INDEFINITE_FUTURE;

        emit Stopped(msg.sender);
    }

The issue, however, is that a user can reset the mintTime.lastMintTime after it has been set to INDEFINITE_FUTURE, which violates a key aspect of the protocol: once the stop function is executed, it should be irreversible.

The user can do so simply by calling calculateIssuanceWithCheck:

    function calculateIssuanceWithCheck(address _human) external returns (uint256, uint256, uint256) {
        // check if v1 Circles is known to be stopped and update status
        _checkHumanV1CirclesStatus(_human);
        // calculate issuance for the human avatar, but don't mint
        return _calculateIssuance(_human);
    }

This will invoke _updateMintV1Status if V1 Circles has not been stopped:

    function _updateMintV1Status(address _human, address _mintV1Status) internal {
        MintTime storage mintTime = mintTimes[_human];
        // 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);
        }
        // if the status has changed, update the last mint time
        // to avoid possible overlap of the mint between Hub v1 and Hub v2
        if (mintTime.mintV1Status != _mintV1Status) {
            mintTime.mintV1Status = _mintV1Status;
=>            mintTime.lastMintTime = uint96(block.timestamp);
        }
    }

In this case, if the mintV1Status changed, mintTime.lastMintTime is reset to block.timestamp to prevent potential overlap between Hub v1 and Hub v2. However, it does not verify whether mintTime.lastMintTime has already been set to INDEFINITE_FUTURE.

A change in mintV1Status can easily occur if the status is modified within the V1 contract.

This ultimately will allow a user to reset the stop functionality

Fix

Only allow updating the mintTime.lastMintTime if it is not set to INDEFINITE_FUTURE

[benjaminbollen]

Thank you for your report. We've confirmed this behavior is working as intended, preventing double minting between v1 and v2 tokens. While it allows 'unstopping' v2 tokens, this doesn't pose a significant security risk. We've categorized this as a low-priority issue. We appreciate your attention to detail.

[whoismxuse]

Hi @benjaminbollen thank you for your comment !

I agree that I incorrectly classified this issue as a high-severity vulnerability, as it doesn't meet any of the criteria for high severity outlined in the Hats severity guidelines.

However, it does fall under the medium severity category according to the Hats documentation:

Issues that lead to an economic loss but do not lead to direct loss of on-chain assets. Examples are:

• Attacks that make essential functionality of the contracts temporarily unusable or inaccessible.

This issue allows users to mint when they shouldn't by resetting the INDEFINITE_FUTURE, making the essential stop functionality temporarily inaccessible. It's important to note that for an issue to be classified as medium severity, the term "temporarily" is key, meaning the disruption can occur for even a short period of time.

Secondly since the user is able to mint when he should not this effectively causes economic loss, whether it may be important to the protocol or not. This does not have to be a direct loss of funds as that would be classified as a HIGH vulnerability.

I want to reiterate that I agree this doesn't pose a significant security risk. However, it still qualifies as medium severity, as that classification doesn't require a significant security threat.

I hereby kindly ask you to reassess this issue.

[benjaminbollen]

This issue allows users to mint when they shouldn't by resetting the INDEFINITE_FUTURE, making the essential stop functionality temporarily inaccessible.

What does it even mean "to make the stop functionality temporarily inaccessible"? At no point is the stop() function unavailable, even temporarily.

Furthermore,

1. stop() is a user's choice to indicate that they no longer want to mint; it is not essential because we could have equally opted for pause() and unpause() to indicate user preferences, which with the current bug it is a "one-time unpause". To be clear, it is called stop() and the intended design is for it to irrevocably stop, not once-pause-unpause-stop().
2. with Circles v2 we do rely on the correct behaviour of v1:stop() to avoid double mint. So yes, the expectation is that for a Circles v3 in some future, v2:stop() functions correctly to then build on it. But for the concern of v2, this is only a deviation of the intended design/common sense. If any financial loss could have come from it, can only occur in a Circles v3 wrongly building on a broken v2:stop(); but errors in unwritten future code are clearly out of scope of any/this security review.

In conclusion, we stay with our judgment of (a very good!) low issue.