search

code-423n4 / 2022-02-aave-lens-findings

0 stars 0 forks source link

_setGovernance should be two step process #3

Closed code423n4 closed 2 years ago

code423n4 commented 2 years ago

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 https://github.com/code-423n4/2021-11-bootfinance-findings/issues/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 commented 2 years ago

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 commented 2 years ago

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.