Y2k Finance contest - __141345__'s results

FOR-LOOP LENGTH COULD BE CACHED

The for loop length can be cached to memory before the loop, even for memory variables. The demo of the loop gas comparison can be seen here.

src/Vault.sol
443:        for (uint256 i = 0; i < epochsLength(); i++) {

Suggestion: Cache the length before the loop.

    uint length = names.length;

FOR-LOOP unchecked{++i} COSTS LESS GAS COMPARED TO i++ OR i += 1

Use ++i instead of i++ to increment the value of an uint variable, and for guaranteed non-overflow/underflow, it can be unchecked. And the optimizer need to be turned on.

The demo of the loop gas comparison can be seen here.

src/Vault.sol
443:        for (uint256 i = 0; i < epochsLength(); i++) {

Suggestion: For readability, uncheck the whole for loop is the same.

        unchecked{
                for (uint256 i; i < length; ++i) {
                }
        }

NO NEED TO EXPLICITLY INITIALIZE VARIABLES WITH DEFAULT VALUES

If a variable is not set/initialized, it is assumed to have the default value (0 for uint, false for bool, address(0) for address…). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

src/Vault.sol
443:    for (uint256 i = 0; i < epochsLength(); i++) {

src/VaultFactory.sol
159:    marketIndex = 0;

src/rewards/StakingRewards.sol
36:     uint256 public periodFinish = 0;

The demo of the loop gas comparison can be seen here.

X = X + Y / X = X - Y IS CHEAPER THAN X += Y / X -= Y

The demo of the gas comparison can be seen here.

Consider use X = X + Y / X = X - Y to save gas.

src/VaultFactory.sol
195:    marketIndex += 1;

!= 0 uses less gas than > 0

!= 0 costs less gas compared to > 0 for unsigned integers with the optimizer enabled (6 gas). Here is the code as a demo. Opcode discussion.

src/Vault.sol
187:    require(msg.value > 0, "ZeroValue");

src/oracles/PegOracle.sol
98:     require(price1 > 0, "Chainlink price <= 0");
121:    require(price2 > 0, "Chainlink price <= 0");

src/rewards/StakingRewards.sol
119:    require(amount > 0, "Cannot withdraw 0");
134:    if (reward > 0) {

Suggestion:

• Changing > 0 with != 0.
• Enable the Optimizer.

REDUCE THE SIZE OF ERROR MESSAGES

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.

src/SemiFungibleVault.sol
116-119:
        require(
            msg.sender == owner || isApprovedForAll(owner, receiver),
            "Only owner can withdraw, or owner has approved receiver for all"
        );

src/oracles/PegOracle.sol
23-24:
        require(_oracle1 != address(0), "oracle1 cannot be the zero address");
        require(_oracle2 != address(0), "oracle2 cannot be the zero address");

src/rewards/StakingRewards.sol
217-220:
        require(
            tokenAddress != address(stakingToken),
            "Cannot withdraw the staking token"
        );
226-229:
        require(
            block.timestamp > periodFinish,
            "Previous rewards period must be complete before changing the duration for the new period"
        );

Suggestion: Shortening the revert strings to fit in 32 bytes, or using custom errors as described next.

USE CUSTOM ERRORS INSTEAD OF REVERT STRINGS

Custom errors from Solidity 0.8.4 are more gas efficient than revert strings (lower deployment cost and runtime cost when the revert condition is met) (reference)

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

The demo of the gas comparison can be seen here.

The following files have multiple require() statements can use custom errors:

src/SemiFungibleVault.sol
91:     require((shares = previewDeposit(id, assets)) != 0, "ZERO_SHARES");
116-119:
        require(
            msg.sender == owner || isApprovedForAll(owner, receiver),
            "Only owner can withdraw, or owner has approved receiver for all"
        );

src/Vault.sol
165:    require((shares = previewDeposit(id, assets)) != 0, "ZeroValue");
187:    require(msg.value > 0, "ZeroValue");

src/oracles/PegOracle.sol
23-30:
        require(_oracle1 != address(0), "oracle1 cannot be the zero address");
        require(_oracle2 != address(0), "oracle2 cannot be the zero address");
        require(_oracle1 != _oracle2, "Cannot be same Oracle");

        require(
            (priceFeed1.decimals() == priceFeed2.decimals()),
            "Decimals must be the same"
        );
98-103:
        require(price1 > 0, "Chainlink price <= 0");
        require(
            answeredInRound1 >= roundID1,
            "RoundID from Oracle is outdated!"
        );
        require(timeStamp1 != 0, "Timestamp == 0 !");
121-126:
        require(price2 > 0, "Chainlink price <= 0");
        require(
            answeredInRound2 >= roundID2,
            "RoundID from Oracle is outdated!"
        );
        require(timeStamp2 != 0, "Timestamp == 0 !");

src/rewards/StakingRewards.sol
96:     require(amount != 0, "Cannot stake 0");
119:    require(amount > 0, "Cannot withdraw 0");
220-205:
        require(
            rewardRate <= balance.div(rewardsDuration),
            "Provided reward too high"
        );
217-220:
        require(
            tokenAddress != address(stakingToken),
            "Cannot withdraw the staking token"
        );
226-229:
        require(
            block.timestamp > periodFinish,
            "Previous rewards period must be complete before changing the duration for the new period"
        );

Some frequently called numbers can be save into a constant

The expression is re-calculated each time the constant is referenced.

src/Vault.sol
388-389:
        keccak256(abi.encodePacked(symbol)) ==
        keccak256(abi.encodePacked("rY2K"))

The consequence is: each usage costs ~100gas more on each access (it is still a little better than storing the result in storage, but not much..)

Suggestion: 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.

reference: https://github.com/ethereum/solidity/issues/9232

MAKING SOME CONSTANTS/IMMUTABLE AS NON-PUBLIC

Changing the visibility from public to private or internal can save gas when a constant isn’t used outside of its contract.

Savings are due to the compiler not having to create non-payable getter functions for deployment calldata, and not adding another entry to the method ID table.

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

A cheap way to store constants in a single location is to create an internal constant in a library. If the variable is a local cache of another contract’s value, consider making the cache variable internal or private, which will require external users to query the contract with the source of truth, so that callers don’t get out of sync.

The followings can be changed from public to internal or private:

src/Controller.sol
    uint256 public constant VAULTS_LENGTH = 2;

Use bytes32 instead of string

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

Bytes32 can be used instead of string in the following:

src/VaultFactory.sol
199-217:
        Vault hedge = new Vault(
            WETH,
            string(abi.encodePacked(_name,"HEDGE")),
            "hY2K",
            treasury,
            _token,
            _strikePrice,
            controller
        );

        Vault risk = new Vault(
            WETH,
            string(abi.encodePacked(_name,"RISK")),
            "rY2K",
            treasury,
            _token,
            _strikePrice,
            controller
        );

BOOLEAN COMPARISON

Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value.

The demo of the gas comparison can be seen here.

Suggestion: Use if (value) / if (!value) instead of if (value == true) / if (value == false)

in the following:

src/Vault.sol
80:     if(idExists[id] != true)
96:     if((block.timestamp < id) && idDepegged[id] == false)
215-217:
        if(
            msg.sender != owner &&
            isApprovedForAll(owner, receiver) == false)
314:    if(idExists[epochEnd] == true)

src/Controller.sol
93:     if(vault.idExists(epochEnd) == false)
211:    if(insrVault.idExists(epochEnd) == false || riskVault.idExists(epochEnd) == false)

src/rewards/RewardsFactory.sol
96:     if(Vault(_insrToken).idExists(_epochEnd) == false || Vault(_riskToken).idExists(_epochEnd) == false)

MULTIPLE ADDRESS MAPPINGS CAN BE COMBINED INTO A SINGLE MAPPING OF AN ADDRESS TO A STRUCT WHERE APPROPRIATE

Saves a storage slot for the mapping. Depending on the circumstances and sizes of types, can avoid a Gsset (20000 gas) per mapping combined. Reads and subsequent writes can also be cheaper when a function requires both values and they both fit in the same storage slot.

src/Vault.sol
47-55:
    mapping(uint256 => uint256) public idFinalTVL;
    mapping(uint256 => uint256) public idClaimTVL;

    mapping(uint256 => uint256) public idEpochBegin;

    mapping(uint256 => bool) public idDepegged;

    mapping(uint256 => bool) public idExists;
    mapping(uint256 => uint256) public epochFee;

src/VaultFactory.sol
119-120:
    mapping(uint256 => address[]) public indexVaults; //[0] hedge and [1] risk vault
    mapping(uint256 => uint256[]) public indexEpochs; //all epochs in the market

src/rewards/StakingRewards.sol
43-47:
    mapping(address => uint256) public userRewardPerTokenPaid;
    mapping(address => uint256) public rewards;

    mapping(address => uint256) private _balances;

Unnecessary vault share calculation

Since the share is 1:1 ratio with the deposit, there is no need to call previewDeposit():

// src/Vault.sol
165:    require((shares = previewDeposit(id, assets)) != 0, "ZeroValue");

Duplicate call to totalAssets(id)

totalAssets(id) call is duplicate, it can be called before and pass the result.

// src/Controller.sol
220:    insrVault.endEpoch(epochEnd, false);
221:    riskVault.endEpoch(epochEnd, false);

// src/Vault.sol
    function endEpoch(uint256 id, bool depeg)
        public
        onlyController
        marketExists(id)
    {
        idDepegged[id] = depeg;
        idFinalTVL[id] = totalAssets(id);
    }

Duplicate call to getLatestPrice()

In triggerDepeg(), the call to getLatestPrice() is duplicate, including in the modifier. The result can be saved in a local variable, instead of multiple gas intensive calls.

// src/Controller.sol

    function triggerDepeg(uint256 marketIndex, uint256 epochEnd)
        public
        isDisaster(marketIndex, epochEnd)
    {
        // ...
        emit DepegInsurance(...,
            getLatestPrice(insrVault.tokenInsured())
        );
    }

    modifier isDisaster(uint256 marketIndex, uint256 epochEnd) {
        // ...
        if(
            vault.strikePrice() < getLatestPrice(vault.tokenInsured())
            )
            revert PriceNotAtStrikePrice(getLatestPrice(vault.tokenInsured()));
        // ...
    }

Recycle ended vaults for gas refund

The ended epoch or settled epoch with depeg can delete the vault storage to get some gas refund.