search

code-423n4 / 2022-10-juicebox-findings

2 stars 0 forks source link

Delegate contracts ownership can be lost to Deployer. #135

Open code423n4 opened 1 year ago

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

Impact

When deploying a delegate, if the _deployTiered721DelegateData.owner input is not address(0), the delegate deployer contract, JBTiered721DelegateDeployer would transfer ownership of the delegate contract to the specified owner address input. However, if _deployTiered721DelegateData.owner == address(0), then contract does not transfer ownership. This will prevent any further changes to delegate contract once delegate contract is deployed and initialized. For example, JBTiered721Delegate.mintFor() cannot be called by owner since JBTiered721DelegateDeployer is the owner due to being the initializer and does not have a contract function that makes the external mintFor() call.

Proof of Concept

  1. Alice calls JBTiered721DelegateDeployer.deployDelegateFor() with the necessary input where _deployTiered721DelegateData.owner is address(0).
  2. The new delegate contract is initialized.
  3. In the if statement in line 99, since _deployTiered721DelegateData.owner is address(0) , no action is taken.
  4. New delegate contract is operational and contract functions are called and work as expected.
  5. However when Alice decides to mint tokens for beneficiary of a tier, calling any of the mintFor() in JBTiered721Delegate contract, it is not possible since there is no defined function in JBTiered721DelegateDeployer contract to make the external call to JBTiered721Delegate.mintFor().

Tools Used

Manual review

Recommended Mitigation Steps

Two possible ways to resolve this issue:

  1. Apply a zero address check for the _deployTiered721DelegateData.owner input.
  2. Add an additional function to the contract with necessary access control that can make the external call to transfer ownership of delegate contract.
drgorillamd commented 1 year ago

Duplicate #186

c4-judge commented 1 year ago

Picodes changed the severity to QA (Quality Assurance)

c4-judge commented 1 year ago

Picodes marked the issue as grade-b