alcueca
commented
3 years ago This bit is actually unchanged from OpenZeppelin. When we designed AccessControl.sol we weighted the trade-offs, and adding the account as a parameter works as a check that the user knows what he is doing, same as when github asks you to type the name of a repository you are about to delete.
Personally, I thought the safety check is overprotective, and might do as suggested and remove it.
I've disputed the finding, because I don't think that the risk of failing a transaction in an emergency situation outweighs the risk of users renouncing to permissions accidentally.
Handle
0xRajeev
Vulnerability details
Impact
Given that one can only renounce for self, there is no need to have the
accountparameter but instead simply use msg.sender. This is enforced in the check too.Given that this is expected to be used in an emergency situation of compromised account credentials (as noted in the function’s Natspec), it will reduce accidental transaction reverts if an account value is used that is different from msg.sender. It will also help to have a renounceRoles similar to revokeRoles to renounce all roles for an account instead of expecting individual calls for each role. This will be helpful during the emergency situation of compromised credentials.
Proof of Concept
Alice suspects a compromise of her account credentials and wants to renounce her role. She submits a transaction to do so but accidentally uses a different address for the account parameter. The renounceRole() call fails giving the attacker with the stolen credentials extra time or even success in subverting the access control permissions before Alice can resubmit another transaction with the correct account parameter.
https://github.com/code-423n4/2021-05-yield/blob/e4c8491cd7bfa5dc1b59eb1b257161cd5bf8c6b0/contracts/utils/access/AccessControl.sol#L207-L225
Tools Used
Manual Analysis
Recommended Mitigation Steps
Remove account parameter and directly use msg.sender in the call to _revokeRole()