Due to revert found during testing it would be wise to implement a rollback and re-whitelist should it be needed

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/test/migrations/Migrations.t.sol#L107

Vulnerability details

Impact

During testing with the Mainnet fork for the migration testing. We needed to make a distinction between V1 Profiles and V2 profiles within the test code, but this is not so in the standard V2 code, and may cause unforeseen issues after the upgrade. The V2 code assumes the profile has been migrated which may not be the case immediately after the upgrade.

During the test while loading a profile without casting the address to a LensHub V1 proxy Interface, it reverted.

The LensV2Upgrade contract has a way to upgrade and un-whitelist and whitelist the correct modules. Should there for some unforeseen reason be a need to rollback this will take quite a while to do and setup, however this can easily be enabled in the current contract so that it can be a seamless quick process.

PoC

During testing with mainnet fork it was discussed with the Dev team, that it would fail unless changed to be from the LenHub V1.

The code that failed was :

address followNFTAddress = hub.getProfile(idOfProfileFollowed).followNFT;

and needed to be changed to :

//add to top of contract 
interface OldLensHub {
    struct ProfileStruct {
        uint256 pubCount;
        address followModule;
        address followNFT;
        string handle;
        string imageURI;
        string followNFTURI;
    }

    function getProfile(uint256 profileId) external view returns (ProfileStruct memory);
}

//change line to this
address followNFTAddress = OldLensHub(address(hub)).getProfile(idOfProfileFollowed).followNFT;

Tools Used

Manual Audit

Recommended Mitigation Steps

It could be considered to implement a rollback function as below due to the fact that we already have all the valid data within the storage of the LensV2Upgrade contract.

I have added a rollback function to the LensV2Upgrade contract below:

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.19;

import {ProxyAdmin} from 'contracts/misc/access/ProxyAdmin.sol';
import {Governance} from 'contracts/misc/access/Governance.sol';
import {ImmutableOwnable} from 'contracts/misc/ImmutableOwnable.sol';

contract LensV2UpgradeContract is ImmutableOwnable {
    ProxyAdmin public immutable PROXY_ADMIN;
    Governance public immutable GOVERNANCE;
    address public immutable newImplementation;
    address[] public oldFollowModulesToUnwhitelist;
    address[] public newFollowModulesToWhitelist;
    address[] public oldReferenceModulesToUnwhitelist;
    address[] public newReferenceModulesToWhitelist;
    address[] public oldCollectModulesToUnwhitelist;
    address[] public newActionModulesToWhitelist;

    constructor(
        address proxyAdminAddress,
        address governanceAddress,
        address owner,
        address lensHub,
        address newImplementationAddress,
        address[] memory oldFollowModulesToUnwhitelist_,
        address[] memory newFollowModulesToWhitelist_,
        address[] memory oldReferenceModulesToUnwhitelist_,
        address[] memory newReferenceModulesToWhitelist_,
        address[] memory oldCollectModulesToUnwhitelist_,
        address[] memory newActionModulesToWhitelist_
    ) ImmutableOwnable(owner, lensHub) {
        PROXY_ADMIN = ProxyAdmin(proxyAdminAddress);
        GOVERNANCE = Governance(governanceAddress);
        newImplementation = newImplementationAddress;
        oldFollowModulesToUnwhitelist = oldFollowModulesToUnwhitelist_;
        newFollowModulesToWhitelist = newFollowModulesToWhitelist_;
        oldReferenceModulesToUnwhitelist = oldReferenceModulesToUnwhitelist_;
        newReferenceModulesToWhitelist = newReferenceModulesToWhitelist_;
        oldCollectModulesToUnwhitelist = oldCollectModulesToUnwhitelist_;
        newActionModulesToWhitelist = newActionModulesToWhitelist_;
    }

    function executeLensV2Upgrade() external onlyOwner {
        // _preUpgradeChecks();
        _upgrade();
        // _postUpgradeChecks();
    }

    function executeLensRollback() external onlyOwner {
        _rollback();
    }

    function _upgrade() internal {
        _unwhitelistOldFollowModules(false);
        _unwhitelistOldReferenceModules(false);
        _unwhitelistOldCollectModules(false);

        PROXY_ADMIN.proxy_upgrade(newImplementation);

        _whitelistNewFollowModules(true);
        _whitelistNewReferenceModules(true);
        _whitelistNewActionModules(true);

        GOVERNANCE.clearControllerContract();
    }

    function _rollback() internal {
        // Reverse the upgrade function and unwhiltelist and re-whitelist correct modules.
        // we already have the correct modules in memory so it is not taking up any extra storage
        // to implement this function
        _whitelistNewFollowModules(false);
        _whitelistNewReferenceModules(false);
        _whitelistNewActionModules(false);

        PROXY_ADMIN.proxy_upgrade(PROXY_ADMIN.previousImplementation());
        _unwhitelistOldFollowModules(true);
        _unwhitelistOldReferenceModules(true);
        _unwhitelistOldCollectModules(true);

        GOVERNANCE.clearControllerContract();
    }

    function _unwhitelistOldFollowModules(bool mustWhiteList) internal {
        uint256 oldFollowModulesToUnwhitelistLength = oldFollowModulesToUnwhitelist.length;
        uint256 i;
        while (i < oldFollowModulesToUnwhitelistLength) {
            GOVERNANCE.lensHub_whitelistFollowModule(oldFollowModulesToUnwhitelist[i], mustWhiteList);
            unchecked {
                ++i;
            }
        }
    }

    function _unwhitelistOldReferenceModules(bool mustWhiteList) internal {
        uint256 oldReferenceModulesToUnwhitelistLength = oldReferenceModulesToUnwhitelist.length;
        uint256 i;
        while (i < oldReferenceModulesToUnwhitelistLength) {
            GOVERNANCE.lensHub_whitelistReferenceModule(oldReferenceModulesToUnwhitelist[i], mustWhiteList);
            unchecked {
                ++i;
            }
        }
    }

    function _unwhitelistOldCollectModules(bool mustWhiteList) internal {
        uint256 oldCollectModulesToUnwhitelistLength = oldCollectModulesToUnwhitelist.length;
        uint256 i;
        while (i < oldCollectModulesToUnwhitelistLength) {
            GOVERNANCE.lensHub_whitelistCollectModule(oldCollectModulesToUnwhitelist[i], mustWhiteList);
            unchecked {
                ++i;
            }
        }
    }

    function _whitelistNewFollowModules(bool mustWhiteList) internal {
        uint256 newFollowModulesToWhitelistLength = newFollowModulesToWhitelist.length;
        uint256 i;
        while (i < newFollowModulesToWhitelistLength) {
            GOVERNANCE.lensHub_whitelistFollowModule(newFollowModulesToWhitelist[i], mustWhiteList);
            unchecked {
                ++i;
            }
        }
    }

    function _whitelistNewReferenceModules(bool mustWhiteList) internal {
        uint256 newReferenceModulesToWhitelistLength = newReferenceModulesToWhitelist.length;
        uint256 i;
        while (i < newReferenceModulesToWhitelistLength) {
            GOVERNANCE.lensHub_whitelistReferenceModule(newReferenceModulesToWhitelist[i], mustWhiteList);
            unchecked {
                ++i;
            }
        }
    }

    function _whitelistNewActionModules(bool mustWhiteList) internal {
        uint256 newActionModulesToWhitelistLength = newActionModulesToWhitelist.length;
        uint256 i;
        while (i < newActionModulesToWhitelistLength) {
            GOVERNANCE.lensHub_whitelistActionModule(newActionModulesToWhitelist[i], mustWhiteList);
            unchecked {
                ++i;
            }
        }
    }
}

Assessed type

Upgradable

[141345]

QA might be more appropriate.

[vicnaum]

The POC provided is valid for tests, as mentioned, but is not valid for the LensHub contract itself - because once upgraded the interfaces will change accordingly and everything will work.

As per advised Rollback implementation, it will not be possible in the provided example, because once upgraded the Upgrade contract loses it's role as a controller contract (GOVERNANCE.clearControllerContract();) and this was done for security purposes to make it one-time use.

But we also have the rollback function in the PROXY_ADMIN contract itself. It won't whitelist/unwhitelist the modules, yes - this has to be done manually, but it can rollback the upgrade, because we save it in the lastImplementation and the rollbackLastUpgrade() function.

[c4-sponsor]

vicnaum marked the issue as sponsor disputed

[Picodes]

This report points toward a test file, which is out of scope, and does not pass the burden of proof for High severity

[c4-judge]

Picodes marked the issue as unsatisfactory: Invalid