fibonacci - FeeManager's admin cannot grant or revoke any role

[sherlock-admin3]

fibonacci

high

FeeManager's admin cannot grant or revoke any role

Summary

The FeeManager contract lacks an interface for the admin to grant or revoke roles.

Vulnerability Detail

The contest's README states:

ADMIN_ROLE
...
On FeeManager, this role can:
...
3) Grant or revoke any role to/from any address (`grantRole`, `revokeRole`).

The FeeManager contract, inherited from solady's OwnableRoles contract, only permits the owner to manage roles.

/// @dev Allows the owner to grant `user` `roles`.
/// If the `user` already has a role, then it will be a no-op for the role.
function grantRoles(address user, uint256 roles) public payable virtual onlyOwner {
    _grantRoles(user, roles);
}

/// @dev Allows the owner to remove `user` `roles`.
/// If the `user` does not have a role, then it will be a no-op for the role.
function revokeRoles(address user, uint256 roles) public payable virtual onlyOwner {
    _removeRoles(user, roles);
}

The FeeManager contract itself does not provide any interfaces that enable the admin to grant or revoke roles.

Impact

The admin cannot grant or revoke any roles. This limitation cannot be changed since the contract is not upgradable. A new role can only be granted or revoked by the owner, which is the TitlesCore contract, and it also lacks the corresponding functions.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/README.md#q-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-cant-doimpact
https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/fees/FeeManager.sol#L115

Tool used

Manual Review

Recommendation

Implement admin interfaces to manage roles

function grantRoles(address guy, uint256 roles)
    public
    payable
    override
    onlyOwnerOrRoles(ADMIN_ROLE)
{
    _grantRoles(guy, roles);
}

function revokeRoles(address guy, uint256 roles)
    public
    payable
    override
    onlyOwnerOrRoles(ADMIN_ROLE)
{
    _removeRoles(guy, roles);
}

[0xf1b0]

Escalate

I believe this is a valid issue. The implementation does not align with what is described in the README.

[sherlock-admin3]

Escalate

I believe this is a valid issue. The implementation does not align with what is described in the README.

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.

[WangSecurity]

Agree with escalation that it's valid, but I believe it could be spotted on deployment and fixed with re-deploy without losing funds. Moreover, the roles can still be set and revoked. Hence, believe medium is approriate here, planning to accept the escalation.

[WangSecurity]

Again, agree with escalation and making it a valid medium. Planning to make a new family of issues with the core issue "roles are set incorrectly or not set at all" with the following duplicates:

#166, #197, #213, #146.

[0xjuaan]

hi @WangSecurity, #240 is a dupe of this

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0xf1b0: accepted

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/titlesnyc/wallflower-contract-v2/pull/1

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.