Position.sol: usage of an incorrect version of Ownable library can potentially malfunction all onlyOwner functions

[code423n4]

Lines of code

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L14

Vulnerability details

Impact

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Ownable.sol#L3

// From https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/Ownable.sol

The current implementaion is using a non-upgradeable version of the Ownable library isnstead of the upgradeable version: @openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol.

A regular, non-upgradeable Ownable library will make the deployer the default owner in the constructor. Due to a requirement of the proxy-based upgradeability system, no constructors can be used in upgradeable contracts. Therefore, there will be no owner when the contract is deployed as a proxy contract

Proof of Concept

https://github.com/code-423n4/2023-04-frankencoin/blob/main/contracts/Position.sol#L14

    contract Position is Ownable, IPosition, MathUtil {

Tools Used

Manual review

Recommended Mitigation Steps

Use @openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol instead

[c4-pre-sort]

0xA5DF marked the issue as low quality report

[0xA5DF]

Invalid There's not immutable variables in the Ownable contract, so it doesn't make a difference (the only difference is probably the gap which isn't needed here)

[c4-judge]

hansfriese marked the issue as selected for report

[c4-judge]

hansfriese marked the issue as not selected for report

[c4-judge]

hansfriese marked the issue as unsatisfactory: Invalid