Governor can not accept the ownership of `ERC20Issuance_v1.sol`

[hats-bug-reporter[bot]]

Github username: @0xRizwan Twitter username: 0xRizwann Submission hash (on-chain): 0xcbcf02e21c22ac8f5168ff95bf1032fdce7453ac34bcd89f1f5c38699c677f21 Severity: medium

Description: Description\ Governor_v1.acceptOwnership() is used to accept the ownership of other contracts. This function specifically designed for only this purpose and it can not accept the ownership of its own. Community Multisig address can simply call `Governor_v1.acceptOwnership() with contract address as function argument to invoke the Ownable2Step.acceptOwnership() function so that ownership of that particular contract can be accepted by governor contract.

The issue is that ERC20Issuance_v1.sol does not implement Ownable2Step function so acceptOwnership() can not be called by governor contract. This is due to ERC20Issuance_v1 has inherited single step ownable contract which directly transfers the ownership of contract without any cross-acceptance.

contract ERC20Issuance_v1 is IERC20Issuance_v1, ERC20, Ownable {

It should be noted that, to serve the intended design of Governor_v1.acceptOwnership() function, following contracts have implemented the ownable2step contract.

1) ModuleFactory_v1 2) OrchestratorFactory_v1 3) InverterBeacon_v1 4) InverterProxyAdmin_v1

All of these functions wont have issue in transferring the ownership to governor contract to manage it as governor contract can simply accept their ownership.

However, This is not possible in case of ERC20Issuance_v1 to accept it ownership. This is only contract deviating from the others and also breaking the intended design of Governor_v1.acceptOwnership() for it.

Impact\ The sole purpose of Governor_v1.acceptOwnership()is to accept the ownership of other Invertor contracts as outlined above so that onlyOwner related functionalities can be managed by it, However, this is not possible in case of ERC20Issuance_v1 which breaks intended design of Governor_v1.acceptOwnership() function and protocol.

Recommedation to fix\ Based on above vulnerability explanation and to be inline with Invertor's intended design for Governor_v1.acceptOwnership()function, Recommend to use ownable2step to acheive its sole purpose.

Consider below changes in ERC20Issuance_v1.sol:

- import {Ownable} from "@oz/access/Ownable.sol";
+ import {Ownable2Step} from "@oz/access/Ownable2Step.sol";

- contract ERC20Issuance_v1 is IERC20Issuance_v1, ERC20, Ownable {
+ contract ERC20Issuance_v1 is IERC20Issuance_v1, ERC20, Ownable2Step {

[0xRizwan]

@Judges- This is not duplicate of #42. #42 is suggestion/best security practice and does not mention any vulnerabilty w.r.t to Inverter contracts. This issue highlights potential issue detailed above.

[PlamenTSV]

The Governor deals with the proxies. How the protocol chooses to handle Ownership transfers is at their own discretion, I do not see anywhere their intention of wanting to call acceptOwnership on the issuance token, this is just an assumption. This is exactly a duplicate of #42. The issuance cotract can transfer its ownership instead of accepting it.

[0xmahdirostami]

@PlamenTSV thanks.

[0xRizwan]

@PlamenTSV On asking about Governor_v1.acceptOwnership() function to sponsors, This was their response.

The intend of the function was that the governor itself has the ability to accept ownership of other contracts, not itself.

So, the intended design is that for all Inverter contracts the governor should explicitely accept the ownership of contracts instead of single ownership transfer by contract.

[0xRizwan]

@PlamenTSV On asking about Governor_v1.acceptOwnership() function to sponsors, This was their response.

The intend of the function was that the governor itself has the ability to accept ownership of other contracts, not itself.

So, the intended design is that for all Inverter contracts the governor should explicitely accept the ownership of contracts instead of single ownership transfer by contract.

@FHieser Isn't this issue?

[FHieser]

The accept ownership functionality was an edgecase implementation from ourside to make sure that in the seldom case, where a beacon doesnt sets the governor during the construction, that it would still be able to get ownership