Volt Protocol contest - 0xkatana's results

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"
    );
    _;
}

1. Use calldata instead of memory for function parameters

Impact

Use calldata instead of memory for function parameters. Having function arguments use calldata instead of memory can save gas.

Proof of Concept

At least two cases exist of function arguments using memory instead of calldata https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/OracleRef.sol#L84 https://github.com/code-423n4/2022-03-volt/blob/main/contracts/peg/NonCustodialPSM.sol#L456

Tools Used

Manual analysis

Recommended Mitigation Steps

Change function arguments from memory to calldata

2. Add payable to functions that won't receive ETH

Impact

Marking a function as payable saves gas. Functions that have the onlyOwner modifier cannot be called by normal users and will not mistakenly receive ETH. These functions can be payable to save gas.

Proof of Concept

One function was found with the onlyOwner modifier that can be payable https://github.com/code-423n4/2022-03-volt/blob/main/contracts/oracle/OraclePassThrough.sol#L53

But all functions in Permissions.sol that have the onlyGovernor modifier or the onlyGuardian modifier can receive the same gas optimization

Tools Used

Manual analysis

Recommended Mitigation Steps

Add payable to functions with access control for gas savings

3. Split up require statements instead of &&

Impact

Combining require statement conditions with && logic uses unnecessary gas. It is better to split up each part of the logical statement into a separate require statements

Proof of Concept

One instance of this issue was found https://github.com/code-423n4/2022-03-volt/blob/main/contracts/volt/Volt.sol#L91

Tools Used

Manual analysis

Recommended Mitigation Steps

Use separate require statements instead of concatenating with && to save gas

4. Short require strings save gas

Impact

Strings in solidity are handled in 32 byte chunks. A require string longer than 32 bytes uses more gas. Shortening these strings will save gas.

Proof of Concept

Several cases of this gas optimization were found. These are a few examples, but more may exist

1. https://github.com/code-423n4/2022-03-volt/blob/main/contracts/pcv/compound/ERC20CompoundPCVDeposit.sol#L36
2. https://github.com/code-423n4/2022-03-volt/blob/main/contracts/utils/RateLimited.sol#L48
3. https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L48
4. https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L56
5. https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L72
6. https://github.com/code-423n4/2022-03-volt/blob/main/contracts/refs/CoreRef.sol#L82

Tools Used

Manual analysis

Recommended Mitigation Steps

Shorten require strings

5. Use Solidity errors instead of require

Impact

Solidity errors introduced in version 0.8.4 can save gas on revert conditions https://blog.soliditylang.org/2021/04/21/custom-errors/ https://twitter.com/PatrickAlphaC/status/1505197417884528640

Proof of Concept

Error messages are not used in the contract, so all require blocks could receive a gas optimization

Tools Used

Manual analysis

Recommended Mitigation Steps

Replace require blocks with new solidity errors from solidity 0.8.4 described in https://blog.soliditylang.org/2021/04/21/custom-errors/