Volt Protocol contest - Jujic's results

Lines of code

https://github.com/code-423n4/2022-03-volt/blob/main/contracts/core/Permissions.sol

Vulnerability details

Impact

The onlyGovernor() modifier is used on contracts to prevent unauthorized accounts from calling restricted functions. Once an account is considered trusted, it is allowed to add and remove accounts by calling permission functions as they see fit. However, if any single account has its private keys compromised or decides to become malicious on their own, they can remove all other trusted accounts. As a result, he/she is effectively able to take over the trusted group that controls all restricted functions in the protocol.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/core/Permissions.sol#L28

Tools Used

Manual code review.

Recommended Mitigation Steps

The Governor account should point to a multisig managed by the Volt team or by a community DAO.

#1. Long Revert Strings

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

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/utils/MultiRateLimited.sol#L150

There are several other places throughout the codebase where the same optimization can be used.

##Tools

https://planetcalc.com/9029/ ##Recommended Mitigation Steps Shorten the revert strings to fit in 32 bytes.

2. Adding unchecked directive can save gas (6 findings)

Impact

For the arithmetic operations that will never over/underflow, using the unchecked directive (Solidity v0.8 has default overflow/underflow checks) can save some gas from the unnecessary internal over/underflow checks.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/oracle/ScalingPriceOracle.sol#L123

int256 delta = int128(currentMonth) - int128(previousMonth);

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/peg/NonCustodialPSM.sol#L290

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

        underlyingToken.safeTransferFrom(
            msg.sender,
            address(pcvDeposit),
            amountIn
        );

        uint256 amountFeiToTransfer = Math.min(
            volt().balanceOf(address(this)),
            amountVoltOut
        );
        uint256 amountFeiToMint = amountVoltOut - amountFeiToTransfer;

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/utils/MultiRateLimited.sol#L341

require(amount <= newBuffer, "MultiRateLimited: rate limit hit");

        rateLimitPerAddress[rateLimitedAddress].bufferStored = uint112(
            newBuffer - amount
        );

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/utils/RateLimited.sol#L106

require(usedAmount <= newBuffer, "RateLimited: rate limit hit");

        bufferStored = newBuffer - usedAmount;

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/utils/Timed.sol#L46

function remainingTime() public view returns (uint256) {
        return duration - timeSinceStart(); // duration always >= timeSinceStart which is on [0,d]
    }

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/utils/Timed.sol#L57

uint256 timePassed = block.timestamp - startTime; // block timestamp always >= startTime

##Recommended Mitigation Steps Consider using 'unchecked' where it is safe to do so.

3. Use Custom Errors to save Gas

Impact

Custom errors are cheaper than revert strings.

Proof of Concept

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

Recommended Mitigation Steps

Replace revert strings with custom errors.

4. Upgrade pragma to at least 0.8.4

Impact

Using newer compiler versions and the optimizer gives gas optimizations and additional safety checks are available for free. The advantages of versions 0.8.* over <0.8.0 are: • Safemath by default from 0.8.0 (can be more gas efficient than library based safemath.) • Low level inliner : from 0.8.2, leads to cheaper runtime gas. Especially relevant when the contract has small functions. For example, OpenZeppelin libraries typically have a lot of small helper functions and if they are not inlined, they cost an additional 20 to 40 gas because of 2 extra jump instructions and additional stack operations needed for function calls. • Optimizer improvements in packed structs: Before 0.8.3, storing packed structs, in some cases used an additional storage read operation. After EIP-2929, if the slot was already cold, this means unnecessary stack operations and extra deploy time costs. However, if the slot was already warm, this means additional cost of 100 gas alongside the same unnecessary stack operations and extra deploy time costs. • Custom errors from 0.8.4, leads to cheaper deploy time cost and run time cost. Note: the run time cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors. https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/pcv/compound/CompoundPCVDepositBase.sol#L2

Recommended Mitigation Steps

Consider to upgrade pragma to at least 0.8.4.

5. Avoid use of state variables in event emissions to save gas

Impact

Where possible, use equivalent function parameters or local variables in event emits instead of state variables to prevent expensive SLOADs. Post-Berlin, SLOADs on state variables accessed first-time in a transaction increased from 800 gas to 2100, which is a 2.5x increase.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/oracle/OraclePassThrough.sol#L58

function updateScalingPriceOracle(IScalingPriceOracle newScalingPriceOracle)
        external
        override
        onlyOwner
    {
        IScalingPriceOracle oldScalingPriceOracle = scalingPriceOracle;
        scalingPriceOracle = newScalingPriceOracle;

        emit ScalingPriceOracleUpdate(
            oldScalingPriceOracle,
            newScalingPriceOracle
        );
    }

6. Use of constant keccak variables results in extra hashing (and so gas).

Impact Increase gas costs on all onlyAdmin operations Proof of Concept This variables are marked as constant:

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/core/Permissions.sol#L10-L15 This results in the keccak operation being performed whenever the variable is used, increasing gas costs relative to just storing the output hash. Changing to immutable will only perform hashing on contract deployment which will save gas. See: ethereum/solidity#9232 (comment) Recommended Mitigation Steps Change the variable to be immutable rather than constant

7. Free gas savings for using solidity 0.8.10+

Impact

Gas savings

Proof of Concept

Solidity 0.8.10 has a useful change which reduced gas costs of external calls which expect a return value: https://blog.soliditylang.org/2021/11/09/solidity-0.8.10-release-announcement/

Code Generator: Skip existence check for external contract if return data is expected. In this case, the ABI decoder will revert if the contract does not exist

LIFI protocol is using 0.8.6: Updating to the newer version of solc will allow contracts to take advantage of these lower costs for external calls.

Recommended Mitigation Steps

Update to 0.8.10+

8. Using require instead of && can save gas

Impact Instead of using operator && on single require check using double require check can save more gas: Proof of Concept https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/volt/Volt.sol#L90-L93

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

Recommended Mitigation Steps Change to:

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

9. Less than 256 uints are not gas efficient

Impact

Lower than uint256 size storage instance variables are actually less gas efficient. E.g. using uint32 does not give any efficiency, actually, it is the opposite as EVM operates on default of 256-bit values so uint32 is more expensive in this case as it needs a conversion. It only gives improvements in cases where you can pack variables together, e.g. structs.

Proof of Concept

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/oracle/ScalingPriceOracle.sol#L41-L44

Recommended Mitigation Steps

Consider to review all uint types. Change them with uint256 If the integer is not necessary to present with uint32.

10. Change encode to encodePacked

Impact In the Volt.sol, change abi.encode to abi.encodePacked can save gas. https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/volt/Volt.sol#L26 https://github.com/ConnorBlockchain/Solidity-Encode-Gas-Comparison

11. Avoid unnecessary code execution can save gas

Impact

function timeSinceStart() public view returns (uint256) {
        if (!isTimeStarted()) {
            return 0; // uninitialized
        }
        uint256 _duration = duration;
        uint256 timePassed = block.timestamp - startTime; 
        return timePassed > _duration ? _duration : timePassed;
    }

Change to:

function timeSinceStart() public view returns (uint256) {
        if (isTimeStarted()) {
        uint256 _duration = duration;
        uint256 timePassed = block.timestamp - startTime; 
        return timePassed > _duration ? _duration : timePassed;
    }

12. Remove redundant _setRoleAdmin() can save gas

Vulnerability details

constructor() {
        // Appointed as a governor so guardian can have indirect access to revoke ability
        _setupGovernor(address(this));

        _setRoleAdmin(MINTER_ROLE, GOVERN_ROLE);
        _setRoleAdmin(BURNER_ROLE, GOVERN_ROLE);
        _setRoleAdmin(PCV_CONTROLLER_ROLE, GOVERN_ROLE);
        _setRoleAdmin(GOVERN_ROLE, GOVERN_ROLE);
        _setRoleAdmin(GUARDIAN_ROLE, GOVERN_ROLE);
    }

https://github.com/code-423n4/2022-03-volt/blob/f1210bf3151095e4d371c9e9d7682d9031860bbd/contracts/core/Permissions.sol#L17

Impact

By default, the admin role for all roles is GOVERN_ROLE. Therefore, _setRoleAdmin(***_ROLE, GOVERN_ROLE); is redundant. Removing it will make the code simpler and save some gas. https://github.com/OpenZeppelin/openzeppelin-contracts/blob/783ac759a902a7b4a218c2d026a77e6a26b6c42d/contracts/access/AccessControl.sol#L40-L43

• By default, the admin role for all roles is DEFAULT_ADMIN_ROLE, which means
• that only accounts with this role will be able to grant or revoke other
• roles. More complex role relationships can be created by using
• {_setRoleAdmin}. https://docs.openzeppelin.com/contracts/3.x/access-control#granting-and-revoking «AccessControl includes a special role, called DEFAULT_ADMIN_ROLE, which acts as the default admin role for all roles. An account with this role will be able to manage any other role, unless _setRoleAdmin is used to select a new admin role.»

Recommendation

Remove the redundant code.