Potential Reentrancy in _mintAndUpdateTotalSupply

[hats-bug-reporter[bot]]

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

Description:

Description

The _mintAndUpdateTotalSupply function in the Circles contract updates the discountedTotalSupplies after minting tokens. This order of operations could potentially lead to a reentrancy vulnerability that affects the total supply calculation, particularly if used in external contracts or third-party integrations.

While this vulnerability doesn't appear to be directly exploitable within the current Circles contract implementation, it could pose risks in future integrations or extensions of the system.

Attack Scenario

1. An external contract or integration relies on the totalSupply value for critical calculations or decision-making.
2. An attacker initiates a transaction that calls a function triggering _mintAndUpdateTotalSupply.
3. Before the discountedTotalSupplies is updated, the attacker's contract reenters the Circles contract or the integrating contract.
4. The reentrant call sees an outdated (lower) total supply, potentially leading to incorrect calculations or decisions in the external system.
5. This could result in temporary inflation of the attacker's token proportion or other unintended consequences, depending on how the external system uses the total supply information.

Attachments

Revised Code

function _mintAndUpdateTotalSupply(address _account, uint256 _id, uint256 _value, bytes memory _data) internal {
        // Update total supply first
        uint64 today = day(block.timestamp);
        DiscountedBalance memory totalSupplyBalance = discountedTotalSupplies[_id];
        uint256 newTotalSupply =
            _calculateDiscountedBalance(totalSupplyBalance.balance, today - totalSupplyBalance.lastUpdatedDay) + _value;
        if (newTotalSupply > MAX_VALUE) {
            revert CirclesDemurrageAmountExceedsMaxUint190(_account, _id, newTotalSupply, 2);
        }
        totalSupplyBalance.balance = uint192(newTotalSupply);
        totalSupplyBalance.lastUpdatedDay = today;
        discountedTotalSupplies[_id] = totalSupplyBalance;

        // Then perform the mint
        _mint(_account, _id, _value, _data);
    }

[benjaminbollen]

Thank you for your report on the potential reentrancy in _mintAndUpdateTotalSupply. After review, we've determined this is not an issue.

While we appreciate the attention to good coding practices, the current implementation does not pose a security risk. Unlike the situation in Issue #8, the total supply is not used as a status gate in our code.

However, we acknowledge your point about following best practices. We'll consider adjusting the order of operations in future updates for improved style, even though it doesn't affect security in this case.

We appreciate your thorough examination of our minting process and your commitment to identifying potential vulnerabilities. Thank you for your valuable contribution to this security review.