Volt Protocol contest - Dravee's results

QA Report

1. approve should be replaced with safeApprove

approve is subject to a known front-running attack. As SafeERC20 is already used in the contract, consider using safeApprove here:

File: ERC20CompoundPCVDeposit.sol
31:         token.approve(address(cToken), amount);

2. Immutable addresses should be 0-checked

Consider adding an address(0) check for these:

contracts/oracle/ScalingPriceOracle.sol:
  58:     address public immutable oracle;
  61:     bytes32 public immutable jobId;
  64:     uint256 public immutable fee;

contracts/peg/NonCustodialPSM.sol:
  42:     IERC20 public immutable override underlyingToken;
  49:     uint256 public immutable override MAX_FEE = 300;

contracts/refs/CoreRef.sol:
  11:     ICore private immutable _core;
  12:     IVolt private immutable _volt;
  13:     IERC20 private immutable _vcon;

contracts/utils/RateLimited.sol:
  11:     uint256 public immutable MAX_RATE_LIMIT_PER_SECOND;

3. Missing comments

The following comments are missing (see @audit tags):

contracts/peg/NonCustodialPSM.sol:
  218:     /// @param minAmountOut the minimum amount out otherwise the TX will fail //@audit missing @return amountOut
  258:     /// @param minVoltAmountOut the minimum amount of VOLT out otherwise the TX will fail //@audit missing @return amountVoltOut

contracts/utils/MultiRateLimited.sol:
  328:     /// @param amount the amount to remove from the rateLimitedAddress's buffer //@audit missing @return uint256

4. Avoid floating and different pragmas: the version should be locked. All pragma declarations are floating in the solution, and some are inferior to the dominant version used across the solution 0.8.4:

contracts/pcv/compound/CompoundPCVDepositBase.sol:
  2: pragma solidity ^0.8.0;

contracts/pcv/compound/ERC20CompoundPCVDeposit.sol:
  2: pragma solidity ^0.8.0;

Gas Report

Table of Contents:

• Gas Report
  • Foreword
  • Findings
    • Version
      • Upgrade pragma to at least 0.8.4
    • Storage
      • Tighly pack storage variables
    • Variables
      • "constants" expressions are expressions, not constants.
    • Comparisons
      • >= is cheaper than >
    • Arithmetics
      • Unchecking arithmetics operations that can't underflow/overflow
    • Errors
      • Reduce the size of error messages (Long revert Strings)
      • Use Custom Errors instead of Revert Strings to save Gas

Foreword

• @audit tags

The code is annotated at multiple places with //@audit comments to pinpoint the issues. Please, pay attention to them for more details.

Findings

Version

Upgrade pragma to at least 0.8.4

Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.

The advantages here are:

• Low level inliner (>= 0.8.2): Cheaper runtime gas (especially relevant when the contract has small functions).
• Optimizer improvements in packed structs (>= 0.8.3)
• Custom errors (>= 0.8.4): cheaper deployment cost and runtime cost. Note: the runtime cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.

Consider upgrading pragma to at least 0.8.4:

pcv/compound/CompoundPCVDepositBase.sol:2:pragma solidity ^0.8.0;
pcv/compound/ERC20CompoundPCVDeposit.sol:2:pragma solidity ^0.8.0;

Storage

Tighly pack storage variables

Here, OracleRef.sol can be tightly packed from:

File: OracleRef.sol
19:     /// @notice the backup oracle reference by the contract
20:     IOracle public override backupOracle; //@audit 20 bytes
21: 
22:     /// @notice number of decimals to scale oracle price by, i.e. multiplying by 10^(decimalsNormalizer)
23:     int256 public override decimalsNormalizer; //@audit 32 bytes
24: 
25:     bool public override doInvert; //@audit gas: 1 byte. Can be tightly packed by being moved next to an address

to

File: OracleRef.sol
    /// @notice the backup oracle reference by the contract
    IOracle public override backupOracle; //@audit 20 bytes

    bool public override doInvert; //@audit gas: 1 byte. 

    /// @notice number of decimals to scale oracle price by, i.e. multiplying by 10^(decimalsNormalizer)
    int256 public override decimalsNormalizer; //@audit 32 bytes

Which would save 1 storage slot.

Variables

"constants" expressions are expressions, not constants.

Due to how constant variables are implemented (replacements at compile-time), an expression assigned to a constant variable is recomputed each time that the variable is used, which wastes some gas.

If the variable was immutable instead: the calculation would only be done once at deploy time (in the constructor), and then the result would be saved and read directly at runtime rather than being recalculated.

See: ethereum/solidity#9232

Consequences: each usage of a "constant" costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..). since these are not real constants, they can't be referenced from a real constant environment (e.g. from assembly, or from another library )

core/Permissions.sol:10:    bytes32 public constant override BURNER_ROLE = keccak256("BURNER_ROLE");
core/Permissions.sol:11:    bytes32 public constant override MINTER_ROLE = keccak256("MINTER_ROLE");
core/Permissions.sol:12:    bytes32 public constant override PCV_CONTROLLER_ROLE =
core/Permissions.sol:13:        keccak256("PCV_CONTROLLER_ROLE");
core/Permissions.sol:14:    bytes32 public constant override GOVERN_ROLE = keccak256("GOVERN_ROLE");
core/Permissions.sol:15:    bytes32 public constant override GUARDIAN_ROLE = keccak256("GUARDIAN_ROLE");
core/TribeRoles.sol:18:    bytes32 internal constant GOVERNOR = keccak256("GOVERN_ROLE");
core/TribeRoles.sol:21:    bytes32 internal constant GUARDIAN = keccak256("GUARDIAN_ROLE");
core/TribeRoles.sol:24:    bytes32 internal constant PCV_CONTROLLER = keccak256("PCV_CONTROLLER_ROLE");
core/TribeRoles.sol:27:    bytes32 internal constant MINTER = keccak256("MINTER_ROLE");
core/TribeRoles.sol:34:    bytes32 internal constant PARAMETER_ADMIN = keccak256("PARAMETER_ADMIN");
core/TribeRoles.sol:37:    bytes32 internal constant ORACLE_ADMIN = keccak256("ORACLE_ADMIN_ROLE");
core/TribeRoles.sol:40:    bytes32 internal constant TRIBAL_CHIEF_ADMIN =
core/TribeRoles.sol:41:        keccak256("TRIBAL_CHIEF_ADMIN_ROLE");
core/TribeRoles.sol:44:    bytes32 internal constant PCV_GUARDIAN_ADMIN =
core/TribeRoles.sol:45:        keccak256("PCV_GUARDIAN_ADMIN_ROLE");
core/TribeRoles.sol:48:    bytes32 internal constant MINOR_ROLE_ADMIN = keccak256("MINOR_ROLE_ADMIN");
core/TribeRoles.sol:51:    bytes32 internal constant FUSE_ADMIN = keccak256("FUSE_ADMIN");
core/TribeRoles.sol:54:    bytes32 internal constant VETO_ADMIN = keccak256("VETO_ADMIN");
core/TribeRoles.sol:57:    bytes32 internal constant MINTER_ADMIN = keccak256("MINTER_ADMIN");
core/TribeRoles.sol:60:    bytes32 internal constant OPTIMISTIC_ADMIN = keccak256("OPTIMISTIC_ADMIN");
core/TribeRoles.sol:67:    bytes32 internal constant LBP_SWAP_ROLE = keccak256("SWAP_ADMIN_ROLE");
core/TribeRoles.sol:70:    bytes32 internal constant VOTIUM_ROLE = keccak256("VOTIUM_ADMIN_ROLE");
core/TribeRoles.sol:73:    bytes32 internal constant MINOR_PARAM_ROLE = keccak256("MINOR_PARAM_ROLE");
core/TribeRoles.sol:76:    bytes32 internal constant ADD_MINTER_ROLE = keccak256("ADD_MINTER_ROLE");
core/TribeRoles.sol:79:    bytes32 internal constant PSM_ADMIN_ROLE = keccak256("PSM_ADMIN_ROLE");

Change these expressions from constant to immutable and implement the calculation in the constructor or hardcode these values in the constants and add a comment to say how the value was calculated.

Comparisons

>= is cheaper than >

Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas)

I suggest using >= instead of > to avoid some opcodes here:

utils/Timed.sol:58:        return timePassed > _duration ? _duration : timePassed;

Arithmetics

Unchecking arithmetics operations that can't underflow/overflow

Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

I suggest wrapping with an unchecked block here (see @audit tags for more details):

contracts/peg/NonCustodialPSM.sol:
  286          uint256 amountFeiToTransfer = Math.min(
  287              volt().balanceOf(address(this)),
  288              amountVoltOut
  289          );
  290:         uint256 amountFeiToMint = amountVoltOut - amountFeiToTransfer; //@audit can't underflow due to lines above

contracts/utils/MultiRateLimited.sol:
  338          require(amount <= newBuffer, "MultiRateLimited: rate limit hit");
  339  
  340          rateLimitPerAddress[rateLimitedAddress].bufferStored = uint112(
  341:             newBuffer - amount //@audit can't underflow due to line 338
  342          );

  348          emit IndividualBufferUsed(
  349              rateLimitedAddress,
  350              amount,
  351:             newBuffer - amount //@audit can't underflow due to line 338

contracts/utils/RateLimited.sol:
  104          require(usedAmount <= newBuffer, "RateLimited: rate limit hit");
  105  
  106:         bufferStored = newBuffer - usedAmount; //@audit can't underflow due to line above

Errors

Reduce the size of error messages (Long revert Strings)

Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition is met.

Revert strings that are longer than 32 bytes require at least one additional mstore, along with additional overhead for computing memory offset, etc.

Revert strings > 32 bytes:

core/Permissions.sol:31:            "Permissions: Caller is not a governor"
core/Permissions.sol:39:            "Permissions: Caller is not a guardian"
core/Permissions.sol:134:            "Permissions: Guardian cannot revoke governor"
oracle/ScalingPriceOracle.sol:141:            "ScalingPriceOracle: cannot request data before the 15th"
oracle/ScalingPriceOracle.sol:177:            "ScalingPriceOracle: Chainlink data outside of deviation threshold"
pcv/compound/ERC20CompoundPCVDeposit.sol:36:            "ERC20CompoundPCVDeposit: deposit error"
peg/NonCustodialPSM.sol:117:        require(!redeemPaused, "PegStabilityModule: Redeem paused");
peg/NonCustodialPSM.sol:123:        require(!mintPaused, "PegStabilityModule: Minting paused");
peg/NonCustodialPSM.sol:239:            "PegStabilityModule: Redeem not enough out"
peg/NonCustodialPSM.sol:277:            "PegStabilityModule: Mint not enough out"
peg/NonCustodialPSM.sol:402:            "PegStabilityModule: Invalid new GlobalRateLimitedMinter"
peg/NonCustodialPSM.sol:415:            "PegStabilityModule: Mint fee exceeds max fee"
peg/NonCustodialPSM.sol:428:            "PegStabilityModule: Redeem fee exceeds max fee"
peg/NonCustodialPSM.sol:441:            "PegStabilityModule: Invalid new PCVDeposit"
peg/NonCustodialPSM.sol:445:            "PegStabilityModule: Underlying token mismatch"
refs/CoreRef.sol:48:            "CoreRef: Caller is not a PCV controller"
refs/CoreRef.sol:56:            "CoreRef: Caller is not a governor or contract admin"
refs/CoreRef.sol:64:            "CoreRef: Caller is not a governor"
refs/CoreRef.sol:72:            "CoreRef: Caller is not a guardian or governor"
refs/CoreRef.sol:82:            "CoreRef: Caller is not governor or guardian or admin"
utils/MultiRateLimited.sol:58:            "MultiRateLimited: max buffer cap invalid"
utils/MultiRateLimited.sol:68:            "MultiRateLimited: rate limit address does not exist"
utils/MultiRateLimited.sol:85:            "MultiRateLimited: exceeds global max rate limit per second"
utils/MultiRateLimited.sol:107:            "MultiRateLimited: exceeds global buffer cap"
utils/MultiRateLimited.sol:146:                "MultiRateLimited: rate limit per second exceeds non governor allowable amount"
utils/MultiRateLimited.sol:150:                "MultiRateLimited: max buffer cap exceeds non governor allowable amount"
utils/MultiRateLimited.sol:155:            "MultiRateLimited: buffercap too high"
utils/MultiRateLimited.sol:268:            "MultiRateLimited: rate limit address does not exist"
utils/MultiRateLimited.sol:272:            "MultiRateLimited: rateLimitPerSecond too high"
utils/MultiRateLimited.sol:299:            "MultiRateLimited: new buffercap too high"
utils/MultiRateLimited.sol:303:            "MultiRateLimited: address already added"
utils/MultiRateLimited.sol:307:            "MultiRateLimited: rateLimitPerSecond too high"
utils/MultiRateLimited.sol:337:        require(newBuffer != 0, "MultiRateLimited: no rate limit buffer");
utils/RateLimited.sol:48:            "RateLimited: rateLimitPerSecond too high"
utils/RateLimited.sol:64:            "RateLimited: rateLimitPerSecond too high"
utils/RateLimited.sol:103:        require(newBuffer != 0, "RateLimited: no rate limit buffer");
volt/Volt.sol:28:                    "EIP712Domain(string name,string version,uint256 chainId,address verifyingContract)" 

I suggest shortening the revert strings to fit in 32 bytes, or that using custom errors as described next.

Use Custom Errors instead of Revert Strings to save Gas

Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)

Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:

Starting from Solidity v0.8.4, there is a convenient and gas-efficient way to explain to users why an operation failed through the use of custom errors. Until now, you could already use strings to give more information about failures (e.g., revert("Insufficient funds.");), but they are rather expensive, especially when it comes to deploy cost, and it is difficult to use dynamic information in them.

Custom errors are defined using the error statement, which can be used inside and outside of contracts (including interfaces and libraries).

Instances include:

core/Permissions.sol:29:        require(
core/Permissions.sol:37:        require(
core/Permissions.sol:132:        require(
oracle/ScalingPriceOracle.sol:139:        require(
oracle/ScalingPriceOracle.sol:172:        require(
pcv/compound/CompoundPCVDepositBase.sol:32:        require(cToken.isCToken(), "CompoundPCVDeposit: Not a cToken");
pcv/compound/CompoundPCVDepositBase.sol:44:        require(
pcv/compound/ERC20CompoundPCVDeposit.sol:34:        require(
peg/NonCustodialPSM.sol:117:        require(!redeemPaused, "PegStabilityModule: Redeem paused");
peg/NonCustodialPSM.sol:123:        require(!mintPaused, "PegStabilityModule: Minting paused");
peg/NonCustodialPSM.sol:237:        require(
peg/NonCustodialPSM.sol:275:        require(
peg/NonCustodialPSM.sol:400:        require(
peg/NonCustodialPSM.sol:413:        require(
peg/NonCustodialPSM.sol:426:        require(
peg/NonCustodialPSM.sol:439:        require(
peg/NonCustodialPSM.sol:443:        require(
refs/CoreRef.sol:36:        require(_core.isMinter(msg.sender), "CoreRef: Caller is not a minter");
refs/CoreRef.sol:41:        require(_core.isBurner(msg.sender), "CoreRef: Caller is not a burner");
refs/CoreRef.sol:46:        require(
refs/CoreRef.sol:54:        require(
refs/CoreRef.sol:62:        require(
refs/CoreRef.sol:70:        require(
refs/CoreRef.sol:78:        require(
refs/CoreRef.sol:89:        require(_core.hasRole(role, msg.sender), "UNAUTHORIZED");
refs/CoreRef.sol:95:        require(
refs/CoreRef.sol:108:        require(
refs/CoreRef.sol:123:        require(
refs/CoreRef.sol:140:        require(
refs/CoreRef.sol:152:        require(msg.sender == address(_volt), "CoreRef: Caller is not VOLT");
refs/OracleRef.sol:106:        require(valid, "OracleRef: oracle invalid");
refs/OracleRef.sol:126:        require(newOracle != address(0), "OracleRef: zero address");
utils/MultiRateLimited.sol:56:        require(
utils/MultiRateLimited.sol:66:        require(
utils/MultiRateLimited.sol:83:        require(
utils/MultiRateLimited.sol:105:        require(
utils/MultiRateLimited.sol:144:            require(
utils/MultiRateLimited.sol:148:            require(
utils/MultiRateLimited.sol:153:        require(
utils/MultiRateLimited.sol:266:        require(
utils/MultiRateLimited.sol:270:        require(
utils/MultiRateLimited.sol:297:        require(
utils/MultiRateLimited.sol:301:        require(
utils/MultiRateLimited.sol:305:        require(
utils/MultiRateLimited.sol:337:        require(newBuffer != 0, "MultiRateLimited: no rate limit buffer");
utils/MultiRateLimited.sol:338:        require(amount <= newBuffer, "MultiRateLimited: rate limit hit");
utils/RateLimited.sol:46:        require(
utils/RateLimited.sol:62:        require(
utils/RateLimited.sol:103:        require(newBuffer != 0, "RateLimited: no rate limit buffer");
utils/RateLimited.sol:104:        require(usedAmount <= newBuffer, "RateLimited: rate limit hit");
utils/Timed.sol:22:        require(isTimeStarted(), "Timed: time not started");
utils/Timed.sol:23:        require(!isTimeEnded(), "Timed: time ended");
utils/Timed.sol:28:        require(isTimeEnded(), "Timed: time not ended");
utils/Timed.sol:33:        require(isTimeEnded(), "Timed: time not ended, init");
utils/Timed.sol:72:        require(newDuration != 0, "Timed: zero duration");
volt/Volt.sol:72:        require(deadline >= block.timestamp, "Fei: EXPIRED");
volt/Volt.sol:90:        require(

I suggest replacing revert strings with custom errors.