Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it
To help the optimizer, declare a storage type variable and use it instead of repeatedly fetching the reference in a map or an array.
The effect can be quite significant.
As an example, instead of repeatedly calling someMap[someIndex], save its reference like this: SomeStruct storage someStruct = someMap[someIndex] and use it.
Affected code (check the @audit tags):
contracts/AuraLocker.sol:
177: rewardData[token].rewardPerTokenStored = newRewardPerToken.to96(); //@audit gas: should declare "RewardData storage _rewardData = rewardData[token];" and use it
178: rewardData[token].lastUpdateTime = _lastTimeRewardApplicable(rewardData[token].periodFinish).to32();//@audit gas: should use suggested _rewardData
196: require(rewardData[_rewardsToken].lastUpdateTime == 0, "Reward already exists"); //@audit gas: should declare "RewardData storage _rewardData = rewardData[token];" and use it
199: rewardData[_rewardsToken].lastUpdateTime = uint32(block.timestamp);//@audit gas: should use suggested _rewardData
200: rewardData[_rewardsToken].periodFinish = uint32(block.timestamp);//@audit gas: should use suggested _rewardData
308: uint256 reward = userData[_account][_rewardsToken].rewards; //@audit gas: should declare "UserData storage _userData = userData[_account][_rewardsToken];" and use it
310: userData[_account][_rewardsToken].rewards = 0;//@audit gas: should use suggested _userData
412: if (locks[i].unlockTime > expiryTime) break; //@audit gas: should declare a storage variable for locks[i]
415: locked = locked.add(locks[i].amount); //@audit gas: should use the suggested storage variable for locks[i]
421: uint256 epochsover = currentEpoch.sub(uint256(locks[i].unlockTime)).div(rewardsDuration); //@audit gas: should use the suggested storage variable for locks[i]
423: reward = reward.add(uint256(locks[i].amount).mul(rRate).div(denominator)); //@audit gas: should use the suggested storage variable for locks[i]
697: if (locks[i].unlockTime > block.timestamp) { //@audit gas: should declare a storage variable for locks[i]
701: lockData[idx] = locks[i]; //@audit gas: should use the suggested storage variable for locks[i]
703: locked = locked.add(locks[i].amount); //@audit gas: should use the suggested storage variable for locks[i]
705: unlockable = unlockable.add(locks[i].amount); //@audit gas: should use the suggested storage variable for locks[i]
807: uint256(rewardData[_rewardsToken].rewardPerTokenStored).add( //@audit gas: should declare a storage variable for rewardData[_rewardsToken]
808: _lastTimeRewardApplicable(rewardData[_rewardsToken].periodFinish) //@audit gas: should use the suggested storage variable for rewardData[_rewardsToken]
809: .sub(rewardData[_rewardsToken].lastUpdateTime)//@audit gas: should use the suggested storage variable for rewardData[_rewardsToken]
810: .mul(rewardData[_rewardsToken].rewardRate)//@audit gas: should use the suggested storage variable for rewardData[_rewardsToken]
convex-platform/contracts/contracts/Booster.sol:
225: if(feeTokens[_feeToken].distro == address(0)){ //@audit gas: should declare "FeeDistro storage _feeDistro = feeTokens[_feeToken];" and use it
239: feeTokens[_feeToken] = FeeDistro({ //@audit gas: should use suggested storage variable _feeDistro
247: feeTokens[_feeToken].distro = _feeDistro;//@audit gas: should use suggested storage variable _feeDistro
258: require(feeTokens[_feeToken].distro != address(0), "Fee doesn't exist");//@audit gas: should declare "FeeDistro storage _feeDistro = feeTokens[_feeToken];" and use it
260: feeTokens[_feeToken].active = _active; //@audit gas: should use suggested storage variable _feeDistro
559: address stash = poolInfo[_pid].stash; //@audit gas: should declare a storage variable for poolInfo[_pid] and use it to retrieve .stash
561: address gauge = poolInfo[_pid].gauge; //@audit gas: should use the suggested storage variable for poolInfo[_pid] to retrieve gauge
AuraLocker.emergencyWithdraw(): Use of the memory keyword when storage should be used
When copying a state struct in memory, there are as many SLOADs and MSTOREs as there are slots. When reading the whole struct multiple times is not needed, it's better to actually only read the relevant field(s). When only some of the fields are read several times, these particular values should be cached instead of the whole state struct.
Here L355, the storage keyword should be used instead of memory (see @audit tags):
File: AuraLocker.sol
55: struct LockedBalance {
56: uint112 amount; //@audit-info : SLOT 1
57: uint32 unlockTime; //@audit-info : SLOT 1
58: }
...
352: function emergencyWithdraw() external nonReentrant {
...
355: LockedBalance[] memory locks = userLocks[msg.sender]; //@audit gas: change memory with storage like L377. Each LockedBalance is 1 SLOAD (1 SLOT used) so, too many copies in memory here when only the array's length is requires
...
362: userBalance.nextUnlockIndex = locks.length.to32(); //@audit-info : locks only used for its length property
...
377: LockedBalance[] storage locks = userLocks[_account]; //@audit-info : L355 should do the same thing as here (storage instead of memory)
Caching storage values in memory
The code can be optimized by minimising the number of SLOADs.
SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.
See the @audit tags for details about the multiple SLOADs where a cached value should be used instead of SLOAD 2 and above:
contracts/AuraBalRewardPool.sol:
90: rewardPerTokenStored = rewardPerToken(); //@audit gas: future SLOAD avoidable by caching rewardPerToken() in a memory variable
94: userRewardPerTokenPaid[account] = rewardPerTokenStored; //@audit gas: SLOAD avoidable by using the suggested cache on rewardPerToken()
contracts/AuraMerkleDrop.sol:
67: startTime = block.timestamp + _startDelay; //@audit should cache "block.timestamp + _startDelay"
70: expiryTime = startTime + _expiresAfter; //@audit SLOAD can be avoided by using cached "block.timestamp + _startDelay"
138: rewardPerTokenStored = rewardPerToken();//@audit gas: should cache rewardPerToken() to avoid a future SLOAD
142: userRewardPerTokenPaid[account] = rewardPerTokenStored;//@audit gas: should use suggested cache to avoid SLOAD
188: if (block.number <= pool.lastRewardBlock) { //@audit gas: SLOAD 1 (pool.lastRewardBlock), should cache it
196: uint256 multiplier = getMultiplier(pool.lastRewardBlock, block.number); //@audit gas: SLOAD 2 (pool.lastRewardBlock), should use cache
216: .mul(pool.accCvxPerShare) //@audit gas: SLOAD 1 (pool.accCvxPerShare)
226: user.amount = user.amount.add(_amount); //@audit gas: should cache "user.amount.add(_amount)" in memory to avoid a future SLOAD
227: user.rewardDebt = user.amount.mul(pool.accCvxPerShare).div(1e12); //@audit gas: SLOAD 2 (pool.accCvxPerShare)
232: _rewarder.onReward(_pid, msg.sender, msg.sender, 0, user.amount); //@audit gas: should use suggested cache for "user.amount.add(_amount)"
242: require(user.amount >= _amount, "withdraw: not good"); //@audit gas: SLOAD 1 (user.amount), should cache it
244: uint256 pending = user.amount.mul(pool.accCvxPerShare).div(1e12).sub(//@audit gas: SLOAD 2 (user.amount), should use suggested cache
248: user.amount = user.amount.sub(_amount); //@audit gas: should cache "user.amount.sub(_amount)" in memory to avoid a future SLOAD, using previous cache for "user.amount"
249: user.rewardDebt = user.amount.mul(pool.accCvxPerShare).div(1e12);//@audit gas: should use suggested cache for "user.amount.sub(_amount)"
255: _rewarder.onReward(_pid, msg.sender, msg.sender, pending, user.amount);//@audit gas: should use suggested cache for "user.amount.sub(_amount)"
267: uint256 pending = user.amount.mul(pool.accCvxPerShare).div(1e12).sub( //@audit gas: SLOAD 1 (user.amount), should cache it
271: user.rewardDebt = user.amount.mul(pool.accCvxPerShare).div(1e12);//@audit gas: SLOAD 2 (user.amount), should use suggested cache
276: _rewarder.onReward(_pid, _account, _account, pending, user.amount);//@audit gas: SLOAD 3 (user.amount), should use suggested cache
286: pool.lpToken.safeTransfer(address(msg.sender), user.amount); //@audit gas: SLOAD 1 (user.amount), should cache it
287: emit EmergencyWithdraw(msg.sender, _pid, user.amount);//@audit gas: SLOAD 2 (user.amount), should use suggested cache
193: _asset.safeApprove(rewardDeposit, 0); //@audit gas: SLOAD 1 (rewardDeposit), should cache it
194: _asset.safeApprove(rewardDeposit, balance); //@audit gas: SLOAD 2 (rewardDeposit), should use suggested cache
195: IRewardDeposit(rewardDeposit).addReward(address(_asset), balance); //@audit gas: SLOAD 3 (rewardDeposit), should use suggested cache
306: require(msg.sender == operator, "!auth"); //@audit gas: SLOAD 1 (operator), should cache it
311: IERC20(crv).safeTransfer(operator, _balance); //@audit gas: SLOAD 2 (operator), should use suggested cache
334: require(msg.sender == operator, "!auth"); //@audit gas: SLOAD 1 (operator), should cache it
337: IERC20(_token).safeTransfer(operator, _balance); //@audit gas: SLOAD 2 (operator), should use suggested cache
Not using SafeMath can save gas on arithmetics operations that can't underflow/overflow
When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), SafeMath isn't necessary (see @audit tags):
209 if (_balance < _amount) {
210: _amount = _withdrawSome(_gauge, _amount.sub(_balance)); //@audit gas: SafeMath not necessary due to L209
SafeMath is not needed when using Solidity version 0.8.11
Solidity version 0.8.11 already implements overflow and underflow checks by default.
Using the SafeMath library from OpenZeppelin (which is more gas expensive than the 0.8+ overflow checks) is therefore redundant.
I suggest using the built-in checks instead of SafeMath and remove SafeMath here:
contracts/AuraBalRewardPool.sol:
2: pragma solidity ^0.8.11;
5: import { SafeMath } from "@openzeppelin/contracts-0.8/utils/math/SafeMath.sol";
24: using SafeMath for uint256;
contracts/AuraStakingProxy.sol:
2: pragma solidity ^0.8.11;
7: import { SafeMath } from "@openzeppelin/contracts-0.8/utils/math/SafeMath.sol";
35: using SafeMath for uint256;
Unchecking arithmetics operations that can't underflow/overflow
I suggest wrapping with an unchecked block here (see @audit tags for more details):
contracts/AuraBalRewardPool.sol:
182 } else {
183 uint256 penalty = (reward * 2) / 10;
184 pendingPenalty += penalty;
185: rewardToken.safeTransfer(msg.sender, reward - penalty); //@audit should be unchecked due to L183 (penalty == 0.2 * reward)
contracts/AuraLocker.sol:
278 if (idx == 0 || userLocks[_account][idx - 1].unlockTime < unlockTime) {
279 userLocks[_account].push(LockedBalance({ amount: lockAmount, unlockTime: uint32(unlockTime) }));
280 } else {
281: LockedBalance storage userL = userLocks[_account][idx - 1]; //@audit should be unchecked due to L278 (idx != 0 here)
282 userL.amount = userL.amount.add(lockAmount);
283 }
471 require(len > 0, "Nothing to delegate");
...
484: uint256 i = len - 1; //@audit should be unchecked due to L471 (len > 0)
520 if (ckpts.length > 0) {
521: DelegateeCheckpoint memory prevCkpt = ckpts[ckpts.length - 1]; //@audit should be unchecked due to L520 (ckpts.length > 0)
640: return high == 0 ? DelegateeCheckpoint(0, 0) : ckpts[high - 1]; //@audit should be unchecked due to condition on same line
863 if (block.timestamp >= rdata.periodFinish) {
864 rdata.rewardRate = _reward.div(rewardsDuration).to96();
865 } else {
866: uint256 remaining = uint256(rdata.periodFinish).sub(block.timestamp); //@audit gas: Should be unchecked due to L863 + SafeMath not necessary due Solidity 0.8+
contracts/AuraMerkleDrop.sol:
134 } else {
135 // If there is an address for auraLocker, and not locking, apply 20% penalty
136 uint256 penalty = address(auraLocker) == address(0) ? 0 : (_amount * 2) / 10;
137 pendingPenalty += penalty;
138: aura.safeTransfer(msg.sender, _amount - penalty); //@audit should be unchecked as penalty is at most 20% of _amount
139 }
contracts/AuraVestedEscrow.sol:
49 constructor(
...
57 require(endtime_ > starttime_, "end must be greater");
...
65: totalTime = endTime - startTime; //@audit should be unchecked due to L57 (endtime_ > starttime_)
158 if (_time < startTime) {
159 return 0;
160 }
161 uint256 locked = totalLocked[_recipient];
162: uint256 elapsed = _time - startTime; //@audit should be unchecked due to L158
contracts/ExtraRewardsDistributor.sol:
74: require(len == 0 || rewardEpochs[_token][len - 1] < _epoch, "Cannot backdate to this epoch"); //@audit should be unchecked due to 1st condition protecting the 2nd's underflow
102: if (len == 0 || rewardEpochs[_token][len - 1] < _epoch) { //@audit should be unchecked due to 1st condition protecting the 2nd's underflow
Booster: Tighly pack storage variables
Here, the storage variables can be tightly packed from:
address public lockRewards; //@audit 20 bytes
bool public isShutdown; //@audit 1 byte (same slot as above)
mapping(address => FeeDistro) public feeTokens;
Which would save 1 storage slot.
CrvDepositor: Tighly pack storage variables
From:
File: CrvDepositor.sol
32: address public feeManager;
33: address public daoOperator; //@audit 20 bytes
34: address public immutable staker;
35: address public immutable minter;
36: uint256 public incentiveCrv = 0;
37: uint256 public unlockTime; //@audit 32 bytes
38:
39: bool public cooldown; //@audit 1 byte
to:
address public feeManager;
address public daoOperator; //@audit 20 bytes
bool public cooldown; //@audit 1 byte (same slot as the above address)
address public immutable staker;
address public immutable minter;
uint256 public incentiveCrv = 0;
uint256 public unlockTime; //@audit 32 bytes
Which would save 1 storage slot.
ExtraRewardStashV3: Tighly pack storage variables
From:
File: ExtraRewardStashV3.sol
36: address public gauge;
37: address public rewardFactory; //@audit 20 bytes
38:
39: mapping(address => uint256) public historicalRewards;
40: bool public hasRedirected; //@audit 1 byte
41: bool public hasCurveRewards; //@audit 1 byte (same slot as above)
42:
43: struct TokenInfo {
44: address token;
45: address rewardAddress;
46: }
47:
48: //use mapping+array so that we dont have to loop check each time setToken is called
49: mapping(address => TokenInfo) public tokenInfo;
to:
address public gauge;
address public rewardFactory; //@audit 20 bytes
bool public hasRedirected; //@audit 1 byte(same slot as above)
bool public hasCurveRewards; //@audit 1 byte (same slot as above)
mapping(address => uint256) public historicalRewards;
struct TokenInfo {
address token;
address rewardAddress;
}
//use mapping+array so that we dont have to loop check each time setToken is called
mapping(address => TokenInfo) public tokenInfo;
Which would save 1 storage slot.
Boolean comparisons
Comparing to a constant (true or false) is a bit more expensive than directly checking the returned boolean value.
I suggest using if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false) here:
> 0 is less efficient than != 0 for unsigned integers (with proof)
!= 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled (6 gas)
Proof: While it may seem that > 0 is cheaper than !=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in a require statement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706
Strict inequalities (>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas). This also holds true between <= and <.
Consider replacing strict inequalities with non-strict ones to save some gas here:
Splitting require() statements that use && saves gas
If you're using the Optimizer at 200, instead of using the && operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement:
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.
contracts/AuraLocker.sol:
114: bool public isShutdown = false;
contracts/AuraVestedEscrow.sol:
33: bool public initialised = false;
convex-platform/contracts/contracts/Booster.sol:
54: bool public isShutdown;
convex-platform/contracts/contracts/BoosterOwner.sol:
49: bool public isSealed;
53: bool public isForceTimerStarted;
convex-platform/contracts/contracts/CrvDepositor.sol:
39: bool public cooldown;
convex-platform/contracts/contracts/ExtraRewardStashV3.sol:
40: bool public hasRedirected;
41: bool public hasCurveRewards;
convex-platform/contracts/contracts/PoolManagerProxy.sol:
72: bool gaugeExists = IPools(pools).gaugeMap(_gauge);
convex-platform/contracts/contracts/PoolManagerSecondaryProxy.sol:
24: bool public isShutdown;
convex-platform/contracts/contracts/PoolManagerV3.sol:
22: bool public protectAddPool;
Shift Right instead of Dividing by 2
A division by 2 can be calculated by shifting one to the right.
While the DIV opcode uses 5 gas, the SHR opcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.
An array's length should be cached to save gas in for-loops
Reading array length at each iteration of the loop consumes more gas than necessary.
In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration.
In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.
Here, I suggest storing the array's length in a variable before the for-loop, and use this new variable instead:
contracts/AuraClaimZap.sol:143: for (uint256 i = 0; i < rewardContracts.length; i++) {
contracts/AuraClaimZap.sol:147: for (uint256 i = 0; i < extraRewardContracts.length; i++) {
contracts/AuraClaimZap.sol:151: for (uint256 i = 0; i < tokenRewardContracts.length; i++) {
contracts/AuraLocker.sol:696: for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
contracts/AuraVestedEscrow.sol:100: for (uint256 i = 0; i < _recipient.length; i++) {
convex-platform/contracts/contracts/ArbitartorVault.sol:49: for(uint256 i = 0; i < _toPids.length; i++){
convex-platform/contracts/contracts/BaseRewardPool.sol:214: for(uint i=0; i < extraRewards.length; i++){
convex-platform/contracts/contracts/BaseRewardPool.sol:230: for(uint i=0; i < extraRewards.length; i++){
convex-platform/contracts/contracts/BaseRewardPool.sol:262: for(uint i=0; i < extraRewards.length; i++){
convex-platform/contracts/contracts/BaseRewardPool.sol:296: for(uint i=0; i < extraRewards.length; i++){
convex-platform/contracts/contracts/Booster.sol:379: for(uint i=0; i < poolInfo.length; i++){
convex-platform/contracts/contracts/Booster.sol:538: for(uint256 i = 0; i < _gauge.length; i++){
convex-platform/contracts/contracts/PoolManagerSecondaryProxy.sol:69: for(uint i=0; i < usedList.length; i++){
++i costs less gas compared to i++ or i += 1
++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.
The same is also true for i--.
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
Affected code:
Increments:
contracts/AuraClaimZap.sol:143: for (uint256 i = 0; i < rewardContracts.length; i++) {
contracts/AuraClaimZap.sol:147: for (uint256 i = 0; i < extraRewardContracts.length; i++) {
contracts/AuraClaimZap.sol:151: for (uint256 i = 0; i < tokenRewardContracts.length; i++) {
contracts/AuraLocker.sol:174: for (uint256 i = 0; i < rewardTokensLength; i++) {
contracts/AuraLocker.sol:306: for (uint256 i; i < rewardTokensLength; i++) {
contracts/AuraLocker.sol:410: for (uint256 i = nextUnlockIndex; i < length; i++) {
contracts/AuraLocker.sol:426: nextUnlockIndex++;
contracts/AuraLocker.sol:696: for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
contracts/AuraLocker.sol:702: idx++;
contracts/AuraLocker.sol:773: for (uint256 i = 0; i < userRewardsLength; i++) {
contracts/AuraVestedEscrow.sol:100: for (uint256 i = 0; i < _recipient.length; i++) {
contracts/BalLiquidityProvider.sol:51: for (uint256 i = 0; i < 2; i++) {
contracts/ExtraRewardsDistributor.sol:233: for (uint256 i = epochIndex; i < tokenEpochs; i++) {
convex-platform/contracts/contracts/ArbitartorVault.sol:49: for(uint256 i = 0; i < _toPids.length; i++){
convex-platform/contracts/contracts/BaseRewardPool.sol:214: for(uint i=0; i < extraRewards.length; i++){
convex-platform/contracts/contracts/BaseRewardPool.sol:230: for(uint i=0; i < extraRewards.length; i++){
convex-platform/contracts/contracts/BaseRewardPool.sol:262: for(uint i=0; i < extraRewards.length; i++){
convex-platform/contracts/contracts/BaseRewardPool.sol:296: for(uint i=0; i < extraRewards.length; i++){
convex-platform/contracts/contracts/Booster.sol:379: for(uint i=0; i < poolInfo.length; i++){
convex-platform/contracts/contracts/Booster.sol:538: for(uint256 i = 0; i < _gauge.length; i++){
convex-platform/contracts/contracts/BoosterOwner.sol:144: for(uint256 i = 0; i < poolCount; i++){
convex-platform/contracts/contracts/ExtraRewardStashV3.sol:125: for(uint256 i = 0; i < maxRewards; i++){
convex-platform/contracts/contracts/ExtraRewardStashV3.sol:199: for(uint i=0; i < tCount; i++){
convex-platform/contracts/contracts/PoolManagerSecondaryProxy.sol:69: for(uint i=0; i < usedList.length; i++){
Decrements:
contracts/AuraLocker.sol:
497: i--;
664: for (uint256 i = locksLength; i > 0; i--) {
726: for (uint256 i = epochIndex + 1; i > 0; i--) {
Consider using ++i instead of i++ to increment the value of an uint variable. The same holds true with decrements (--i vs i--)
Increments/Decrements can be unchecked
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.
contracts/AuraClaimZap.sol:143: for (uint256 i = 0; i < rewardContracts.length; i++) {
contracts/AuraClaimZap.sol:147: for (uint256 i = 0; i < extraRewardContracts.length; i++) {
contracts/AuraClaimZap.sol:151: for (uint256 i = 0; i < tokenRewardContracts.length; i++) {
contracts/AuraLocker.sol:174: for (uint256 i = 0; i < rewardTokensLength; i++) {
contracts/AuraLocker.sol:306: for (uint256 i; i < rewardTokensLength; i++) {
contracts/AuraLocker.sol:410: for (uint256 i = nextUnlockIndex; i < length; i++) {
contracts/AuraLocker.sol:696: for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
contracts/AuraLocker.sol:773: for (uint256 i = 0; i < userRewardsLength; i++) {
contracts/AuraVestedEscrow.sol:100: for (uint256 i = 0; i < _recipient.length; i++) {
contracts/BalLiquidityProvider.sol:51: for (uint256 i = 0; i < 2; i++) {
contracts/ExtraRewardsDistributor.sol:233: for (uint256 i = epochIndex; i < tokenEpochs; i++) {
Decrement:
contracts/AuraLocker.sol:
497: i--;
664: for (uint256 i = locksLength; i > 0; i--) {
726: for (uint256 i = epochIndex + 1; i > 0; i--) {
The code would go from:
for (uint256 i; i < numIterations; i++) { //or i--
// ...
}
to:
for (uint256 i; i < numIterations;) {
// ...
unchecked { ++i; } //or unchecked { --i; }
}
The risk of overflow is inexistant for uint256 here.
Please, notice that in convex-platform/contracts/, the syntax used is already unchecked, as the Solidity version used is 0.6.12, which doesn't implement a default overflow check. Only contracts under contracts/ are concerned.
Use calldata instead of memory
When arguments are read-only on external functions, the data location should be calldata:
File: PoolManagerSecondaryProxy.sol
68: function setUsedAddress(address[] memory usedList) external onlyOwner{ //@audit gas: should use calldata instead of memory
69: for(uint i=0; i < usedList.length; i++){
70: usedMap[usedList[i]] = true;
71: }
72: }
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.
As an example: for (uint256 i = 0; i < numIterations; ++i) { should be replaced with for (uint256 i; i < numIterations; ++i) {
Affected code:
contracts/AuraBalRewardPool.sol:35: uint256 public pendingPenalty = 0;
contracts/AuraBalRewardPool.sol:38: uint256 public periodFinish = 0;
contracts/AuraBalRewardPool.sol:39: uint256 public rewardRate = 0;
contracts/AuraClaimZap.sol:143: for (uint256 i = 0; i < rewardContracts.length; i++) {
contracts/AuraClaimZap.sol:147: for (uint256 i = 0; i < extraRewardContracts.length; i++) {
contracts/AuraClaimZap.sol:151: for (uint256 i = 0; i < tokenRewardContracts.length; i++) {
contracts/AuraLocker.sol:72: uint256 public queuedCvxCrvRewards = 0;
contracts/AuraLocker.sol:114: bool public isShutdown = false;
contracts/AuraLocker.sol:174: for (uint256 i = 0; i < rewardTokensLength; i++) {
contracts/AuraLocker.sol:381: uint256 reward = 0;
contracts/AuraLocker.sol:485: uint256 futureUnlocksSum = 0;
contracts/AuraLocker.sol:540: uint256 unlocksSinceLatestCkpt = 0;
contracts/AuraLocker.sol:630: uint256 low = 0;
contracts/AuraLocker.sol:773: for (uint256 i = 0; i < userRewardsLength; i++) {
contracts/AuraMerkleDrop.sol:29: uint256 public pendingPenalty = 0;
contracts/AuraVestedEscrow.sol:33: bool public initialised = false;
contracts/AuraVestedEscrow.sol:99: uint256 totalAmount = 0;
contracts/AuraVestedEscrow.sol:100: for (uint256 i = 0; i < _recipient.length; i++) {
contracts/BalLiquidityProvider.sol:51: for (uint256 i = 0; i < 2; i++) {
contracts/ExtraRewardsDistributor.sol:231: uint256 claimableTokens = 0;
convex-platform/contracts/contracts/ArbitartorVault.sol:49: for(uint256 i = 0; i < _toPids.length; i++){
convex-platform/contracts/contracts/BaseRewardPool.sol:71: uint256 public periodFinish = 0;
convex-platform/contracts/contracts/BaseRewardPool.sol:72: uint256 public rewardRate = 0;
convex-platform/contracts/contracts/BaseRewardPool.sol:75: uint256 public queuedRewards = 0;
convex-platform/contracts/contracts/BaseRewardPool.sol:76: uint256 public currentRewards = 0;
convex-platform/contracts/contracts/BaseRewardPool.sol:77: uint256 public historicalRewards = 0;
convex-platform/contracts/contracts/Booster.sol:29: uint256 public platformFee = 0; //possible fee to build treasury
convex-platform/contracts/contracts/Booster.sol:538: for(uint256 i = 0; i < _gauge.length; i++){
convex-platform/contracts/contracts/BoosterOwner.sol:144: for(uint256 i = 0; i < poolCount; i++){
convex-platform/contracts/contracts/ConvexMasterChef.sol:63: uint256 public totalAllocPoint = 0;
convex-platform/contracts/contracts/ConvexMasterChef.sol:180: for (uint256 pid = 0; pid < length; ++pid) {
convex-platform/contracts/contracts/CrvDepositor.sol:36: uint256 public incentiveCrv = 0;
convex-platform/contracts/contracts/ExtraRewardStashV3.sol:125: for(uint256 i = 0; i < maxRewards; i++){
convex-platform/contracts/contracts/VirtualBalanceRewardPool.sol:89: uint256 public periodFinish = 0;
convex-platform/contracts/contracts/VirtualBalanceRewardPool.sol:90: uint256 public rewardRate = 0;
convex-platform/contracts/contracts/VirtualBalanceRewardPool.sol:93: uint256 public queuedRewards = 0;
convex-platform/contracts/contracts/VirtualBalanceRewardPool.sol:94: uint256 public currentRewards = 0;
convex-platform/contracts/contracts/VirtualBalanceRewardPool.sol:95: uint256 public historicalRewards = 0;
convex-platform/contracts/contracts/VoterProxy.sol:308: uint256 _balance = 0;
I suggest removing explicit initializations for default values.
Upgrade pragma to at least 0.8.4
Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.
The advantages here are:
Low level inliner (>= 0.8.2): Cheaper runtime gas (especially relevant when the contract has small functions).
Optimizer improvements in packed structs (>= 0.8.3)
Custom errors (>= 0.8.4): cheaper deployment cost and runtime cost. Note: the runtime cost is only relevant when the revert condition is met. In short, replace revert strings by custom errors.
Consider upgrading pragma to at least 0.8.4 for contracts in convex-platform/contracts (ideally 0.8.11 for consistency), as they use Solidity 0.6.12.
Reduce the size of error messages (Long revert Strings)
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.
I suggest shortening the revert strings to fit in 32 bytes.
Use Custom Errors instead of Revert Strings to save Gas
Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
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).
I suggest replacing all revert strings with custom errors in /contracts.
Table of Contents:
AuraLocker.emergencyWithdraw(): Use of thememorykeyword whenstorageshould be usedBooster: Tighly pack storage variablesCrvDepositor: Tighly pack storage variablesExtraRewardStashV3: Tighly pack storage variables> 0is less efficient than!= 0for unsigned integers (with proof)>=is cheaper than>(and<=cheaper than<)CrvDepositor.sol#setFees(): Tautology on "_lockIncentive >= 0" which is always true as_lockIncentiveis uint256require()statements that use&&saves gasboolfor storage incurs overhead++icosts less gas compared toi++ori += 1calldatainstead ofmemoryCheap Contract Deployment Through Clones
Here, contract deployments are made:
There's a way to save a significant amount of gas on deployment using Clones: https://www.youtube.com/watch?v=3Mw-pMmJ7TA .
This is a solution that was adopted, as an example, by Porter Finance. They realized that deploying using clones was 10x cheaper:
I suggest applying a similar pattern.
Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it
To help the optimizer, declare a
storagetype variable and use it instead of repeatedly fetching the reference in a map or an array.The effect can be quite significant.
As an example, instead of repeatedly calling
someMap[someIndex], save its reference like this:SomeStruct storage someStruct = someMap[someIndex]and use it.Affected code (check the
@audittags):AuraLocker.emergencyWithdraw(): Use of thememorykeyword whenstorageshould be usedWhen copying a state struct in memory, there are as many SLOADs and MSTOREs as there are slots. When reading the whole struct multiple times is not needed, it's better to actually only read the relevant field(s). When only some of the fields are read several times, these particular values should be cached instead of the whole state struct.
Here L355, the
storagekeyword should be used instead ofmemory(see@audittags):Caching storage values in memory
The code can be optimized by minimising the number of SLOADs.
SLOADs are expensive (100 gas after the 1st one) compared to MLOADs/MSTOREs (3 gas each). Storage values read multiple times should instead be cached in memory the first time (costing 1 SLOAD) and then read from this cache to avoid multiple SLOADs.
See the
@audittags for details about the multiple SLOADs where a cached value should be used instead ofSLOAD 2and above:Not using SafeMath can save gas on arithmetics operations that can't underflow/overflow
When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), SafeMath isn't necessary (see
@audittags):SafeMath is not needed when using Solidity version 0.8.11
Solidity version 0.8.11 already implements overflow and underflow checks by default. Using the SafeMath library from OpenZeppelin (which is more gas expensive than the 0.8+ overflow checks) is therefore redundant.
I suggest using the built-in checks instead of SafeMath and remove SafeMath here:
Unchecking arithmetics operations that can't underflow/overflow
Solidity version 0.8+ comes with implicit overflow and underflow checks on unsigned integers. When an overflow or an underflow isn't possible (as an example, when a comparison is made before the arithmetic operation), some gas can be saved by using an
uncheckedblock: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmeticI suggest wrapping with an
uncheckedblock here (see@audittags for more details):Booster: Tighly pack storage variablesHere, the storage variables can be tightly packed from:
to
Which would save 1 storage slot.
CrvDepositor: Tighly pack storage variablesFrom:
to:
Which would save 1 storage slot.
ExtraRewardStashV3: Tighly pack storage variablesFrom:
to:
Which would save 1 storage slot.
Boolean comparisons
Comparing to a constant (
trueorfalse) is a bit more expensive than directly checking the returned boolean value. I suggest usingif(directValue)instead ofif(directValue == true)andif(!directValue)instead ofif(directValue == false)here:> 0is less efficient than!= 0for unsigned integers (with proof)!= 0costs less gas compared to> 0for unsigned integers inrequirestatements with the optimizer enabled (6 gas)Proof: While it may seem that
> 0is cheaper than!=, this is only true without the optimizer enabled and outside a require statement. If you enable the optimizer at 10k AND you're in arequirestatement, this will save gas. You can see this tweet for more proofs: https://twitter.com/gzeon/status/1485428085885640706I suggest changing
> 0with!= 0here:Also, please enable the Optimizer.
>=is cheaper than>(and<=cheaper than<)Strict inequalities (
>) are more expensive than non-strict ones (>=). This is due to some supplementary checks (ISZERO, 3 gas). This also holds true between<=and<.Consider replacing strict inequalities with non-strict ones to save some gas here:
CrvDepositor.sol#setFees(): Tautology on "_lockIncentive >= 0" which is always true as_lockIncentiveis uint256As a variable of type
uintwill always be>= 0, such a check isn't necessary.Consider deleting the
>= 0check L75:Splitting
require()statements that use&&saves gasIf you're using the Optimizer at 200, instead of using the
&&operator in a single require statement to check multiple conditions, I suggest using multiple require statements with 1 condition per require statement:Using
boolfor storage incurs overheadBooleans 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.
Shift Right instead of Dividing by 2
A division by 2 can be calculated by shifting one to the right.
While the
DIVopcode uses 5 gas, theSHRopcode only uses 3 gas. Furthermore, Solidity's division operation also includes a division-by-0 prevention which is bypassed using shifting.I suggest replacing
/ 2with>> 1here:An array's length should be cached to save gas in for-loops
Reading array length at each iteration of the loop consumes more gas than necessary.
In the best case scenario (length read on a memory variable), caching the array length in the stack saves around 3 gas per iteration. In the worst case scenario (external calls at each iteration), the amount of gas wasted can be massive.
Here, I suggest storing the array's length in a variable before the for-loop, and use this new variable instead:
++icosts less gas compared toi++ori += 1++icosts less gas compared toi++ori += 1for unsigned integer, as pre-increment is cheaper (about 5 gas per iteration). This statement is true even with the optimizer enabled.The same is also true for
i--.i++incrementsiand returns the initial value ofi. Which means:But
++ireturns the actual incremented value:In the first case, the compiler has to create a temporary variable (when used) for returning
1instead of2Affected code:
Consider using
++iinstead ofi++to increment the value of an uint variable. The same holds true with decrements (--ivsi--)Increments/Decrements can be unchecked
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.
ethereum/solidity#10695
Affected code:
The code would go from:
to:
The risk of overflow is inexistant for
uint256here.Please, notice that in
convex-platform/contracts/, the syntax used is already unchecked, as the Solidity version used is 0.6.12, which doesn't implement a default overflow check. Only contracts undercontracts/are concerned.Use
calldatainstead ofmemoryWhen arguments are read-only on external functions, the data location should be
calldata:No need to explicitly initialize variables with default values
If a variable is not set/initialized, it is assumed to have the default value (
0foruint,falseforbool,address(0)for address...). Explicitly initializing it with its default value is an anti-pattern and wastes gas.As an example:
for (uint256 i = 0; i < numIterations; ++i) {should be replaced withfor (uint256 i; i < numIterations; ++i) {Affected code:
I suggest removing explicit initializations for default values.
Upgrade pragma to at least 0.8.4
Using newer compiler versions and the optimizer give gas optimizations. Also, additional safety checks are available for free.
The advantages here are:
Consider upgrading pragma to at least 0.8.4 for contracts in
convex-platform/contracts(ideally 0.8.11 for consistency), as they use Solidity 0.6.12.Reduce the size of error messages (Long revert Strings)
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.
Revert strings > 32 bytes:
I suggest shortening the revert strings to fit in 32 bytes.
Use Custom Errors instead of Revert Strings to save Gas
Solidity 0.8.4 introduced custom errors. They are more gas efficient than revert strings, when it comes to deploy cost as well as runtime cost when the revert condition is met. Use custom errors instead of revert strings for gas savings.
Custom errors from Solidity 0.8.4 are cheaper than revert strings (cheaper deployment cost and runtime cost when the revert condition is met)
Source: https://blog.soliditylang.org/2021/04/21/custom-errors/:
Custom errors are defined using the
errorstatement, which can be used inside and outside of contracts (including interfaces and libraries).I suggest replacing all revert strings with custom errors in
/contracts.