rollbackLastUpgrade: function Rollback address can be the same as current address breaking the rollback functionality

[code423n4]

Lines of code

https://github.com/code-423n4/2023-07-lens/blob/main/contracts/misc/access/ProxyAdmin.sol#L25-L45

Vulnerability details

Impact

The code implements a rollback function:

function rollbackLastUpgrade() external onlyOwner {
        LENS_HUB_PROXY.upgradeTo(previousImplementation);
    }

The function relies on the value of the previousImplementation variable being correctly set to that of the previous implementation.

Although the issue can be rectified by issuing a call to proxy_upgrade with the valid previous address as input, the functionality is available and therefore should be protected. The functionality can accidentally or intentionally be broken by the Owner calling proxy_upgrade or proxy_upgradeAndCall twice with the same address thereby rendering the functionality of rollbackLastUpgrade ineffective by overwriting the previousImplementation variable with the current implemetation address as in the code below.

function proxy_upgrade(address newImplementation) external onlyOwnerOrControllerContract {
        previousImplementation = LENS_HUB_PROXY.implementation();
        LENS_HUB_PROXY.upgradeTo(newImplementation);
        delete controllerContract;
    }

Proof of Concept

Test written in foundry and output

Test function:

    function testUpgradeTwice() public {
        address hubImplV2 = address(new MockContract());

        address prevImpl = address(uint160(uint256(vm.load(hubProxyAddr, PROXY_IMPLEMENTATION_STORAGE_SLOT))));
        assertTrue(prevImpl != hubImplV2);

        vm.expectCall(address(hubAsProxy), abi.encodeCall(hubAsProxy.upgradeTo, (hubImplV2)));
        vm.startPrank(proxyAdminContractOwner);
        console.log("[+] Implementation address before upgrade : " , prevImpl);
        console.log("[+] Previous Implementation currently set to : " , proxyAdminContract.previousImplementation());
        proxyAdminContract.proxy_upgrade(hubImplV2);
        prevImpl = address(uint160(uint256(vm.load(hubProxyAddr, PROXY_IMPLEMENTATION_STORAGE_SLOT))));
        console.log("[+] Implementation address after 1st upgrade : " , prevImpl);
        console.log("[+] Previous Implementation address now set to : " , proxyAdminContract.previousImplementation());
        proxyAdminContract.proxy_upgrade(hubImplV2);
        prevImpl = address(uint160(uint256(vm.load(hubProxyAddr, PROXY_IMPLEMENTATION_STORAGE_SLOT))));
        console.log("[+] Implementation address after 2nd upgrade : " , prevImpl);
        console.log("[+] Previous Implementation address now set to : " , proxyAdminContract.previousImplementation());
        console.log("[+] Previous Implementation : " , proxyAdminContract.previousImplementation(), " is now equal to current implementation : ",prevImpl);
        address upgradedImpl = address(uint160(uint256(vm.load(hubProxyAddr, PROXY_IMPLEMENTATION_STORAGE_SLOT))));
        assertEq(upgradedImpl, hubImplV2);

        //assertEq(proxyAdminContract.previousImplementation(), prevImpl);
        assertEq(proxyAdminContract.controllerContract(), address(0)); // Controller is cleared after upgrade
        vm.stopPrank();
    }

Output:

  [+] Implementation address before upgrade :  0x62237016348f87F9aFD2EaE4c466901b0D065f0E
  [+] Previous Implementation currently set to :  0x62237016348f87F9aFD2EaE4c466901b0D065f0E
  [+] Implementation address after 1st upgrade :  0x75529BF343F8f42A8f3f75814C54877377a26D06
  [+] Previous Implementation address now set to :  0x62237016348f87F9aFD2EaE4c466901b0D065f0E
  [+] Implementation address after 2nd upgrade :  0x75529BF343F8f42A8f3f75814C54877377a26D06
  [+] Previous Implementation address now set to :  0x75529BF343F8f42A8f3f75814C54877377a26D06
  [+] Previous Implementation :  0x75529BF343F8f42A8f3f75814C54877377a26D06  is now equal to current implementation :  0x75529BF343F8f42A8f3f75814C54877377a26D06

Tools Used

Manual Audit Foundry

Recommended Mitigation Steps

It could be considered to check that the new value being set in proxy_upgrade and proxy_upgradeAndCall are not equivalent to the current implementation as below

function proxy_upgrade(address newImplementation) external onlyOwnerOrControllerContract {
        previousImplementation = LENS_HUB_PROXY.implementation();
        require(newImplementation != previousImplementation,"Upgrading to same address");
        LENS_HUB_PROXY.upgradeTo(newImplementation);
        delete controllerContract;
    }

    function proxy_upgradeAndCall(address newImplementation, bytes calldata data)
        external
        onlyOwnerOrControllerContract
    {
        previousImplementation = LENS_HUB_PROXY.implementation();
        require(newImplementation != previousImplementation,"Upgrading to same address");
        LENS_HUB_PROXY.upgradeToAndCall(newImplementation, data);
        delete controllerContract;
    }

Assessed type

Invalid Validation

[141345]

admin mistake

at most QA

[donosonaumczuk]

We do not accept this issue. From "Publicly Known Issues / Clarifications & Assumptions" section in the README:

"Governance and Proxy Admins are trusted. Issues that can come from Governance malicious executions or Proxy Admins malicious upgrades will not be taken into account"

[c4-sponsor]

donosonaumczuk marked the issue as sponsor disputed

[c4-judge]

Picodes marked the issue as unsatisfactory: Out of scope