Inconsistent Human Check in Migrate Function Allows Non-Human Entities to Migrate Tokens After invitationOnlyTime

[hats-bug-reporter]

Github username: @0xmahdirostami
Twitter username: 0xmahdirostami
Submission hash (on-chain): 0x62526d48951dac943e33dd81e513234b3009e538faf775839335b85186111f42
Severity: low

Description:

Description

The migrate function in the contract is designed to only allow humans to migrate tokens after the bootstrap period, as indicated by the comment:

// Only humans can migrate v1 tokens after the bootstrap period.

However, the actual implementation allows non-human entities (e.g., groups) to migrate if the invitation cost is zero, leading to a mismatch between the intended behavior and the code.

        uint256 cost = INVITATION_COST * _ensureAvatarsRegistered(_avatars);

        // Invitation cost only applies after the bootstrap period
        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);
        }

Impact

The contract not working as intended.

Mitigation

If the contract just wants to allow humans to migrate other users after invitationOnlyTime, the check to ensure only humans can migrate should be independent of the invitation cost. The condition should be modified to always check if the owner is a human after the bootstrap period, regardless of the cost.

Updated Code:

// Invitation cost only applies after the bootstrap period
if (block.timestamp > invitationOnlyTime) {
    // Ensure only humans can migrate v1 tokens after the bootstrap period
    if (!isHuman(_owner)) {
        revert CirclesHubMustBeHuman(_owner, 4);
    }
    if (cost > 0) {
        _burnAndUpdateTotalSupply(_owner, toTokenId(_owner), cost);
    }
}

Otherwise, if comments are wrong, try to rewrite the document.

[0xpinky]

for non-human, it reverts in _ensureAvatarsRegistered

[0xmahdirostami]

@0xpinky thanks for commenting , the issue is about the owner, not avatars.

[0xpinky]

Yes @0xmahdirostami ..

I saw this #50. which is mentioning about unauthorized caller calling the migrate.

Also, the migration is permissionless and anyone call call the migrate.

The reason for having human if (!isHuman(_owner)) { is just sanity check. even without this, the call will revert when it tried to burn the token from the caller.

I think, there is not security vulnerability.

[0xmahdirostami]

@0xpinky thanks for commenting, it seems like a misunderstanding in my issue, so i could explain it better:

the whole issue that i pointed here is that, comments says:

// Only humans can migrate v1 tokens after the bootstrap period.

But if cost is 0, anyone could migrate others after bootstrap period. (it doesn't relate to amount being 0, amount could be 0 or higher)

[0xpinky]

lets see sponsor's comments..

issue #50 has this impact section.

migration function not working as intended. users could migrate other users even without holding their token. it would be even worse if those users want to register as org or group which will not be applicable anymore

[benjaminbollen]

Pulling a very clearly scoped line comment out of context does not qualify an issue.

[benjaminbollen]

The reason for having human if (!isHuman(_owner)) { is just sanity check. even without this, the call will revert when it tried to burn the token from the caller.

indeed, that was meant as a sanity check. And partially it could also revert on burning; but it could be that a group owns some of it s own token, and then it would proceed to burn group tokens without handling the associated collateral; so this sanity check is required in my understanding.