QA Report - Githubissues

Low Risk Issues

Division by zero

calculateDeviationThresholdBasisPoints() was important enough to be in a separate library rather than being just a normal function of another contract so it should be generic enough for other contracts to use it. If the input argument a is zero then the function performs a division by zero and will throw an exception. If this behavior is what is wanted, the NatSpec should make this explicit and a revert() should be added with an appropriate error message.

File: contracts/utils/Deviation.sol (lines 16-23)

/// @notice return the percent deviation between a and b in basis points terms
function calculateDeviationThresholdBasisPoints(int256 a, int256 b)
    internal
    pure
    returns (uint256)
{
    int256 delta = a - b;
    int256 basisPoints = (delta * Constants.BP_INT) / a;

Unsafe calls to `decimals()`

decimals() was optional in the original ERC20 specification, so there are functions that do not implement it. It's not safe to cast arbitrary token addresses in order to call this function. If IERC20Metadata is to be relied on, that should be the variable type of the token variable, rather than it being address, so the compiler can verify that types correctly match, rather than this being a runtime failure. See this prior instance of this issue which was marked as Low risk. Do this to resolve the issue.

File: contracts/refs/OracleRef.sol (line 162)

        int256(uint256(IERC20Metadata(token).decimals()));

Cross-chain replay attack

See this issue from a prior contest for details.

File: contracts/volt/Volt.sol (lines 11-36)

bytes32 public DOMAIN_SEPARATOR;
// keccak256("Permit(address owner,address spender,uint256 value,uint256 nonce,uint256 deadline)");
bytes32 public constant PERMIT_TYPEHASH =
    0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9;
mapping(address => uint256) public nonces;

/// @notice Fei token constructor
/// @param core Fei Core address to reference
constructor(address core) ERC20("VOLT", "VOLT") CoreRef(core) {
    uint256 chainId;
    // solhint-disable-next-line no-inline-assembly
    assembly {
        chainId := chainid()
    }
    DOMAIN_SEPARATOR = keccak256(
        abi.encode(
            keccak256(
                "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)"
            ),
            keccak256(bytes(name())),
            keccak256(bytes("1")),
            chainId,
            address(this)
        )
    );
}

Missing checks for `address(0x0)` when assigning values to `address` state variables

File: contracts/oracle/ScalingPriceOracle.sol (line 88)
```
    oracle = _oracle;
```

Extrapolation, not interpolation

The NatSpec says that the code is doing an interpolation, which is finding a value between two endpoints. The code however is doing extrapolation - projecting a future value based on prior values. From the judging criteria guidelines: Low: Assets are not at risk. State handling, function incorrect as to spec, issues with comments.

File: contracts/oracle/ScalingPriceOracle.sol (lines 16-17)

/// @notice contract that receives a chainlink price feed and then linearly interpolates that rate over
/// a 28 day period into the VOLT price. Interest is compounded monthly when the rate is updated

Modifier does not always execute `_;`

It is dangerous to have _; only be executed from some code paths. If the function using it has return values, they'll remain uninitialized, which can lead to unexpected behavior

File: contracts/refs/CoreRef.sol (lines 29-33)

modifier ifMinterSelf() {
    if (_core.isMinter(address(this))) {
        _;
    }
}

Non-critical Issues

Fei is not the only underlying token

The variable name amountFeiToTransfer is misleading because it is not only Fei. It should be named something like amountStablecoinToTransfer

File: contracts/peg/NonCustodialPSM.sol (line 286)

Inconsistent CPI/CPI-U terminology

The code refers to both CPI and CPI-U in various places, using CPI-U for calls to getMonthlyAPR(). It should consistently use one or the other in all places

File: contracts/oracle/ScalingPriceOracle.sol (line 38)

/// ---------- Mutable CPI Variables Packed Into Single Storage Slot to Save an SSTORE & SLOAD ----------

File: contracts/oracle/ScalingPriceOracle.sol (line 40)
```
/// @notice the current month's CPI data
```
File: contracts/oracle/ScalingPriceOracle.sol (line 43)
```
/// @notice the previous month's CPI data
```

File: contracts/oracle/ScalingPriceOracle.sol (line 48)

/// @notice the time frame over which all changes in CPI data are applied

File: contracts/oracle/ScalingPriceOracle.sol (line 60)
```
/// @notice job id that retrieves the latest CPI data
```

File: contracts/oracle/ScalingPriceOracle.sol (line 97)

    /// calculate new monthly CPI-U rate in basis points based on current and previous month

File: contracts/oracle/ScalingPriceOracle.sol (line 157)
```
/// @param _cpiData latest CPI data from BLS
```
File: contracts/oracle/ScalingPriceOracle.sol (line 169)
```
/// @param _cpiData latest CPI data from BLS
```
File: contracts/oracle/ScalingPriceOracle.sol (line 180)
```
    /// store CPI data, removes stale data
```

File: contracts/oracle/ScalingPriceOracle.sol (line 183)

    /// calculate new monthly CPI-U rate in basis points

APR is Annual Percentage Rate

CPI is an index not an interest rate and is monthly not annual, so using 'APR' is misleading. Rename to something like getMonthOverMonthPercentageChange()

File: contracts/oracle/ScalingPriceOracle.sol (lines 120-122)

/// @notice get APR from chainlink data by measuring (current month - previous month) / previous month
/// @return percentageChange percentage change in basis points over past month
function getMonthlyAPR() public view returns (int256 percentageChange) {

`public` functions not called by the contract should be declared `external` instead

Contracts are allowed to override their parents' functions and change the visibility from external to public.

File: contracts/oracle/OraclePassThrough.sol (line 27)
```
function update() public {}
```

`constant`s should be defined rather than using magic numbers

File: contracts/oracle/OraclePassThrough.sol (line 40)

    price = Decimal.from(currentPrice).div(1e18);

File: contracts/oracle/ScalingPriceOracle.sol (line 84)
```
    if (chainId == 1 || chainId == 42) {
```
File: contracts/oracle/ScalingPriceOracle.sol (line 140)
```
        getDay(block.timestamp) > 14,
```
File: contracts/refs/OracleRef.sol (line 160)
```
    int256 feiDecimals = 18;
```

Use a more recent version of solidity

Use a solidity version of at least 0.8.12 to get string.concat() to be used instead of abi.encodePacked(<str>,<str>)

File: contracts/volt/Volt.sol (line 2)
```
pragma solidity ^0.8.4;
```

Variable names that consist of all capital letters should be reserved for `const`/`immutable` variables

If the variable needs to be different based on which class it comes from, a view/pure function should be used instead (e.g. like this).

File: contracts/volt/Volt.sol (line 11)
```
bytes32 public DOMAIN_SEPARATOR;
```

File: contracts/refs/CoreRef.sol (line 16)

bytes32 public override CONTRACT_ADMIN_ROLE;

Non-library/interface-only files should use fixed compiler versions, not floating ones

File: contracts/pcv/compound/ERC20CompoundPCVDeposit.sol (line 2)
```
pragma solidity ^0.8.0;
```
File: contracts/oracle/OraclePassThrough.sol (line 2)
```
pragma solidity ^0.8.4;
```
File: contracts/oracle/ScalingPriceOracle.sol (line 2)
```
pragma solidity ^0.8.4;
```
File: contracts/utils/GlobalRateLimitedMinter.sol (line 2)
```
pragma solidity ^0.8.4;
```
File: contracts/volt/Volt.sol (line 2)
```
pragma solidity ^0.8.4;
```
File: contracts/core/Core.sol (line 2)
```
pragma solidity ^0.8.4;
```
File: contracts/core/Permissions.sol (line 2)
```
pragma solidity ^0.8.4;
```
File: contracts/peg/NonCustodialPSM.sol (line 2)
```
pragma solidity ^0.8.4;
```

Code should use the same solidity versions

File: contracts/pcv/compound/CompoundPCVDepositBase.sol (line 2)
```
pragma solidity ^0.8.0;
```
File: contracts/pcv/PCVDeposit.sol (line 2)
```
pragma solidity ^0.8.4;
```

Typos

File: contracts/peg/NonCustodialPSM.sol (line 455)
```
/// @notice overriden function in the price bound PSM
```
overriden -> overridden

NatSpec is incomplete

File: contracts/utils/MultiRateLimited.sol (lines 327-332)

/// @param rateLimitedAddress the address whose buffer will be depleted
/// @param amount the amount to remove from the rateLimitedAddress's buffer
function _depleteIndividualBuffer(
    address rateLimitedAddress,
    uint256 amount
) internal returns (uint256) {

Missing: @return

File: contracts/volt/Volt.sol (lines 58-70)

/// @notice permit spending of FEI
/// @param owner the FEI holder
/// @param spender the approved operator
/// @param value the amount approved
/// @param deadline the deadline after which the approval is no longer valid
function permit(
    address owner,
    address spender,
    uint256 value,
    uint256 deadline,
    uint8 v,
    bytes32 r,
    bytes32 s

Missing: @param v @param r @param s

File: contracts/peg/NonCustodialPSM.sol (lines 217-230)

/// @param amountVoltIn the amount of VOLT to sell
/// @param minAmountOut the minimum amount out otherwise the TX will fail
function redeem(
    address to,
    uint256 amountVoltIn,
    uint256 minAmountOut
)
    external
    virtual
    override
    nonReentrant
    whenNotPaused
    whileRedemptionsNotPaused
    returns (uint256 amountOut)

Missing: @return

File: contracts/peg/NonCustodialPSM.sol (lines 257-270)

/// @param amountIn the amount of external asset to sell to the PSM
/// @param minVoltAmountOut the minimum amount of VOLT out otherwise the TX will fail
function mint(
    address to,
    uint256 amountIn,
    uint256 minVoltAmountOut
)
    external
    virtual
    override
    nonReentrant
    whenNotPaused
    whileMintingNotPaused
    returns (uint256 amountVoltOut)

Missing: @return

Event is missing `indexed` fields

Each event should use three indexed fields if there are three or more fields

File: contracts/utils/Timed.sol (line 13)

event DurationUpdate(uint256 oldDuration, uint256 newDuration);

File: contracts/utils/Timed.sol (line 15)
```
event TimerReset(uint256 startTime);
```

File: contracts/utils/Deviation.sol (line 14)

event DeviationThresholdUpdate(uint256 oldThreshold, uint256 newThreshold);

File: contracts/utils/RateLimited.sol (line 28)

event BufferUsed(uint256 amountUsed, uint256 bufferRemaining);

File: contracts/utils/RateLimited.sol (line 29)

event BufferCapUpdate(uint256 oldBufferCap, uint256 newBufferCap);

File: contracts/utils/RateLimited.sol (lines 30-33)

event RateLimitPerSecondUpdate(
    uint256 oldRateLimitPerSecond,
    uint256 newRateLimitPerSecond
);

Multiplication on the result of a division

All multiplications should be done first before doing any divisions, to avoid loss of precision

ScalingPriceOracle.getCurrentOraclePrice() (contracts/oracle/ScalingPriceOracle.sol#110-118) performs a multiplication on the result of a division:
    -pricePercentageChange = oraclePriceInt * monthlyChangeRateBasisPoints / Constants.BP_INT (contracts/oracle/ScalingPriceOracle.sol#114)
    -priceDelta = pricePercentageChange * timeDelta / TIMEFRAME.toInt256() (contracts/oracle/ScalingPriceOracle.sol#115)
OracleRef.readOracle() (contracts/refs/OracleRef.sol#101-123) performs a multiplication on the result of a division:
    -_peg = _peg.div(scalingFactor) (contracts/refs/OracleRef.sol#112)
    -_peg = _peg.mul(scalingFactor) (contracts/refs/OracleRef.sol#115)
Deviation.calculateDeviationThresholdBasisPoints(int256,int256) (contracts/utils/Deviation.sol#17-26) performs a multiplication on the result of a division:
    -basisPoints = (delta * Constants.BP_INT) / a (contracts/utils/Deviation.sol#23)
    -(basisPoints * - 1).toUint256() (contracts/utils/Deviation.sol#25)

Non-exploitable reentrancies

Follow the best-practice of the Checks-Effects-Interactions pattern

Reentrancy in NonCustodialPSM.mint(address,uint256,uint256) (contracts/peg/NonCustodialPSM.sol#259-303):
    External calls:
    - updateOracle() (contracts/peg/NonCustodialPSM.sol#272)
        - oracle.update() (contracts/refs/OracleRef.sol#95)
    - underlyingToken.safeTransferFrom(msg.sender,address(pcvDeposit),amountIn) (contracts/peg/NonCustodialPSM.sol#280-284)
    - IERC20(volt()).safeTransfer(to,amountFeiToTransfer) (contracts/peg/NonCustodialPSM.sol#293)
    - rateLimitedMinter.mintVolt(to,amountFeiToMint) (contracts/peg/NonCustodialPSM.sol#297)
    State variables written after the call(s):
    - _replenishBuffer(amountVoltOut) (contracts/peg/NonCustodialPSM.sol#300)
        - bufferStored = Math.min(newBuffer + amount,_bufferCap) (contracts/utils/RateLimited.sol#128)

Reentrancy in PCVDeposit._withdrawERC20(address,address,uint256) (contracts/pcv/PCVDeposit.sol#25-32):
    External calls:
    - IERC20(token).safeTransfer(to,amount) (contracts/pcv/PCVDeposit.sol#30)
    Event emitted after the call(s):
    - WithdrawERC20(msg.sender,token,to,amount) (contracts/pcv/PCVDeposit.sol#31)
Reentrancy in ERC20CompoundPCVDeposit.deposit() (contracts/pcv/compound/ERC20CompoundPCVDeposit.sol#28-40):
    External calls:
    - token.approve(address(cToken),amount) (contracts/pcv/compound/ERC20CompoundPCVDeposit.sol#31)
    - require(bool,string)(CErc20(address(cToken)).mint(amount) == 0,ERC20CompoundPCVDeposit: deposit error) (contracts/pcv/compound/ERC20CompoundPCVDeposit.sol#34-37)
    Event emitted after the call(s):
    - Deposit(msg.sender,amount) (contracts/pcv/compound/ERC20CompoundPCVDeposit.sol#39)
Reentrancy in NonCustodialPSM.mint(address,uint256,uint256) (contracts/peg/NonCustodialPSM.sol#259-303):
    External calls:
    - updateOracle() (contracts/peg/NonCustodialPSM.sol#272)
        - oracle.update() (contracts/refs/OracleRef.sol#95)
    - underlyingToken.safeTransferFrom(msg.sender,address(pcvDeposit),amountIn) (contracts/peg/NonCustodialPSM.sol#280-284)
    - IERC20(volt()).safeTransfer(to,amountFeiToTransfer) (contracts/peg/NonCustodialPSM.sol#293)
    - rateLimitedMinter.mintVolt(to,amountFeiToMint) (contracts/peg/NonCustodialPSM.sol#297)
    Event emitted after the call(s):
    - Mint(to,amountIn,amountVoltOut) (contracts/peg/NonCustodialPSM.sol#302)
Reentrancy in NonCustodialPSM.redeem(address,uint256,uint256) (contracts/peg/NonCustodialPSM.sol#219-251):
    External calls:
    - updateOracle() (contracts/peg/NonCustodialPSM.sol#234)
        - oracle.update() (contracts/refs/OracleRef.sol#95)
    - IERC20(volt()).safeTransferFrom(msg.sender,address(this),amountVoltIn) (contracts/peg/NonCustodialPSM.sol#242-246)
    - pcvDeposit.withdraw(to,amountOut) (contracts/peg/NonCustodialPSM.sol#248)
    Event emitted after the call(s):
    - Redeem(to,amountVoltIn,amountOut) (contracts/peg/NonCustodialPSM.sol#250)
Reentrancy in CompoundPCVDepositBase.withdraw(address,uint256) (contracts/pcv/compound/CompoundPCVDepositBase.sol#38-50):
    External calls:
    - require(bool,string)(cToken.redeemUnderlying(amountUnderlying) == 0,CompoundPCVDeposit: redeem error) (contracts/pcv/compound/CompoundPCVDepositBase.sol#44-47)
    Event emitted after the call(s):
    - Withdrawal(msg.sender,to,amountUnderlying) (contracts/pcv/compound/CompoundPCVDepositBase.sol#49)
Reentrancy in NonCustodialPSM.withdrawERC20(address,address,uint256) (contracts/peg/NonCustodialPSM.sol#201-208):
    External calls:
    - IERC20(token).safeTransfer(to,amount) (contracts/peg/NonCustodialPSM.sol#206)
    Event emitted after the call(s):
    - WithdrawERC20(msg.sender,token,to,amount) (contracts/peg/NonCustodialPSM.sol#207)
Reentrancy in PCVDeposit.withdrawETH(address,uint256) (contracts/pcv/PCVDeposit.sol#37-45):
    External calls:
    - Address.sendValue(to,amountOut) (contracts/pcv/PCVDeposit.sol#43)
    Event emitted after the call(s):
    - WithdrawETH(msg.sender,to,amountOut) (contracts/pcv/PCVDeposit.sol#44)

code-423n4 / 2022-03-volt-findings

QA Report #48

Low Risk Issues

Division by zero

Unsafe calls to `decimals()`

Cross-chain replay attack

Missing checks for `address(0x0)` when assigning values to `address` state variables

Extrapolation, not interpolation

Modifier does not always execute `_;`

Non-critical Issues

Fei is not the only underlying token

Inconsistent CPI/CPI-U terminology

APR is Annual Percentage Rate

`public` functions not called by the contract should be declared `external` instead

`constant`s should be defined rather than using magic numbers

Use a more recent version of solidity

Variable names that consist of all capital letters should be reserved for `const`/`immutable` variables

Non-library/interface-only files should use fixed compiler versions, not floating ones

Code should use the same solidity versions

Typos

NatSpec is incomplete

Event is missing `indexed` fields

Multiplication on the result of a division

Non-exploitable reentrancies

Division by zero

code-423n4 / 2022-03-volt-findings

QA Report #48

Low Risk Issues

Division by zero

Unsafe calls to decimals()

Cross-chain replay attack

Missing checks for address(0x0) when assigning values to address state variables

Extrapolation, not interpolation

Modifier does not always execute _;

Non-critical Issues

Fei is not the only underlying token

Inconsistent CPI/CPI-U terminology

APR is Annual Percentage Rate

public functions not called by the contract should be declared external instead

constants should be defined rather than using magic numbers

Use a more recent version of solidity

Variable names that consist of all capital letters should be reserved for const/immutable variables

Non-library/interface-only files should use fixed compiler versions, not floating ones

Code should use the same solidity versions

Typos

NatSpec is incomplete

Event is missing indexed fields

Multiplication on the result of a division

Non-exploitable reentrancies

Division by zero

Unsafe calls to `decimals()`

Missing checks for `address(0x0)` when assigning values to `address` state variables

Modifier does not always execute `_;`

`public` functions not called by the contract should be declared `external` instead

`constant`s should be defined rather than using magic numbers

Variable names that consist of all capital letters should be reserved for `const`/`immutable` variables

Event is missing `indexed` fields