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

Access control vulnerability updated. #87

Closed
wants to merge 6 commits into from

Conversation

0xSandyy
Copy link
Contributor

Related Issue

Checklist

Describe the changes you've made:

I updated the insufficient access control vulnerability by adding some access control vulnerabilities and some mitigation strategies/best practices.

Type of change

Select the appropriate checkbox:

  • Bug fix (fixing an issue with existing vulnerability data)
  • New feature (adding a new vulnerability or category)
  • Documentation update (improving existing information)

Copy link
Owner

@kadenzipfel kadenzipfel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tbh I feel like most of this can really be simplified. The core of the issue mostly just comes down to lack of access control being the problem and adding access control being the solution. The additional explanations kinda take away from it and are mostly not relevant to the core issue

}
```
If the contract assigns multiple roles with ``onlyOwner`` privileges, it increases the attack surface. An attacker compromising a single owner account could execute critical functions.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This shouldn't be possible with OZ Ownable though

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad. OZ Ownable is the fix for role based access control with multiple onlyOwner privileges. Will fix.

@0xSandyy
Copy link
Contributor Author

Yeah, I overdid it. How about adding a simple example like a withdraw() function or a critical function which has no access control with a fix using a require / onlyOwner modifier? Simple example to showcase lack of access control and it's fix.

- https://metaschool.so/articles/access-control-vulnerabilities-in-smart-contracts/
- https://medium.com/rektify-ai/how-to-mitigate-access-control-vulnerability-6df74c82af98
- https://github.com/Quillhash/Solidity-Attack-Vectors/blob/main/data/1.md
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@0xSandyy, Could you please convert links to hyper links

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the remainder!

@kadenzipfel
Copy link
Owner

Replaced by: #101

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Insufficient Access Control vulnerability.
3 participants