Ownable Code Style Findings

OEL-01C: Generic Typographic Mistake

Type	Severity	Location
Code Style		Ownable.sol:L35

Description:

The referenced line contains a typographical mistake (i.e. private variable without an underscore prefix) or generic documentational error (i.e. copy-paste) that should be corrected.

Example:

Copy
35function senderCheckOwner() internal view {

Recommendation:

We advise this to be done so to enhance the legibility of the codebase.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The Ownable::senderCheckOwner function has been properly prefixed with an underscore (_) to indicate its internal nature, addressing this exhibit.

OEL-02C: Inefficient Event Emission

Type	Severity	Location
Gas Optimization		Ownable.sol:L25, L27

Description:

The referenced event emission is inefficient as it will store a local variable, adjust its storage counterpart, and then emit the event.

Example:

Copy
21function claimOwnership() external {
22    if (msg.sender != _newOwner) {
23        revert Errors.InvalidSender();
24    }
25    address _oldOwner = _owner;
26    _owner = _newOwner;
27    emit ClaimedOwnership(_owner, _oldOwner);
28}

Recommendation:

We advise the event to be emitted before the variable is adjusted from storage, rendering the local variable redundant.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The referenced code has been optimized albeit using a different approach as multiple storage variables are read from, caching them to local variables and emitting the variable at the end using local variables rather than storage entries. As such, we consider the code optimized in a way that addresses this exhibit.

OEL-03C: Non-Standard Initialization of Owner

Type	Severity	Location
Standard Conformity		Ownable.sol:L8

Description:

The Ownable contract has no constructor or initializer defined, instead requiring derivative contracts to assign to the _owner variable directly.

Example:

Copy
7abstract contract Ownable {
8    address internal _owner;
9    address internal _newOwner;

Recommendation:

We advise both a constructor and an initializer to be introduced, with the constructor invoking the initializer with the correct argument. This mechanism will ensure that the Ownable implementation is compatible with both non-upgradeable and upgradeable contracts (as that is the desire of the Myso Finance team) and will also ensure that the contract is always initialized properly, a trait not guaranteed as presently contacts adjust the _owner variable directly in derivative implementations which is non-standard.

Alleviation (c740f7c6b5ebd365618fd2d7ea77370599e1ca11):

The code was refactored as advised, introducing an Ownable::constructor as well as the Ownable::_initialize function that it invokes, standardizing the way the Ownable contract is initialized and enabling derivative implementations to use the Ownable::_initialize function to affect the _owner entry rather than directly writing to it.