User can forcefully migrate others avatars

[hats-bug-reporter[bot]]

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

Description: function Migration.Migrate.sol allows for any user to migrate any avatars.

    function migrate(address[] calldata _avatars, uint256[] calldata _amounts) external returns (uint256[] memory) {
        if (_avatars.length != _amounts.length) {
            // Arrays length mismatch.
            revert CirclesArraysLengthMismatch(_avatars.length, _amounts.length, 0);
        }

        uint256[] memory convertedAmounts = new uint256[](_avatars.length);

        for (uint256 i = 0; i < _avatars.length; i++) {
            ITokenV1 circlesV1 = ITokenV1(hubV1.userToToken(_avatars[i]));
            if (address(circlesV1) == address(0)) {
                // Invalid avatar, not registered in hub V1.
                revert CirclesAddressCannotBeZero(2);
            }
            convertedAmounts[i] = convertFromV1ToDemurrage(_amounts[i]);
            // transfer the v1 Circles to this contract to be locked
            circlesV1.transferFrom(msg.sender, address(this), _amounts[i]);
        }

        // mint the converted amount of v2 Circles
        hubV2.migrate(msg.sender, _avatars, convertedAmounts);

        return convertedAmounts;
    }

• Malicious user can fill up the _avatars array with other people's _avatars and fill up the _amounts data with dust value or just normal values, since these will be minted back anyway on V2.
• the value will be locked inside the contract and the _avatars will be migrated to V2.

function migrate(address _owner, address[] calldata _avatars, uint256[] calldata _amounts) external onlyMigration {
        if (avatars[_owner] == address(0)) {
            // Only registered avatars can migrate v1 tokens.
            revert CirclesAvatarMustBeRegistered(_owner, 1);
        }
        if (_avatars.length != _amounts.length) {
            revert CirclesArraysLengthMismatch(_avatars.length, _amounts.length, 0);
        }

        // register all unregistered avatars as humans, and check that registered avatars are humans
        // after the bootstrap period, the _owner needs to pay the equivalent invitation cost for all newly registered humans
        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);
        }

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

• Inside Hub.sol.migrate the process will be finalised.
• And at the end all the migrated balances will be minted to the _owner which will be the malicious user migrating other people's avatars and the malicious user has successfully migrated others avatars

Recommendation

do not allow any user to migrate others avatars, rather, make sure this is properly handled

[0xmahdirostami]

• No impact section.

            ITokenV1 circlesV1 = ITokenV1(hubV1.userToToken(_avatars[i]));
            if (address(circlesV1) == address(0)) {
                // Invalid avatar, not registered in hub V1.
                revert CirclesAddressCannotBeZero(2);
            }
            convertedAmounts[i] = convertFromV1ToDemurrage(_amounts[i]);
            // transfer the v1 Circles to this contract to be locked
            circlesV1.transferFrom(msg.sender, address(this), _amounts[i]);

User must provide tokens from these avatars.

[whoismxuse]

hi @0xmahdirostami

Your statement is incorrect. The amounts array is assumed to represent the amounts corresponding to each avatar, but there's no validation in place to confirm this. A malicious user could manipulate the array by providing their own specified values, which would then be associated with the avatars via the following code:

        uint256[] memory convertedAmounts = new uint256[](_avatars.length);

convertedAmounts is then filled with the amounts array:

            convertedAmounts[i] = convertFromV1ToDemurrage(_amounts[i]);

Even if your point were to be valid, it would still allow a malicious user to migrate avatars as long as they provide tokens for them. There is no issue here since these tokens get minted back to the malicious user upon arrival.

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

[benjaminbollen]

Thank you for your report. After careful review, we've determined that this is not a valid issue. The migration process only allows users to migrate balances they own themselves. While it's true that users can be auto-registered in v2 if they're already self-registered in v1, this is an intended feature, not a vulnerability. We appreciate your effort in examining our system, but in this case, the behavior is working as designed.