Volt Protocol contest - rayn's results

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Core.sol#L27 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L22 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L199

Vulnerability details

Impact

vcon address is allowed to be updated by GOVERNOR in Core, however, this change will not be reflected in CoreRef._vcon. Moreover, since CoreRef._vcon cannot be updated due to contract design, it is also impossible to fix this manually. We are not yet sure how vcon will be used throughout the volt protocol, since details have not yet been made clear and code does not include related implementations. Consequently, it is impossible to estimate the exact impact. However, this desync between contracts seem dangerous enough to raise our attention, hence this report to inform the volt team about it.

Proof of Concept

In Core, vcon is allowed to be updated by GOVERNORs

    function setVcon(IERC20 _vcon) external onlyGovernor {
        vcon = _vcon;

        emit VconUpdate(_vcon);
    }

But in CoreRef, a contract inherited by several other ones including NonCustodialPSM, GlobalRateLimitedMinter, ERC20CompountPCVDeposit and Volt, _vcon is fixed upon initialization and cannot be further updated

    IERC20 private immutable _vcon;
    ...
    constructor(address coreAddress) {
        ...
        _vcon = ICore(coreAddress).vcon();
        ...
    }

Thus if GOVERNORS ever updated vcon in Core, the state between Core and all other Volt protocol components will mismatch.

Currently _vcon is not used in any place within the Volt protocol, but judging from the description in whitepapaer, future governance will be based on it, thus any potential desync will be devastating.

Tools Used

vim, ganache-cli

Recommended Mitigation Steps

There are several possible solutions.

The first is to dynamically fetch vcon from the Core whenever CoreRef uses it, and avoid storing a static copy locally.

    function vcon() public view override returns (IERC20) {
        return _volt.vcon();
    }

The second is to expose a public API to update _vcon in CoreRef, however, this approach might not be especially favorable since many components will require updates at once, and it is highly possible that future GOVERNORs miss some of them while doing updates.

Judge @jack-the-pug has assessed the second item in QA Report #64 as Medium risk. The relevant finding follows:

Impact

In RateLimited.sol BufferCap should be the upper bound of BufferStored, However in _setBufferCap() it calls _updateBufferStored() before replacing the old BufferCap. If old BufferCap is larger than new BufferCap, BufferStored could be larger than the new BufferCap. This is still a low risk, since BufferStored won't be directly used in `RateLimited.sol. But it could be a potential threat in the future.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L141

    function _setBufferCap(uint256 newBufferCap) internal {
        _updateBufferStored();

        uint256 oldBufferCap = bufferCap;
        bufferCap = newBufferCap;

        emit BufferCapUpdate(oldBufferCap, newBufferCap);
    }

Recommended Mitigation Steps

Fixing the order can solve this problem.

    function _setBufferCap(uint256 newBufferCap) internal {
        uint256 oldBufferCap = bufferCap;
        bufferCap = newBufferCap;

       _updateBufferStored();

        emit BufferCapUpdate(oldBufferCap, newBufferCap);
    }

Summary

We list 4 low-critical findings and 5 non-critical findings here:

• (Low) Roles management
• (Low) BufferStored could be larger than BufferCap after _setBufferCap()
• (Low) oraclePrice will be imprecise
• (Low) Unnecessary usage of init()
• (Non) Using ecrecover is against best practice
• (Non) valid in OraclePassThrough/read() always returns true
• (Non) Use Address.sendValue() without import @openzeppelin/contracts/utils/Address.sol
• (Non) setPublicChainlinkToken only called on chainId 1 and 42
• (Non) Unused functions and modifiers

In summary of recommended security practices, complex role management easily creates confusions over the privilege of each role, it's better to simplify the design of roles. Also, it’s better to use a verified library like ECDSA, delete unused functions, set unused variables to private, and import contracts correctly for best practice.

(Low) Roles management

After reviewing the entire volt protocol, we found that role management has been made unnecessarily complex. While there are no immediate fatal flaws in the current role assignment, from our prior experience in dealing with privilege management, we worry that such a complex system will likely lead to future problems, especially when management gradually moves from the hands of few reliable developers to an open vcon based governance. Due to this, we feel that there is a need to express our concerns as well as highlight a few role assignments that we find "strange".

First. let's review the roles included in Permissions and Tribes. The admin/members shown below are the ones explicitly assigned in contracts.

DEFAULT_ADMIN_ROLE							(native)
	admin DEFAULT_ADMIN_ROLE
	members

MINTER_ROLE/MINTER			"MINTER_ROLE"			(native) (tribe-major)
	admin GOVERN_ROLE
	members

BURNER_ROLE				"BURNER_ROLE"			(native)			unused
	admin GOVERN_ROLE
	members

PCV_CONTROLLER_ROLE/PCV_CONTROLLER	"PCV_CONTROLLER_ROLE"		(native) (tribe-major)
	admin GOVERN_ROLE
	members

GOVERN_ROLE/GOVERNOR			"GOVERN_ROLE"			(native) (tribe-major)
	admin GOVERN_ROLE
	members
		core
		init() caller

GUARDIAN_ROLE/GUARDIAN			"GUARDIAN_ROLE"			(native) (tribe-major)
	admin GOVERN_ROLE
	members

PARAMETER_ADMIN				"PARAMETER_ADMIN"		(tribe-admin)
	admin DEFAULT_ADMIN_ROLE
	members

ORACLE_ADMIN				"ORACLE_ADMIN_ROLE"		(tribe-admin)			unused
	admin DEFAULT_ADMIN_ROLE
	members

TRIBAL_CHIEF_ADMIN			"TRIBAL_CHIEF_ADMIN_ROLE"	(tribe-admin)			unused
	admin DEFAULT_ADMIN_ROLE
	members

PCV_GUARDIAN_ADMIN			"PCV_GUARDIAN_ADMIN_ROLE"	(tribe-admin)			unused
	admin DEFAULT_ADMIN_ROLE
	members

MINOR_ROLE_ADMIN			"MINOR_ROLE_ADMIN"		(tribe-admin)			unused
	admin DEFAULT_ADMIN_ROLE
	members

FUSE_ADMIN				"FUSE_ADMIN"			(tribe-admin)			unused
	admin DEFAULT_ADMIN_ROLE
	members

VETO_ADMIN				"VETO_ADMIN"			(tribe-admin)			unused
	admin DEFAULT_ADMIN_ROLE
	members

MINTER_ADMIN				"MINTER_ADMIN"			(tribe-admin)			unused
	admin DEFAULT_ADMIN_ROLE
	members

OPTIMISTIC_ADMIN			"OPTIMISTIC_ADMIN"		(tribe-admin)			unused
	admin DEFAULT_ADMIN_ROLE
	members

LBP_SWAP_ROLE				"SWAP_ADMIN_ROLE"		(tribe-minor)			unused
	admin DEFAULT_ADMIN_ROLE
	members

VOTIUM_ROLE				"VOTIUM_ADMIN_ROLE"		(tribe-minor)			unused
	admin DEFAULT_ADMIN_ROLE
	members

MINOR_PARAM_ROLE			"MINOR_PARAM_ROLE"		(tribe-minor)			unused
	admin DEFAULT_ADMIN_ROLE
	members

ADD_MINTER_ROLE				"ADD_MINTER_ROLE"		(tribe-minor)
	admin DEFAULT_ADMIN_ROLE
	members

PSM_ADMIN_ROLE				"PSM_ADMIN_ROLE"		(tribe-minor)
	admin DEFAULT_ADMIN_ROLE
	members

Notice how a several of those roles are not used in any of the contracts (marked as unused). This is the first problem. While it is understandable that the protocol is incomplete yet, introducing redundant roles does not make management easier. AccessControl.sol allows introducing new roles post-deployment, so it might be a better idea to keep a list of dynamically introduced roles instead of listing a lot of unused ones upfront, especially since there are roles with similar names (PCV_GUARDIAN_ADMIN and PCV_CONTROLLER), between which the difference is not made clear.

Next, we carry on to see the design of Permissions. We note that this contract is modified from the implementation of fei protocol, but on the other hand, disagree that the implementation is anywhere near optimal. To support our argument, we discuss the logic of role granting and revoking below. So let's look at the grantMinter function.

    function grantMinter(address minter) external override onlyGovernor {
        grantRole(MINTER_ROLE, minter);

The function specifies that it is a onlyGovernor function, which should expectedly mean that GOVERNOR, and only GOVERNOR have the privilege to add MINTER_ROLE members. However, if we dig a bit deeper, it is easy to see that this is not the case.

The grantRole function defined in AccessControl.sol is as below. Notice that it also has a modifier that specifies only admins of the specific role has privilege to add new members. Additionally, this function is public.

    function grantRole(bytes32 role, address account) public virtual override onlyRole(getRoleAdmin(role)) {
        _grantRole(role, account);
    }

Combining this logic with the grantMinter one above, we can see that the workflow essentially becomes

1. caller is allowed to add new users if it only has admin of MINTER_ROLE (call grantRole directly)
2. caller is allowed to add new users if it has admin of MINTER_ROLE and GOVERNOR role (call grantRole or grantMinter)
3. caller is not allowed to add new users if it only has GOVERNOR role (blocked by grantRole)

It is clear that the grantMinter function now becomes semi-useless, since callers with only GOVERNOR role cannot do anything without admin of MINTER_ROLE, and admin of MINTER_ROLE can always call grantRole directly even if it does not have GOVERNOR role.

Our best guess of the original intention is that GOVERNOR has full privilege over role management, while admin of roles are neglected. To realize this concept, it might be better to override the grantRole function and make it non-public, so that callers can't circumvent the GOVERNOR check. Finally, change grantMinter to use the internal function _grantRole. Similar modifications should also be done to the revokeXXX series of functions.

Now we've gone through Permissions, time to look at CoreRef and OracleRef. One of the more interesting design choice in CoreRef is the introduction of CONTRACT_ADMIN_ROLE. This allows an additional role to be granted admin over the specific contract. However, throughout the volt protocol, CONTRACT_ADMIN_ROLE does not serve any particularly useful purpose. Moreover, in some places, the usage of CONTRACT_ADMIN_ROLE does not make much sense.

For instance, let's look at implementation of RateLimited and MultiRateLimited. RateLimited defines a global limit to the bufferCap and rateLimitPerSecond, and MultiRateLimited defines the upper limit of individualMaxBufferCap and individualMaxRateLimitPerSecond. In our opinion, for the system to make sense, a user granted permission to change bufferCap and rateLimitPerSecond should also have permission to change individualMaxBufferCap and individualMaxRateLimitPerSecond. However, it can be seen that CONTRACT_ADMIN_ROLES are allowed to change the global limits through onlyGovernorOrAdmin modifier, while individual limits can only be changed by GOVERNOR since onlyGovernor is used. The flexibility introduced through CONTRACT_ADMIN_ROLE is not properly utilized in volt protocol, and as we see in the example above, leads to potential role privilege confusions, thus we deemed it more appropriate to remove the CONTRACT_ADMIN_ROLE mechanism altogether for simplicity.

The next aspect we would like to discuss is more of a design choice, and not really a management problem. Take it with a grain of salt. Throughout the contract, there are several places that use the onlyGovernor modifier. However given that the roles already included controllers/admins for each specific component (PCV_CONTROLLER_ROLE, PSM_ADMIN_ROLE), it is probably more appropriate to limit GOVERNOR to only manage the Core contract. If a governor needs to modify stuff from other contracts, add the corresponding admin role to itself and use that role to authenticate further actions. This design can create a clean cut between metadata management, and actually protocol management, at the cost of slightly more gas spent in granting roles. From our limited experience, this kind of management is more robust and greatly lowers the probability of mis-management in the future.

Now we've discussed all general suggestions we have for role management, we finally note a few role modifier usage that we find "strange", but are uncertain whether intended or not.

1. NonCustodialPSM.withdrawERC20 uses modifier onlyPCVController. We find it strange that a function in the PSM module requires PCVController role. Shouldn't this require PSM_ADMIN_ROLE instead?
2. CompoundPCVDepositBase.withdraw uses the modifier onlyPCVController. This would require NonCustodialPSM to have the PCV_CONTROLLER_ROLE to call withdraw, and while it is not a problem strictly speaking, we find it strange to grant such a role. A more usual implementation would be to have CompoundPCVDepositBase do internal bookkeeping on the amount each address deposited, and allow withdraw with respect to those values (similar to implementation of ERC20). This avoids the introduction of an additional role (a.k.a potential point of failure due to mis-management).

Overall, complexity in role management easily creates confusions over the privilege of each role, and in the specific case of volt protocol, does not really introduce any benefits. We thus urge the developers to re-think the current role management system, and preferably simplify the design.

(Low) BufferStored could be larger than BufferCap after _setBufferCap()

Impact

In RateLimited.sol BufferCap should be the upper bound of BufferStored, However in _setBufferCap() it calls _updateBufferStored() before replacing the old BufferCap. If old BufferCap is larger than new BufferCap, BufferStored could be larger than the new BufferCap. This is still a low risk, since BufferStored won't be directly used in `RateLimited.sol. But it could be a potential threat in the future.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L141

    function _setBufferCap(uint256 newBufferCap) internal {
        _updateBufferStored();

        uint256 oldBufferCap = bufferCap;
        bufferCap = newBufferCap;

        emit BufferCapUpdate(oldBufferCap, newBufferCap);
    }

Recommended Mitigation Steps

Fixing the order can solve this problem.

    function _setBufferCap(uint256 newBufferCap) internal {
        uint256 oldBufferCap = bufferCap;
        bufferCap = newBufferCap;

       _updateBufferStored();

        emit BufferCapUpdate(oldBufferCap, newBufferCap);
    }

(Low) oraclePrice will be imprecise

Impact

Public oraclePrice variable will be imprecise and confused. If other contracts try to get oracle price by calling oraclePrice() rather than calling getCurrentOraclePrice(), it will get wrong price.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/oracle/ScalingPriceOracle.sol#L36

Recommended Mitigation Steps

Declare oraclePrice to private, and only use getCurrentOraclePrice() to get the price instead.

(Low) Unnecessary usage of init()

Impact

Since Core contract does not go through a proxy, and the caller of init() is the same as the deployer in deploy script, it is unnecessary to use init functions.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Core.sol#L20

Recommended Mitigation Steps

Consider putting init() logic into constructor instead and stop inheriting Initializable.

(Non) Using ecrecover is against best practice

Impact

Using ecrecover is against best practice. Preferably use ECDSA.recover instead. EIP-2 still allows signature malleability for ecrecover(). Remove this possibility and make the signature unique. However it should be impossible to be a threat by now.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/volt/Volt.sol#L89

Recommended Mitigation Steps

Take these implementation into consideration

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/4a9cc8b4918ef3736229a5cc5a310bdc17bf759f/contracts/utils/cryptography/draft-EIP712.sol

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/token/ERC20/extensions/draft-ERC20Permit.sol

(Non) valid in OraclePassThrough/read() always returns true

Impact

In OraclePassThrough.sol, valid is always true in read(). Since OracleRef uses valid to determine validity. There should be a invalid case. Otherwise, the validity check is meaningless.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/oracle/OraclePassThrough.sol#L41

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/OracleRef.sol#L102

Recommended Mitigation Steps

Define the invalid case.

(Non) Use Address.sendValue() without import @openzeppelin/contracts/utils/Address.sol

Impact

Although @openzeppelin/contracts/proxy/utils/Initializable.sol is imported in Core.sol. A good coding practice should import @openzeppelin/contracts/utils/Address.sol when using it.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/pcv/PCVDeposit.sol#L43

Recommended Mitigation Steps

Import @openzeppelin/contracts/utils/Address.sol in PCVDeposit.sol

(Non) setPublicChainlinkToken only called on chainId 1 and 42

Impact

ScalingPriceOracle checks chainId in contract constructor, it might be better to upfront reject construction if it is not intended to serve other chains.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/oracle/ScalingPriceOracle.sol#L84-L86

Recommended Mitigation Steps

It’s better to upfront check and reject construction if it is not intended to serve other chains.

(Non) Unused functions and modifiers

Impact

These functions are noted as for backward compatibility, but not used anywhere, should probably be removed.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L10

Unused functions:

• _burnVoltHeld

Unused modifiers:

• onlyVolt
• hasAnyOfFiveRoles
• hasAnyOfFourRoles
• hasAnyOfThreeRoles
• onlyGovernorOrGuardianOrAdmin
• ifMinterSelf

Recommended Mitigation Steps

Remove these unused functions and modifiers.

Declare private function to save gas

invert function in OracleRef.sol can be declared private.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/OracleRef.sol#L84

Recommendation

Declare the function to private.

Delete public volt() function and change _volt to internal

volt() function only returns _volt value. The function can be deleted and declare _volt internal to save gas.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L12 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L193-L195

Recommendation

Change _volt to internal to allow inheriting contracts such as NonCustodialPSM to access directly.