code423n4 commented 2 years ago

Lines of code

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

Vulnerability details

Impact

JBTiered721DelegateDeployer.deployDelegateFor cast every governance type to JB721GlobalGovernance. If the chosen governance contract type is JB721TieredGovernance or JB721GlobalGovernance, the cloned contract is casted to the wrong contract type (JB721GlobalGovernance). Therefore, regardless of the choice of the caller, the contract type of the delegate is JB721GlobalGovernance instead of the chosen type of governance.

Proof of Concept

There is 3 types of delegate depending to the governance type attached to it:

JB721GlobalGovernance (on-chain governance across all tiers)
JB721TieredGovernance (on-chain governance per-tier)
JBTiered721Delegate (no on-chain governance)

In the JBTiered721DelegateDeployer.deployDelegateFor function the newly deployed delegate of the chosen type of the caller is casted to the JB721GlobalGovernance contract type and assigned to the newDelegate variable of type JB721GlobalGovernance regardless of the governance type.

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

    JB721GlobalGovernance newDelegate = JB721GlobalGovernance(_clone(codeToCopy));

Tools Used

Manual review.

Recommended Mitigation Steps

I recommend to clone, deploy and cast to the corresponding contract type depending on the governance type of the delegate in the conditionnal statements like this:

    if (_deployTiered721DelegateData.governanceType == JB721GovernanceType.NONE)
      codeToCopy = address(noGovernance);
      JBTiered721Delegate newDelegate = JBTiered721Delegate(_clone(codeToCopy));
    else if (_deployTiered721DelegateData.governanceType == JB721GovernanceType.TIERED)
      codeToCopy = address(tieredGovernance);
      JB721TieredGovernance newDelegate = JB721TieredGovernance(_clone(codeToCopy));
    else if (_deployTiered721DelegateData.governanceType == JB721GovernanceType.GLOBAL)
      codeToCopy = address(globalGovernance);
      JB721GlobalGovernance newDelegate = JB721GlobalGovernance(_clone(codeToCopy));
    else revert INVALID_GOVERNANCE_TYPE();

From my understanding the team want the return variable of the JBTiered721DelegateDeployer.deployDelegateFor function to be a governance agnostic delegate, which make sense considering how the variable is used in JBTiered721DelegateProjectDeployer (only to set the delegate address as the data source). Therefore using IJBTiered721Delegate as return variable type is fine.

simon-something commented 2 years ago

Good catch

No value leak/functional impact imo (the interface has unimplemented functions, without fallback, if the caller "forget" what they wanted to deploy, calling these "ghost" fn will revert)

Picodes commented 2 years ago

Downgrading as QA as there is no risk involved with the finding.

c4-judge commented 2 years ago

Picodes changed the severity to QA (Quality Assurance)