code423n4 commented 1 year ago

Lines of code

https://github.com/jbx-protocol/juice-nft-rewards/blob/f9893b1497098241dd3a664956d8016ff0d0efd0/contracts/JBTiered721DelegateDeployer.sol#L99-L100

Vulnerability details

Description

deployDelegateFor() is used to create and initialize a new delegate for the project:

JB721GlobalGovernance newDelegate = JB721GlobalGovernance(_clone(codeToCopy));
newDelegate.initialize(
  _projectId,
  _deployTiered721DelegateData.directory,
  _deployTiered721DelegateData.name,
  _deployTiered721DelegateData.symbol,
  _deployTiered721DelegateData.fundingCycleStore,
  _deployTiered721DelegateData.baseUri,
  _deployTiered721DelegateData.tokenUriResolver,
  _deployTiered721DelegateData.contractUri,
  _deployTiered721DelegateData.pricing,
  _deployTiered721DelegateData.store,
  _deployTiered721DelegateData.flags
);
// Transfer the ownership to the specified address.
if (_deployTiered721DelegateData.owner != address(0))
  newDelegate.transferOwnership(_deployTiered721DelegateData.owner);

The initialize() call transfers ownership of the delegate to the deployer contract. If owner is not 0, newDelegate.transferOwnership() transfers ownership to the desired owner. The issue is that if owner is set to 0x0000..., the function silently leaks ownership of delegate to the deployer contract, which is unable to maintain it. Therefore, all onlyOwner functionality in JBTiered721Delegate is forever lost. Because the function returns successfully, project will think the deployment worked and will start funding it, etc. Only later will they try to use owner functionality, like setDefaultReservedTokenBeneficiary(), and figure out they are locked out.

If the lack of require(owner != address(0) check is intentional, which may be the case because it seems project thought of this case, then the problem is that DelegateDeployer now has the authority to manage all zero owner projects. It introduces a very unnecessary centralization threat where the deployer implementation contract could be updated with malicious management code.

Impact

Project is initialized as unmaintainable if owner address passed is 0.

Tools Used

Manual audit

Recommended Mitigation Steps

Add a zero address check for passed owner parameter.

Discussion

This vulnerability is similar to NounsDao flaw, where if a contract is initialized with zero value, it introduces major lateral risks.

drgorillamd commented 1 year ago

where the deployer implementation contract could be updated with malicious management code. deployer is non-upgradable

Having ownership to the address 0 or to the deployer is, in practice, the same -> project owner cannot modify the delegate characteristics. This is per design (params set in stone or updatable), the choice to keep the ownership to the deployer contract (which has no way to interact with the delegate) is for gas saving.

Picodes commented 1 year ago

DelegateDeployer being a contract its owner won't be able to modify parameters so no centralization is added.

The behavior seems intentional as we have this line if (_deployTiered721DelegateData.owner != address(0)), and the intent is that _deployTiered721DelegateData.owner == address(0) means the parameters are immutable, so downgrading to QA.

c4-judge commented 1 year ago

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