Users are not able to migrate after bootstrap period unless CirclesV2 are acquired in other ways

[hats-bug-reporter]

Github username: @yixxas
Twitter username: yixxas
Submission hash (on-chain): 0x77975f558b7339b6617e512cd7d14b5dc2db575e32187bf9371d58374b1fa68f
Severity: medium

Description:
Description
In the Migration.sol contract, V1 Circles with their corresponding avatar. They will transfer V1 Circles, lock it into the contract, and new V2 Circles will be minted through calling hubV2.migrate().

In the migrate() function in Hub.sol, an invitation cost has to be paid for any human that registers after the bootstrap period. However, the issue here is that the cost is attempted to be taken from users before the V2 Circles are minted to them.

When _burnAndUpdateTotalSupply() is called, users who are attempting to migrate will likely not have any V2 Circles to begin with, hence it will revert.

This issue will affect every users who attempt to migrate after bootstrap period. Users should not be expected to acquire V2 Circles before they can migrate, when they are already holding sufficient V1 Circles.

Recommendation
Consider swapping the order for which Circles are minted and burned. By minting tokens to users before the invitation cost are taken from them, users will be able to pay for these cost with their V1 Circles, instead of having to acquire it in other ways, which is a massive inconvenience for users.

+        for (uint256 i = 0; i < _avatars.length; i++) {
+            // mint the migrated balances to _owner
+            _mintAndUpdateTotalSupply(_owner, toTokenId(_avatars[i]), +_amounts[i], ""); 
+        }  

        if (block.timestamp > invitationOnlyTime && cost > 0) { 
            // personal Circles are required to burn the invitation cost
            if (!isHuman(_owner)) {
                // Only humans can migrate v1 tokens after the bootstrap period.
                revert CirclesHubMustBeHuman(_owner, 4);
            }    
            _burnAndUpdateTotalSupply(_owner, toTokenId(_owner), cost);
        }    

-        for (uint256 i = 0; i < _avatars.length; i++) {
-            // mint the migrated balances to _owner
-            _mintAndUpdateTotalSupply(_owner, toTokenId(_avatars[i]), _amounts[i], ""); 
-        }  

[yixxas]

Hello @benjaminbollen , would like to check why this issue is invalid since there isn't any comments written.

[benjaminbollen]

because it neither low, let alone medium or high; at best it is a feature request which we do not agree with

[benjaminbollen]

there are also technical reasons why I dont swap this order; in particular because the mint does an acceptanceCall and the contract is coded under compile size constraints so at many places there is no luxury to add an explicit nonReentrant modifier, as is the case here

[yixxas]

Thanks for the response @benjaminbollen, but I would like to hear your reasons for why this is not an issue when viewed from the optics of users. When approaching a protocol's implementation, I like to think from how users are going to interact with it. Unless there is a way in which V2Circles are disseminated to users in other ways outside of migration, this issue is practically going to affect every user of Circles who are migrating after bootstrap period.

On the technical reasons, why is there a need to use a nonReentrant modifier here? Control granted to users does not necessitate a nonReentrant modifier even when it is not coded at the end of the function. There is nothing a malicious user can do using _burnAndUpdateTotalSupply since it only enforces the payment of CirclesV2 at the end of the call.

[benjaminbollen]

hey @yixxas the other comment to make here is that there is only a cost to be paid for auto-registering other humans of whom you may have a balance in v1; so you're never charged for you migrating your own balances because you're already registered.

While it is a nice suggestion, we will not adopt it because I don't want to burn myself on a possible reentrancy problem (inverting the canonical order); and I am battling a compile size constraint, so adding a nonReentrancy modifier are extra opcodes I cannot afford.

(also we don't expect people to keep migrating after invitation period is over, while of course possible, and then can mint their own V2 Circles to pay for that cost)

so I appreciate the contribution, but I have to stay on "well written but invalid", thanks