Volt Protocol contest - robee's results

Lines of code

https://github.com/code-423n4/2022-03-volt/tree/main/contracts/utils/Deviation.sol#L23

Vulnerability details

Division by 0 can lead to accidentally revert, (An example of a similar issue - https://github.com/code-423n4/2021-10-defiprotocol-findings/issues/84)

    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/utils/Deviation.sol#L23 a might be 0

It's internal function but since it is used in another internal functions that are used in public and neither of them has this protection I thought it can be considered as medium (e.g. isWithinDeviationThreshold)

Thanks.

Title: Add a timelock Severity: Low Risk

To give more trust to users: functions that set key/critical variables should be put behind a timelock.

    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/peg/NonCustodialPSM.sol#L167
    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/utils/RateLimited.sol#L57
    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/utils/RateLimited.sol#L72
    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/peg/NonCustodialPSM.sol#L157

Title: Missing non reentrancy modifier Severity: Low Risk

The following functions are missing reentrancy modifier although some other pulbic/external functions does use reentrancy modifer. Even though I did not find a way to exploit it, it seems like those functions should have the nonReentrant modifier as the other functions have it as well..

    NonCustodialPSM.sol, withdrawERC20 is missing a reentrancy modifier
    NonCustodialPSM.sol, setGlobalRateLimitedMinter is missing a reentrancy modifier
    NonCustodialPSM.sol, setMintFee is missing a reentrancy modifier
    NonCustodialPSM.sol, pauseRedeem is missing a reentrancy modifier

Title: Missing fee parameter validation Severity: Low Risk

Some fee parameters of functions are not checked for invalid values. Validate the parameters:

    ScalingPriceOracle.constructor (_fee)
    NonCustodialPSM._setRedeemFee (newRedeemFeeBasisPoints)
    NonCustodialPSM._setMintFee (newMintFeeBasisPoints)
    NonCustodialPSM.setMintFee (newMintFeeBasisPoints)
    NonCustodialPSM.setRedeemFee (newRedeemFeeBasisPoints)

Title: Never used parameters Severity: Low Risk

Those are functions and parameters pairs that the function doesn't use the parameter. In case those functions are external/public this is even worst since the user is required to put value that never used and can misslead him and waste its time.

    NonCustodialPSM.sol: function _validatePriceRange parameter price isn't used. (_validatePriceRange is internal)

Title: In the following public update functions no value is returned Severity: Low Risk

In the following functions no value is returned, due to which by default value of return will be 0. We assumed that after the update you return the latest new value. (similar issue here: https://github.com/code-423n4/2021-10-badgerdao-findings/issues/85).

    OraclePassThrough.sol, updateScalingPriceOracle
    OracleRef.sol, updateOracle
    OraclePassThrough.sol, update

Title: Two Steps Verification before Transferring Ownership Severity: Low Risk

The following contracts have a function that allows them an admin to change it to a different address. If the admin accidentally uses an invalid address for which they do not have the private key, then the system gets locked. It is important to have two steps admin change where the first is announcing a pending new admin and the new address should then claim its ownership. A similar issue was reported in a previous contest and was assigned a severity of medium: code-423n4/2021-06-realitycards-findings#105

    CoreRef.sol
    Permissions.sol

Title: Not verified input Severity: Low Risk

external / public functions parameters should be validated to make sure the address is not 0.
Otherwise if not given the right input it can mistakenly lead to loss of user funds.


    Vcon.sol.balanceOf account
    PCVDeposit.sol.withdrawETH to
    CoreRef.sol._mintVolt to
    OracleRef.sol._setBackupOracle newBackupOracle
    Permissions.sol._setupGovernor governor

Title: Check transfer receiver is not 0 to avoid burned money Severity: Low Risk

Transferring tokens to the zero address is usually prohibited to accidentally avoid "burning" tokens by sending them to an unrecoverable zero address.

    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/peg/NonCustodialPSM.sol#L242
    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/vcon/Vcon.sol#L101
    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/peg/NonCustodialPSM.sol#L280

Title: Not verified owner Severity: Low Risk

    owner param should be validated to make sure the owner address is not address(0).
    Otherwise if not given the right input all only owner accessible functions will be unaccessible.


    Volt.sol.permit owner
    Vcon.sol.permit owner

Title: Named return issue Severity: Low Risk

Users can mistakenly think that the return value is the named return, but it is actually the actualreturn statement that comes after. To know that the user needs to read the code and is confusing. Furthermore, removing either the actual return or the named return will save gas.

    ScalingPriceOracle.sol, requestCPIData

Title: Missing commenting Severity: Low Risk

    The following functions are missing commenting as describe below:

    NonCustodialPSM.sol, _validatePriceRange (internal), parameter price not commented
    ScalingPriceOracle.sol, getCurrentOraclePrice (public), @return is missing
    Permissions.sol, _setupGovernor (internal), parameter governor not commented
    Volt.sol, permit (external), parameter r not commented

Title: Override function but with different argument location Severity: Low/Med Risk

    RateLimited.sol.constructor inherent CoreRef.sol.constructor but the parameters does not match
    RateLimitedMinter.sol.constructor inherent CoreRef.sol.constructor but the parameters does not match
    NonCustodialPSM.sol.constructor inherent OracleRef.sol.constructor but the parameters does not match
    OracleRef.sol.constructor inherent CoreRef.sol.constructor but the parameters does not match
    ScalingPriceOracle.sol.constructor inherent Timed.sol.constructor but the parameters does not match
    NonCustodialPSM.sol.constructor inherent RateLimited.sol.constructor but the parameters does not match
    NonCustodialPSM.sol.constructor inherent CoreRef.sol.constructor but the parameters does not match

Title: Rearrange state variables Severity: GAS

You can change the order of the storage variables to decrease memory uses.

In Vcon.sol,rearranging the storage fields can optimize to: 7 slots from: 8 slots. The new order of types (you choose the actual variables):

    1. string
    2. string
    3. uint256
    4. bytes32
    5. bytes32
    6. bytes32
    7. address
    8. uint8

Title: Storage double reading. Could save SLOAD Severity: GAS

Reading a storage variable is gas costly (SLOAD). In cases of multiple read of a storage variable in the same scope, caching the first read (i.e saving as a local variable) can save gas and decrease the overall gas uses. The following is a list of functions and the storage variables that you read twice:

    Vcon.sol: totalSupply is read twice in mint
    Vcon.sol: minter is read twice in setMinter
    Vcon.sol: minter is read twice in mint

Title: Unused state variables Severity: GAS

Unused state variables are gas consuming at deployment (since they are located in storage) and are a bad code practice. Removing those variables will decrease deployment gas cost and improve code quality. This is a full list of all the unused storage variables we found in your code base.

    TribeRoles.sol, GOVERNOR
    TribeRoles.sol, VOTIUM_ROLE
    TribeRoles.sol, LBP_SWAP_ROLE

Title: Internal functions to private Severity: GAS

The following functions could be set private to save gas and improve code quality:

    NonCustodialPSM.sol, _setGlobalRateLimitedMinter
    Vcon.sol, _delegate
    OracleRef.sol, _setOracle
    RateLimited.sol, _depleteBuffer
    ScalingPriceOracle.sol, _updateCPIData
    RateLimited.sol, _replenishBuffer
    Deviation.sol, isWithinDeviationThreshold
    Vcon.sol, safe32
    ScalingPriceOracle.sol, _oracleUpdateChangeRate
    Vcon.sol, _moveDelegates
    Vcon.sol, sub96

Title: Use != 0 instead of > 0 Severity: GAS

Using != 0 is slightly cheaper than > 0. (see https://github.com/code-423n4/2021-12-maple-findings/issues/75 for similar issue)

    Vcon.sol, 443: change 'amount > 0' to 'amount != 0'
    Vcon.sol, 484: change 'nCheckpoints > 0' to 'nCheckpoints != 0'

Title: Public functions to external Severity: GAS

The following functions could be set external to save gas and improve code quality. External call cost is less expensive than of public functions.

    CoreRef.sol, vcon
    CoreRef.sol, voltBalance
    CoreRef.sol, vconBalance

Title: Unnecessary constructor Severity: GAS

The following constructors are empty. (A similar issue https://github.com/code-423n4/2021-11-fei-findings/issues/12)

    RateLimitedMinter.sol.constructor
    GlobalRateLimitedMinter.sol.constructor

Title: Unused inheritance Severity: GAS

Some of your contract inherent contracts but aren't use them at all.
We recommend not to inherent those contracts.

    PCVDeposit.sol; the inherited contracts CoreRef not used
    RateLimited.sol; the inherited contracts CoreRef not used

Title: Do not cache msg.sender Severity: Gas

We recommend not to cache msg.sender since calling it is 2 gas while reading a variable is more.

    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/core/Permissions.sol#L123
    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/pcv/PCVDeposit.sol#L33
    https://github.com/code-423n4/2022-03-volt/tree/main/contracts/refs/CoreRef.sol#L214

Title: Use bytes32 instead of string to save gas whenever possible Severity: GAS

Use bytes32 instead of string to save gas whenever possible.
String is a dynamic data structure and therefore is more gas consuming then bytes32.


    Vcon.sol (L10), string public constant name = "Volt Controller";
    Vcon.sol (L14), string public constant symbol = "VCON";

Title: Inline one time use functions Severity: GAS

The following functions are used exactly once. Therefore you can inline them and save gas and improve code clearness.

    Timed.sol, _setDuration
    ScalingPriceOracle.sol, _updateCPIData
    ScalingPriceOracle.sol, _addNewMonth
    RateLimitedMinter.sol, _mintVolt
    Permissions.sol, _setupGovernor
    Deviation.sol, calculateDeviationThresholdBasisPoints
    PCVDeposit.sol, _withdrawERC20
    Timed.sol, _initTimed
    Vcon.sol, safe32

Title: Unused imports Severity: GAS

In the following files there are contract imports that aren't used Import of unnecessary files costs deployment gas (and is a bad coding practice that is important to ignore)

    OraclePassThrough.sol, line 4, import {CoreRef} from "./../refs/CoreRef.sol";
    ScalingPriceOracle.sol, line 12, import {ERC20, IERC20} from "@openzeppelin/contracts/token/ERC20/ERC20.sol";
    ScalingPriceOracle.sol, line 4, import {CoreRef} from "./../refs/CoreRef.sol";

Title: Short the following require messages Severity: GAS

The following require messages are of length more than 32 and we think are short enough to short them into exactly 32 characters such that it will be placed in one slot of memory and the require function will cost less gas. The list:

    Solidity file: RateLimited.sol, In line 62, Require message length to shorten: 40, The message: RateLimited: rateLimitPerSecond too high
    Solidity file: RateLimited.sol, In line 103, Require message length to shorten: 33, The message: RateLimited: no rate limit buffer
    Solidity file: NonCustodialPSM.sol, In line 275, Require message length to shorten: 39, The message: PegStabilityModule: Mint not enough out
    Solidity file: RateLimited.sol, In line 46, Require message length to shorten: 40, The message: RateLimited: rateLimitPerSecond too high

Title: Use calldata instead of memory Severity: GAS

Use calldata instead of memory for function parameters In some cases, having function arguments in calldata instead of memory is more optimal.

    Vcon.safe32 (errorMessage)
    NonCustodialPSM._validatePriceRange (price)
    Vcon.add96 (errorMessage)
    Vcon.sub96 (errorMessage)
    OracleRef.invert (price)
    Vcon.safe96 (errorMessage)

Title: Unnecessary cast Severity: Gas

    GlobalRateLimitedMinter NonCustodialPSM.sol.setGlobalRateLimitedMinter - unnecessary casting GlobalRateLimitedMinter(newMinter)

Title: State variables that could be set immutable Severity: GAS

In the following files there are state variables that could be set immutable to save gas.

    DOMAIN_SEPARATOR in Volt.sol