Aura Finance contest - Kaiziron's results

Low risk issues

Unspecific compiler version pragma

Information : L003 - Unspecific Compiler Version Pragma Consensys Audit of 1inch Solidity docs

Instances include :

ClaimFeesHelper.sol:2:pragma solidity ^0.8.11;
mocks/MockAuraMath.sol:2:pragma solidity ^0.8.11;
AuraVestedEscrow.sol:2:pragma solidity ^0.8.11;
BalLiquidityProvider.sol:2:pragma solidity ^0.8.11;
AuraStakingProxy.sol:2:pragma solidity ^0.8.11;
AuraLocker.sol:2:pragma solidity ^0.8.11;
AuraMinter.sol:2:pragma solidity ^0.8.11;
AuraMerkleDrop.sol:2:pragma solidity ^0.8.11;
Aura.sol:2:pragma solidity ^0.8.11;
ExtraRewardsDistributor.sol:2:pragma solidity ^0.8.11;
AuraBalRewardPool.sol:2:pragma solidity ^0.8.11;
AuraMath.sol:2:pragma solidity ^0.8.11;
AuraPenaltyForwarder.sol:2:pragma solidity ^0.8.11;
AuraClaimZap.sol:2:pragma solidity ^0.8.11;

Recommendation

It is recommended to use a concrete compiler version, for example :

ClaimFeesHelper.sol:2:pragma solidity 0.8.11;

Don't use deprecated library functions

Information : L005 - Do not use Deprecated Library Functions SafeERC20.sol - safeApprove

Instances include :

AuraVestedEscrow.sol:186:            rewardToken.safeApprove(address(auraLocker), claimable);
BalLiquidityProvider.sol:59:            tkn.safeApprove(address(bVault), 0);
BalLiquidityProvider.sol:60:            tkn.safeApprove(address(bVault), bal);
CrvDepositorWrapper.sol:52:        IERC20(WETH).safeApprove(address(BALANCER_VAULT), type(uint256).max);
CrvDepositorWrapper.sol:53:        IERC20(BAL).safeApprove(address(BALANCER_VAULT), type(uint256).max);
AuraStakingProxy.sol:147:        IERC20(crv).safeApprove(crvDepositorWrapper, 0);
AuraStakingProxy.sol:148:        IERC20(crv).safeApprove(crvDepositorWrapper, type(uint256).max);
AuraStakingProxy.sol:150:        IERC20(cvxCrv).safeApprove(rewards, 0);
AuraStakingProxy.sol:151:        IERC20(cvxCrv).safeApprove(rewards, type(uint256).max);
AuraStakingProxy.sol:215:            _token.safeApprove(rewards, 0);
AuraStakingProxy.sol:216:            _token.safeApprove(rewards, type(uint256).max);
AuraLocker.sol:240:        IERC20(cvxCrv).safeApprove(cvxcrvStaking, 0);
AuraLocker.sol:241:        IERC20(cvxCrv).safeApprove(cvxcrvStaking, type(uint256).max);
AuraMerkleDrop.sol:131:            aura.safeApprove(address(auraLocker), 0);
AuraMerkleDrop.sol:132:            aura.safeApprove(address(auraLocker), _amount);
AuraBalRewardPool.sol:75:        rewardToken.safeApprove(_auraLocker, type(uint256).max);
AuraPenaltyForwarder.sol:41:        token.safeApprove(address(distributor), type(uint256).max);
AuraClaimZap.sol:98:        IERC20(crv).safeApprove(crvDepositWrapper, 0);
AuraClaimZap.sol:99:        IERC20(crv).safeApprove(crvDepositWrapper, type(uint256).max);
AuraClaimZap.sol:101:        IERC20(cvxCrv).safeApprove(cvxCrvRewards, 0);
AuraClaimZap.sol:102:        IERC20(cvxCrv).safeApprove(cvxCrvRewards, type(uint256).max);
AuraClaimZap.sol:104:        IERC20(cvx).safeApprove(locker, 0);
AuraClaimZap.sol:105:        IERC20(cvx).safeApprove(locker, type(uint256).max);

Recommendation

Usage of deprecated library functions, for instance safeApprove from OpenZeppelin's SafeERC20 library is discouraged, it is recommended to use safeIncreaseAllowance and safeDecreaseAllowance instead, for example :

AuraVestedEscrow.sol:186:            rewardToken.safeIncreaseAllowance(address(auraLocker), claimable);

Unsafe ERC20 Operation(s)

Information : L001 - Unsafe ERC20 Operation(s)

Instances include :

mocks/balancer/MockBalancerVault.sol:67:            IERC20(tokenB).transferFrom(funds.sender, address(this), singleSwap.amount);
mocks/balancer/MockBalancerVault.sol:68:            IERC20(tokenA).transfer(funds.recipient, singleSwap.amount);
mocks/balancer/MockBalancerVault.sol:71:            IERC20(tokenA).transferFrom(funds.sender, address(this), singleSwap.amount);
mocks/balancer/MockBalancerVault.sol:72:            IERC20(tokenB).transfer(funds.recipient, singleSwap.amount);
mocks/curve/MockCurveVoteEscrow.sol:48:        IERC20(token).transferFrom(msg.sender, address(this), amount);
mocks/curve/MockCurveVoteEscrow.sol:58:        IERC20(token).transferFrom(msg.sender, address(this), amount);
mocks/curve/MockCurveVoteEscrow.sol:78:        IERC20(token).transfer(msg.sender, amount);
mocks/curve/MockCurveMinter.sol:21:        crv.transfer(msg.sender, rate);
mocks/curve/MockCurveGauge.sol:25:        IERC20(lp_token).transferFrom(msg.sender, address(this), amount);
mocks/curve/MockCurveGauge.sol:30:        IERC20(lp_token).transfer(msg.sender, amount);
mocks/curve/MockCurveGauge.sol:37:            IERC20(reward_tokens[i]).transfer(msg.sender, amount);

Recommendation

It is recommended to always use OpenZeppelin's SafeERC20 library, for example :

import {SafeERC20} from "openzeppelin/token/utils/SafeERC20.sol";
...
IERC20(tokenB).safeTransferFrom(funds.sender, address(this), singleSwap.amount);

Non-critical issues

Typos

Instances include:

distirbuted, constructoor, transferrable

Aura.sol:18: *          distirbuted along a supply curve (cliffs etc). Fork of ConvexToken.
ExtraRewardsDistributor.sol:33:     * @dev Simple constructoor
AuraBalRewardPool.sol:55:     * @dev Simple constructoor
mocks/curve/MockCurveVoteEscrow.sol:28:        revert("Not transferrable");
mocks/curve/MockCurveVoteEscrow.sol:36:        revert("Not transferrable");

Don't explicitly initialize variables with the default value

Uninitialized variables are assigned with the default value of their type, initializing a variable with its default value costs unnecessary gas.

Instances include :

mocks/balancer/MockFeeDistro.sol:19:        for (uint256 i = 0; i < _tokens.length; i++) {
mocks/balancer/MockFeeDistro.sol:38:        for (uint256 i = 0; i < tokens.length; i++) {
mocks/curve/MockCurveGauge.sol:36:        for (uint256 i = 0; i < reward_tokens.length; i++) {
AuraVestedEscrow.sol:99:        uint256 totalAmount = 0;
AuraVestedEscrow.sol:100:        for (uint256 i = 0; i < _recipient.length; i++) {
BalLiquidityProvider.sol:51:        for (uint256 i = 0; i < 2; i++) {
AuraLocker.sol:72:    uint256 public queuedCvxCrvRewards = 0;
AuraLocker.sol:174:            for (uint256 i = 0; i < rewardTokensLength; i++) {
AuraLocker.sol:381:        uint256 reward = 0;
AuraLocker.sol:485:        uint256 futureUnlocksSum = 0;
AuraLocker.sol:540:                    uint256 unlocksSinceLatestCkpt = 0;
AuraLocker.sol:630:        uint256 low = 0;
AuraLocker.sol:773:        for (uint256 i = 0; i < userRewardsLength; i++) {
AuraMerkleDrop.sol:29:    uint256 public pendingPenalty = 0;
ExtraRewardsDistributor.sol:231:        uint256 claimableTokens = 0;
AuraBalRewardPool.sol:35:    uint256 public pendingPenalty = 0;
AuraBalRewardPool.sol:38:    uint256 public periodFinish = 0;
AuraBalRewardPool.sol:39:    uint256 public rewardRate = 0;
AuraClaimZap.sol:143:        for (uint256 i = 0; i < rewardContracts.length; i++) {
AuraClaimZap.sol:147:        for (uint256 i = 0; i < extraRewardContracts.length; i++) {
AuraClaimZap.sol:151:        for (uint256 i = 0; i < tokenRewardContracts.length; i++) {
AuraVestedEscrow.sol:33:    bool public initialised = false;
AuraLocker.sol:114:    bool public isShutdown = false;

Recommendation

It is recommended to initialize variables without assigning them the default value, for example :

AuraVestedEscrow.sol:99:        uint256 totalAmount;
AuraVestedEscrow.sol:33:    bool public initialised;

Cache array length outside of for loop

Caching the array length outside a loop saves reading it on each iteration, as long as the array's length is not changed during the loop.

Instances include :

mocks/balancer/MockFeeDistro.sol:19:        for (uint256 i = 0; i < _tokens.length; i++) {
mocks/balancer/MockFeeDistro.sol:38:        for (uint256 i = 0; i < tokens.length; i++) {
mocks/curve/MockCurveGauge.sol:36:        for (uint256 i = 0; i < reward_tokens.length; i++) {
AuraVestedEscrow.sol:100:        for (uint256 i = 0; i < _recipient.length; i++) {
AuraLocker.sol:696:        for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
AuraClaimZap.sol:143:        for (uint256 i = 0; i < rewardContracts.length; i++) {
AuraClaimZap.sol:147:        for (uint256 i = 0; i < extraRewardContracts.length; i++) {
AuraClaimZap.sol:151:        for (uint256 i = 0; i < tokenRewardContracts.length; i++) {

Recommendation

It is recommended to cache the array length on a variable before running the loop, then it doesn't need to read the length on every iteration, which cost gas, for example :

uint256 len = _tokens.length;
for (uint256 i = 0; i < len; i++) {

Use != 0 instead of > 0 when comparing unsigned integers

!= 0 will do the same as > 0 for unsigned integers, but != 0 costs less gas compared to > 0 for unsigned integers in require statements with the optimizer enabled.

Instances include :

mocks/curve/MockCurveVoteEscrow.sol:43:        require(amount > 0, "!amount");
mocks/curve/MockCurveVoteEscrow.sol:53:        require(lockAmounts[msg.sender] > 0, "Must have a lock");
mocks/curve/MockCurveVoteEscrow.sol:55:        require(amount > 0, "!amount");
mocks/curve/MockCurveVoteEscrow.sol:63:        require(lockAmounts[msg.sender] > 0, "Must have a lock");
AuraVestedEscrow.sol:118:        require(totalLocked[_recipient] > 0, "!funding");
BalLiquidityProvider.sol:57:            require(bal > 0 && bal == _request.maxAmountsIn[i], "!bal");
BalLiquidityProvider.sol:70:        require(balAfter > 0, "!mint");
AuraLocker.sol:210:        require(rewardData[_rewardsToken].lastUpdateTime > 0, "Reward does not exist");
AuraLocker.sol:259:        require(_amount > 0, "Cannot stake 0");
AuraLocker.sol:359:        require(amt > 0, "Nothing locked");
AuraLocker.sol:385:        require(length > 0, "no locks");
AuraLocker.sol:431:        require(locked > 0, "no exp locks");
AuraLocker.sol:471:        require(len > 0, "Nothing to delegate");
AuraLocker.sol:822:        require(_rewards > 0, "No reward");
AuraLocker.sol:851:        require(_reward > 0, "No reward");
AuraMerkleDrop.sol:122:        require(_amount > 0, "!amount");
Aura.sol:68:        require(_amount > 0, "Must mint something");
ExtraRewardsDistributor.sol:171:        require(_index > 0 && _index < rewardEpochs[_token].length - 1, "!past");
AuraBalRewardPool.sol:121:        require(_amount > 0, "RewardPool : Cannot stake 0");
AuraBalRewardPool.sol:139:        require(_amount > 0, "RewardPool : Cannot stake 0");
AuraBalRewardPool.sol:157:        require(amount > 0, "RewardPool : Cannot withdraw 0");
AuraBalRewardPool.sol:210:        require(rewardsAvailable > 0, "!balance");
AuraPenaltyForwarder.sol:52:        require(bal > 0, "!empty");

Recommendation

It is recommended to replace > 0 with != 0, as they do the same thing for unsigned integers, and '!= 0' costs less gas compared to > 0 in require statements with the optimizer enabled, also enable the optimizer.

For example :

mocks/curve/MockCurveVoteEscrow.sol:43:        require(amount != 0, "!amount");

If possible, use bitwise shift instead of division/multiplication

If the divisor/multiplier x is a power of 2, it can be calculated by shifting log2(x) to the right/left. Division with / cost more gas compared to bitwise shifting.

Instances include :

AuraMath.sol:36:        return (a / 2) + (b / 2) + (((a % 2) + (b % 2)) / 2);
AuraBalRewardPool.sol:183:                uint256 penalty = (reward * 2) / 10;

Recommendation

It is recommended to replace / and * with >> and << respectively and divisor/multiplier x to log2(x), for division/multiplication where the divisor/multiplier is a power of 2, for example :

AuraBalRewardPool.sol:183:                uint256 penalty = (reward << 1) / 10;

If possible, use prefix increment instead of postfix increment

Prefix increment ++i returns the updated value after it's incremented and postfix increment i++ returns the original value then increments it. Prefix increment costs less gas compared to postfix increment.

Instances includes :

mocks/balancer/MockFeeDistro.sol:19:        for (uint256 i = 0; i < _tokens.length; i++) {
mocks/balancer/MockFeeDistro.sol:38:        for (uint256 i = 0; i < tokens.length; i++) {
mocks/MockVoteStorage.sol:64:            len++;
mocks/curve/MockVoting.sol:23:            votesFor[voteId]++;
mocks/curve/MockVoting.sol:25:            votesAgainst[voteId]++;
mocks/curve/MockCurveGauge.sol:36:        for (uint256 i = 0; i < reward_tokens.length; i++) {
AuraVestedEscrow.sol:100:        for (uint256 i = 0; i < _recipient.length; i++) {
BalLiquidityProvider.sol:51:        for (uint256 i = 0; i < 2; i++) {
AuraLocker.sol:174:            for (uint256 i = 0; i < rewardTokensLength; i++) {
AuraLocker.sol:306:        for (uint256 i; i < rewardTokensLength; i++) {
AuraLocker.sol:410:            for (uint256 i = nextUnlockIndex; i < length; i++) {
AuraLocker.sol:426:                nextUnlockIndex++;
AuraLocker.sol:696:        for (uint256 i = nextUnlockIndex; i < locks.length; i++) {
AuraLocker.sol:702:                idx++;
AuraLocker.sol:773:        for (uint256 i = 0; i < userRewardsLength; i++) {
ExtraRewardsDistributor.sol:233:        for (uint256 i = epochIndex; i < tokenEpochs; i++) {
AuraClaimZap.sol:143:        for (uint256 i = 0; i < rewardContracts.length; i++) {
AuraClaimZap.sol:147:        for (uint256 i = 0; i < extraRewardContracts.length; i++) {
AuraClaimZap.sol:151:        for (uint256 i = 0; i < tokenRewardContracts.length; i++) {
AuraLocker.sol:497:                i--;
AuraLocker.sol:664:        for (uint256 i = locksLength; i > 0; i--) {
AuraLocker.sol:726:        for (uint256 i = epochIndex + 1; i > 0; i--) {

Recommendation

It is recommended to use prefix increment instead of postfix one when the return value is not needed, as both of them will give the same result and prefix increment costs less gas.

For example :

mocks/balancer/MockFeeDistro.sol:19:        for (uint256 i = 0; i < _tokens.length; ++i) {

Don't compare a boolean to a constant

Comparing a boolean to a constant costs more gas than directly checking the returned value of the boolean.

Instance include :

AuraMerkleDrop.sol:        require(hasClaimed[msg.sender] == false, "already claimed");

Recommendation

For example in an if statement, it is recommended to use if(directValue) instead of if(directValue == true) and if(!directValue) instead of if(directValue == false)

Changes :

AuraMerkleDrop.sol:        require(!hasClaimed[msg.sender], "already claimed");