Lines of code

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L155 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L44 https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L127

Vulnerability details

Impact

In RoleManager.sol, _revokeRole() doesn’t remove account from _roleMembers[role]. getRoleMember() and getRoleMemberCount() could then return wrong information. It could result in many problems, e.g., misinformation.

And the most critical problem lies in renounceGovernance(). Since account is not removed from _roleMembers[role] after calling _revokeRole(). getRoleMemberCount() will return the same result after calling renounceGovernance(). Then In renounceGovernance(), getRoleMemberCount(Roles.GOVERNANCE) > 1 is still true even if there is only one governor. In consequence, governors could accidentally remove all the governors of the contract.

Proof of Concept

Alice and Bob are the governors of the contract.
Bob want to give up governance, so he calls renounceGovernance(), but his account is not removed from _roleMembers[role]

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L44

    function renounceGovernance() external onlyGovernance {
        require(getRoleMemberCount(Roles.GOVERNANCE) > 1, Error.CANNOT_REVOKE_ROLE);
        _revokeRole(Roles.GOVERNANCE, msg.sender);
    }

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L155

   function _revokeRole(bytes32 role, address account) internal {
        _roles[role].members[account] = false;
        emit RoleRevoked(role, account, msg.sender);
    }

Alice wants to give up governance too. It should be forbidden because she is the last governor. But getRoleMemberCount(Roles.GOVERNANCE) > 1 is still passed, since _roleMembers[role].length() doesn’t change after bob calling _revokeRole().

https://github.com/code-423n4/2022-04-backd/blob/main/backd/contracts/access/RoleManager.sol#L127

    function getRoleMemberCount(bytes32 role) public view virtual override returns (uint256) {
        if (
            role == Roles.ADDRESS_PROVIDER || role == Roles.POOL_FACTORY || role == Roles.CONTROLLER
        ) {
            return 1;
        }
        if (role == Roles.POOL) {
            return addressProvider.poolsCount();
        }
        if (role == Roles.VAULT) {
            return addressProvider.vaultsCount();
        }
        return _roleMembers[role].length();
    }

Now there is no governor in this contract.

Tools Used

vim

Recommended Mitigation Steps

account should be removed from _roleMembers[role]

code-423n4 / 2022-04-backd-findings