Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

_setupRole() Deprecated and Not Using With Constructor Effectively Circumventing the Admin System #115

Open
code423n4 opened this issue Nov 29, 2021 · 1 comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")

Comments

@code423n4
Copy link
Contributor

Handle

Meta0xNull

Vulnerability details

Impact

 * [WARNING]
 * ====
 * This function should only be called from the constructor when setting
 * up the initial roles for the system.
 *
 * Using this function in any other way is effectively circumventing the admin
 * system imposed by {AccessControl}.
 * ====
 *
 * NOTE: This function is deprecated in favor of {_grantRole}.

There are multiple contracts that import Permissions.sol and using Deprecated Function _setupRole() with Security Problem that Applicable to all these contracts because all of the contracts use initialize() Rather Than Constructor.

Proof of Concept

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L174-L186
https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Permissions.sol#L53
https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Permissions.sol#L117
https://github.com/code-423n4/2021-11-malt/blob/main/src/contracts/Permissions.sol#L121

Tools Used

Manual Review

Recommended Mitigation Steps

Replace _setupRole() with _grantRole()

@code423n4 code423n4 added 1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working labels Nov 29, 2021
code423n4 added a commit that referenced this issue Nov 29, 2021
@0xScotch 0xScotch added the sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity") label Dec 8, 2021
@GalloDaSballo
Copy link
Collaborator

Great find by the warden, in lack of any exploitative POC, I agree with low severity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1 (Low Risk) Assets are not at risk. State handling, function incorrect as to spec, issues with comments bug Something isn't working sponsor confirmed Sponsor agrees this is a problem and intends to fix it (OK to use w/ "disagree with severity")
Projects
None yet
Development

No branches or pull requests

3 participants