SimplifiedModule remains un-upgradeable

[hats-bug-reporter[bot]]

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

Description: Description\

The SimplifiedModule contract is designed to be upgradeable but is not upgradeable. SimplifiedModule is designed as upgradeable proxy which passes transaction for execution by programmable account.

The issue lies in deviating from correct implementation of upgradeable contracts, refers to OZ’s docs.

Proper implementation of upgradeable contracts is possible only by using initializer which in turn will call the base contract's initializers.

Attack Scenario\

The absence of initialization call will be unable to call the below mentioned functions that are the main logic in theOwnableUpgradeable contract.

    function __Ownable_init(address initialOwner) internal onlyInitializing {
        __Ownable_init_unchained(initialOwner);
    }

    function __Ownable_init_unchained(address initialOwner) internal onlyInitializing {
        if (initialOwner == address(0)) {
            revert OwnableInvalidOwner(address(0));
        }
        _transferOwnership(initialOwner);
    }

The result will be owner address will always remain address(0) and use of any onlyOwner based functions will always revert.

Attachments

https://github.com/hoprnet/hoprnet/blob/master/packages/ethereum/contracts/src/node-stake/permissioned-module/SimplifiedModule.sol#L6

1. Proof of Concept (PoC) File

The use of onlyOwner based function in SimplifiedModule on Line#79 is linked as below:

/**
* @dev Override {_authorizeUpgrade} to only allow owner to upgrade the contract
 */
function _authorizeUpgrade(address) internal override(UUPSUpgradeable) onlyOwner { }

The impact is clearly understandable from the comments as when the upgrade mechanism will be processed, the function checks for caller authorization, and as a result it will revert.

Simply create a test for SimplifiedModule contract and run the below test.

address ownerAddress = '0x1234...';

function test_ownerCheck() public {
        expect(SimplifiedModule.owner()).to.be.equal(ownerAddress);
    }

The test clearly indicates after deployment the owner is address(0).

This makes the SimplifiedModule contract to be clearly un-upgradeable. Whenever any functionality will be required to upgraded, it would not be possible due to lack of any owner.

This will also impact the related contract NodeManagementModule that have various onlyOwner administered functions which will become inoperable due to lack of owner based role.

2. Revised Code File (Optional)

Implement initialize function in the SimplifiedModule.sol as mentioned in OZ’s docs.

function initialize() public initializer {
    __Ownable_init();
  }

[QYuQianchen]

initialize is implemented in "HoprNodeManagementModule". Also, wwner of the singleton of the Module is not relevant.

[Abbasjafri-syed]

In the HoprNodeManagementModule contract, the OwnableUpgradeable is not initialized as it needs to explicitly call _Ownable_init() in the initialize function, refer to OZ’s guide that states:

‘Constructors are replaced by internal initializer functions following the naming convention __{ContractName}_init. Since these are internal, you must always define your own public initializer function and call the parent initializer of the contract you extend’.

The OwnableUpgradeable contract is initialize through __Ownable_init(), as the initializer sets the deployer as the initial owner of the contract. The _transferOwnership() function cannot be called without initializing OwnableUpgradeable, as it will set address(0) as owner of the contract.

Check this audit finding , by Openzeppelin itself that is same to the issue identified for this codebase to get more clarification.

Having initialize() does not make OwnableUpgradeable contract work, it needs to be properly initialise using the _Ownable_init function. This is based on requirement of upgradeability system as no constructors can be used in upgradeable contracts. Therefore, there will be no owner when the contract is deployed as a proxy contract.

Inheritance of SimplifiedModule expands the onlyOwner scope as there will be total of 13 methods of which 12 exists in HoprNodeManagementModule as when the upgrade will be carried out all the function having onlyOwner become useless.

[QYuQianchen]

Appreciate the detailed breakdown. This issue has been discovered and an issue has been opened for it before the audit contest started. All those information was communicated prior to the contest.

[Abbasjafri-syed]

Appreciate the detailed breakdown. This issue has been discovered and an issue has been opened for it before the audit contest started. All those information was communicated prior to the contest.

The discovered issue does not pertain to my findings as it is related to using UUPS method to upgrade the module implementation.

My issue clearly mentioned un-initializaton of OwnableUpgradeable contract as when the upgradeable mechanism will be run, it will check for Owner and will revert when found address(0) due to un-initializaton.

You can refer to Openzeppelin UUPS documentation for UUPS upgradeable mechanism that checks for owner before implementation.

[QYuQianchen]

As you've mentioned, as correctly calling the _Ownable_init is a must for correctly using OwnableUpgradeable. The said issue has implied this function call, among others, to be implemented. This is not an unknown issue.

[Abbasjafri-syed]

Appreciate the detailed breakdown. This issue has been discovered and an issue has been opened for it before the audit contest started. All those information was communicated prior to the contest.

i don't see the finding included in the issues section... as others are there

[QYuQianchen]

Please check the hyperlink provided above. It's part of a tracking epic.

[Abbasjafri-syed]

Please check the hyperlink provided above. It's part of a tracking epic.

If you are referring to Post-Providence i am not seeing any of it, mentioning my finding...

Can you precisely provide the tracking number..so it clarifies everything

[QYuQianchen]

https://github.com/hoprnet/hoprnet/issues/5530 as in

Appreciate the detailed breakdown. This issue has been discovered and an issue has been opened for it before the audit contest started. All those information was communicated prior to the contest.

[Abbasjafri-syed]

Appreciate the detailed breakdown. This issue has been discovered and an issue has been opened for it before the audit contest started. All those information was communicated prior to the contest.

As i already replied to this its related to enhancing upgradeability mechanism and not initialisation of OwnableUpgradeable.

The discovered issue does not pertain to my findings as it is related to using UUPS method to upgrade the module implementation.

My issue clearly mentioned un-initializaton of OwnableUpgradeable contract as when the upgradeable mechanism will be run, it will check for Owner and will revert when found address(0) due to un-initializaton.

You can refer to Openzeppelin UUPS documentation for UUPS upgradeable mechanism that checks for owner before implementation.

If you mean including my issue afterwards in the tracking....didn't this make it a valid finding...

[QYuQianchen]

No I didn't mean "including your issue afterwards in the tracking". Your submission is a duplicate to #5530

[Abbasjafri-syed]

As you've mentioned, as correctly calling the _Ownable_init is a must for correctly using OwnableUpgradeable. The said issue has implied this function call, among others, to be implemented. This is not an unknown issue.

i don't see it as a duplicate as it refers to completely different matter... as you also mentioned

[QYuQianchen]

The ownership is actually transferred in the initialize() function https://github.com/hats-finance/SafeStaking-by-HOPR-0x607386df18b663cf5ee9b879fbc1f32466ad5a85/blob/8822abcfa5348b8e1f45c1d9fa5a5135090e0622/packages/ethereum/contracts/src/node-stake/permissioned-module/NodeManagementModule.sol#L99