Volt Protocol contest - IllIllI's results

I'm upgrading the following issue from a QA report (issue https://github.com/code-423n4/2022-03-volt-findings/issues/48 ) to Medium risk:

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.

1. 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;

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.

1. 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.

1. 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.

1. 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

1. 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.

1. 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

1. 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

1. 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

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

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

2. File: contracts/oracle/ScalingPriceOracle.sol (line 40)

    /// @notice the current month's CPI data

3. File: contracts/oracle/ScalingPriceOracle.sol (line 43)

    /// @notice the previous month's CPI data

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

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

5. File: contracts/oracle/ScalingPriceOracle.sol (line 60)

    /// @notice job id that retrieves the latest CPI data

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

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

7. File: contracts/oracle/ScalingPriceOracle.sol (line 157)

    /// @param _cpiData latest CPI data from BLS

8. File: contracts/oracle/ScalingPriceOracle.sol (line 169)

    /// @param _cpiData latest CPI data from BLS

9. File: contracts/oracle/ScalingPriceOracle.sol (line 180)

        /// store CPI data, removes stale data

10. 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()

1. 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.

1. File: contracts/oracle/OraclePassThrough.sol (line 27)

    function update() public {}

constants should be defined rather than using magic numbers

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

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

2. File: contracts/oracle/ScalingPriceOracle.sol (line 84)

        if (chainId == 1 || chainId == 42) {

3. File: contracts/oracle/ScalingPriceOracle.sol (line 140)

            getDay(block.timestamp) > 14,

4. 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>)

1. 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).

1. File: contracts/volt/Volt.sol (line 11)

    bytes32 public DOMAIN_SEPARATOR;

2. 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

1. File: contracts/pcv/compound/ERC20CompoundPCVDeposit.sol (line 2)

pragma solidity ^0.8.0;

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

pragma solidity ^0.8.4;

3. File: contracts/oracle/ScalingPriceOracle.sol (line 2)

pragma solidity ^0.8.4;

4. File: contracts/utils/GlobalRateLimitedMinter.sol (line 2)

pragma solidity ^0.8.4;

5. File: contracts/volt/Volt.sol (line 2)

pragma solidity ^0.8.4;

6. File: contracts/core/Core.sol (line 2)

pragma solidity ^0.8.4;

7. File: contracts/core/Permissions.sol (line 2)

pragma solidity ^0.8.4;

8. File: contracts/peg/NonCustodialPSM.sol (line 2)

pragma solidity ^0.8.4;

Code should use the same solidity versions

1. File: contracts/pcv/compound/CompoundPCVDepositBase.sol (line 2)

pragma solidity ^0.8.0;

2. File: contracts/pcv/PCVDeposit.sol (line 2)

pragma solidity ^0.8.4;

Typos

1. File: contracts/peg/NonCustodialPSM.sol (line 455)

    /// @notice overriden function in the price bound PSM

overriden -> overridden

NatSpec is incomplete

1. 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

2. 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

3. 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

4. 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

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

    event DurationUpdate(uint256 oldDuration, uint256 newDuration);

2. File: contracts/utils/Timed.sol (line 15)

    event TimerReset(uint256 startTime);

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

    event DeviationThresholdUpdate(uint256 oldThreshold, uint256 newThreshold);

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

    event BufferUsed(uint256 amountUsed, uint256 bufferRemaining);

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

    event BufferCapUpdate(uint256 oldBufferCap, uint256 newBufferCap);

6. 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)

Store the timestamp endpoint rather than re-calculating it every time

The isTimeEnded() function does a lot of calculations every time it's called. It's called from multiple modifiers so it's important for it to be efficient. Rather than storing the duration, the code can calculate and store the ending timestamp, so isTimeEnded() can just be a direct comparison of two uint256s. The duration can be calculated by subtracting the start timestamp from the ending timestamp.

1. File: contracts/utils/Timed.sol (lines 38-59)

    /// @notice return true if time period has ended
    function isTimeEnded() public view returns (bool) {
        return remainingTime() == 0;
    }

    /// @notice number of seconds remaining until time is up
    /// @return remaining
    function remainingTime() public view returns (uint256) {
        return duration - timeSinceStart(); // duration always >= timeSinceStart which is on [0,d]
    }

    /// @notice number of seconds since contract was initialized
    /// @return timestamp
    /// @dev will be less than or equal to duration
    function timeSinceStart() public view returns (uint256) {
        if (!isTimeStarted()) {
            return 0; // uninitialized
        }
        uint256 _duration = duration;
        uint256 timePassed = block.timestamp - startTime; // block timestamp always >= startTime
        return timePassed > _duration ? _duration : timePassed;
    }

Lots of duplicated code between RateLimited.sol and MultiRateLimited.sol

The functionality of RateLimited.sol can be achieved by using either address(0) or address(this) as the rateLimitedAddress so having a separate RateLimited.sol contract is a waste of deployment gas.

1. File: contracts/utils/RateLimited.sol (Various lines throughout the file)
2. File: contracts/utils/MultiRateLimited.sol (Various lines throughout the file)

require()/revert() strings longer than 32 bytes cost extra gas

1. File: contracts/pcv/compound/ERC20CompoundPCVDeposit.sol (lines 34-37)

        require(
            CErc20(address(cToken)).mint(amount) == 0,
            "ERC20CompoundPCVDeposit: deposit error"
        );

2. File: contracts/oracle/ScalingPriceOracle.sol (lines 139-142)

        require(
            getDay(block.timestamp) > 14,
            "ScalingPriceOracle: cannot request data before the 15th"
        );

3. File: contracts/oracle/ScalingPriceOracle.sol (lines 172-178)

        require(
            MAXORACLEDEVIATION.isWithinDeviationThreshold(
                currentMonth.toInt256(),
                _cpiData.toInt256()
            ),
            "ScalingPriceOracle: Chainlink data outside of deviation threshold"
        );

4. File: contracts/utils/MultiRateLimited.sol (lines 56-59)

        require(
            _individualMaxBufferCap < _globalBufferCap,
            "MultiRateLimited: max buffer cap invalid"
        );

5. File: contracts/utils/MultiRateLimited.sol (lines 66-69)

        require(
            rateLimitPerAddress[rateLimitedAddress].lastBufferUsedTime != 0,
            "MultiRateLimited: rate limit address does not exist"
        );

6. File: contracts/utils/MultiRateLimited.sol (lines 83-86)

        require(
            newRateLimitPerSecond <= MAX_RATE_LIMIT_PER_SECOND,
            "MultiRateLimited: exceeds global max rate limit per second"
        );

7. File: contracts/utils/MultiRateLimited.sol (lines 105-108)

        require(
            newBufferCap <= bufferCap,
            "MultiRateLimited: exceeds global buffer cap"
        );

8. File: contracts/utils/MultiRateLimited.sol (lines 144-147)

            require(
                _rateLimitPerSecond <= individualMaxRateLimitPerSecond,
                "MultiRateLimited: rate limit per second exceeds non governor allowable amount"
            );

9. File: contracts/utils/MultiRateLimited.sol (lines 148-151)

            require(
                _bufferCap <= individualMaxBufferCap,
                "MultiRateLimited: max buffer cap exceeds non governor allowable amount"
            );

10. File: contracts/utils/MultiRateLimited.sol (lines 153-156)

        require(
            _bufferCap <= bufferCap,
            "MultiRateLimited: buffercap too high"
        );

11. File: contracts/utils/MultiRateLimited.sol (lines 266-269)

        require(
            rateLimitData.lastBufferUsedTime != 0,
            "MultiRateLimited: rate limit address does not exist"
        );

12. File: contracts/utils/MultiRateLimited.sol (lines 270-273)

        require(
            _rateLimitPerSecond <= MAX_RATE_LIMIT_PER_SECOND,
            "MultiRateLimited: rateLimitPerSecond too high"
        );

13. File: contracts/utils/MultiRateLimited.sol (lines 297-300)

        require(
            _bufferCap <= bufferCap,
            "MultiRateLimited: new buffercap too high"
        );

14. File: contracts/utils/MultiRateLimited.sol (lines 301-304)

        require(
            rateLimitPerAddress[rateLimitedAddress].lastBufferUsedTime == 0,
            "MultiRateLimited: address already added"
        );

15. File: contracts/utils/MultiRateLimited.sol (lines 305-308)

        require(
            _rateLimitPerSecond <= MAX_RATE_LIMIT_PER_SECOND,
            "MultiRateLimited: rateLimitPerSecond too high"
        );

16. File: contracts/utils/MultiRateLimited.sol (line 337)

        require(newBuffer != 0, "MultiRateLimited: no rate limit buffer");

17. File: contracts/utils/RateLimited.sol (lines 46-49)

        require(
            _rateLimitPerSecond <= _maxRateLimitPerSecond,
            "RateLimited: rateLimitPerSecond too high"
        );

18. File: contracts/utils/RateLimited.sol (lines 62-65)

        require(
            newRateLimitPerSecond <= MAX_RATE_LIMIT_PER_SECOND,
            "RateLimited: rateLimitPerSecond too high"
        );

19. File: contracts/utils/RateLimited.sol (line 103)

        require(newBuffer != 0, "RateLimited: no rate limit buffer");

20. File: contracts/refs/CoreRef.sol (lines 46-49)

        require(
            _core.isPCVController(msg.sender),
            "CoreRef: Caller is not a PCV controller"
        );

21. File: contracts/refs/CoreRef.sol (lines 54-57)

        require(
            _core.isGovernor(msg.sender) || isContractAdmin(msg.sender),
            "CoreRef: Caller is not a governor or contract admin"
        );

22. File: contracts/refs/CoreRef.sol (lines 62-65)

        require(
            _core.isGovernor(msg.sender),
            "CoreRef: Caller is not a governor"
        );

23. File: contracts/refs/CoreRef.sol (lines 70-73)

        require(
            _core.isGovernor(msg.sender) || _core.isGuardian(msg.sender),
            "CoreRef: Caller is not a guardian or governor"
        );

24. File: contracts/refs/CoreRef.sol (lines 78-83)

        require(
            _core.isGovernor(msg.sender) ||
                _core.isGuardian(msg.sender) ||
                isContractAdmin(msg.sender),
            "CoreRef: Caller is not governor or guardian or admin"
        );

25. File: contracts/core/Permissions.sol (lines 29-32)

        require(
            isGovernor(msg.sender),
            "Permissions: Caller is not a governor"
        );

26. File: contracts/core/Permissions.sol (lines 37-40)

        require(
            isGuardian(msg.sender),
            "Permissions: Caller is not a guardian"
        );

27. File: contracts/core/Permissions.sol (lines 132-135)

        require(
            role != GOVERN_ROLE,
            "Permissions: Guardian cannot revoke governor"
        );

28. File: contracts/peg/NonCustodialPSM.sol (line 117)

        require(!redeemPaused, "PegStabilityModule: Redeem paused");

29. File: contracts/peg/NonCustodialPSM.sol (line 123)

        require(!mintPaused, "PegStabilityModule: Minting paused");

30. File: contracts/peg/NonCustodialPSM.sol (lines 237-240)

        require(
            amountOut >= minAmountOut,
            "PegStabilityModule: Redeem not enough out"
        );

31. File: contracts/peg/NonCustodialPSM.sol (lines 275-278)

        require(
            amountVoltOut >= minVoltAmountOut,
            "PegStabilityModule: Mint not enough out"
        );

32. File: contracts/peg/NonCustodialPSM.sol (lines 400-403)

        require(
            address(newMinter) != address(0),
            "PegStabilityModule: Invalid new GlobalRateLimitedMinter"
        );

33. File: contracts/peg/NonCustodialPSM.sol (lines 413-416)

        require(
            newMintFeeBasisPoints <= MAX_FEE,
            "PegStabilityModule: Mint fee exceeds max fee"
        );

34. File: contracts/peg/NonCustodialPSM.sol (lines 426-429)

        require(
            newRedeemFeeBasisPoints <= MAX_FEE,
            "PegStabilityModule: Redeem fee exceeds max fee"
        );

35. File: contracts/peg/NonCustodialPSM.sol (lines 439-442)

        require(
            address(newPCVDeposit) != address(0),
            "PegStabilityModule: Invalid new PCVDeposit"
        );

36. File: contracts/peg/NonCustodialPSM.sol (lines 443-446)

        require(
            newPCVDeposit.balanceReportedIn() == address(underlyingToken),
            "PegStabilityModule: Underlying token mismatch"
        );

Use a more recent version of solidity

Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

1. File: contracts/pcv/PCVDeposit.sol (line 2)

pragma solidity ^0.8.4;

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

pragma solidity ^0.8.4;

3. File: contracts/oracle/ScalingPriceOracle.sol (line 2)

pragma solidity ^0.8.4;

4. File: contracts/utils/Timed.sol (line 2)

pragma solidity ^0.8.4;

5. File: contracts/utils/Deviation.sol (line 2)

pragma solidity ^0.8.4;

6. File: contracts/utils/GlobalRateLimitedMinter.sol (line 2)

pragma solidity ^0.8.4;

7. File: contracts/utils/MultiRateLimited.sol (line 2)

pragma solidity ^0.8.4;

8. File: contracts/utils/RateLimited.sol (line 2)

pragma solidity ^0.8.4;

9. File: contracts/volt/Volt.sol (line 2)

pragma solidity ^0.8.4;

10. File: contracts/refs/OracleRef.sol (line 2)

pragma solidity ^0.8.4;

11. File: contracts/refs/CoreRef.sol (line 2)

pragma solidity ^0.8.4;

12. File: contracts/core/Core.sol (line 2)

pragma solidity ^0.8.4;

13. File: contracts/core/TribeRoles.sol (line 2)

pragma solidity ^0.8.4;

14. File: contracts/core/Permissions.sol (line 2)

pragma solidity ^0.8.4;

15. File: contracts/peg/NonCustodialPSM.sol (line 2)

pragma solidity ^0.8.4;

Use a more recent version of solidity

Use a solidity version of at least 0.8.2 to get compiler automatic inlining Use a solidity version of at least 0.8.3 to get better struct packing and cheaper multiple storage reads Use a solidity version of at least 0.8.4 to get custom errors, which are cheaper at deployment than revert()/require() strings Use a solidity version of at least 0.8.10 to have external calls skip contract existence checks if the external call has a return value

1. File: contracts/pcv/compound/CompoundPCVDepositBase.sol (line 2)

pragma solidity ^0.8.0;

2. File: contracts/pcv/compound/ERC20CompoundPCVDeposit.sol (line 2)

pragma solidity ^0.8.0;

Using bools for storage incurs overhead

    // Booleans are more expensive than uint256 or any type that takes up a full
    // word because each write operation emits an extra SLOAD to first read the
    // slot's contents, replace the bits taken up by the boolean, and then write
    // back. This is the compiler's defense against contract upgrades and
    // pointer aliasing, and it cannot be disabled.

https://github.com/OpenZeppelin/openzeppelin-contracts/blob/58f635312aa21f947cae5f8578638a85aa2519f5/contracts/security/ReentrancyGuard.sol#L23-L27

1. File: contracts/utils/RateLimited.sol (line 23)

    bool public doPartialAction;

2. File: contracts/refs/OracleRef.sol (line 25)

    bool public override doInvert;

3. File: contracts/peg/NonCustodialPSM.sol (line 52)

    bool public redeemPaused;

4. File: contracts/peg/NonCustodialPSM.sol (line 55)

    bool public mintPaused;

internal functions only called once can be inlined to save gas

1. File: contracts/oracle/ScalingPriceOracle.sol (line 171)

    function _updateCPIData(uint256 _cpiData) internal {

2. File: contracts/oracle/ScalingPriceOracle.sol (line 218)

    function _addNewMonth(uint128 newMonth) internal {

3. File: contracts/core/Permissions.sol (line 211)

    function _setupGovernor(address governor) internal {

State variables should be cached in stack variables rather than re-reading them from storage

The instances below point to the second access of a state variable within a function. Less obvious optimizations include having local storage variables of mappings within state variable mappings or mappings within state variable structs, having local storage variables of structs within mappings, or having local caches of state variable contracts/addresses.

1. File: contracts/pcv/compound/CompoundPCVDepositBase.sol (line 32)

        require(cToken.isCToken(), "CompoundPCVDeposit: Not a cToken");

2. File: contracts/pcv/compound/CompoundPCVDepositBase.sol (line 57)

            (cToken.balanceOf(address(this)) * exchangeRate) /

3. File: contracts/pcv/compound/ERC20CompoundPCVDeposit.sol (line 31)

        token.approve(address(cToken), amount);

4. File: contracts/oracle/ScalingPriceOracle.sol (line 124)

        percentageChange = (delta * Constants.BP_INT) / int128(previousMonth);

5. File: contracts/utils/MultiRateLimited.sol (line 344)

        rateLimitPerAddress[rateLimitedAddress].lastBufferUsedTime = block

6. File: contracts/utils/RateLimited.sol (line 110)

        emit BufferUsed(usedAmount, bufferStored);

7. File: contracts/refs/OracleRef.sol (line 104)

            (_peg, valid) = backupOracle.read();

8. File: contracts/refs/OracleRef.sol (line 111)

            scalingFactor = 10**(-1 * decimalsNormalizer).toUint256();

Splitting require() statements that use && saves gas

See this issue for an example

1. File: contracts/volt/Volt.sol (lines 90-93)

        require(
            recoveredAddress != address(0) && recoveredAddress == owner,
            "Fei: INVALID_SIGNATURE"
        );

Usage of uints/ints smaller than 32 bytes (256 bits) incurs overhead

When using elements that are smaller than 32 bytes, your contract’s gas usage may be higher. This is because the EVM operates on 32 bytes at a time. Therefore, if the element is smaller than that, the EVM must use more operations in order to reduce the size of the element from 32 bytes to the desired size.

https://docs.soliditylang.org/en/v0.8.11/internals/layout_in_storage.html Use a larger size then downcast where needed

1. File: contracts/oracle/ScalingPriceOracle.sol (line 41)

    uint128 public currentMonth;

2. File: contracts/oracle/ScalingPriceOracle.sol (line 44)

    uint128 public previousMonth;

3. File: contracts/oracle/ScalingPriceOracle.sol (line 75)

        uint128 _currentMonth,

4. File: contracts/oracle/ScalingPriceOracle.sol (line 76)

        uint128 _previousMonth

5. File: contracts/oracle/ScalingPriceOracle.sol (line 218)

    function _addNewMonth(uint128 newMonth) internal {

6. File: contracts/utils/MultiRateLimited.sol (line 22)

        uint32 lastBufferUsedTime;

7. File: contracts/utils/MultiRateLimited.sol (line 23)

        uint112 bufferCap;

8. File: contracts/utils/MultiRateLimited.sol (line 24)

        uint112 bufferStored;

9. File: contracts/utils/MultiRateLimited.sol (line 25)

        uint112 rateLimitPerSecond;

10. File: contracts/utils/MultiRateLimited.sol (line 122)

        uint112 _rateLimitPerSecond,

11. File: contracts/utils/MultiRateLimited.sol (line 123)

        uint112 _bufferCap

12. File: contracts/utils/MultiRateLimited.sol (line 134)

        uint112 _rateLimitPerSecond,

13. File: contracts/utils/MultiRateLimited.sol (line 135)

        uint112 _bufferCap

14. File: contracts/utils/MultiRateLimited.sol (line 208)

        returns (uint112)

15. File: contracts/utils/MultiRateLimited.sol (line 259)

        uint112 _rateLimitPerSecond,

16. File: contracts/utils/MultiRateLimited.sol (line 260)

        uint112 _bufferCap

17. File: contracts/utils/MultiRateLimited.sol (line 275)

        uint112 oldRateLimitPerSecond = rateLimitData.rateLimitPerSecond;

18. File: contracts/utils/MultiRateLimited.sol (line 294)

        uint112 _rateLimitPerSecond,

19. File: contracts/utils/MultiRateLimited.sol (line 295)

        uint112 _bufferCap

20. File: contracts/volt/Volt.sol (line 68)

        uint8 v,

Expressions for constant values such as a call to keccak256(), should use immutable rather than constant

See this issue for a detail description of the issue

1. File: contracts/core/TribeRoles.sol (line 18)

    bytes32 internal constant GOVERNOR = keccak256("GOVERN_ROLE");

2. File: contracts/core/TribeRoles.sol (line 21)

    bytes32 internal constant GUARDIAN = keccak256("GUARDIAN_ROLE");

3. File: contracts/core/TribeRoles.sol (line 24)

    bytes32 internal constant PCV_CONTROLLER = keccak256("PCV_CONTROLLER_ROLE");

4. File: contracts/core/TribeRoles.sol (line 27)

    bytes32 internal constant MINTER = keccak256("MINTER_ROLE");

5. File: contracts/core/TribeRoles.sol (line 34)

    bytes32 internal constant PARAMETER_ADMIN = keccak256("PARAMETER_ADMIN");

6. File: contracts/core/TribeRoles.sol (line 37)

    bytes32 internal constant ORACLE_ADMIN = keccak256("ORACLE_ADMIN_ROLE");

7. File: contracts/core/TribeRoles.sol (lines 40-41)

    bytes32 internal constant TRIBAL_CHIEF_ADMIN =
        keccak256("TRIBAL_CHIEF_ADMIN_ROLE");

8. File: contracts/core/TribeRoles.sol (lines 44-45)

    bytes32 internal constant PCV_GUARDIAN_ADMIN =
        keccak256("PCV_GUARDIAN_ADMIN_ROLE");

9. File: contracts/core/TribeRoles.sol (line 48)

    bytes32 internal constant MINOR_ROLE_ADMIN = keccak256("MINOR_ROLE_ADMIN");

10. File: contracts/core/TribeRoles.sol (line 51)

    bytes32 internal constant FUSE_ADMIN = keccak256("FUSE_ADMIN");

11. File: contracts/core/TribeRoles.sol (line 54)

    bytes32 internal constant VETO_ADMIN = keccak256("VETO_ADMIN");

12. File: contracts/core/TribeRoles.sol (line 57)

    bytes32 internal constant MINTER_ADMIN = keccak256("MINTER_ADMIN");

13. File: contracts/core/TribeRoles.sol (line 60)

    bytes32 internal constant OPTIMISTIC_ADMIN = keccak256("OPTIMISTIC_ADMIN");

14. File: contracts/core/TribeRoles.sol (line 67)

    bytes32 internal constant LBP_SWAP_ROLE = keccak256("SWAP_ADMIN_ROLE");

15. File: contracts/core/TribeRoles.sol (line 70)

    bytes32 internal constant VOTIUM_ROLE = keccak256("VOTIUM_ADMIN_ROLE");

16. File: contracts/core/TribeRoles.sol (line 73)

    bytes32 internal constant MINOR_PARAM_ROLE = keccak256("MINOR_PARAM_ROLE");

17. File: contracts/core/TribeRoles.sol (line 76)

    bytes32 internal constant ADD_MINTER_ROLE = keccak256("ADD_MINTER_ROLE");

18. File: contracts/core/TribeRoles.sol (line 79)

    bytes32 internal constant PSM_ADMIN_ROLE = keccak256("PSM_ADMIN_ROLE");

19. File: contracts/core/Permissions.sol (line 10)

    bytes32 public constant override BURNER_ROLE = keccak256("BURNER_ROLE");

20. File: contracts/core/Permissions.sol (line 11)

    bytes32 public constant override MINTER_ROLE = keccak256("MINTER_ROLE");

21. File: contracts/core/Permissions.sol (lines 12-13)

    bytes32 public constant override PCV_CONTROLLER_ROLE =
        keccak256("PCV_CONTROLLER_ROLE");

22. File: contracts/core/Permissions.sol (line 14)

    bytes32 public constant override GOVERN_ROLE = keccak256("GOVERN_ROLE");

23. File: contracts/core/Permissions.sol (line 15)

    bytes32 public constant override GUARDIAN_ROLE = keccak256("GUARDIAN_ROLE");

Using private rather than public for constants, saves gas

If needed, the value can be read from the verified contract source code

1. File: contracts/oracle/ScalingPriceOracle.sol (line 50)

    uint256 public constant override TIMEFRAME = 28 days;

2. File: contracts/oracle/ScalingPriceOracle.sol (line 55)

    uint256 public constant override MAXORACLEDEVIATION = 2_000;

3. File: contracts/oracle/ScalingPriceOracle.sol (line 61)

    bytes32 public immutable jobId;

4. File: contracts/oracle/ScalingPriceOracle.sol (line 64)

    uint256 public immutable fee;

5. File: contracts/utils/RateLimited.sol (line 11)

    uint256 public immutable MAX_RATE_LIMIT_PER_SECOND;

6. File: contracts/volt/Volt.sol (lines 13-14)

    bytes32 public constant PERMIT_TYPEHASH =
        0x6e71edae12b1b97f4d1f60370fef10105fa2faae0126114a169c64845d6126c9;

7. File: contracts/core/Permissions.sol (line 10)

    bytes32 public constant override BURNER_ROLE = keccak256("BURNER_ROLE");

8. File: contracts/core/Permissions.sol (line 11)

    bytes32 public constant override MINTER_ROLE = keccak256("MINTER_ROLE");

9. File: contracts/core/Permissions.sol (lines 12-13)

    bytes32 public constant override PCV_CONTROLLER_ROLE =
        keccak256("PCV_CONTROLLER_ROLE");

10. File: contracts/core/Permissions.sol (line 14)

    bytes32 public constant override GOVERN_ROLE = keccak256("GOVERN_ROLE");

11. File: contracts/core/Permissions.sol (line 15)

    bytes32 public constant override GUARDIAN_ROLE = keccak256("GUARDIAN_ROLE");

12. File: contracts/peg/NonCustodialPSM.sol (line 49)

    uint256 public immutable override MAX_FEE = 300;

Duplicated require()/revert() checks should be refactored to a modifier or function

1. File: contracts/utils/MultiRateLimited.sol (lines 305-308)

        require(
            _rateLimitPerSecond <= MAX_RATE_LIMIT_PER_SECOND,
            "MultiRateLimited: rateLimitPerSecond too high"
        );

State variables only set in the constructor should be declared immutable

1. File: contracts/pcv/compound/ERC20CompoundPCVDeposit.sol (line 16)

    IERC20 public token;

require() or revert() statements that check input arguments should be at the top of the function

Checks that involve constants should come before checks that involve state variables

1. File: contracts/utils/MultiRateLimited.sol (lines 270-273)

        require(
            _rateLimitPerSecond <= MAX_RATE_LIMIT_PER_SECOND,
            "MultiRateLimited: rateLimitPerSecond too high"
        );

2. File: contracts/utils/MultiRateLimited.sol (lines 305-308)

        require(
            _rateLimitPerSecond <= MAX_RATE_LIMIT_PER_SECOND,
            "MultiRateLimited: rateLimitPerSecond too high"
        );

3. File: contracts/utils/RateLimited.sol (lines 46-49)

        require(
            _rateLimitPerSecond <= _maxRateLimitPerSecond,
            "RateLimited: rateLimitPerSecond too high"
        );

Use custom errors rather than revert()/require() strings to save deployment gas

1. File: contracts/oracle/ScalingPriceOracle.sol (Various lines throughout the file)
2. File: contracts/utils/Timed.sol (Various lines throughout the file)
3. File: contracts/utils/MultiRateLimited.sol (Various lines throughout the file)
4. File: contracts/utils/RateLimited.sol (Various lines throughout the file)
5. File: contracts/volt/Volt.sol (Various lines throughout the file)
6. File: contracts/refs/OracleRef.sol (Various lines throughout the file)
7. File: contracts/refs/CoreRef.sol (Various lines throughout the file)
8. File: contracts/core/Permissions.sol (Various lines throughout the file)
9. File: contracts/peg/NonCustodialPSM.sol (Various lines throughout the file)

Not using the named return variables when a function returns, wastes deployment gas

1. File: contracts/oracle/ScalingPriceOracle.sol (line 150)

        return sendChainlinkRequestTo(oracle, request, fee);