Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gas Optimizations #336

Open
code423n4 opened this issue May 25, 2022 · 0 comments
Open

Gas Optimizations #336

code423n4 opened this issue May 25, 2022 · 0 comments
Labels
bug Something isn't working G (Gas Optimization)

Comments

@code423n4
Copy link
Contributor

Table of Contents:

Cheap Contract Deployment Through Clones

Here, contract deployments are made:

convex-platform/contracts/contracts/RewardFactory.sol:61:        BaseRewardPool4626 rewardPool = new BaseRewardPool4626(_pid,_depositToken,crv,operator, address(this), _lptoken);
convex-platform/contracts/contracts/RewardFactory.sol:75:        VirtualBalanceRewardPool rewardPool = new VirtualBalanceRewardPool(_mainRewards,_token,_operator);
convex-platform/contracts/contracts/TokenFactory.sol:44:        DepositToken dtoken = new DepositToken(operator,_lptoken,namePostfix,symbolPrefix);

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 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"
   97:         require(msg.sender == dao, "!auth"); //@audit SLOAD 1 (dao)
  100:         aura.safeTransfer(dao, amt); //@audit SLOAD 2 (dao)
  119:         require(merkleRoot != bytes32(0), "!root"); //@audit SLOAD 1 (merkleRoot)
  126:         require(MerkleProof.verify(_proof, merkleRoot, leaf), "invalid proof"); //@audit SLOAD 2 (merkleRoot)
  131:             aura.safeApprove(address(auraLocker), 0);//@audit SLOAD 1 (auraLocker)
  132:             aura.safeApprove(address(auraLocker), _amount);//@audit SLOAD 2 (auraLocker)
  • contracts/AuraStakingProxy.sol:
  117:         require(pendingOwner != address(0), "invalid owner");//@audit gas: SLOAD 1 (pendingOwner)
  119:         owner = pendingOwner;//@audit gas: SLOAD 2 (pendingOwner)
  147:         IERC20(crv).safeApprove(crvDepositorWrapper, 0);//@audit gas: SLOAD 1 (crvDepositorWrapper)
  148:         IERC20(crv).safeApprove(crvDepositorWrapper, type(uint256).max);//@audit gas: SLOAD 2 (crvDepositorWrapper)
  150:         IERC20(cvxCrv).safeApprove(rewards, 0);//@audit gas: SLOAD 1 (rewards)
  151:         IERC20(cvxCrv).safeApprove(rewards, type(uint256).max);//@audit gas: SLOAD 2 (rewards)
  178:             uint256 minOut = ICrvDepositor(crvDepositorWrapper).getMinOut(crvBal, outputBps);//@audit gas: SLOAD 1 (crvDepositorWrapper)
  179:             ICrvDepositor(crvDepositorWrapper).deposit(crvBal, minOut, true, address(0));//@audit gas: SLOAD 2 (crvDepositorWrapper)
  215:             _token.safeApprove(rewards, 0);//@audit gas: SLOAD 1 (rewards)
  216:             _token.safeApprove(rewards, type(uint256).max);//@audit gas: SLOAD 2 (rewards)
  219:             IAuraLocker(rewards).notifyRewardAmount(address(_token), bal);//@audit gas: SLOAD 3 (rewards)
  • contracts/AuraVestedEscrow.sol:
  117:         require(msg.sender == admin, "!auth");//@audit gas: SLOAD 1 (admin)
  123:         rewardToken.safeTransfer(admin, delta);//@audit gas: SLOAD 2 (admin)
  185:             require(address(auraLocker) != address(0), "!auraLocker");//@audit gas: SLOAD 1 (auraLocker)
  186:             rewardToken.safeApprove(address(auraLocker), claimable);//@audit gas: SLOAD 2 (auraLocker)
  • convex-platform/contracts/contracts/BaseRewardPool.sol:
  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
  329:         if (block.timestamp >= periodFinish) {//@audit gas: SLOAD 1 (periodFinish)
  336:         uint256 elapsedTime = block.timestamp.sub(periodFinish.sub(duration));//@audit gas: SLOAD 2 (periodFinish)
  356:         if (block.timestamp >= periodFinish) {//@audit gas: SLOAD 1 (periodFinish)
  359:             uint256 remaining = periodFinish.sub(block.timestamp);//@audit gas: SLOAD 2 (periodFinish)
  • convex-platform/contracts/contracts/BaseRewardPool4626.sol:
  59:         uint256 balBefore = stakingToken.balanceOf(address(this));//@audit gas: SLOAD 1 (stakingToken)
  61:         uint256 balAfter = stakingToken.balanceOf(address(this));//@audit gas: SLOAD 2 (stakingToken)
  • convex-platform/contracts/contracts/Booster.sol:
  220:         require(lockRewards != address(0) && rewardFactory != address(0), "!initialised"); //@audit gas: SLOAD 1 (lockRewards), should cache it
  232:                     rewards: lockRewards, //@audit gas: SLOAD 2 (lockRewards), should use suggested cache
  235:                 emit FeeInfoUpdated(_feeDistro, lockRewards, crv); //@audit gas: SLOAD 3 (lockRewards), should use suggested cache
  238:                 address rewards = IRewardFactory(rewardFactory).CreateTokenRewards(_feeToken, lockRewards, address(this)); //@audit gas: SLOAD 2 (lockRewards), should use suggested cache
  323:         address newRewardPool = IRewardFactory(rewardFactory).CreateCrvRewards(pid,token,_lptoken); //@audit gas: SLOAD 1 (rewardFactory), should cache it
  345:             IRewardFactory(rewardFactory).setAccess(stash,true); //@audit gas: SLOAD 2 (rewardFactory), should use suggested cache
  464:         if (!pool.shutdown) { //@audit gas: SLOAD 1 (!pool.shutdown), should cache it
  471:         if(stash != address(0) && !isShutdown && !pool.shutdown){ //@audit gas: SLOAD 2 (!pool.shutdown), should use suggested cache
  602:             if(treasury != address(0) && treasury != address(this) && platformFee > 0){ //@audit gas: SLOAD 1 & 2 (treasury) and SLOAD 1 (platformFee), should cache them
  604:                 uint256 _platform = crvBal.mul(platformFee).div(FEE_DENOMINATOR); //@audit gas: SLOAD 2 (platformFee), should use suggested cache
  606:                 IERC20(crv).safeTransfer(treasury, _platform); //@audit gas: SLOAD 2 (treasury), should use suggested cache
  621:             IERC20(crv).safeTransfer(lockRewards, _lockIncentive); //@audit gas: SLOAD 1 (lockRewards), should cache it
  622:             IRewards(lockRewards).queueNewRewards(_lockIncentive); //@audit gas: SLOAD 2 (lockRewards), should use suggested cache
  • convex-platform/contracts/contracts/BoosterOwner.sol:
   96:         require(pendingowner == msg.sender, "!pendingowner"); //@audit gas: SLOAD 1 (pendingowner)
   97:         owner = pendingowner; //@audit gas: SLOAD 2 (pendingowner), should use msg.sender (costs 2 gas)
   99:         emit AcceptedOwnership(owner); //@audit gas: SLOAD 3 (pendingowner, now owner), should use msg.sender (costs 2 gas)
  163:         forceTimestamp = block.timestamp + FORCE_DELAY; //@audit gas: should cache "block.timestamp + FORCE_DELAY" to avoid a future SLOAD
  165:         emit ShutdownStarted(forceTimestamp); //@audit gas: should use suggested cache for "block.timestamp + FORCE_DELAY"
  • convex-platform/contracts/contracts/ConvexMasterChef.sol:
  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
  • convex-platform/contracts/contracts/CrvDepositor.sol:
  145:         if(incentiveCrv > 0){ //@audit gas: SLOAD 1 (incentiveCrv), should cache it
  146:             ITokenMinter(minter).mint(msg.sender,incentiveCrv); //@audit gas: SLOAD 2 (incentiveCrv), should use suggested cache
  175:             if(incentiveCrv > 0){ //@audit gas: SLOAD 1 (incentiveCrv), should cache it
  177:                 _amount = _amount.add(incentiveCrv); //@audit gas: SLOAD 2 (incentiveCrv), should use suggested cache
  184:             uint256 callIncentive = _amount.mul(lockIncentive).div(FEE_DENOMINATOR); //@audit gas: SLOAD 1 (incentiveCrv), should cache it
  188:             incentiveCrv = incentiveCrv.add(callIncentive); //@audit gas: SLOAD 2 (incentiveCrv), should use suggested cache
  • convex-platform/contracts/contracts/ExtraRewardStashV3.sol:
   97:         require(msg.sender == operator, "!operator"); //@audit gas: SLOAD 1 (operator), should cache it
  104:             IDeposit(operator).setGaugeRedirect(pid); //@audit gas: SLOAD 2 (operator), should use suggested cache
  111:             IDeposit(operator).claimRewards(pid,gauge); //@audit gas: SLOAD 2 (operator), should use suggested cache
  196:         require(msg.sender == operator, "!operator"); //@audit gas: SLOAD 1 (operator), should cache it
  209:                     IERC20(token).safeTransfer(operator, amount); //@audit gas: SLOAD 2 (operator), should use suggested cache
  • convex-platform/contracts/contracts/StashFactoryV2.sol:
  60:             require(v3Implementation!=address(0),"0 impl"); //@audit gas: SLOAD 1 (v3Implementation), should cache it
  61:             address stash = IProxyFactory(proxyFactory).clone(v3Implementation);//@audit gas: SLOAD 2 (v3Implementation), should use suggested cache
  67:             require(v1Implementation!=address(0),"0 impl");//@audit gas: SLOAD 1 (v1Implementation), should cache it
  68:             address stash = IProxyFactory(proxyFactory).clone(v1Implementation);//@audit gas: SLOAD 2 (v1Implementation), should use suggested cache
  74:             require(v2Implementation!=address(0),"0 impl");//@audit gas: SLOAD 1 (v2Implementation), should cache it
  75:             address stash = IProxyFactory(proxyFactory).clone(v2Implementation);//@audit gas: SLOAD 2 (v2Implementation), should use suggested cache
  • convex-platform/contracts/contracts/VoterProxy.sol:
  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):

  • convex-platform/contracts/contracts/VirtualBalanceRewardPool.sol:
  241          if (block.timestamp >= periodFinish) {
  242              rewardRate = reward.div(duration);
  243          } else {
  244:             uint256 remaining = periodFinish.sub(block.timestamp); //@audit gas: SafeMath not necessary due to L241
  • convex-platform/contracts/contracts/VoterProxy.sol:
  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

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 unchecked block: https://docs.soliditylang.org/en/v0.8.10/control-structures.html#checked-or-unchecked-arithmetic

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:

File: Booster.sol
45:     address public lockRewards; //@audit 20 bytes
46: 
47:     mapping(address => FeeDistro) public feeTokens;
48:     struct FeeDistro {
49:         address distro;
50:         address rewards;
51:         bool active;
52:     }
53: 
54:     bool public isShutdown; //@audit 1 byte
55: 
56:     struct PoolInfo {
57:         address lptoken;
58:         address token;
59:         address gauge;
60:         address crvRewards;
61:         address stash;
62:         bool shutdown;
63:     }
64: 
65:     //index(pid) -> pool
66:     PoolInfo[] public poolInfo;

to

    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:

contracts/AuraMerkleDrop.sol:123:        require(hasClaimed[msg.sender] == false, "already claimed");
convex-platform/contracts/contracts/Booster.sol:400:        require(pool.shutdown == false, "pool is closed");
convex-platform/contracts/contracts/Booster.sol:574:        require(pool.shutdown == false, "pool is closed");
convex-platform/contracts/contracts/RewardFactory.sol:72:        require(msg.sender == operator || rewardAccess[msg.sender] == true, "!auth");
convex-platform/contracts/contracts/VoterProxy.sol:107:        require(operator == address(0) || IDeposit(operator).isShutdown() == true, "needs shutdown");
convex-platform/contracts/contracts/VoterProxy.sol:168:        if(protectedTokens[_token] == false){
convex-platform/contracts/contracts/VoterProxy.sol:171:        if(protectedTokens[_gauge] == false){
convex-platform/contracts/contracts/VoterProxy.sol:190:        require(protectedTokens[address(_asset)] == false, "protected");

> 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

I suggest changing > 0 with != 0 here:

contracts/Aura.sol:68:        require(_amount > 0, "Must mint something");
contracts/AuraBalRewardPool.sol:121:        require(_amount > 0, "RewardPool : Cannot stake 0");
contracts/AuraBalRewardPool.sol:139:        require(_amount > 0, "RewardPool : Cannot stake 0");
contracts/AuraBalRewardPool.sol:157:        require(amount > 0, "RewardPool : Cannot withdraw 0");
contracts/AuraBalRewardPool.sol:210:        require(rewardsAvailable > 0, "!balance");
contracts/AuraLocker.sol:210:        require(rewardData[_rewardsToken].lastUpdateTime > 0, "Reward does not exist");
contracts/AuraLocker.sol:259:        require(_amount > 0, "Cannot stake 0");
contracts/AuraLocker.sol:359:        require(amt > 0, "Nothing locked");
contracts/AuraLocker.sol:385:        require(length > 0, "no locks");
contracts/AuraLocker.sol:431:        require(locked > 0, "no exp locks");
contracts/AuraLocker.sol:471:        require(len > 0, "Nothing to delegate");
contracts/AuraLocker.sol:822:        require(_rewards > 0, "No reward");
contracts/AuraLocker.sol:851:        require(_reward > 0, "No reward");
contracts/AuraMerkleDrop.sol:122:        require(_amount > 0, "!amount");
contracts/AuraPenaltyForwarder.sol:52:        require(bal > 0, "!empty");
contracts/AuraVestedEscrow.sol:118:        require(totalLocked[_recipient] > 0, "!funding");
contracts/BalLiquidityProvider.sol:57:            require(bal > 0 && bal == _request.maxAmountsIn[i], "!bal");
contracts/BalLiquidityProvider.sol:70:        require(balAfter > 0, "!mint");
contracts/ExtraRewardsDistributor.sol:171:        require(_index > 0 && _index < rewardEpochs[_token].length - 1, "!past");
convex-platform/contracts/contracts/interfaces/BoringMath.sol:20:        require(b > 0, "BoringMath: division by zero");
convex-platform/contracts/contracts/interfaces/BoringMath.sol:102:        require(b > 0, "BoringMath: division by zero");
convex-platform/contracts/contracts/interfaces/BoringMath.sol:123:        require(b > 0, "BoringMath: division by zero");
convex-platform/contracts/contracts/interfaces/BoringMath.sol:143:        require(b > 0, "BoringMath: division by zero");
convex-platform/contracts/contracts/BaseRewardPool.sol:211:        require(_amount > 0, 'RewardPool : Cannot stake 0');
convex-platform/contracts/contracts/BaseRewardPool.sol:227:        require(amount > 0, 'RewardPool : Cannot withdraw 0');
convex-platform/contracts/contracts/Booster.sol:223:        require(IFeeDistributor(_feeDistro).getTokenTimeCursor(_feeToken) > 0, "!distro");
convex-platform/contracts/contracts/CrvDepositor.sol:169:        require(_amount > 0,"!>0");
convex-platform/contracts/contracts/PoolManagerSecondaryProxy.sol:104:        require(weight > 0, "must have weight");

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:

contracts/AuraLocker.sol:724:        uint256 epochIndex = _epoch > lastIndex ? lastIndex : _epoch;
contracts/AuraMath.sol:11:        return a < b ? a : b;
contracts/ExtraRewardsDistributor.sol:225:        epochIndex = _startIndex > epochIndex ? _startIndex : epochIndex;
convex-platform/contracts/contracts/interfaces/MathUtil.sol:12:        return a < b ? a : b;
convex-platform/contracts/contracts/ConvexMasterChef.sol:146:        uint256 clampedTo = _to > endBlock ? endBlock : _to;
convex-platform/contracts/contracts/ConvexMasterChef.sol:147:        uint256 clampedFrom = _from > endBlock ? endBlock : _from;

CrvDepositor.sol#setFees(): Tautology on "_lockIncentive >= 0" which is always true as _lockIncentive is uint256

As a variable of type uint will always be >= 0, such a check isn't necessary.

Consider deleting the >= 0 check L75:

File: CrvDepositor.sol
72:     function setFees(uint256 _lockIncentive) external{
73:         require(msg.sender==feeManager, "!auth");
74: 
75:         if(_lockIncentive >= 0 && _lockIncentive <= 30){ //@audit gas: Tautology on the 1st condition
76:             lockIncentive = _lockIncentive;
77:        }

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:

contracts/AuraStakingProxy.sol:
   90:         require(_outputBps > 9000 && _outputBps < 10000, "Invalid output bps");
  159:         require(_token != crv && _token != cvx && _token != cvxCrv, "not allowed");
  203:         require(address(_token) != crv && address(_token) != cvxCrv, "not allowed");

contracts/BalLiquidityProvider.sol:
  48:         require(_request.assets.length == 2 && _request.maxAmountsIn.length == 2, "!valid");
  57:             require(bal > 0 && bal == _request.maxAmountsIn[i], "!bal");

contracts/ExtraRewardsDistributor.sol:
  171:         require(_index > 0 && _index < rewardEpochs[_token].length - 1, "!past");

convex-platform/contracts/contracts/Booster.sol:
  220:         require(lockRewards != address(0) && rewardFactory != address(0), "!initialised"); 
  222:         require(_feeToken != address(0) && _feeDistro != address(0), "!addresses");
  278:         require(_lockFees >= 300 && _lockFees <= 1500, "!lockFees");
  279:         require(_stakerFees >= 300 && _stakerFees <= 1500, "!stakerFees");
  280:         require(_callerFees >= 10 && _callerFees <= 100, "!callerFees");
  313:         require(msg.sender==poolManager && !isShutdown, "!add");
  314:         require(_gauge != address(0) && _lptoken != address(0),"!param");

convex-platform/contracts/contracts/PoolManagerSecondaryProxy.sol:
  111:         require(!usedMap[_lptoken] && !usedMap[_gauge], "cant force used pool");

convex-platform/contracts/contracts/StashFactoryV2.sol:
  83:         require(!isV1 && !isV2 && !isV3,"stash version mismatch");

Using bool for storage incurs overhead

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.

I suggest replacing / 2 with >> 1 here:

contracts/AuraMath.sol:36:        return (a / 2) + (b / 2) + (((a % 2) + (b % 2)) / 2);

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.

ethereum/solidity#10695

Affected code:

  • Increment:
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.

Revert strings > 32 bytes:

contracts/AuraLocker.sol:197:        require(_rewardsToken != address(stakingToken), "Cannot add StakingToken as reward"); //@audit-issue : length == 33

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/:

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.

@code423n4 code423n4 added bug Something isn't working G (Gas Optimization) labels May 25, 2022
code423n4 added a commit that referenced this issue May 25, 2022
@0xMaharishi 0xMaharishi added the duplicate This issue or pull request already exists label May 28, 2022
@dmvt dmvt removed the duplicate This issue or pull request already exists label Jun 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working G (Gas Optimization)
Projects
None yet
Development

No branches or pull requests

3 participants