code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/libraries/MigrationLib.sol#L69-L85

Vulnerability details

Bug Description

Profiles that exist before the V2 upgrade are migrated using the batchMigrateProfiles() function, which works by minting the profile's handle and linking it to their profile:

MigrationLib.sol#L69-L85

            string memory handle = StorageLib.getProfile(profileId).__DEPRECATED__handle;
            if (bytes(handle).length == 0) {
                return; // Already migrated
            }
            bytes32 handleHash = keccak256(bytes(handle));
            // We check if the profile is the "lensprotocol" profile by checking profileId != 1.
            // "lensprotocol" is the only edge case without the .lens suffix:
            if (profileId != LENS_PROTOCOL_PROFILE_ID) {
                assembly {
                    let handle_length := mload(handle)
                    mstore(handle, sub(handle_length, DOT_LENS_SUFFIX_LENGTH)) // Cut 5 chars (.lens) from the end
                }
            }
            // We mint a new handle on the LensHandles contract. The resulting handle NFT is sent to the profile owner.
            uint256 handleId = lensHandles.migrateHandle(profileOwner, handle);
            // We link it to the profile in the TokenHandleRegistry contract.
            tokenHandleRegistry.migrationLink(handleId, profileId);

For example, a profile with the handle "alice.lens" will receive a "alice" LensHandles NFT post-migration.

However, whitelisted profile creators are able to mint any handle using mintHandle() in the LensHandles contract. This makes it possible for any whitelisted profile creator to mint a handle corresponding to a V1 profile before the profile is migrated.

If this occurs, batchMigrateProfiles() will always revert for the corresponding V1 profile as the same handle cannot be minted twice, thereby breaking migration for that profile.

Impact

If a whitelisted profile creator accidentally mints a handle that already belongs to a V1 profile, that profile cannot be migrated.

Proof of Concept

The Foundry test below demonstrates how batchMigrateProfiles() will revert if a V1 profile's handle has already been minted. It can be run with the following command:

forge test --match-test testProfileCreatorCanBreakProfileMigration -vvv

// SPDX-License-Identifier: MIT
pragma solidity ^0.8.13;

import 'test/base/BaseTest.t.sol';

contract ProfileMigration_POC is BaseTest {
    LensHubHelper hubProxy;

    function setUp() public override {
        super.setUp();

        // Add toLegacyV1Profile() function to LensHub
        LensHubHelper implementation = new LensHubHelper({
            lensHandlesAddress: address(lensHandles),
            tokenHandleRegistryAddress: address(tokenHandleRegistry),
            tokenGuardianCooldown: PROFILE_GUARDIAN_COOLDOWN
        });
        vm.prank(deployer);
        hubAsProxy.upgradeTo(address(implementation));

        // Cast proxy to LensHubHelper interface
        hubProxy = LensHubHelper(address(hubAsProxy));
    }

    function testProfileCreatorCanBreakProfileMigration() public {
        // Create a v1 profile with the handle "alice.lens"
        uint256 profileId = _createProfile(address(this));
        hubProxy.toLegacyV1Profile(profileId, "alice.lens");

        // Whitelisted profile creator accidentally mints a "alice.lens" handle
        vm.prank(lensHandles.OWNER());
        lensHandles.mintHandle(address(this), "alice");

        // V1 profile will revert when migrated as the handle already exists
        vm.expectRevert("ERC721: token already minted");
        hubProxy.batchMigrateProfiles(_toUint256Array(profileId));
    }
}

contract LensHubHelper is LensHub {
    constructor(
        address lensHandlesAddress,
        address tokenHandleRegistryAddress,
        uint256 tokenGuardianCooldown
    ) LensHub(
        address(0),
        address(0),
        address(0),
        lensHandlesAddress,
        tokenHandleRegistryAddress,
        address(0),
        address(0),
        address(0),
        tokenGuardianCooldown
    ) {}

    function toLegacyV1Profile(uint256 profileId, string memory handle) external {
        Types.Profile storage profile = StorageLib.getProfile(profileId);
        profile.__DEPRECATED__handle = handle;
        delete profile.metadataURI;
    }
}

Recommended Mitigation

Ensure that the handle of a V1 profile cannot be minted through mintHandle(). This validation will probably have to be done off-chain, as it is unfeasible to check all existing handles on-chain with a reasonable gas cost.

Assessed type

Upgradable

c4-pre-sort commented 1 year ago

141345 marked the issue as primary issue

c4-sponsor commented 1 year ago

donosonaumczuk marked the issue as sponsor confirmed

donosonaumczuk commented 1 year ago

We found this issue once the contest was already in progress, so we weren't allowed to push it, but we already mitigated it by adding this function in the LensHub:

    function getProfileIdByHandleHash(bytes32 handleHash) external view returns (uint256) {
        return StorageLib.profileIdByHandleHash()[handleHash];
    }

And then making the ProfileCreationProxy to validate against it:

    function proxyCreateProfileWithHandle(
        Types.CreateProfileParams memory createProfileParams,
        string calldata handle
    ) external onlyOwner returns (uint256, uint256) {
        // Check if LensHubV1 already has a profile with this handle that was not migrated yet:
        bytes32 handleHash = keccak256(bytes(string.concat(handle, '.lens')));
        if (LensV2Migration(LENS_HUB).getProfileIdByHandleHash(handleHash) != 0) {
            revert ProfileAlreadyExists();
        }

        // ...
     }

Note that we add the validation at ProfileCreationProxy instead of LensHub, as we don't want LensHub to "be aware" of the Handles, architecturally-wise.

c4-judge commented 1 year ago

Picodes marked the issue as selected for report