rvierdiiev - AccessControlFacet doesn't have ability to set admin for the role

[sherlock-admin2]

rvierdiiev

medium

AccessControlFacet doesn't have ability to set admin for the role

Summary

AccessControlFacet doesn't have ability to set admin for the role.

Vulnerability Detail

AccessControlFacet is created to allow protocol manage different roles. Facet extends AccessControlInternal, which has different methods and uses LibAccessControl library to store executed roles actions.

While LibAccessControl library has setRoleAdmin function, both AccessControlFacet and AccessControlInternal don't use it. And as result it's not possible to set admin for the role.

Because of that only default admin will be used as parent role and protocol will not be able to granulary manage their roles.

Impact

Admin for the role can't be set.

Code Snippet

Provided above

Tool used

Manual Review

Recommendation

Add function on the facet that will set admin for the role.

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

Owner is set as a default admin, it should work

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

auditsea commented:

Owner is set as a default admin, it should work

[molecula451]

this looks like an invalid because the admin can set new roles:

by "granting" its just the function its named grantRoles instead of set!

the submission claims that the Facet don't use it, the admin can indeed use it!

@rndquu @gitcoindev

[gitcoindev]

Granting roles is tested at https://github.com/sherlock-audit/2023-12-ubiquity/blob/d9c39e8dfd5601e7e8db2e4b3390e7d8dff42a8e/ubiquity-dollar/packages/contracts/test/diamond/facets/AccessControlFacet.t.sol , could we please get feedback from the Watson who submitted this issue directly or from the @AuditSea perhaps something was misunderstood here?

[rndquu]

this looks like an invalid because the admin can set new roles:

by "granting" its just the function its named grantRoles instead of set!

the submission claims that the Facet don't use it, the admin can indeed use it!

@rndquu @gitcoindev

This issue is about setting a role admin for some other role, not about granting roles.

Here is the setRoleAdmin() method which is missing in the AccessControlFacet contract. Should we want to set some role to be admin for another role we would need to upgrade the contracts by adding the setRoleAdmin() method to facets.

I would consider this valid and "will fix".

[molecula451]

good catch, yeah we missed the setRoleAdmin external function on the facet

[molecula451]

PR Fix Confirmation: ubiquity/ubiquity-dollar#880 by @gitcoindev

[0xArz]

Escalate

I think this should be a low/qa as the impact here is not severe. There is no loss of funds and the roles can always be managed by the DEFAULT_ADMIN_ROLE which is set

[sherlock-admin2]

Escalate

I think this should be a low/qa as the impact here is not severe. There is no loss of funds and the roles can always be managed by the DEFAULT_ADMIN_ROLE which is set

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.

[nevillehuang]

@0xArz you might want to reconsider you escalation based on this comment here. Based on sherlock medium severity rules, This breaks intended contract functionality, so it will likely be warranted medium severity even if there is no loss of funds

[molecula451]

We fixed this one check: #90 (comment)

[molecula451]

we will leave it as a medium

[0xArz]

@0xArz you might want to reconsider you escalation based on this comment here. Based on sherlock medium severity rules, This breaks intended contract functionality, so it will likely be warranted medium severity even if there is no loss of funds

I get that yeah but it doesnt break intended contract functionality if the admin of all roles is the DEFAULT_ADMIN_ROLE which will always be able to manage the roles and no fix is needed. Having seperate admins to manage the roles would ofc be good but i believe this should be a low so i will keep my escalation.

[ubiquibot]

+ Evaluating results. Please wait...

[0x4007]

@molecula451 its the character length bug. I can try to push a workaround real quick.

• 65536 GitHub Comment Character Limit ubiquibot/comment-incentives#19

[molecula451]

@pavlovcik go ahead

[ubiquibot]

# No linked pull requests to close

[ubiquibot]

+ Evaluating results. Please wait...

[ubiquibot]

[ 9 WXDAI ]

@pavlovcik

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	3	9

Conversation Incentives

Comment	Formatting	Relevance	Reward
@molecula451 https://github.com/sherlock-audit/2023-12-ubiquity-...	1.2	-	1.2
> ```diff > ! action has an uncaught error > ``` I think th...	4.5 code: count: 2 score: "2" words: 2	-	4.5
@molecula451 its the character length bug. I can try to push a w...	3.3 li: count: 1 score: "1" words: 8	-	3.3

[ 12.6 WXDAI ]

@molecula451

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	8	12.6

Conversation Incentives

Comment	Formatting	Relevance	Reward
this looks like an invalid because the admin can [set new roles]...	5.2 a: count: 1 score: "1" words: 3	-	5.2
good catch, yeah we missed the setRoleAdmin external function on...	1.2	-	1.2
PR Fix Confirmation: https://github.com/ubiquity/ubiquity-dollar...	1.3	-	1.3
We fixed this one check: https://github.com/sherlock-audit/2023-...	1.8	-	1.8
we will leave it as a medium...	0.7	-	0.7
> Would like to add that this is not a bug, it is missing admi...	1.4	-	1.4
@Evert0x can you reopen - reclose this one...	0.7	-	0.7
@pavlovcik go ahead...	0.3	-	0.3

[ 4.7 WXDAI ]

@gitcoindev

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	1	4.7

Conversation Incentives

Comment	Formatting	Relevance	Reward
Granting roles is tested at https://github.com/sherlock-audit/20...	4.7	-	4.7

[ 11.3 WXDAI ]

@rndquu

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	1	11.3

Conversation Incentives

Comment	Formatting	Relevance	Reward
> this looks like an invalid because the admin can [set new role...	11.3 a: count: 3 score: "3" words: 5 code: count: 2 score: "2" words: 2	-	11.3

[0x4007]

In the future @molecula451 be sure to link a pull request for assignees to get their credit.

[molecula451]

i think we can do it again, the bot won't trigger for the comments? @pavlovcik

[molecula451]

looks like gitcoindev PR comment did not hook here ubiquity/ubiquity-dollar#880 (comment)

[ubiquibot]

@gitcoindev the deadline is at 2024-02-18T12:03:12.950Z

[ubiquibot]

+ Evaluating results. Please wait...

[ubiquibot]

[ 10.7 WXDAI ]

@pavlovcik

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	4	10.7

Conversation Incentives

Comment	Formatting	Relevance	Reward
@molecula451 https://github.com/sherlock-audit/2023-12-ubiquity-...	1.2	-	1.2
> ```diff > ! action has an uncaught error > ``` I think th...	4.5 code: count: 2 score: "2" words: 2	-	4.5
@molecula451 its the character length bug. I can try to push a w...	3.3 li: count: 1 score: "1" words: 8	-	3.3
In the future @molecula451 be sure to link a pull request for as...	1.7	-	1.7

[ 16.1 WXDAI ]

@molecula451

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	10	16.1

Conversation Incentives

Comment	Formatting	Relevance	Reward
this looks like an invalid because the admin can [set new roles]...	5.2 a: count: 1 score: "1" words: 3	-	5.2
good catch, yeah we missed the setRoleAdmin external function on...	1.2	-	1.2
PR Fix Confirmation: https://github.com/ubiquity/ubiquity-dollar...	1.3	-	1.3
We fixed this one check: https://github.com/sherlock-audit/2023-...	1.8	-	1.8
we will leave it as a medium...	0.7	-	0.7
> Would like to add that this is not a bug, it is missing admi...	1.4	-	1.4
@Evert0x can you reopen - reclose this one...	0.7	-	0.7
@pavlovcik go ahead...	0.3	-	0.3
i think we can do it again, the bot won't trigger for the commen...	1.6	-	1.6
looks like gitcoindev PR comment did not hook here https://githu...	1.9	-	1.9

[ 204.7 WXDAI ]

@gitcoindev

Contributions Overview

View	Contribution	Count	Reward
Issue	Task	1.00	200
Issue	Comment	1	0
Issue	Comment	1	4.7

Conversation Incentives

Comment	Formatting	Relevance	Reward
Granting roles is tested at https://github.com/sherlock-audit/20...	-	-	-
Granting roles is tested at https://github.com/sherlock-audit/20...	4.7	-	4.7

[ 11.3 WXDAI ]

@rndquu

Contributions Overview

View	Contribution	Count	Reward
Issue	Comment	1	11.3

Conversation Incentives

Comment	Formatting	Relevance	Reward
> this looks like an invalid because the admin can [set new role...	11.3 a: count: 3 score: "3" words: 5 code: count: 2 score: "2" words: 2	-	11.3

[molecula451]

yeah pavlovick it's not hooking cross-side PR comment, tested with different keywords @pavlovcik

[0x4007]

yeah pavlovick it's not hooking cross-side PR comment, tested with different keywords @pavlovcik

The pull request must be linked to this issue. It currently is not that's why the bot did not associate the conversation.

[sherlock-admin]

The protocol team fixed this issue in PR/commit ubiquity/ubiquity-dollar#880.

[sherlock-admin]

The Lead Senior Watson signed off on the fix.