AsManageable Manual Review Findings

AME-01M: Invalid Conditional Evaluation

Type	Severity	Location
Logical Fault		AsManageable.sol:L86-L96, L172

Description:

The AsManageable::_checkRoleAcceptance function will return early if the role being accepted is the KEEPER_ROLE, however, the pendingAcceptance entry will never have the KEEPER_ROLE as a role due to the fact that the AsManageable::grantRole will configure it solely when the role is either the DEFAULT_ADMIN_ROLE or the MANAGER_ROLE.

Example:

Copy
70/**
71 * @notice Grant a role to an account
72 *
73 * @dev If the role is admin, the account will have to accept the role
74 * The acceptance period will expire after TIMELOCK_PERIOD has passed
75 */
76function grantRole(
77    bytes32 role,
78    address account
79)
80    public
81    override
82    onlyRole(getRoleAdmin(role))
83{
84    require(!hasRole(role, account));
85
86    if (role == DEFAULT_ADMIN_ROLE || role == MANAGER_ROLE) {
87
88        pendingAcceptance[account] = PendingAcceptance({
89            // only get replaced if admin, managers can coexist
90            replacing: role == DEFAULT_ADMIN_ROLE
91                ? msg.sender
92                : address(0),
93            timestamp: block.timestamp,
94            role: role
95        });
96    } else {
97        _grantRole(role, account);
98    }
99}

Recommendation:

We advise the code to be updated, potentially by adjusting the if conditional of the AsManageable::grantRole function to also execute the pendingAcceptance path when the input role is the KEEPER_ROLE.

Alleviation (59b75fbee1d8f3dee807c928f18be41c58b904e1):

The Astrolab DAO team indicated that they will address this point with an acceptable remediation, however, it remains open in the latest version of the codebase and specifically the AccessController::acceptRole function that implements the original contract's purpose.

As the exhibit does not pose a security concern, we will consider it acknowledged but advise the Astrolab DAO team to potentially revisit it.

AME-02M: Detachment of Authorized Role

Type	Severity	Location
Logical Fault		AsManageable.sol:L152, L158, L160

Description:

The AsManageable::acceptRole function will permit the caller to accept any role they wish regardless of what was initially authorized to them via the AsManageable::grantRole function as the input role argument is utilized instead of the acceptance.role data entry.

Impact:

It is presently possible to acquire a different role than the one you have been authorized for (i.e. acquire the DEFAULT_ADMIN_ROLE while authorized for the MANAGER_ROLE) as well as cause the deletion of the DEFAULT_ADMIN_ROLE by accepting such an authorization whilst granting a different role.

Example:

Copy
145/**
146 * @notice Accept an admin role and revoke the old admin
147 *
148 * @dev If the role is admin or manager, the account will have to accept the role
149 * The acceptance will expire after TIMELOCK_PERIOD + VALIDITY_PERIOD has passed
150 * Old admin will be revoked and new admin will be granted
151 */
152function acceptRole(bytes32 role) external {
153    PendingAcceptance memory acceptance = pendingAcceptance[msg.sender];
154
155    _checkRoleAcceptance(acceptance);
156    if (acceptance.replacing != address(0)) {
157        // if replacing, revoke the old role
158        _revokeRole(acceptance.role, acceptance.replacing);
159    }
160    _grantRole(role, msg.sender);
161    delete pendingAcceptance[msg.sender];
162}

Recommendation:

We advise the code to remove the role argument entirely, and to utilize the pendingAcceptance payload for all the data it requires.

Alleviation (59b75fbee1d8f3dee807c928f18be41c58b904e1):

Role acceptance is properly validated in the AccessController::acceptRole function and specifically the AccessController::checkRoleAcceptance validation mechanism which has replaced the original AsManageable implementation.