-
Notifications
You must be signed in to change notification settings - Fork 2
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
pkqs90 - DEPOSIT_VAULT_ADMIN_ROLE/REDEMPTION_VAULT_ADMIN_ROLE have larger permission than expected: they shouldn't be able to pause vaults #112
Comments
Escalate, this issue has no impact given all listed roles are trusted except function _setupRoles() private {
address admin = msg.sender;
_setupRole(DEFAULT_ADMIN_ROLE, admin);
_setupRole(DEPOSIT_VAULT_ADMIN_ROLE, admin);
_setupRole(REDEMPTION_VAULT_ADMIN_ROLE, admin);
_setRoleAdmin(BLACKLISTED_ROLE, BLACKLIST_OPERATOR_ROLE);
_setRoleAdmin(GREENLISTED_ROLE, GREENLIST_OPERATOR_ROLE);
_setupRole(GREENLIST_OPERATOR_ROLE, admin);
_setupRole(BLACKLIST_OPERATOR_ROLE, admin);
_setupRole(M_TBILL_MINT_OPERATOR_ROLE, admin);
_setupRole(M_TBILL_BURN_OPERATOR_ROLE, admin);
_setupRole(M_TBILL_PAUSE_OPERATOR_ROLE, admin); |
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. |
I agree with escalation, these are trusted roles. |
I think it is about the difference between the readMe and the code. that's why it should be accepted. |
The entire protocol uses a role-based access control to manage permissions, a significant part of the code base is ensuring that each role can only execute the functions they are specifically permitted to, and nothing more or less. The README specifically asked. ''provide a more comprehensive description of what a role can and can't do/impact'', a comprehensive list was provided and roles of The README was also asked; The statement made here by @nevillehuang "The contest READ.ME simply states the role purpose, not the only permission it can perform, but the code logic indicates the admin role can perform all the trusted actions." Is actually inaccurate here is why:
I think I have made my point here for the sherlock judge to make final verdict. |
I assumed they're different roles with different permissions and they shouldn't have more permission than what described in docs, also saying admins are trusted means they won't perform malicious actions by having defined permissions and it doesn't mean they can perform actions that they don't have permission to |
Just to add another point of view. I would say this issue is valid or not depending on the definition of
Then I think it is valid. Personally I thought of To conclude, if the definition is the one said by @MxAxM , I think the issue is valid, if it is the one I said, I think it is invalid. |
All this will not change the fact that the admin is TRUSTED, he cannot be treated as someone who will do something bad, which reduces the impact of this "issue".
Just as it is not written that "he can pause", it is also not written that "he cannot pause". If the project contains a hierarchy of permissions, then each role would have to list all the roles it inherits, which would be crazy. |
Just to add to the comments I made earlier In the README "Additional audit information" section, sponsor specifically mentioned that one of the attack vectors they would want watson's to look out for is ensuring roles work properly. Which means verify that only authorized roles can execute their respective functions. Which also goes to underscore the fact that each admin should only be able to execute their specified functions as outlined in the README. The list of what a role can do was clearly stated and its logical to assume what was not in the list of what a role can do are things it can't do. There is no point to having a hierarchy of permissions. every role has its own explicit permissioned function. |
As said above, indeed, the Additional audit information specifically asks Watsons to see if all the roles work properly. Hence, I believe the question about the roles gave a full explanation what each role should do and they should be able to do nothing less or more. Yes, they're TRUSTED and we have to assume they won't harm the protocol in any way. But, I don't believe this issue is about completing malicious actions and the report doesn't talk about it. The problem is that under Sherlock's rules, if there is an issue breaking the statement/invariant explicitly mentioned in the README, it will be assigned Medium severity, regardless of the impact. Hence, I believe this issue has to stay valid. Planning to reject the escalation and leave the issue as it is. |
Also #83 is not a duplicate of this issue but an independent issue on its own that is worth looking into |
Hey @WangSecurity, this finding says , "High permission role can pause vaults". And if they could do so, there is nothing wrong to assume I specifically want to highlight issue F-2023-0290, which represent the same issue as this one, in one of previous audit report of Midas. Also, Readme,
|
Thank you for sharing, but I don't see how this issue is connected to this report. The issue from Hacken is about the Burner role easily accessing user's funds, this report is about several roles having permission for functions they shouldn't have. Hence, the decision remains the same. Planning to reject the escalation and leave the issue as it is. |
@WangSecurity The Anyways feel free to resolve this issue will respect your decision |
The README(contest details) asked watsons to ensure the roles work properly. The only way watson can do this is to check each role against it permissioned function as described by the contest details, And this issue is describing a role which has access to a function its not permitted by the README. This indeed goes against the contest details. |
@nevillehuang fair point, but still it doesn't change the fact that in fact DEPOSIT_VAULT_ADMIN_ROLE/REDEMPTIOJ_VAULT_ADMIN_ROLE have more rights then stated in the README. And README specifically asked to check the roles being set properly. I agree it's low, but under rules, since it breaks assumption from the README, it indeed should be Medium. The decision remains the same, reject the escalation and leave the issue as it is. |
Result: |
Escalations have been resolved successfully! Escalation status:
|
pkqs90
medium
DEPOSIT_VAULT_ADMIN_ROLE/REDEMPTION_VAULT_ADMIN_ROLE have larger permission than expected: they shouldn't be able to pause vaults
Summary
The accessibility of DEPOSIT_VAULT_ADMIN_ROLE and REDEMPTION_VAULT_ADMIN_ROLE has larger permission than what the contest readme claims: they can pause the vault and stop users from depositing/redeeming.
Vulnerability Detail
According to the contest readme:
DEPOSIT_VAULT_ADMIN_ROLE
has the role ofHandles freeFromMinDeposit, setMinAmountToDeposit, withdrawToken, addPaymentToken, removePaymentToken in DepositVault
.REDEMPTION_VAULT_ADMIN_ROLE
has the role ofHandles withdrawToken, addPaymentToken, removePaymentToken in RedemptionVault
.However, these two roles are also capable of pausing the depositVault/redemptionVault, which is unexpected.
https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/access/Pausable.sol#L18-L38
https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/abstract/ManageableVault.sol#L139-L141
https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/DepositVault.sol
https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/RedemptionVault.sol
Impact
The two roles DEPOSIT_VAULT_ADMIN_ROLE/REDEMPTION_VAULT_ADMIN_ROLE have larger permission than they are expected to have.
Code Snippet
Tool used
Manual review
Recommendation
Remove the pausability permission for DEPOSIT_VAULT_ADMIN_ROLE and REDEMPTION_VAULT_ADMIN_ROLE.
The text was updated successfully, but these errors were encountered: