Platform: Code4rena
Start Date: 14/09/2022
Pot Size: $50,000 USDC
Total HM: 25
Participants: 110
Period: 5 days
Judge: hickuphh3
Total Solo HM: 9
Id: 162
League: ETH
Rank: 29/110
Findings: 3
Award: $245.01
🌟 Selected for report: 0
🚀 Solo Findings: 0
🌟 Selected for report: Lambda
Also found by: Deivitto, R2, Rolezn, csanuragjain
155.5605 USDC - $155.56
There are ERC20 tokens that charge fee for every transfer().
Vault.sol#depositETH() assumes that the received amount is the same as the transfer amount, and uses it to make a deposit, while the actual transferred amount can be lower if asset is an ERC20 token with charge fee.
function depositETH(uint256 id, address receiver) external payable returns (uint256 shares) { require(msg.value > 0, "ZeroValue"); IWETH(address(asset)).deposit{value: msg.value}(); assert(IWETH(address(asset)).transfer(msg.sender, msg.value)); return deposit(id, msg.value, receiver);//@audit wrong deposit value if erc20 fee }
Consider comparing before and after balance of asset to get the actual transferred amount.
#0 - HickupHH3
2022-10-31T14:18:19Z
partial credit because Vault uses WETH only. It however applies to SemiFungibleVault.
#1 - HickupHH3
2022-10-31T14:18:24Z
dup #221
🌟 Selected for report: Respx
Also found by: 0x1f8b, 0xDecorativePineapple, 0xNazgul, 0xPanas, 0xSmartContract, 0xc0ffEE, 0xmuxyz, Aymen0909, Bahurum, Bnke0x0, CodingNameKiki, Deivitto, Jeiwan, Lambda, Picodes, PwnPatrol, R2, RaymondFam, Rolezn, Ruhum, Saintcode_, SooYa, Tointer, V_B, ajtra, ak1, async, auditor0517, brgltd, c3phas, carrotsmuggler, cccz, csanuragjain, datapunk, djxploit, durianSausage, eierina, erictee, gogo, imare, joestakey, jonatascm, kv, ladboy233, leosathya, lukris02, oyc_109, pashov, pauliax, rbserver, robee, rokinot, rvierdiiev, scaraven, simon135, unforgiven, wagmi, zzzitron
36.6223 USDC - $36.62
Divisions by 0 will revert the code. In PegOracle.sol, price1 and price 2 can be 0 if for example oracle fails.
That will cause to revert on latestRoundData. Oracle price1 and price2 variable should be checked previously to the division and emit some event if something goes wrong
Navigate to the following contracts,
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L68 nowPrice = (price2 * 10000) / price1;//@audit revert div if 0
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L70 nowPrice = (price1 * 10000) / price2;//@audit revert div if 0
If oracle fails, variable will be equal to zero therefore div by zero will occur.
Recommend making sure division by 0 won’t occur by checking the variables beforehand and handling this edge case.
address state or immutable variablesZero address should be checked for state variables, immutable variables. A zero address can lead into problems.
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L70-L72 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L68-L70 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L81-L83
Check zero address before assigning or using it
changeTreasury()Zero address should be checked for some function parameters.
treasury should be checked that is not the zero address while assigned, even if this is later checked in Vault.sol, code can use unnecesary gas for assigning tresuary value to 0 and then realize of the error and change it again.
Check zero address before assigning it
Risk of using block.timestamp for time should be considered.
block.timestamp is not an ideal proxy for time because of issues with synchronization, miner manipulation and changing block times.
This kind of issue may affect the code allowing or reverting the code before the expected deadline, modifying the normal functioning or reverting sometimes.
SWC ID: 116
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L198-L248 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L261-L312 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L155-L157 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L183-L210 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L225-L232
Consider using an oracle for precision Consider the risk of using block.timestamp as time proxy and evaluate if block numbers can be used as an approximation for the application logic. Both have risks that need to be factored in.
From solidity docs: Properly functioning code should never reach a failing assert statement; if this happens there is a bug in your contract which you should fix. With assert the user pays the gas and with require it doesn't. The ETH network gas isn't cheap and users can see it as a scam. You have reachable asserts in the following locations (which should be replaced by require / are mistakenly left from development phase):
The Solidity assert() function is meant to assert invariants. Properly functioning code should never reach a failing assert statement. A reachable assertion can mean one of two things:
A bug exists in the contract that allows it to enter an invalid state; The assert statement is used incorrectly, e.g. to validate inputs.
SWC ID: 110
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L190 assert(IWETH(address(asset)).transfer(msg.sender, msg.value));
Substitute asserts with require/revert.
Events are important for critical parameter change.
Next functions should emit events for the core addresses changes
Vault.changeController(address)Public external functions where money / tokens are transferred / should emit some type of event for better monitoring
Methods expected to be only called by admins should indeed include the modifier as this can lead to critical damages.
Actually this methods is not expected to be called outside VaultFactory functions that include by now the modifier, however, if changed the code or called from another contract, this can lead to unexpected behaviors.
Functions guaranteed to revert when called by normal users can be marked payableThere are a number of instances where a boolean variable/function is checked.
variable == false to !variable.variable == true to variable.https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L314 if(idExists[epochEnd] == true)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L93 if(vault.idExists(epochEnd) == false)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L211 if(insrVault.idExists(epochEnd) == false || riskVault.idExists(epochEnd) == false)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L96 if((block.timestamp < id) && idDepegged[id] == false)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L217 isApprovedForAll(owner, receiver) == false)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L96 if(Vault(_insrToken).idExists(_epochEnd) == false || Vault(_riskToken).idExists(_epochEnd) == false)
Simplify boolean comparisons in order to improve readability and save gas
Magic numbers are hardcoded numbers used in the code which are ambiguous to their intended purpose. These should be replaced with constants to make code more readable and maintainable.
Values are hardcoded and would be more readable and maintainable if declared as a constant
1000000, 10000, 1000 are hardcoded, are so similar and can potentially include a typo.
1e18, 150, 1 ether are hardcoded and would be more legible if saved in constant
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L311 if(_withdrawalFee > 150) https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L78 nowPrice / 1000000, https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L68 nowPrice = (price2 * 10000) / price1;
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L70 nowPrice = (price1 * 10000) / price2;
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L266 return (amount * epochFee[_epoch]) / 1000;
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L177 .div(1e18) https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L400-L421 1 ether
Replace magic hardcoded numbers with declared constants. Define constants for the numbers used throughout the code. Comment what these numbers are intended for
Events without indexed event parameters make it harder and
inefficient for off-chain tools to analyze them.
Indexed parameters (“topics”) are searchable event parameters. They are stored separately from unindexed event parameters in an efficient manner to allow for faster access. This is useful for efficient off-chain-analysis, but it is also more costly gas-wise.
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L49-L56 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L51 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L55-L56
Has so many parameters do only have 1 indexed https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L49-L56
Has so many parameters do only have 2 indexed https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L69-L80
Consider which event parameters could be particularly useful to off-chain tools and should be indexed.
Missing Natspec and regular comments affect readability and maintainability of a codebase.
Contracts has partial or full lack of comments
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L428-L432 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L314-L320 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L134-L228
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L268-L289 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L276-L286
There are 2 instances of misleading comments.
// Ensure the provided reward amount is not more than the balance in the contract. // This keeps the reward rate in the right range, preventing overflows due to // very high values of rewardRate in the earned and rewardsPerToken functions; // Reward + leftover must be less than 2^256 / 10^18 to avoid overflow.
However, since pragma 0.8.0 overflow is by default controlled. Also there is not a unchecked operation there what would explain why the comment about overflow is there. Also, operations involved as SafeMath is there for uint values will revert if overflow too
// 0.5% = multiply by 1000 then divide by 5
However, this assertion is not true, resulting value is 200. Also the code is not implementing what the comment says but what I think is really expectedhttps://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L265 // 0.5% = multiply by 1000 then divide by 5//@audit misleading comment https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L197-L200 // Ensure the provided reward amount is not more than the balance in the contract. // This keeps the reward rate in the right range, preventing overflows due to//@audit overflows // very high values of rewardRate in the earned and rewardsPerToken functions; // Reward + leftover must be less than 2^256 / 10^18 to avoid overflow.
Contract is missing to inherit its dedicated interface
Controller is not inheriting from IController
Add the inheritance to the contract
There is some code that is commented out. It could point to items that are not done or need redesigning, be a mistake, or just be testing overhead.
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L25 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L393-L398
Review and remove or resolve/document the commented out lines if needed.
Code architecture, incentives, and error handling/reporting questions/issues should be resolved before deployment
The code includes a TODO already done that affects readability and focus on the readers/auditors of the contracts
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L196 @notice Withdraw entitled deposited assets, checking if a depeg event //TODO add GOV token rewards
Remove already done TODO
Some lines use // x and some use //x. The instances below point out the usages that don't follow the recommended // x style, within each file
//Taking fee from the amount
But following the style of the other comments would be:
// Taking fee from the amount
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L225 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L406 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L119-L121 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L197 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L25 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L27
In SemiFungibleVault.sol, contract is ERC1155Supply, what is ERC1155.
However, SemiFungibleVault.sol also uses a explicit ERC1155 constructor, so at the end that constructor is being called twice.
Remove is IERC721.
Names being so similar may lead to unexpected calls and assumptions.
Consider making more unique the names of this variables/functions
The message of the require occurs when timestamp == 0, however, a ! is included at the end of the message, this can lead to wrong assumptions as:
! is used for making a bool the opposite, this would mean the message to be: this happens when timestamp is not 0.! is used for what grammatically is used for in english, there shouldn't be a whitespace between condition and exclamation and for clarity, timestamp == 0 should be separate from it with some kind of symbol as (). If the point is to get attention when monitoring, consider just adding some text in caps like ALERT or WARNING.https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L126 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L103 require(timeStamp1 != 0, "Timestamp == 0 !");
Change Timestamp == 0 ! to ALERT:Timestamp == 0 or similar changes
Long lines should be wrapped to conform with Solidity Style guidelines.
Lines that exceed the 79 (or 99) character length suggested by the Solidity Style guidelines. Reference: https://docs.soliditylang.org/en/v0.8.10/style-guide.html#maximum-line-length
Reduce line length to less than 99 at least to improve maintainability and readability of the code
Multiples of 10 can be declared as constants with scientific notation so it's easier to read them and less prone to miss/exceed a 0 of the expected value.
Values 1000000, 10000, 1000 can be used in scientific notation
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L78 nowPrice / 1000000, https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L68 nowPrice = (price2 * 10000) / price1;
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L70 nowPrice = (price1 * 10000) / price2;
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L266 return (amount * epochFee[_epoch]) / 1000;
Replace hardcoded numbers with constants that represent the scientific corresponding notation
In this case we find that a bool values are saved into a variable to only being used in next condition.
This is actually developed as:
// Answer == 0: Sequencer is up // Answer == 1: Sequencer is down bool isSequencerUp = answer == 0; if (!isSequencerUp) { revert SequencerDown(); }
and can be converted to:
if (answer != 0) { revert SequencerDown(); }
Consider reducing the statement
Saving values to be used just once can be confusing as when a variable is defined, readers expect to found that variable at least twice. However, this variables are declared but they can be inlined.
} else { uint256 remaining = periodFinish.sub(block.timestamp); uint256 leftover = remaining.mul(rewardRate); rewardRate = reward.add(leftover).div(rewardsDuration); }
and can be
} else { rewardRate = reward.add( (periodFinish.sub(block.timestamp)).mul(rewardRate) ).div(rewardsDuration); }
Consider removing unnecessary variables
Modifier Controller.onlyAdmin is defined but not used. This affects admin variable that is only used here.
Expected admin settings https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L250-L252
Remove or use it
🌟 Selected for report: pfapostol
Also found by: 0x040, 0x1f8b, 0x4non, 0xNazgul, 0xSmartContract, 0xc0ffEE, 0xkatana, Aymen0909, Bnke0x0, Deivitto, Diana, JAGADESH, KIntern_NA, Lambda, MiloTruck, R2, RaymondFam, Respx, ReyAdmirado, Rohan16, RoiEvenHaim, Rolezn, Ruhum, Saintcode_, Samatak, Sm4rty, SnowMan, Tomio, Tomo, WilliamAmbrozic, _Adam, __141345__, ajtra, ak1, async, c3phas, ch0bu, cryptostellar5, d3e4, delfin454000, dharma09, djxploit, durianSausage, eierina, erictee, fatherOfBlocks, gianganhnguyen, gogo, ignacio, imare, jag, jonatascm, leosathya, lukris02, malinariy, oyc_109, pashov, pauliax, peanuts, peiw, prasantgupta52, robee, rokinot, rotcivegaf, rvierdiiev, seyni, simon135, slowmoses, sryysryy, tnevler, zishansami
52.8286 USDC - $52.83
Both functions can be simplified into just one function, with address parametrized. Also this can be achieved creating an internal function that runs with the logic and keeping the public functions such as:
function getOracle1_Price() public view returns (int256 price) { price = _getOracle_Price(priceFeed1); } function getOracle2_Price() public view returns (int256 price) { price = _getOracle_Price(priceFeed2); }
and _getOracle_Price such as
function _getOracle_Price(address priceFeed) internal view returns (int256 price) { ( uint80 roundID, int256 price, , uint256 timeStamp, uint80 answeredInRound ) = priceFeed.latestRoundData(); require(price > 0, "Chainlink price <= 0"); require( answeredInRound >= roundID, "RoundID from Oracle is outdated!" ); require(timeStamp != 0, "Timestamp == 0 !"); return price;
Consider using parameters rather than deployment the same system twice
SafeMath once the solidity version is 0.8.0 or greaterVersion 0.8.0 introduces internal overflow checks, so using SafeMath is redundant and adds overhead
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L4 import {SafeMath} from "@openzeppelin/contracts/utils/math/SafeMath.sol";
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L29 using SafeMath for uint256;//@audit safemath after 0.8.0
Remove SafeMath for gas optimization
Functions should have the strictest visibility possible. Public functions may lead to more gas usage by forcing the copy of their parameters to memory from calldata.
If a function is never called from the contract it should be marked as external. This will save gas.
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L350-L352 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L287-L289 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L307-L325 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L295-L299 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L438-L451 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L336-L343 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L277-L281 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L360-L366 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L327-L338 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L345-L359 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L178-L239 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L366-L374 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L248-L266 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L385-L391 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L295-L301 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L308-L320 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L189-L198 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L237-L239 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L251-L258 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L244-L246 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L221-L228 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L263-L270 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L145-L151 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L89-L106 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L46-L83 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L198-L248 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L148-L192
Consider changing visibility from public to external.
Custom errors reduce 38 gas if the condition is met and 22 gas otherwise. Also reduces contract size and deployment costs.
Since version 0.8.4 the use of custom errors rather than revert() / require() saves gas as noticed in https://blog.soliditylang.org/2021/04/21/custom-errors/ https://github.com/code-423n4/2022-04-pooltogether-findings/issues/13
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L91 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L116 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L165 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L187 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L23 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L24 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L25 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L28 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L98 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L99 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L103 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L121 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L122 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L126 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L96 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L119 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L202 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L217 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L226
replace each error message in a require by a custom error
duplicated require() / revert() checks should be refactored to a modifier or function to save gas
Event appears twice and can be reduced
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L165 require((shares = previewDeposit(id, assets)) != 0, "ZeroValue");
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L187 require(msg.value > 0, "ZeroValue");
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L23 require(_oracle1 != address(0), "oracle1 cannot be the zero address");
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L24 require(_oracle2 != address(0), "oracle2 cannot be the zero address");
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L126 require(timeStamp2 != 0, "Timestamp == 0 !"); https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L103 require(timeStamp1 != 0, "Timestamp == 0 !");
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L98 require(price1 > 0, "Chainlink price <= 0"); https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L121 require(price2 > 0, "Chainlink price <= 0");
refactor this checks to different functions to save gas
When the optimizer is enabled, gas is wasted by doing a greater-than operation, rather than a not-equals operation inside require() statements. When Using != , the optimizer is able to avoid the EQ, ISZERO, and associated operations, by relying on the JUMPI that comes afterwards, which itself checks for zero.
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L119 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L187
Use != 0 rather than > 0 for unsigned integers in require() statements.
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.
Consider shortening the revert strings to fit in 32 bytes
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 than downcast where needed
Consider using some data type that uses 32 bytes, for example uint256
All these variables could be combine in a Struct in order to reduce the gas cost.
As noticed in: https://gist.github.com/alexon1234/b101e3ac51bea3cbd9cf06f80eaa5bc2 When multiple mappings that access the same addresses, uints, etc, all of them can be mixed into an struct and then that data accessed like: mapping(datatype => newStructCreated) newStructMap; Also, you have this post where it explains the benefits of using Structs over mappings https://medium.com/@novablitz/storing-structs-is-costing-you-gas-774da988895e
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L43-L44 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L47
Consider mixing different mappings into an struct when able in order to save gas.
Gas efficiency can be achieved by tightly packing the struct. Struct variables are stored in 32 bytes each so you can group smaller types to occupy less storage.
You can read more here: https://fravoll.github.io/solidity-patterns/tight_variable_packing.html or in the official documentation: https://docs.soliditylang.org/en/v0.4.21/miscellaneous.html
Order structs to reduce gas usage.
State variables are expected to be ordered by data type, this helps readability and also gas optimization by tightly packing the variables.
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L36-L46 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L34-L38
Order in a proper way the state variables to improve readability and to reduce gas usage.
If needed, the value can be read from the verified contract source code. 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
Consider replacing public for private in constants for gas saving.
It is not needed to initialize variables to the default value. Explicitly initializing it with its default value is an anti-pattern and wastes gas.
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…).
Don't initialize variables to default value
In for loops is not needed to initialize indexes to 0 as it is the default uint/int value. This saves gas.
Don't initialize variables to default value
++i costs less gas than i++, especially when it's used in for loops as pre-increment is cheaper (about 5 gas per iteration).
using ++i doesn't affect the flow of regular for loops and improves gas cost
Substitute to ++i
++i costs less gas compared to i++ or i += 1 for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.
i++ increments i and returns the initial value of i . Which means: uint i = 1; i++; // == 1 but i == 2
But ++i returns the actual incremented value:
uint i = 1; ++i; // == 2 and i == 2 too, so no need for a temporary variable
In the first case, the compiler has to create a temporary variable (when used) for returning 1 instead of 2
Replace to ++i.
Unchecked operations as the ++i on for loops are cheaper than checked one.
In Solidity 0.8+, there’s a default overflow check on unsigned integers. It’s possible to uncheck this in for-loops and save some gas at each iteration, but at the cost of some code readability, as this uncheck cannot be made inline..
The code would go from: for (uint256 i; i < numIterations; i++) { // ... } to for (uint256 i; i < numIterations;) { // ... unchecked { ++i; } } The risk of overflow is inexistent for a uint256 here.
Add unchecked ++i at the end of all the for loop where it's not expected to overflow and remove them from the for header
In loops not assigning the length to a variable so memory accessed a lot (caching local variables)
The overheads outlined below are PER LOOP, excluding the first loop storage arrays incur a Gwarmaccess (100 gas) memory arrays use MLOAD (3 gas) calldata arrays use CALLDATALOAD (3 gas)
Assign the length of the array.length to a local variable in loops for gas savings
Variables read more than once improves gas usage when cached into local variable
In loops or state variables, this is even more gas saving
id https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L125-L129 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L102-L106
asset https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L227 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L189-L190
cast of address(insrStake) and address(riskStake) can be stored in local variables https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L120-L121
admin, govToken https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L100-L111
vaultFactory.getVaults(marketIndex); https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L200-L206
Cache variables used more than one into a local variable.
If a function modifier such as onlyOwner is used, the function will revert if a normal user tries to pay the function.
Marking the function as payable will lower the gas cost for legitimate callers because the compiler will not include checks for whether a payment was provided.
The extra opcodes avoided are: CALLVALUE (2), DUP1 (3), ISZERO (3), PUSH2 (3), JUMPI (10), PUSH1 (3), DUP1 (3), REVERT(0), JUMPDEST (1), POP (2), which costs an average of about 21 gas per call to the function, in addition to the extra deployment cost
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L186 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L253 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L295 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L310 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L329 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L347 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L366 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L85 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L74 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L130 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L53 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L215 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L225 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L216 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/SemiFungibleVault.sol#L117
It's suggested to add payable to functions guaranteed to revert when called by normal users to improve gas costs
Strict inequalities ( > ) are more expensive than non-strict ones ( >= ). This is due to some supplementary checks (ISZERO, 3 gas)
Consider using >= 1 instead of > 0 to avoid some opcodes
x+=y costs more gas than x=x+y for state variables
Don't use += for state variables as it cost more gas.
Using both named returns and a return statement isn’t necessary. Removing one of those can improve code clarity
Also as returns variable is ignored, it wastes extra gas
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L264 returns (int256 nowPrice)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L317 function getVaultFactory() external view returns (address) {
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L162 returns (uint256 shares)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L185 returns (uint256 shares)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L213 returns (uint256 shares)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L263 returns (uint256 feeValue)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L381 returns (uint256 entitledAmount)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Vault.sol#L441 returns (uint256 nextEpochEnd)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L186 ) public onlyAdmin returns (address insr, address rsk) {
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/VaultFactory.sol#L388 returns (address[] memory vaults)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L89 function getOracle1_Price() public view returns (int256 price) {
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/oracles/PegOracle.sol#L112 function getOracle2_Price() public view returns (int256 price) {
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L86 returns (address insr, address risk)
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L148 returns (bytes32 hashedIndex)
Remove return or returns when both used
In general, abi.encodePacked is more gas-efficient.
Changing the abi.encode function to abi.encodePacked can save gas since the abi.encode function pads extra null bytes at the end of the call data, which is unnecessary. Also, in general, abi.encodePacked is more gas-efficient.
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L118 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L150
Consider changing abi.encode to abi.encodePacked
Save ~30 gas per call to each function
epochsLength(); to epochs.length;
Retrieve internal variables directly rather than calling the retrieving function
A value which value is already known can be used directly rather than reading it from the storage
modifier updateReward(address account) { rewardPerTokenStored = rewardPerToken(); lastUpdateTime = lastTimeRewardApplicable(); if (account != address(0)) { rewards[account] = earned(account); userRewardPerTokenPaid[account] = rewardPerTokenStored; } _; }
rewardPerTokenStored is previously calculated with rewardPerToken();, this can be stored in a local variable for not reading again the storage.
Recommendation Change to:
modifier updateReward(address account) { uint _rewardPerToken = rewardPerToken(); rewardPerTokenStored = _rewardPerToken; lastUpdateTime = lastTimeRewardApplicable(); if (account != address(0)) { rewards[account] = earned(account); userRewardPerTokenPaid[account] = _rewardPerToken; } _; }
_rewardsDuration should be used rather than rewardsDuration
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L231Set directly the value to avoid unnecessary storage read to save some gas
State variables which value isn't changed by any function in the contract but constructor, can be declared as a immutable state variable to save some gas during deployment.
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/RewardsFactory.sol#L68-L70 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/Controller.sol#L137
As it follows CEI pattern, even with a reentrancy, this reentrancy won't be harmful. Using nonReentrant in such scenario just consumes extra gas.
https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L92 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L116 https://github.com/code-423n4/2022-09-y2k-finance/blob/bca5080635370424a9fe21fe1aded98345d1f723/src/rewards/StakingRewards.sol#L132
Remove nonReentrant where the code follows CEI pattern.