QA Report

[code423n4]

Low 1. Deprecated _setupRole function used

Impact

The _setupRole function is deprecated according to the Open Zeppelin comment
NOTE: This function is deprecated in favor of {_grantRole}

Use the recommended _grantRole function instead.

Proof of concept

The _setupRole function, which is deprecated, is found in one place
https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol#L212

This Open Zeppelin comment indicates it is deprecated
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/access/AccessControl.sol#L195

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace the _setupRole function with _grantRole from the same Open Zeppelin library

Low 2. Use safeIn/DecreaseAllowance instead of approve

Impact

The approve function is called for an ERC20 without checking the return value. Checking the return value would help confirm the approve was successful, but it is better to use safeIncreaseAllowance or safeDecreaseAllowance. This suggestion is mentioned in this Open Zeppelin comment
https://github.com/OpenZeppelin/openzeppelin-contracts/blob/742e85be7c08dff21410ba4aa9c60f6a033befb8/contracts/token/ERC20/utils/SafeERC20.sol#L38-L44

Proof of concept

The unsafe approve is used in ERC20CompoundPCVDeposit
https://github.com/code-423n4/2022-03-volt/blob/main/contracts/pcv/compound/ERC20CompoundPCVDeposit.sol#L31

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace the unsafe approve call with safeIncreaseAllowance or safeDecreaseAllowance

Low 3. revokeOverride backdoor elevates onlyGuardian role

Impact

The revokeOverride function acts like a backdoor to give the Guardian role the same revoke privileges as a Governor role, with the exception of preventing a guardian revoke. It would be better to create a new modifier for the relevant functions named 'onlyGovernorOrGuardian' to clarify the actual access controls, because the onlyGovernor modifier does not accurately reflect the access control of the revoke functions.

Proof of concept

The revokeOverride gives guardian users a backdoor to functions with the onlyGovernor modifier
https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol#L127

Access controls should not be broken in this manner to give one privilege level access to functions that are limited to another privilege level. With the current code, an address might be granted the Guardian role without the understanding that this allows that address to revoke the roles of other addresses. Instead, the permissions of the onlyGovernor functions that permit the Guardian role should be modified.

Tools Used

Manual analysis

Recommended Mitigation Steps

Add a new modifier named 'onlyGovernorOrGuardian' that permits both Governor and Guardian users. Use this modifier on all relevant revoke functions. A similar modifier is already created in CoreRef.
https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L69

modifier onlyGovernorOrGuardian() {
    require(
        isGovernor(msg.sender) || isGuardian(msg.sender),
        "Permissions: Caller is not a governor or guardian"
    );
    _;
}