From 14ff7a02c41c33834027e02244908ac8dccdf6e0 Mon Sep 17 00:00:00 2001
From: C4 <81770958+code423n4@users.noreply.github.com>
Date: Mon, 19 Sep 2022 11:17:28 +0200
Subject: [PATCH] __141345__ data for issue #257

---
 data/__141345__-G.md | 363 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 363 insertions(+)
 create mode 100644 data/__141345__-G.md

diff --git a/data/__141345__-G.md b/data/__141345__-G.md
new file mode 100644
index 0000000..1e9e340
--- /dev/null
+++ b/data/__141345__-G.md
@@ -0,0 +1,363 @@
+
+#### 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](https://github.com/Flame57/gas_demo/blob/main/loop.sol).
+
+
+```solidity
+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](https://github.com/Flame57/gas_demo/blob/main/loop.sol).
+
+
+```solidity
+src/Vault.sol
+443:        for (uint256 i = 0; i < epochsLength(); i++) {
+```
+
+Suggestion:
+For readability, uncheck the whole for loop is the same.
+
+```solidity
+        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.
+
+
+```solidity
+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](https://github.com/Flame57/gas_demo/blob/main/loop.sol).
+
+
+#### `X = X + Y / X = X - Y` IS CHEAPER THAN `X += Y / X -= Y`
+
+The demo of the gas comparison can be seen [here](https://github.com/Flame57/gas_demo/blob/main/gas_compare.sol).
+
+Consider use `X = X + Y / X = X - Y` to save gas.
+
+
+```solidity
+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](https://github.com/Flame57/gas_demo/blob/main/ltgt0.sol).
+[Opcode discussion](https://twitter.com/gzeon/status/1485428085885640706).
+
+```solidity
+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.
+
+
+```solidity
+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](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).
+
+The demo of the gas comparison can be seen [here](https://github.com/Flame57/gas_demo/blob/main/errrevert.sol).
+
+
+The following files have multiple `require()` statements can use custom errors:
+
+```solidity
+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.
+
+```solidity
+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:
+
+```solidity
+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:
+
+```solidity
+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](https://github.com/Flame57/gas_demo/blob/main/boolean.sol).
+
+
+Suggestion:
+Use 
+`if (value) / if (!value)` 
+instead of 
+`if (value == true) / if (value == false)`
+
+in the following:
+```solidity
+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.
+
+```solidity
+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;
+```
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+
+