personalMint() reentrancy attack

[hats-bug-reporter]

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

Description:

Impact

Attacker can claim the initial issuance as many times as he wants (only block.gaslimit limits how many times the reentrancy can be executed)

Description

The personalMint() function allows a registered user to mint personal Circles for themselves: after registration and v1 status check it calls the _claimIssuance() function which soon calls _mintAndUpdateTotalSupply() -> _mint() which mints and calls .onERC1155Received if the user is not an EOA aka passes the greater than zero to.code.length check in _doSafeTransferAcceptanceCheck().

Hub.sol - _claimIssuance()

    function _claimIssuance(address _human) internal {
        (uint256 issuance, uint256 startPeriod, uint256 endPeriod) = _calculateIssuance(_human);
        if (issuance == 0) {
            // No issuance to claim, simply return without reverting
            return;
        }
        // mint personal Circles to the human
        _mintAndUpdateTotalSupply(_human, toTokenId(_human), issuance, "");
        // update the last mint time
        mintTimes[_human].lastMintTime = uint96(block.timestamp);

        emit PersonalMint(_human, issuance, startPeriod, endPeriod);
    }

The user can reenter the personalMint() function from .onERC1155Received() and claim & mint issuance again and again an arbitrary amount of times because nor registerHuman() nor personalMint() prevents a contract calling these functions

This wouldn't even be a huge problem if the issuance during the reentrancy would use the updated values for calculations in _calculateIssuance(): however mintTimes[_human].lastTime is only updated after the mint as we can see in _claimIssuance() which ultimately allows the attacker to claim their initial issuance as many times they want with continually reentering personalMint() from .onERC1155Received().

Full Execution flow

personalMint() -> _claimIssuance() -> mintAndUpdateTotalSupply() -> _mint() -> _updateWithAcceptanceCheck() -> _acceptanceCheck() -> _doSafeTransferAcceptanceCheck() -> (to).onERC1155Received()

Proof of Concept

1. User registered a contract 0xdead through self-invitation via registerHuman() that calls personalMint() on every .onERC1155Received() call to the contract up to 1000x times
2. User calls personalMint() from 0xdead contract
3. When the Hub mints the issuance tokens via _mint it calls .onERC1155Received from _doSafeTransferAcceptanceCheck()
4. 0xdead contract reenters personalMint() to claim the same amount of issuance tokens again and again -> repeats 1000x times
5. User now claimed 1000x times more issuance tokens that they are supposed to claim

Recommendation

Consider to add a reentrancy guard to personalMint(). Consider to only allow EOAs to call registerHuman() and personalMint(). Change _claimIssuance so the last mint time is updated before the actual mint:

    function _claimIssuance(address _human) internal {
+       // update the last mint time
+       mintTimes[_human].lastMintTime = uint96(block.timestamp);
 
        (uint256 issuance, uint256 startPeriod, uint256 endPeriod) = _calculateIssuance(_human);
        if (issuance == 0) {
            // No issuance to claim, simply return without reverting
            return;
        }
        
        // mint personal Circles to the human
        _mintAndUpdateTotalSupply(_human, toTokenId(_human), issuance, "");

        emit PersonalMint(_human, issuance, startPeriod, endPeriod);
    }

[0xmahdirostami]

Nice and valid issue.

A note on mitigation:

    function _claimIssuance(address _human) internal {
 
        (uint256 issuance, uint256 startPeriod, uint256 endPeriod) = _calculateIssuance(_human);
        if (issuance == 0) {
            // No issuance to claim, simply return without reverting
            return;
        }

+       mintTimes[_human].lastMintTime = uint96(block.timestamp);
        // mint personal Circles to the human
        _mintAndUpdateTotalSupply(_human, toTokenId(_human), issuance, "");

        emit PersonalMint(_human, issuance, startPeriod, endPeriod);
    }

[0xfuje]

Thank you @0xmahdirostami!
At first I wrote the mitigation like you did but later moved the lastMintTime update to the start of the function, just to make sure in case later some additional functionality would be added to the function, mintTime is always updated first. Anyways I think both cases work, only important thing is that the mintTime is updated before the actual mint.

[benjaminbollen]

We greatly appreciate your report on the potential reentrancy attack in the personalMint() function. This is a valid and important finding. You've correctly identified that updating the mint time after minting creates a vulnerability due to the ERC1155 onReceived callback.
We've classified this as a medium-severity issue. While it could allow unlimited minting of CRC tokens, the impact is mitigated by the trust-based nature of the Circles system, where users already need to be cautious about potential Sybil attacks.
Thank you for your careful analysis. This report will help us improve the security of our system. We'll be implementing a fix to update the mint time before minting to prevent this reentrancy issue.