veToken Finance contest - 0xf15ers's results

1. Use of deprecated safeApprove of openZeppelin's SafeERC20

• The use of safeApprove() is deprecated as described in comments
• Use of safeIncreaseAllowance() and safeDecreaseAllowance() is recommended. It appears in following codebase:

./contracts/VoterProxy.sol:101:            IERC20(_token).safeApprove(_gauge, 0);
./contracts/VoterProxy.sol:102:            IERC20(_token).safeApprove(_gauge, balance);
./contracts/VoterProxy.sol:153:        IERC20(veAsset).safeApprove(escrow, 0);
./contracts/VoterProxy.sol:154:        IERC20(veAsset).safeApprove(escrow, _value);
./contracts/VoterProxy.sol:161:        IERC20(veAsset).safeApprove(escrow, 0);
./contracts/VoterProxy.sol:162:        IERC20(veAsset).safeApprove(escrow, _value);
./contracts/Booster.sol:375:            IERC20(token).safeApprove(rewardContract, _amount);
./contracts/VE3DRewardPool.sol:289:                IERC20(_rewardToken).safeApprove(rewardTokenInfo[_rewardToken].veAssetDeposits, 0);
./contracts/VE3DRewardPool.sol:290:                IERC20(_rewardToken).safeApprove(
./contracts/VE3DRewardPool.sol:303:                    IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
./contracts/VE3DRewardPool.sol:307:                    IERC20(rewardTokenInfo[_rewardToken].ve3Token).safeApprove(
./contracts/VeAssetDepositor.sol:178:            IERC20(minter).safeApprove(_stakeAddress, _amount);
./contracts/VE3DLocker.sol:211:                IERC20(rewardData[_rewardsToken].ve3Token).safeApprove(
./contracts/VE3DLocker.sol:218:                IERC20(rewardData[_rewardsToken].ve3Token).safeApprove(
./contracts/VE3DLocker.sol:224:                IERC20(_rewardsToken).safeApprove(rewardData[_rewardsToken].veAssetDeposits, 0);
./contracts/VE3DLocker.sol:225:                IERC20(_rewardsToken).safeApprove(

2. Missing zero-address checks in setter functions and constructors

• Missing zero-address checks may make the protocol non-functional. For example, while updating the setPoolManager() of Booster.sol, accidentally setting to zero address can create unintended consequences.
• Same practice is recommended while setting addresses in constructors too.

Consider adding zero-address checks i.e. require(newAddress != address(0))

• VeAssetDepositor.sol#L38
• VoterProxy.sol#L41
• Booster.sol#L104

3. Use two-step procedure for critiical role changes

To protect from setting to dead/inactive addresses and other unintended consequences, use of two-step procedure is recommended for critical functions.

For example: for setting owner, following pattern can be used

address owner;
address temporaryOwner;

function setOwner(address owner_) external {
  require(msg.sender == owner);
  temporaryOwner = owner_;
}

function claimOwnership() external {
  require(msg.sender == temporaryOwner;
  owner = temporaryOwner;
  temporaryOwner = address(0);
}

Examples: setOwner(), setFeeManager(), setPoolManager(), setArbitrator() in Booster.sol

1. !=0 is cheaper than >0

• In require statement, using !=0 is cheaper than using >0 for uints

> grep -rn "./contracts" -e "require(.* > 0"
./contracts/VE3DRewardPool.sol:210:        require(_amount > 0, "RewardPool : Cannot stake 0");
./contracts/VE3DRewardPool.sol:234:        require(_amount > 0, "RewardPool : Cannot stake 0");
./contracts/VE3DRewardPool.sol:253:        require(_amount > 0, "RewardPool : Cannot withdraw 0");
./contracts/VeAssetDepositor.sol:132:        require(_amount > 0, "!>0");
./contracts/VirtualBalanceRewardPool.sol:137:        // require(amount > 0, 'VirtualDepositRewardPool: Cannot stake 0');
./contracts/VirtualBalanceRewardPool.sol:143:        //require(amount > 0, 'VirtualDepositRewardPool : Cannot withdraw 0');
./contracts/VE3DLocker.sol:180:        require(rewardData[_rewardsToken].lastUpdateTime > 0);
./contracts/VE3DLocker.sol:520:        require(_amount > 0, "Cannot stake 0");
./contracts/VE3DLocker.sol:670:        require(locked > 0, "no exp locks");
./contracts/VE3DLocker.sol:781:        require(_reward > 0, "No reward");
./contracts/BaseRewardPool.sol:173:        require(_amount > 0, "RewardPool : Cannot stake 0");
./contracts/BaseRewardPool.sol:196:        require(_amount > 0, "RewardPool : Cannot stake 0");
./contracts/BaseRewardPool.sol:215:        require(amount > 0, "RewardPool : Cannot withdraw 0");

2. Initilization to default values can be omitted

uint variables are set to zero by default, initializing them to zero (for storage variables) causes extra gas.

• VE3DLocker.sol#L154
• ExtraRewardStashV1.sol#L29

3. Use solidity custom errors to save gas

solidity 0.8.4 introduces custom errors which are cheaper than using revert strings in terms of gas Use the custom error patterns to reduce gas cost.

for eg.

  // Before
  require(condition, "Revert strings");

  // After
  error CustomError();
  if (!condition) {
    revert CustomError();
  }

more details can be found here

4. using prefix increment in loops can save some gas

Prefix increment operation(++i) is more gas effiecient than postfix increment(i++), while few instance of postfix increment doesn't makes much difference in overall gas, in loops these increments are called a large number of times resulting in significant gas savings.

// Before
for (uint i = 0; i < _prices.length; i++) {
}

// After
for (uint i = 0; i < _prices.length; ++i) {
}

Also, since the loop increment starts from zero and only increases, there is no chance of overflow. So the increment can also be added inside a unchecked block so save further gas

for (uint i = 0; i < _prices.length;) {
  unchecked {
    ++i;
  }
}

The instances of prefix increment can be found as follows

> grep -rn './contracts' -e 'for.*i++'
./contracts/VoterProxy.sol:217:            for (uint256 i = 0; i < _tokenVote.length; i++) {
./contracts/Booster.sol:329:        for (uint256 i = 0; i < poolInfo.length; i++) {
./contracts/ExtraRewardStashV3.sol:84:        for (uint256 i = 0; i < maxRewards; i++) {
./contracts/ExtraRewardStashV3.sol:126:        for (uint256 i = 0; i < tokenCount; i++) {
./contracts/VE3DRewardPool.sol:148:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
./contracts/VE3DRewardPool.sol:214:        for (uint256 i = 0; i < length; i++) {
./contracts/VE3DRewardPool.sol:238:        for (uint256 i = 0; i < length; i++) {
./contracts/VE3DRewardPool.sol:257:        for (uint256 i = 0; i < length; i++) {
./contracts/VE3DRewardPool.sol:281:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
./contracts/VE3DRewardPool.sol:326:            for (uint256 i = 0; i < length; i++) {
./contracts/ExtraRewardStashV2.sol:71:            for (uint256 i = 0; i < length; i++) {
./contracts/ExtraRewardStashV2.sol:78:            for (uint256 i = 0; i < length; i++) {
./contracts/ExtraRewardStashV2.sol:137:        for (uint256 i = 0; i < maxRewards; i++) {
./contracts/ExtraRewardStashV2.sol:181:        for (uint256 i = 0; i < tokenCount; i++) {
./contracts/ExtraRewardStashV2.sol:213:        for (uint256 i = 0; i < tokenCount; i++) {
./contracts/RewardFactory.sol:49:        for (uint256 i = 0; i < length; i++) {
./contracts/RewardFactory.sol:66:        for (uint256 i = 0; i < length; i++) {
./contracts/VE3DLocker.sol:207:        for (uint256 i; i < rewardTokens.length; i++) {
./contracts/VE3DLocker.sol:286:        for (uint256 i = 0; i < userRewards.length; i++) {
./contracts/VE3DLocker.sol:425:        for (uint256 i = 0; i < 128; i++) {
./contracts/VE3DLocker.sol:457:        for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
./contracts/VE3DLocker.sol:640:            for (uint256 i = nextUnlockIndex; i < length; i++) {
./contracts/VE3DLocker.sol:720:        for (uint256 i; i < rewardTokens.length; i++) {
./contracts/VE3DLocker.sol:803:            for (uint256 i = 0; i < rewardTokens.length; i++) {
./contracts/BaseRewardPool.sol:176:        for (uint256 i = 0; i < extraRewards.length; i++) {
./contracts/BaseRewardPool.sol:199:        for (uint256 i = 0; i < extraRewards.length; i++) {
./contracts/BaseRewardPool.sol:218:        for (uint256 i = 0; i < extraRewards.length; i++) {
./contracts/BaseRewardPool.sol:245:        for (uint256 i = 0; i < extraRewards.length; i++) {
./contracts/BaseRewardPool.sol:282:            for (uint256 i = 0; i < extraRewards.length; i++) {

5. array length in loops can be cached instead of calculating in every iteration

The loop bounds are calculated with array.length which are calculated in every loop iterations which can result in high gas. The array length can be cached instead of calculating in every loop iteration to save gas.

// Before
for (uint i = 0; i < _tokenVote.length; i++) {
}

// After
uint len = _tokenVote.length;
for (uint i = 0; i < len; i++) {
}

The instances where this pattern can be applied is found as follows

> grep -rn './contracts' -e 'for.*[.]length'
./contracts/VoterProxy.sol:217:            for (uint256 i = 0; i < _tokenVote.length; i++) {
./contracts/Booster.sol:329:        for (uint256 i = 0; i < poolInfo.length; i++) {
./contracts/VE3DRewardPool.sol:148:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
./contracts/VE3DRewardPool.sol:281:        for (uint256 i = 0; i < rewardTokens.length(); i++) {
./contracts/VE3DLocker.sol:207:        for (uint256 i; i < rewardTokens.length; i++) {
./contracts/VE3DLocker.sol:286:        for (uint256 i = 0; i < userRewards.length; i++) {
./contracts/VE3DLocker.sol:315:        for (uint256 i = locks.length - 1; i + 1 != 0; i--) {
./contracts/VE3DLocker.sol:360:        for (uint256 i = locks.length - 1; i + 1 != 0; i--) {
./contracts/VE3DLocker.sol:457:        for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
./contracts/VE3DLocker.sol:720:        for (uint256 i; i < rewardTokens.length; i++) {
./contracts/VE3DLocker.sol:803:            for (uint256 i = 0; i < rewardTokens.length; i++) {
./contracts/BaseRewardPool.sol:176:        for (uint256 i = 0; i < extraRewards.length; i++) {
./contracts/BaseRewardPool.sol:199:        for (uint256 i = 0; i < extraRewards.length; i++) {
./contracts/BaseRewardPool.sol:218:        for (uint256 i = 0; i < extraRewards.length; i++) {
./contracts/BaseRewardPool.sol:245:        for (uint256 i = 0; i < extraRewards.length; i++) {
./contracts/BaseRewardPool.sol:282:            for (uint256 i = 0; i < extraRewards.length; i++) {