Usage of an incorrect version of the `Ownable` library can potentially malfunction all `onlyOwner` functions

[c4-submissions]

Lines of code

https://github.com/code-423n4/2023-10-nextgen/blob/main/smart-contracts/NextGenAdmins.sol#L15

Vulnerability details

Description

As per the Automated Findings, the only contract that uses the onlyOwner modifier throughout the codebase is NextGenAdmins. However, the current contract implementation uses a non-upgradeable version of the Ownable library, named @openzeppelin/contracts/access/Ownable.sol, instead of the upgradeable version, named @openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol.

smart-contracts/NextGenAdmins.sol
13:   import "./Ownable.sol";
14:    
15:   contract NextGenAdmins is Ownable{

The problem with using a regular, non-upgradeable Ownable library is that it sets the deployer as the default owner in the constructor. However, in the upgradeable contracts, constructors cannot be used due to the requirement of the proxy-based upgradeability system. Therefore, when a contract is deployed as a proxy contract, there will be no owner, and all the onlyOwner functions will become inaccessible.

Proof of Concept

To take advantage of this security vulnerability, an attacker could use NextGenAdmins as a proxy contract. However, due to the nature of upgradeable contracts, the owner would not be set as constructors cannot be used. As a result, any functions with the onlyOwner modifier, such as onlyOwnerFunction(), would become inaccessible.

Recommendation

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

Assessed type

Library

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-pre-sort]

141345 marked the issue as insufficient quality report

[c4-pre-sort]

141345 marked the issue as sufficient quality report

[c4-sponsor]

a2rocket (sponsor) disputed

[a2rocket]

This is the intended design as we did not want to use an upgradeable contract, the team will deploy the NextGenAdmins contract and then move the ownership of the contract to a multisig gnosis safe wallet.

[alex-ppg]

The Warden notes that the NextGenAdmins contract is not upgrade-compatible due to the usage of a non-upgradeable Ownable OpenZeppelin variant.

As the Sponsor specifies, the contracts are not expected to be upgradeable and this is further evidenced by their setup throughout the codebase and their construction, rendering this exhibit invalid.

[c4-judge]

alex-ppg marked the issue as unsatisfactory: Invalid