Juicebox contest - seyni's results

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.