sherlock-audit / 2023-04-footium-judging

13 stars 7 forks source link

SanketKogekar - OwnableUpgradeable uses single-step ownership transfer #358

Closed sherlock-admin closed 1 year ago

sherlock-admin commented 1 year ago

SanketKogekar

medium

OwnableUpgradeable uses single-step ownership transfer

Summary

All contracts have OwnableUpgradeable, which can possibly brick the contract. Single-step ownership transfer is not recommended.

Likelihood: Low, because it requires an error on the admin side Impact: High, because important protocol functionality will be bricked

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 OwnableUpgradeable.sol where a single-step transfer is implemented.This can be a problem for all methods marked in onlyOwner throughout the protocol, some of which are core protocol functionality.

Impact

Code Snippet

https://github.com/sherlock-audit/2023-04-footium/blob/main/footium-eth-shareable/contracts/FootiumAcademy.sol#L28-L29

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.