_setGovernance should be two step process

[code423n4]

Lines of code

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L850-L854

Vulnerability details

Impact

Same as code-423n4/2021-11-bootfinance-findings#35.

The current governanceship transfer process involves the current governance calling setGovernance(). This function write the new governance's address into the _governance state variable. If the nominated EOA account is not a valid account, it is entirely possible the governance may accidentally transfer governanceship to an uncontrolled account, breaking all functions with the onlyGov() modifier.

Proof of Concept

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/LensHub.sol#L850-L854

https://github.com/code-423n4/2022-02-aave-lens/blob/main/contracts/core/modules/ModuleGlobals.sol#L94-L99

Tools Used

None

Recommended Mitigation Steps

Implement zero address check and consider implementing a two step process where the governance nominates an account and the nominated account needs to call an acceptGovernance() function for the transfer of governanceship to fully succeed. This ensures the nominated EOA account is a valid and active account.

[Zer0dot]

This is a widely known design pattern that we opted not to go with. Governance is expected to be a timelock and thus the security concern is on the governance executor, not this contract. In other words, if we fuck it up, we deserve it.

[0xleastwood]

As per the sponsor's comment, I agree with the dispute. Governance mis-using a function isn't a concern, especially when a timelock is used.