Aura Finance contest - BowTiedWardens's results

Lines of code

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/ExtraRewardsDistributor.sol#L93 https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/Booster.sol#L404 https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/ConvexMasterChef.sol#L221-L225 https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraBalRewardPool.sol#L146

Vulnerability details

As arbitrary ERC20 tokens can be passed, the amount here should be calculated every time to take into consideration a possible fee-on-transfer or deflation. Also, it's a good practice for the future of the solution.

Affected code:

contracts/ExtraRewardsDistributor.sol:
  93:         IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);

convex-platform/contracts/contracts/Booster.sol:
  404:         IERC20(lptoken).safeTransferFrom(msg.sender, staker, _amount);

convex-platform/contracts/contracts/ConvexMasterChef.sol:
  221:         pool.lpToken.safeTransferFrom(
  222              address(msg.sender),
  223              address(this),
  224              _amount
  225          );

File: AuraBalRewardPool.sol
  146:         stakingToken.safeTransferFrom(msg.sender, address(this), _amount);

Recommended Mitigation Steps

Use the balance before and after the transfer to calculate the received amount instead of assuming that it would be equal to the amount passed as a parameter.

As a template:

        uint256 balanceBefore = ERC20(token).balanceOf(address(this));
        ERC20(token).safeTransferFrom(msg.sender, address(this), amount);
        uint256 realReceivedAmount = ERC20(token).balanceOf(address(this)) - balanceBefore;

Table of Contents:

• [L-01] Deprecated safeApprove() function
• [L-02] Deprecated approve() function
• [L-03] Missing address(0) checks
• [L-04] Unbounded loop on array can lead to DoS
• [L-05] Add a timelock and event to critical functions
• [L-06] Failed transfer with low level call could be overlooked
• [L-07] _safeMint() should be used rather than _mint() wherever possible
• [L-08] BaseRewardPool:donate() CEIP not respected
• [L-09] AuraBalRewardPool:getReward() CEIP not respected
• [N-01] Using simple quotes for strings
• [N-02] Unused named returns
• [N-03] Deprecated library used for Solidity 0.8.+ : SafeMath
• [N-04] It's better to emit after all processing is done
• [N-05] CEI not respected with a call made to the contract's address

[L-01] Deprecated safeApprove() function

Using this deprecated function can lead to unintended reverts and potentially the locking of funds. A deeper discussion on the deprecation of this function is in OZ issue #2219 (OpenZeppelin/openzeppelin-contracts#2219). The OpenZeppelin ERC20 safeApprove() function has been deprecated, as seen in the comments of the OpenZeppelin code.

As recommended by the OpenZeppelin comment, I suggest replacing safeApprove() with safeIncreaseAllowance() or safeDecreaseAllowance() instead:

contracts/AuraBalRewardPool.sol:
  75:         rewardToken.safeApprove(_auraLocker, type(uint256).max);

contracts/AuraClaimZap.sol:
   98:         IERC20(crv).safeApprove(crvDepositWrapper, 0);
   99:         IERC20(crv).safeApprove(crvDepositWrapper, type(uint256).max);
  101:         IERC20(cvxCrv).safeApprove(cvxCrvRewards, 0);
  102:         IERC20(cvxCrv).safeApprove(cvxCrvRewards, type(uint256).max);
  104:         IERC20(cvx).safeApprove(locker, 0);
  105:         IERC20(cvx).safeApprove(locker, type(uint256).max);

contracts/AuraLocker.sol:
  240:         IERC20(cvxCrv).safeApprove(cvxcrvStaking, 0);
  241:         IERC20(cvxCrv).safeApprove(cvxcrvStaking, type(uint256).max);

contracts/AuraMerkleDrop.sol:
  131:             aura.safeApprove(address(auraLocker), 0);
  132:             aura.safeApprove(address(auraLocker), _amount);

contracts/AuraPenaltyForwarder.sol:
  41:         token.safeApprove(address(distributor), type(uint256).max);

contracts/AuraStakingProxy.sol:
  147:         IERC20(crv).safeApprove(crvDepositorWrapper, 0);
  148:         IERC20(crv).safeApprove(crvDepositorWrapper, type(uint256).max);
  150:         IERC20(cvxCrv).safeApprove(rewards, 0);
  151:         IERC20(cvxCrv).safeApprove(rewards, type(uint256).max);
  215:             _token.safeApprove(rewards, 0);
  216:             _token.safeApprove(rewards, type(uint256).max);

contracts/AuraVestedEscrow.sol:
  186:             rewardToken.safeApprove(address(auraLocker), claimable);

contracts/BalLiquidityProvider.sol:
  59:             tkn.safeApprove(address(bVault), 0);
  60:             tkn.safeApprove(address(bVault), bal);

contracts/CrvDepositorWrapper.sol:
  52:         IERC20(WETH).safeApprove(address(BALANCER_VAULT), type(uint256).max);
  53:         IERC20(BAL).safeApprove(address(BALANCER_VAULT), type(uint256).max);

convex-platform/contracts/contracts/BaseRewardPool4626.sol:
  40:         IERC20(asset).safeApprove(operator_, type(uint256).max);

convex-platform/contracts/contracts/Booster.sol:
  422:             IERC20(token).safeApprove(rewardContract,0);
  423:             IERC20(token).safeApprove(rewardContract,_amount);

convex-platform/contracts/contracts/CrvDepositor.sol:
  199:             IERC20(minter).safeApprove(_stakeAddress,0);
  200:             IERC20(minter).safeApprove(_stakeAddress,_amount);

convex-platform/contracts/contracts/VoterProxy.sol:
  176:             IERC20(_token).safeApprove(_gauge, 0);
  177:             IERC20(_token).safeApprove(_gauge, balance);
  193:         _asset.safeApprove(rewardDeposit, 0); 
  194:         _asset.safeApprove(rewardDeposit, balance); 
  244:         IERC20(crvBpt).safeApprove(escrow, 0);
  245:         IERC20(crvBpt).safeApprove(escrow, _value);
  255:         IERC20(crvBpt).safeApprove(escrow, 0);
  256:         IERC20(crvBpt).safeApprove(escrow, _value);

[L-02] Deprecated approve() function

While safeApprove() in itself is deprecated, it is still better than approve which is subject to a known front-running attack. Consider using safeApprove instead (or better: safeIncreaseAllowance()/safeDecreaseAllowance()):

File: CrvDepositorWrapper.sol
119:         require(IERC20(BALANCER_POOL_TOKEN).approve(crvDeposit, type(uint256).max), "!approval");

[L-03] Missing address(0) checks

In the constructors, the solution never checks for address(0) when setting the value of immutable variables. I suggest adding those checks.

List of immutable variables:

contracts/Aura.sol:
  26:     address public immutable vecrvProxy;

contracts/AuraBalRewardPool.sol:
  31:     address public immutable rewardManager;
  34:     address public immutable penaltyForwarder;

contracts/AuraClaimZap.sol:
  43:     address public immutable crv;
  44:     address public immutable cvx;
  45:     address public immutable cvxCrv;
  46:     address public immutable crvDepositWrapper;
  47:     address public immutable cvxCrvRewards;
  48:     address public immutable locker;
  49:     address public immutable owner;

contracts/AuraLocker.sol:
  105:     address public immutable cvxCrv;
  109:     address public immutable cvxcrvStaking;

contracts/AuraMerkleDrop.sol:
  28:     address public immutable penaltyForwarder;

contracts/AuraStakingProxy.sol:
  38:     address public immutable crv;
  39:     address public immutable cvx;
  40:     address public immutable cvxCrv;

contracts/BalLiquidityProvider.sol:
  19:     address private immutable provider;
  20:     address public immutable dao;

contracts/ClaimFeesHelper.sol:
  20:     address public immutable voterProxy;

contracts/CrvDepositorWrapper.sol:
   27:     address public immutable BAL;
   28:     address public immutable WETH;
   29:     address public immutable BALANCER_POOL_TOKEN;
  105:     address public immutable crvDeposit;

convex-platform/contracts/contracts/ArbitartorVault.sol:
  25:     address public immutable depositor;

convex-platform/contracts/contracts/BaseRewardPool.sol:
  67:     address public immutable operator;
  68:     address public immutable rewardManager;

convex-platform/contracts/contracts/Booster.sol:
  22:     address public immutable crv;
  23:     address public immutable voteOwnership;
  24:     address public immutable voteParameter;
  36:     address public immutable staker;
  37:     address public immutable minter;

convex-platform/contracts/contracts/BoosterOwner.sol:
  43:     address public immutable poolManager;
  44:     address public immutable booster;
  45:     address public immutable stashFactory;
  46:     address public immutable rescueStash;

convex-platform/contracts/contracts/CrvDepositor.sol:
  24:     address public immutable crvBpt;
  25:     address public immutable escrow;
  34:     address public immutable staker;
  35:     address public immutable minter;

convex-platform/contracts/contracts/ExtraRewardStashV3.sol:
  30:     address public immutable crv;

convex-platform/contracts/contracts/PoolManagerProxy.sol:
  15:     address public immutable pools;

convex-platform/contracts/contracts/PoolManagerSecondaryProxy.sol:
  19:     address public immutable gaugeController;
  20:     address public immutable pools;
  21:     address public immutable booster;

convex-platform/contracts/contracts/PoolManagerV3.sol:
  18:     address public immutable pools;
  19:     address public immutable gaugeController;

convex-platform/contracts/contracts/RewardFactory.sol:
  24:     address public immutable operator;
  25:     address public immutable crv;

convex-platform/contracts/contracts/RewardHook.sol:
  24:     address public immutable stash;
  25:     address public immutable rewardToken;

convex-platform/contracts/contracts/StashFactoryV2.sol:
  23:     address public immutable operator;
  24:     address public immutable rewardFactory;
  25:     address public immutable proxyFactory;

convex-platform/contracts/contracts/TokenFactory.sol:
  20:     address public immutable operator;

convex-platform/contracts/contracts/VirtualBalanceRewardPool.sol:
  87:     address public immutable operator;

convex-platform/contracts/contracts/VoterProxy.sol:
  23:     address public immutable crv;
  24:     address public immutable crvBpt;
  26:     address public immutable escrow;

[L-04] Unbounded loop on array can lead to DoS

As this array can grow quite large, the transaction's gas cost could exceed the block gas limit and make it impossible to call this function at all.

convex-platform/contracts/contracts/ConvexMasterChef.sol:
  180:         for (uint256 pid = 0; pid < length; ++pid) {


convex-platform/contracts/contracts/ExtraRewardStashV3.sol:
  199:         for(uint i=0; i < tCount; i++){

Consider introducing a reasonable upper limit based on block gas limits and adding a method to remove elements in the array.

[L-05] Add a timelock and event to critical functions

It is a good practice to give time for users to react and adjust to critical changes. A timelock provides more guarantees and reduces the level of trust required, thus decreasing risk for users. It also indicates that the project is legitimate (less risk of a malicious owner making a sandwich attack on a user).

Consider adding a timelock to:

convex-platform/contracts/contracts/Booster.sol:
  272:     function setFees(uint256 _lockFees, uint256 _stakerFees, uint256 _callerFees, uint256 _platform) external{

Consider adding a timelock and event to:

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

[L-06] Failed transfer with low level call could be overlooked

Low-level calls return true even if the account called is non-existent (per EVM design). Account existence must be checked prior to calling.

Affected code:

convex-platform/contracts/contracts/BoosterOwner.sol:
  187:         (bool success, bytes memory result) = _to.call{value:_value}(_data);

convex-platform/contracts/contracts/VoterProxy.sol:
  352:         (bool success, bytes memory result) = _to.call{value:_value}(_data);

Consider checking for account-existence before the call() to make this safely extendable to user-controlled address contexts in future. At the very least, check for address(0).

[L-07] _safeMint() should be used rather than _mint() wherever possible

_mint() is discouraged in favor of _safeMint() which ensures that the recipient is either an EOA or implements IERC721Receiver. Both open OpenZeppelin and solmate have versions of this function so that NFTs aren't lost if they're minted to contracts that cannot transfer them back out.

File: contracts/NFTLoanTicket.sol (line 34)

contracts/Aura.sol:
   71:         _mint(_to, _amount);
  121:             _mint(_to, amount);
  131:         _mint(_to, _amount);

convex-platform/contracts/contracts/cCrv.sol:
  49:         _mint(_to, _amount);

convex-platform/contracts/contracts/DepositToken.sol:
  50:         _mint(_to, _amount);

[L-08] BaseRewardPool:donate() CEIP not respected

Check Effects Interactions pattern should always be respected, be it for the current state of the solution, the future of the solution or a future fork of the solution:

File: BaseRewardPool.sol
311:     /**
312:      * @dev Donate some extra rewards to this contract
313:      */
314:     function donate(uint256 _amount) external returns(bool){
315:         IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), _amount); // @audit-info LOW CEI not respected, this is low as this call is happening to the contract's address 
316:         queuedRewards = queuedRewards.add(_amount);
317:     }

[L-09] AuraBalRewardPool:getReward() CEIP not respected

Check Effects Interactions pattern should always be respected, be it for the current state of the solution, the future of the solution or a future fork of the solution:

File: AuraBalRewardPool.sol
152:     function AuraBalRewardPool(
153:         uint256 amount,
154:         bool claim,
155:         bool lock
156:     ) public updateReward(msg.sender) returns (bool) {
157:         require(amount > 0, "RewardPool : Cannot withdraw 0");
158: 
159:         _totalSupply = _totalSupply.sub(amount);
160:         _balances[msg.sender] = _balances[msg.sender].sub(amount);
161: 
162:         stakingToken.safeTransfer(msg.sender, amount); // @audit-info HIGH CEI not respected which can open attack vectors for re-entrancy attacks. Consider moving this at the final and add a reentrancy guard
163:         emit Withdrawn(msg.sender, amount);
164: 
165:         if (claim) {
166:             getReward(lock);
167:         }
168: 
169:         return true;
170:     }

[N-01] Using simple quotes for strings

To be consistent with the style used in the solution, consider using double quotes instead of simple quotes here:

convex-platform/contracts/contracts/BaseRewardPool.sol:
  211:         require(_amount > 0, 'RewardPool : Cannot stake 0');
  227:         require(amount > 0, 'RewardPool : Cannot withdraw 0');

[N-02] Unused named returns

Using both named returns and a return statement isn't necessary. Removing one of those can improve code clarity:

contracts/AuraLocker.sol:
  648:     function balanceOf(address _user) external view returns (uint256 amount) {

  653:     function balanceAtEpochOf(uint256 _epoch, address _user) public view returns (uint256 amount) {

  739:     function findEpochId(uint256 _time) public view returns (uint256 epoch) {

  769:     function claimableRewards(address _account) external view returns (EarnedData[] memory userRewards) {

convex-platform/contracts/contracts/VoterProxy.sol:
  188:     function withdraw(IERC20 _asset) external returns (uint256 balance) {

[N-03] Deprecated library used for Solidity 0.8.+ : SafeMath

Affected code:

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;

[N-04] It's better to emit after all processing is done

Affected code:

contracts/AuraBalRewardPool.sol:
  163:         emit Withdrawn(msg.sender, amount);

contracts/AuraLocker.sol:
  365:         emit Withdrawn(msg.sender, amt, false);
  479:         emit DelegateChanged(msg.sender, oldDelegatee, newDelegatee);

convex-platform/contracts/contracts/BaseRewardPool.sol:
  238:         emit Withdrawn(msg.sender, amount);
  291:             emit RewardPaid(_account, reward);

[N-05] CEI not respected with a call made to the contract's address

Affected code:

File: BaseRewardPool.sol
311:     /**
312:      * @dev Donate some extra rewards to this contract
313:      */
314:     function donate(uint256 _amount) external returns(bool){
315:         IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), _amount); // @audit-info LOW CEI not respected, this is low as this call is happening to the contract's address 
316:         queuedRewards = queuedRewards.add(_amount);
317:     }

Table of Contents:

• Cheap Contract Deployment Through Clones
• Help the optimizer by saving a storage variable's reference instead of repeatedly fetching it
• AuraLocker.emergencyWithdraw(): Use of the memory keyword when storage should be used
• Caching storage values in memory
• Not using SafeMath can save gas on arithmetics operations that can't underflow/overflow
• SafeMath is not needed when using Solidity version 0.8.11
• Unchecking arithmetics operations that can't underflow/overflow
• Booster: Tighly pack storage variables
• CrvDepositor: Tighly pack storage variables
• ExtraRewardStashV3: Tighly pack storage variables
• Boolean comparisons
• > 0 is less efficient than != 0 for unsigned integers (with proof)
• >= is cheaper than > (and <= cheaper than <)
• CrvDepositor.sol#setFees(): Tautology on "_lockIncentive >= 0" which is always true as _lockIncentive is uint256
• Splitting require() statements that use && saves gas
• Using bool for storage incurs overhead
• Shift Right instead of Dividing by 2
• An array's length should be cached to save gas in for-loops
• ++i costs less gas compared to i++ or i += 1
• Increments/Decrements can be unchecked
• Use calldata instead of memory
• No need to explicitly initialize variables with default values
• Upgrade pragma to at least 0.8.4
• Reduce the size of error messages (Long revert Strings)
• Use Custom Errors instead of Revert Strings to save Gas

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:

• https://github.com/porter-finance/v1-core/issues/15#issuecomment-1035639516
• https://github.com/porter-finance/v1-core/pull/34

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.