AccessController Manual Review Findings

ACR-01M: Potentially Incorrect Dependency Usage

Type	Severity	Location
Language Specific		AccessController.sol:L12

Description:

The contract inherits from the AccessControlUpgradeable contract yet does not invoke its __AccessControl_init initialization hook nor does the contract itself represent an upgrade-able implementation as it contains logic in its constructor.

Example:

Copy
1// SPDX-License-Identifier: GPL-3.0-or-later
2pragma solidity ^0.8.0;
3
4import "../domain/BosonConstants.sol";
5import "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";
6
7/**
8 * @title AccessController
9 *
10 * @notice Implements centralized role-based access for Boson Protocol contracts.
11 */
12contract AccessController is AccessControlUpgradeable {
13    /**
14     * @notice Constructor
15     *
16     * Grants ADMIN role to deployer.
17     * Sets ADMIN as role admin for all other roles.
18     */
19    constructor() {
20        _setupRole(ADMIN, msg.sender);
21        _setRoleAdmin(ADMIN, ADMIN);
22        _setRoleAdmin(PAUSER, ADMIN);
23        _setRoleAdmin(PROTOCOL, ADMIN);
24        _setRoleAdmin(CLIENT, ADMIN);
25        _setRoleAdmin(UPGRADER, ADMIN);
26        _setRoleAdmin(FEE_COLLECTOR, ADMIN);
27    }
28}

Recommendation:

We advise the dependency of the contract to be assessed and either adjusted to the non-upgrade-able one or the contract itself to be adjusted to conform to the upgrade-able contract standards and not contain any logic in its constructor.

Alleviation (44009967e4f68092941d841e9e0f5dd2bb31bf0b):

The non-upgradeable and now-local AccessControl dependency is inherited by the contract instead of the original upgradeable one thus alleviating this exhibit as its concerns are no longer relevant.

ACR-02M: Circular Access Control Dependency

Type	Severity	Location
Standard Conformity		AccessController.sol:L21

Description:

The ADMIN role contains a circular administration meaning that a single ADMIN role holder can completely compromise the system and remove all other members.

Impact:

A single rogue ADMIN role holder can compromise the system entirely as they are capable of removing other ADMIN holders as well as assigning all roles defined in the AccessController contract.

Example:

Copy
21_setRoleAdmin(ADMIN, ADMIN);

Recommendation:

We advise this trait of the system to be re-evaluated and the ADMIN role to potentially be held by a single entity throughout the AccessController contract's lifetime.

Alleviation (44009967e4f68092941d841e9e0f5dd2bb31bf0b):

The Boson Protocol team has opted to retain the current hierarchical structure in place as they wish to retain the ability to transfer ADMIN ownership to multiple parties, initially the multi-signature wallet and later a dedicated DAO module that is planned for the future. Given that alternative albeit a bit more complex workflows do exist to at least ensure a singular ADMIN member remains active at any given moment, we consider this exhibit as acknowledged.