fugazzi - Promo minter role cannot be assigned

[sherlock-admin3]

fugazzi

medium

Promo minter role cannot be assigned

Summary

The EDITION_MINTER_ROLE role used to enable access to the promoMint() function cannot be assigned since no entity can assign roles in the context of the Edition contract.

Vulnerability Detail

When the Edition contract is initialized it assigns the EDITION_MANAGER_ROLE.

function initialize(
    FeeManager feeManager_,
    TitlesGraph graph_,
    address owner_,
    address controller_,
    Metadata calldata metadata_
) external initializer {
    _initializeOwner(owner_);
    FEE_MANAGER = feeManager_;
    GRAPH = graph_;

    _grantRoles(controller_, EDITION_MANAGER_ROLE);
    _grantRoles(owner_, EDITION_PUBLISHER_ROLE);

    _metadata[0] = metadata_;
}

The EDITION_MANAGER_ROLE is given to the controller_ argument, which is the TitlesCore contract.

The Edition contract also restricts access to role management. The default implementation of OwnableRoles lets the owner manage roles, but the Edition contract overrides this behavior:

function grantRoles(address user_, uint256 roles_)
    public
    payable
    override
    onlyRoles(EDITION_MANAGER_ROLE)
{
    _grantRoles(user_, roles_);
}

/// @inheritdoc OwnableRoles
function revokeRoles(address user_, uint256 roles_)
    public
    payable
    override
    onlyRoles(EDITION_MANAGER_ROLE)
{
    _removeRoles(user_, roles_);
}

As we can see, role management is restricted to the EDITION_MANAGER_ROLE role, leaving out the owner.

Since the TitlesCore contract cannot grant roles (because there's no functionality to do so) and the owner is left out of the management role, then there is no way to grant anyone the EDITION_MINTER_ROLE.

Impact

The EDITION_MINTER_ROLE role cannot be assigned to anyone, crippling the promo mint functionality.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/editions/Edition.sol#L423-L430

Tool used

Manual Review

Recommendation

Allow the owner to grant the EDITION_MINTER_ROLE role. Other roles can still be restricted.

Duplicate of #148

[ccashwell]

This is expected, the EDITION_MANAGER_ROLE holder (TitlesCore) will be upgraded at a later point to leverage this functionality for a planned feature. In any case, it's not a vulnerability.

[realfugazzi]

This is expected, the EDITION_MANAGER_ROLE holder (TitlesCore) will be upgraded at a later point to leverage this functionality for a planned feature. In any case, it's not a vulnerability.

You can't predicate about future features or upgrades, which are not documented.

[realfugazzi]

Escalate

This is a valid medium severity issue, as it cripples the functionality of the smart contract, in line with Sherlock's judging rules (refer to https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue).

A future upgrade is merely a speculation which isn't visible in the state of the contract we reviewed during the contest. Additionally, future upgrades or intended future features related to the promo minter role were NOT documented in the contest, rendering these reasons invalid.

Moreover, the promo minter role capabilities are CLEARLY documented in the contest README (https://audits.sherlock.xyz/contests/326):

EDITION_MINTER_ROLE (Restricted) => On an Edition, this role can: 1) Mint promotional copies of any work (promoMint). There are no limitations on this action aside from the work's supply cap and minting period.

[sherlock-admin3]

Escalate

This is a valid medium severity issue, as it cripples the functionality of the smart contract, in line with Sherlock's judging rules (refer to https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue).

A future upgrade is merely a speculation which isn't visible in the state of the contract we reviewed during the contest. Additionally, future upgrades or intended future features related to the promo minter role were NOT documented in the contest, rendering these reasons invalid.

Moreover, the promo minter role capabilities are CLEARLY documented in the contest README (https://audits.sherlock.xyz/contests/326):

EDITION_MINTER_ROLE (Restricted) => On an Edition, this role can: 1) Mint promotional copies of any work (promoMint). There are no limitations on this action aside from the work's supply cap and minting period.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[ccashwell]

In any case, lack of usage of a particular role, especially one which is not important to the system in any way, is certainly not a vulnerability. The owner retains the capability of using the promoMint function. The function and the contract are fully usable, just not by this unused role.

[0xjuaan]

Agreeing with the escalation, as reported in #240

The contest README clearly states the following intended functionality of the EDITION_MANAGER_ROLE:

Set the Edition's ERC2981 royalty receiver (setRoyaltyTarget). This is the only way to change the royalty receiver for the Edition. Grant or revoke any role to/from any address (grantRole, revokeRole).

While the sponsor has now stated that the contract will be upgraded in the future to support these capabilities, this fact was not stated in the README, so it is assumed that the functionality stated in README should be met.

However this report demonstrates that due to an error, the intended functionality of the role is not achievable. This also leads to the EDITION_MINTER_ROLE being unusable, which is another breakage of a requirement stated in the README.

[realfugazzi]

In any case, lack of usage of a particular role, especially one which is not important to the system in any way, is certainly not a vulnerability. The owner retains the capability of using the promoMint function. The function and the contract are fully usable, just not by this unused role.

It breaks the functionality of the contract, that falls in the medium severity according to sherlock's rules.

[WangSecurity]

It's correct that the minter role indeed cannot be granted, but on the other hand, the promoMint function can be called by the owner of the contract, therefore, this functionality is still working and can be used. Hence, believe low/info severity is appropriate here. Planning to reject the escalation and leave the issue as it is.

[0xjuaan]

@WangSecurity it's not just the promoMint() function. As my report #240 explains, setRoyaltyTarget will be permanently uncallable due to this error, so functionality is lost.

[sammy-tm]

@WangSecurity

Since a role mentioned in the QA is not able to perform its given actions (as they cannot be assigned), I believe this falls under "breaking core functionality"

The QA explicitly states that this role should be able to use promoMint() :

Are there any protocol roles? Please list them and provide whether they are TRUSTED or RESTRICTED, or provide a more comprehensive description of what a role can and can't do/impact.

EDITION_MINTER_ROLE (Restricted) =>
On an Edition, this role can:
1) Mint promotional copies of any work (promoMint). There are no limitations on this action aside from the work's supply cap and minting period.

[WangSecurity]

Indeed, the watsons are correct here, planning to accept the escalation and create a new family of medium severity with the core issue of "roles are set incorrectly or not set at all" and duplicate with #148 (also in #148 you can see a more comprehensive list of duplicates.

[Evert0x]

Result: Medium Duplicate of #148

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• realfugazzi: accepted