sherlock-audit / 2024-05-midas-judging

13 stars 6 forks source link

recursiveEth - Vulnerability in Access Control Allows Blacklisted Users to Revoke Blacklist Role #151

Closed sherlock-admin3 closed 5 months ago

sherlock-admin3 commented 6 months ago

recursiveEth

high

Vulnerability in Access Control Allows Blacklisted Users to Revoke Blacklist Role

Summary

A vulnerability exists in the mTBILL contract, which inherits from Blacklistable and uses AccessControlUpgradeable from OpenZeppelin to manage roles. Specifically, the renounceRole function in AccessControlUpgradeable allows any user to renounce their own roles, including the BLACKLISTED_ROLE. This means that blacklisted users can remove themselves from the blacklist, bypassing the restrictions imposed by the _beforeTokenTransfer function and enabling them to transfer tokens despite being blacklisted

Vulnerability Detail

In the mTBILL contract, the _beforeTokenTransfer function is overridden to include checks that prevent blacklisted users from transferring tokens. This is achieved using the onlyNotBlacklisted modifier. The blacklisting mechanism relies on the BLACKLISTED_ROLE, managed through OpenZeppelin's AccessControlUpgradeable contract.

However, the renounceRole function in AccessControlUpgradeable allows any user to renounce (i.e., revoke) any role they hold, including the BLACKLISTED_ROLE. The function does not distinguish between different types of roles, and as long as the user is renouncing their own role, it permits the action

Impact

The ability for blacklisted users to unilaterally remove themselves from the blacklist undermines the integrity of the blacklist mechanism. This vulnerability can lead to unauthorized token transfers by blacklisted users

Code Snippet

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

function _beforeTokenTransfer(
        address from,
        address to,
        uint256 amount
    )
        internal
        virtual
        override(ERC20PausableUpgradeable)
        onlyNotBlacklisted(from)
        onlyNotBlacklisted(to)
    {
        ERC20PausableUpgradeable._beforeTokenTransfer(from, to, amount);
    }

function renounceRole(bytes32 role, address account) public virtual override {
        require(account == _msgSender(), "AccessControl: can only renounce roles for self");

        _revokeRole(role, account);
    }

Tool used

Manual Review

Recommendation

To mitigate this vulnerability, modify the renounceRole function to prevent users from renouncing the BLACKLISTED_ROLE.One approach is to override renounceRole in the Blacklistable contract or in the mTBILL contract to include an additional check that disallows renouncing the blacklist role.

Duplicate of #9