sherlock-admin3 commented 1 month ago

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 of Handles freeFromMinDeposit, setMinAmountToDeposit, withdrawToken, addPaymentToken, removePaymentToken in DepositVault.
REDEMPTION_VAULT_ADMIN_ROLE has the role of Handles 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

    modifier onlyPauseAdmin() {
>       _onlyRole(pauseAdminRole(), msg.sender);
        _;
    }

    /**
     * @dev upgradeable pattern contract`s initializer
     * @param _accessControl MidasAccessControl contract address
     */
    // solhint-disable-next-line func-name-mixedcase
    function __Pausable_init(address _accessControl) internal onlyInitializing {
        __WithMidasAccessControl_init(_accessControl);
    }

    function pause() external onlyPauseAdmin {
        _pause();
    }

    function unpause() external onlyPauseAdmin {
        _unpause();
    }

https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/abstract/ManageableVault.sol#L139-L141

    function pauseAdminRole() public view override returns (bytes32) {
        return vaultRole();
    }

https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/DepositVault.sol

    function vaultRole() public pure override returns (bytes32) {
        return DEPOSIT_VAULT_ADMIN_ROLE;
    }

https://github.com/sherlock-audit/2024-05-midas/blob/main/midas-contracts/contracts/RedemptionVault.sol

    function vaultRole() public pure override returns (bytes32) {
        return REDEMPTION_VAULT_ADMIN_ROLE;
    }

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.

nevillehuang commented 1 month ago

Escalate, this issue has no impact given all listed roles are trusted except GREENLISTED_ROLE and BLACKLISTED_ROLE. Notice that in MidasAccessControl.sol all roles are delegated to the admin, so in fact admin has the power of all trusted roles. 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.

    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);

sherlock-admin3 commented 1 month ago

Escalate, this issue has no impact given all listed roles are trusted except GREENLISTED_ROLE and BLACKLISTED_ROLE. Notice that in MidasAccessControl.sol all roles are delegated to the admin, so in fact admin has the power of all trusted roles. 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.
    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.

pronobis4 commented 1 month ago

I agree with escalation, these are trusted roles.

amankakar commented 1 month ago

I think it is about the difference between the readMe and the code. that's why it should be accepted. reamMe does not stated that the DEPOSIT_VAULT_ADMIN_ROLE/REDEMPTION_VAULT_ADMIN_ROLE can pause/unpasue the deposits/redemption.

Afriaudit commented 1 month ago

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 DEPOSIT_VAULT_ADMIN_ROLE/REDEMPTION_VAULT_ADMIN_ROLE didn't include pausing deposits and redeem.

The README was also asked; "For permissioned functions, please list all checks and requirements that will be made before calling the function." and the response is: "onlyRole() - Checks if the permissioned function is being called by an address with its respective admin role " This is to underscore that each admin Is only supposed to execute only functions they are permitted to. functions which are stated on the readme.

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:

"admin role can perform all the trusted actions": Its only the default admin that has all functions according to the code logic after launch. Its because its literarily the admin by default and according to the README default admin is expected to Grants and revokes roles even to other admins. The code even has a logic implementation for this. showing that the admins of each role is expected to change from default admin after launch.
"The contest READ.ME simply states the role purpose, not the only permission it can perform": I dont agree with this as the README shows a comprehensive list of all permissioned functions and exactly who is to call them except for this particular role. From all being said it's reasonable to conclude that admins are trusted to manage only the roles they are specifically authorized to control, and nothing beyond that. The whole point of the role-based access control will be pointless if an admin role can just arbitrarily call a function outside its own permissioned function because it trusted with its own permitted roles.

I think I have made my point here for the sherlock judge to make final verdict.

MxAxM commented 1 month ago

Escalate, this issue has no impact given all listed roles are trusted except GREENLISTED_ROLE and BLACKLISTED_ROLE. Notice that in MidasAccessControl.sol all roles are delegated to the admin, so in fact admin has the power of all trusted roles. 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.
    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);

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

CarlosAlegreUr commented 1 month ago

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

Just to add another point of view. I would say this issue is valid or not depending on the definition of TRUSTED in Sherlock. As @MxAxM defined it here:

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

Then I think it is valid. Personally I thought of TRUSTED as: Someone who whatever it can do it doesnt matter because it is assumed it will never do something malicious. Thus when auditing I didnt even check what TRUSTED roles could or could not do. I would appreciate a clear definition of what a TRUSTED actor is in Sherlock, could not find it on their docs.

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.

pronobis4 commented 1 month ago

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".

''provide a more comprehensive description of what a role can and can't do/impact''

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.

Afriaudit commented 1 month ago

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.

WangSecurity commented 1 month ago

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.

Afriaudit commented 1 month ago

Also #83 is not a duplicate of this issue but an independent issue on its own that is worth looking into

blutorque commented 1 month ago

Hey @WangSecurity, this finding says , "High permission role can pause vaults". And if they could do so, there is nothing wrong to assume mint/burn actions can also be performed irrespective of your user action.

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,

A note for Sherlock Watsons: Anything in the following section is considered a known issue and will be inelgible for rewards.

Our most recent audit can be found here.

WangSecurity commented 1 month ago

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.

nevillehuang commented 1 month ago

@WangSecurity The DEFAULT_ADMIN_ROLE which is set as the admin inherently holds the right to change these admins at any time. Is this really a security issue that goes against the contest details? The admin will likely never renounce this role.

Anyways feel free to resolve this issue will respect your decision

Afriaudit commented 1 month ago

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.

WangSecurity commented 1 month ago

@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.

WangSecurity commented 1 month ago

Result: Medium Has duplicates

sherlock-admin4 commented 1 month ago

Escalations have been resolved successfully!

Escalation status:

nevillehuang: rejected

sherlock-audit / 2024-05-midas-judging

pkqs90 - DEPOSIT_VAULT_ADMIN_ROLE/REDEMPTION_VAULT_ADMIN_ROLE have larger permission than expected: they shouldn't be able to pause vaults #112

DEPOSIT_VAULT_ADMIN_ROLE/REDEMPTION_VAULT_ADMIN_ROLE have larger permission than expected: they shouldn't be able to pause vaults

Summary

Vulnerability Detail

Impact

Code Snippet

Tool used

Recommendation