If the "renounceOwnership" authorization is used, the project becomes unavailable

[code423n4]

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721Delegate.sol#L27-L29

Vulnerability details

Impact

onlyOwner has another secret import (from Ownable.sol) privilege: renounceOwnership(). They can use this authority whenever he wants, there is no restriction. If he uses this authority, the very important functions detailed below will not be available, updated or added.

We see the use of Openzeppelin in Ownable.sol in many contracts and owners can renounceOwnership() like this project, which is sometimes a positive as it reduces the risk of rugpull but the situation is a bit different here, Owner is constantly needed (For example adding LP) , so security risk is high

However, in this project, there may be a constant need for the Owner for many reasons, for example, the central server where NFT's metadata is located may become obsolete and the TokenbaseURI address may need to be changed.

Key powers of OnlyOwner;

JBTiered721Delegate.sol:

 // Manually mint NFTs from tiers.
  function mintFor(uint16[] memory _tierIds, address _beneficiary) public override onlyOwner

 // Set a token URI resolver
  function setTokenUriResolver(IJBTokenUriResolver _tokenUriResolver) external override onlyOwner {

 // Set a contract metadata URI to contain opensea-style metadata.
  function setContractUri(string calldata _contractUri) external override onlyOwner {

 // Set a base token URI
  function setBaseUri(string memory _baseUri) external override onlyOwner {

 // Sets the beneificiary of the reserved tokens for tiers where a specific beneficiary isn't set
  function setDefaultReservedTokenBeneficiary(address _beneficiary) external override onlyOwner {

 // Adjust the tiers mintable through this contract, adhering to any locked tier constraints.
  function adjustTiers(JB721TierParams[] calldata _tiersToAdd, uint256[] calldata _tierIdsToRemove)
    external
    override
    onlyOwner

 // Mint tokens within the tier for the provided beneficiaries
  function mintFor(JBTiered721MintForTiersData[] memory _mintForTiersData)
    external
    override
    onlyOwner
  {

1 - OnlyOwner does renounceOwnership() based on her authority in the JBTiered721Delegate.sol contract 2 - In the project, the URI server has been disabled and metadata has been moved to the new server. Finally, the BaseTokenURI address from the contract needs to be updated. 3 - Unfortunately, the token cannot be included in the project as OnlyOwner is the only authorized for the URI

Tools Used

Manual Code Review

Recomendation Steps:

Instead of directly importing the Ownable.sol contract, a project-specific Ownable.sol should be used by removing the renounceOwnership() function, which is the subject of the above-mentioned potential problem.

[drgorillamd]

This is OpenZeppelin Ownable library design, which is legit (a lot of method should be only called with great care - approve, transfer, etc - and this is out of scope)

[Picodes]

Indeed renounceOwnership should be called with great care, but I don't see a strong argument on why we should remove it. The URI server example does not hold for IPFS for example

[c4-judge]

Duplicate of https://github.com/code-423n4/2022-10-juicebox-findings/issues/220

[c4-judge]

Picodes marked the issue as grade-b

[c4-judge]

Picodes marked the issue as grade-a