Gas Optimizations

[code423n4]

• Repeated external call to usdm.balanceOf(address(this):

  require(usdm.balanceOf(address(this)) >= totalLiquidity.usdm, "<liquidity");
  ...
  uint256 addingLiquidity = (usdm.balanceOf(address(this)) / step) * step;

• Don't need to call owner() here, you can just use msg.sender:

  function recoverERC20(address tokenAddress, uint256 tokenAmount)
    external
    onlyOwner
  {
    ...
    IERC20(tokenAddress).safeTransfer(owner(), tokenAmount);

• Long revert messages, e.g.: "Previous rewards period must be complete before changing the duration for the new period" Shortening revert strings to fit in 32 bytes will decrease deployment time gas and will decrease runtime gas when the revert condition has been met.

• Variables that are set in the constructor and can't be changed can be marked as immutable:

  uint256 public step;
  uint256 public startLiquidity;
  uint public startBlock;
  uint public endBlock;

• Should be constant:

  uint public concurPerBlock = 100000 gwei; // concur tokens transferred per block
  uint private _concurShareMultiplier = 1e18;
  uint private _perMille = 1000; // 100%

• Variables and events that are not used in any meaningful way can be removed or used where they were intended:

  uint256 public startLiquidity;
  event EmergencyWithdraw(address indexed user, uint indexed _pid, uint _amount);

• Would be cheapier to use local variables when emitting events, e.g.:

  rewardsDuration = _rewardsDuration;
  emit RewardsDurationUpdated(rewardsDuration);
  
  rewardsDistribution = _rewardsDistribution;
  emit RewardsDistributionUpdated(rewardsDistribution);

• Usually under normal conditions, the first check is not neccessary, as GRACE_PERIOD > block.timestamp should always revert:

  require(activated[_token] != 0 && activated[_token] + GRACE_PERIOD > block.timestamp, "too late");

• This could be inlined to optimize for gas usage, from:

  bool transferSuccess = false;
  if (_amount > concurBalance) {
    transferSuccess = concur.transfer(_to, concurBalance);
  } else {
    transferSuccess = concur.transfer(_to, _amount);
  }
  require(transferSuccess, "safeConcurTransfer: transfer failed");

  to:

  if (_amount > concurBalance) {
    require(concur.transfer(_to, concurBalance), "safeConcurTransfer: transfer failed");
  } else {
    require(transferSuccess = concur.transfer(_to, _amount), "safeConcurTransfer: transfer failed");
  }

• Repeated access of storage variables should be cached, e.g. convexBooster is accessed 3 times:

  lpToken.safeApprove(convexBooster, _amount);
  IConvexDeposits(convexBooster).deposit(_pid, _amount, true);
  lpToken.safeApprove(convexBooster, 0);

  masterChef twice:

  uint256 pid = masterChef.pid(address(lpToken));
  masterChef.deposit(msg.sender, pid, _amount);

  convexPool[_pid] twice:

  IRewardStaking(convexPool[_pid]).withdrawAndUnwrap(_amount, false);
  IERC20 lpToken = IERC20(
  IRewardStaking(convexPool[_pid]).poolInfo(_pid).lptoken
  );

  registeredRewards[_pid] twice:

  registeredRewards[_pid][crv] = CRV_INDEX + 1; //mark registered at index+1
  registeredRewards[_pid][cvx] = CVX_INDEX + 1; //mark registered at index+1

• The initialization to default variables is not neccessary:

  uint256 public periodFinish = 0;
  uint256 public rewardRate = 0;

• If the same MasterChef implementation will be used, then the pid of the token can't change, so you should cache it once (in the constructor call), and do not make the external calls again and again in stake and withdraw functions of StakingRewards:

  uint256 pid = masterChef.pid(address(stakingToken));

• Contract MasterChef has declared a compiler version of ^0.8.11 but it still uses the SafeMath library:

  using SafeMath for uint;
  // e.g.:
  return _to.sub(_from);
  uint concurReward = multiplier.mul(concurPerBlock).mul(pool.allocPoint).div(totalAllocPoint);
  accConcurPerShare = accConcurPerShare.add(concurReward.mul(_concurShareMultiplier).div(lpSupply));

  Overflow/underflow protection is built-in starting from version 0.8, so you don't need to use SafeMath here.

• Would be cheaper if you checked not the storage but the _amount paramater against 0 in the first sentence:

  require(user.amount > 0, "MasterChef: nothing to withdraw");
  require(user.amount >= _amount, "MasterChef: withdraw not allowed");

• currentEpoch() is called twice, should cache after the first call:

  require(request.epoch < currentEpoch() && deposits[_pid][msg.sender].epoch + 1 < currentEpoch(), "wait");

• You already have the address of rewardToken() here, no need to fetch it again:

  address extraToken = IRewardStaking(extraPool).rewardToken();
  ...
  else if (registeredRewards[_pid][extraToken] == 0) {
    //add new token to list
    rewards[_pid].push(
        RewardType({
            token: IRewardStaking(extraPool).rewardToken()

• In _calcRewardIntegral I think these statements:

  //update remaining reward here since balance could have changed if claiming
  if (bal != reward.remaining) {
      reward.remaining = uint128(bal);
  }

  should be placed right after this:

  uint256 d_reward = bal - reward.remaining;

  or you can even return and skip all the heavy calculations and 0 value transfers if the balance hasn't changed.

[GalloDaSballo]

Repeated external call to usdm.balanceOf(address(this):

Would save 100 for the call + 100 for SLOAD = 200 - 6 = 194

Don't need to call owner() here, you can just use msg.sender:

Saves 8 gas (JUMP)

Long revert messages, e.g.:

2500 (1 message shown)

Variables that are set in the constructor and can't be changed can be marked as immutable:

7 * 2100 = 14700

Variables and events that are not used in any meaningful way can be removed or used where they were intended:

Valid but not at runtime

Would be cheapier to use local variables when emitting events, e.g.:

194 gas saved

Usually under normal conditions, the first check is not neccessary, as GRACE_PERIOD > block.timestamp should always revert:

Saves 100 for SLOAD + 3 for check 103

This could be inlined to optimize for gas usage, from:

Saves 9 gas

Repeated access of storage variables should be cached, e.g. convexBooster is accessed 3 times:

94 + 94 - 6 = 182 94 94 Total = 370

The initialization to default variables is not necessary:

200

If the same MasterChef implementation will be used, then the pid of the token can't change, so you should cache it once (in the constructor call), and do not make the external calls again and again in stake and withdraw functions of StakingRewards:

Saves 100 gas

Contract MasterChef has declared a compiler version of ^0.8.11 but it still uses the SafeMath library:

7 * 20 = 140 gas saved

Would be cheaper if you checked not the storage but the _amount paramater against 0 in the first sentence:

Valid but only for "bad path"

currentEpoch() is called twice, should cache after the first call:

91 gas saved (97 -6)

You already have the address of rewardToken() here, no need to fetch it again:

191 (100 call 97 - 6 SLOAD)

In _calcRewardIntegral I think these statements:

Only for bad path

Total Gas Saved: 18800