engineer - VaultFactory despite importing Ownable2Step is not inheriting from it thereby leaving the owner transfership mechanism vulnerable

[sherlock-admin4]

engineer

Medium

VaultFactory despite importing Ownable2Step is not inheriting from it thereby leaving the owner transfership mechanism vulnerable

Summary

1. VaultFactory imports Ownable2Step as seen here.
2. VaultFactory inherits from Ownable as seen here
3. This results in the ownership transfer mechanism to rely on Ownable instead of much safer Ownable2Step.

Vulnerability Detail

Single-step ownership transfer means that if a wrong address was passed when transferring ownership or admin rights it can mean that role is lost forever. The ownership pattern implementation for the protocol is in OpenZeppelin's Ownable.sol where a single-step transfer is implemented. This can be a problem for all methods marked in onlyOwner throughout VaultFactory contract, some of which are core contract functionality.

Impact

High, because important protocol functionality will be bricked

Likelihood

Low, because an error or mistake on the admin's end will cause this

Code Snippet

-   contract VaultFactory is ILidoVaultInitializer, Ownable {
+   contract VaultFactory is ILidoVaultInitializer, Ownable2Step {

Tool used

Manual Review

Recommendation

It is a best practice to use two-step ownership transfer pattern, meaning ownership transfer gets to a pending state and the new owner should claim his new rights, otherwise the old owner still has control of the contract. Consider using OpenZeppelin's Ownable2Step contract.

See Code Snippet section above for the recommended solution.