Aura Finance contest - ellahi's results

Low

[L-01] Use Two-Step Transfer Pattern for Access Controls

Impact

Contracts implementing access control's, e.g. owner, should consider implementing a Two-Step Transfer pattern. Otherwise it's possible that the role mistakenly transfers ownership to the wrong address, resulting in a loss of the role

Proof of Concept

AuraVestedEscrow.sol::setAdmin(), Booster.sol::setOwner(), Booster.sol::setFeeManager(), Booster.sol::setPoolManager(), Booster.sol::setVoteDelegate()

Recommendation

Consider adding a two-step approach when assigning new controllers.

Example

address owner;
address pendingOwner;

// ...

function setPendingOwner(address newPendingOwner) external {
    require(msg.sender == owner, "!owner");
    emit NewPendingOwner(newPendingOwner);
    pendingOwner = newPendingOwner;
}

function acceptOwnership() external {
    require(msg.sender == pendingOwner, "!pendingOwner");
    emit NewOwner(pendingOwner);
    owner = pendingOwner;
    pendingOwner = address(0);
}

[L-02] Do not use Deprecated Library Functions

Impact

The usage of deprecated library functions should be discouraged.

Proof of Concept

  contracts/AuraBalRewardPool.sol::75 => rewardToken.safeApprove(_auraLocker, type(uint256).max);
  contracts/AuraClaimZap.sol::98 => IERC20(crv).safeApprove(crvDepositWrapper, 0);
  contracts/AuraClaimZap.sol::99 => IERC20(crv).safeApprove(crvDepositWrapper, type(uint256).max);
  contracts/AuraClaimZap.sol::101 => IERC20(cvxCrv).safeApprove(cvxCrvRewards, 0);
  contracts/AuraClaimZap.sol::102 => IERC20(cvxCrv).safeApprove(cvxCrvRewards, type(uint256).max);
  contracts/AuraClaimZap.sol::104 => IERC20(cvx).safeApprove(locker, 0);
  contracts/AuraClaimZap.sol::105 => IERC20(cvx).safeApprove(locker, type(uint256).max);
  contracts/AuraLocker.sol::240 => IERC20(cvxCrv).safeApprove(cvxcrvStaking, 0);
  contracts/AuraLocker.sol::241 => IERC20(cvxCrv).safeApprove(cvxcrvStaking, type(uint256).max);
  contracts/AuraMerkleDrop.sol::131 => aura.safeApprove(address(auraLocker), 0);
  contracts/AuraMerkleDrop.sol::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);
  contracts/AuraStakingProxy.sol::148 => IERC20(crv).safeApprove(crvDepositorWrapper, type(uint256).max);
  contracts/AuraStakingProxy.sol::150 => IERC20(cvxCrv).safeApprove(rewards, 0);
  contracts/AuraStakingProxy.sol::151 => IERC20(cvxCrv).safeApprove(rewards, type(uint256).max);
  contracts/AuraStakingProxy.sol::215 => _token.safeApprove(rewards, 0);
  contracts/AuraStakingProxy.sol::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);
  contracts/BalLiquidityProvider.sol::60 => tkn.safeApprove(address(bVault), bal);
  contracts/CrvDepositorWrapper.sol::52 => IERC20(WETH).safeApprove(address(BALANCER_VAULT), type(uint256).max);
  contracts/CrvDepositorWrapper.sol::53 => IERC20(BAL).safeApprove(address(BALANCER_VAULT), type(uint256).max);
  convesx-platform/BaseRewardPool4626.sol::40 => IERC20(asset).safeApprove(operator_, type(uint256).max);
  convesx-platform/Booster.sol::422 => IERC20(token).safeApprove(rewardContract,0);
  convesx-platform/Booster.sol::423 => IERC20(token).safeApprove(rewardContract,_amount);
  convesx-platform/CrvDepositor.sol::199 => IERC20(minter).safeApprove(_stakeAddress,0);
  convesx-platform/CrvDepositor.sol::200 => IERC20(minter).safeApprove(_stakeAddress,_amount);
  convesx-platform/VoterProxy.sol::176 => IERC20(_token).safeApprove(_gauge, 0);
  convesx-platform/VoterProxy.sol::177 => IERC20(_token).safeApprove(_gauge, balance);
  convesx-platform/VoterProxy.sol::193 => _asset.safeApprove(rewardDeposit, 0);
  convesx-platform/VoterProxy.sol::194 => _asset.safeApprove(rewardDeposit, balance);
  convesx-platform/VoterProxy.sol::244 => IERC20(crvBpt).safeApprove(escrow, 0);
  convesx-platform/VoterProxy.sol::245 => IERC20(crvBpt).safeApprove(escrow, _value);
  convesx-platform/VoterProxy.sol::255 => IERC20(crvBpt).safeApprove(escrow, 0);
  convesx-platform/VoterProxy.sol::256 => IERC20(crvBpt).safeApprove(escrow, _value);

Recommendation

Use safeIncreaseAllowance / safeDecreaseAllowance instead of safeApprove.

Tools used

manual, slither

Gas

[G-01] Use != 0 instead of > 0 for Unsigned Integer Comparison in require statements.

Impact

!= 0 is cheapear than > 0 when comparing unsigned integers in require statements.

Proof of Concept

  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");
  convesx-platform/BaseRewardPool.sol::211 => require(_amount > 0, 'RewardPool : Cannot stake 0');
  convesx-platform/BaseRewardPool.sol::227 => require(amount > 0, 'RewardPool : Cannot withdraw 0');
  convesx-platform/Booster.sol::223 => require(IFeeDistributor(_feeDistro).getTokenTimeCursor(_feeToken) > 0, "!distro");
  convesx-platform/CrvDepositor.sol::169 => require(_amount > 0,"!>0");
  convesx-platform/PoolManagerSecondaryProxy.sol::104 => require(weight > 0, "must have weight");
  convesx-platform/interfaces/BoringMath.sol::20 => require(b > 0, "BoringMath: division by zero");
  convesx-platform/interfaces/BoringMath.sol::102 => require(b > 0, "BoringMath: division by zero");
  convesx-platform/interfaces/BoringMath.sol::123 => require(b > 0, "BoringMath: division by zero");
  convesx-platform/interfaces/BoringMath.sol::143 => require(b > 0, "BoringMath: division by zero");

Recommendation

Use != 0 instead of > 0.

[G-02] Cache Array Length Outside of Loop.

Impact

Reading array length at each iteration of the loop takes 6 gas (3 for mload and 3 to place memory_offset) in the stack. Caching the array length in the stack saves around 3 gas per iteration.

Proof of Concept

  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::410 => for (uint256 i = nextUnlockIndex; i < 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++) {
  convesx-platform/ArbitartorVault.sol::49 => for(uint256 i = 0; i < _toPids.length; i++){
  convesx-platform/BaseRewardPool.sol::214 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/BaseRewardPool.sol::230 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/BaseRewardPool.sol::262 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/BaseRewardPool.sol::296 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/Booster.sol::379 => for(uint i=0; i < poolInfo.length; i++){
  convesx-platform/Booster.sol::538 => for(uint256 i = 0; i < _gauge.length; i++){
  convesx-platform/ConvexMasterChef.sol::180 => for (uint256 pid = 0; pid < length; ++pid) {
  convesx-platform/PoolManagerSecondaryProxy.sol::69 => for(uint i=0; i < usedList.length; i++){

Recommendation

Store the array’s length in a variable before the for-loop.

[G-03] ++i costs less gas compared to i++ or i += 1

Impact

++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.

Proof of Concept

  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::410 => for (uint256 i = nextUnlockIndex; i < 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++) {
  convesx-platform/ArbitartorVault.sol::49 => for(uint256 i = 0; i < _toPids.length; i++){
  convesx-platform/BaseRewardPool.sol::214 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/BaseRewardPool.sol::230 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/BaseRewardPool.sol::262 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/BaseRewardPool.sol::296 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/Booster.sol::379 => for(uint i=0; i < poolInfo.length; i++){
  convesx-platform/Booster.sol::538 => for(uint256 i = 0; i < _gauge.length; i++){
  convesx-platform/PoolManagerSecondaryProxy.sol::69 => for(uint i=0; i < usedList.length; i++){

Recommendation

Use ++i instead of i++ to increment the value of an uint variable. Same thing for --i and i--.

[G-04] No need to initialize variables with default values

Impact

If a variable is not set/initialized, it is assumed to have the default value (0, false, 0x0 etc depending on the data type). Explicitly initializing it with its default value is an anti-pattern and wastes gas.

Proof of Concept

  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::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/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;
  convesx-platform/ArbitartorVault.sol::49 => for(uint256 i = 0; i < _toPids.length; i++){
  convesx-platform/BaseRewardPool.sol::214 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/BaseRewardPool.sol::230 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/BaseRewardPool.sol::262 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/BaseRewardPool.sol::296 => for(uint i=0; i < extraRewards.length; i++){
  convesx-platform/Booster.sol::379 => for(uint i=0; i < poolInfo.length; i++){
  convesx-platform/Booster.sol::538 => for(uint256 i = 0; i < _gauge.length; i++){
  convesx-platform/BoosterOwner.sol::144 => for(uint256 i = 0; i < poolCount; i++){
  convesx-platform/ConvexMasterChef.sol::180 => for (uint256 pid = 0; pid < length; ++pid) {
  convesx-platform/ExtraRewardStashV3.sol::125 => for(uint256 i = 0; i < maxRewards; i++){
  convesx-platform/ExtraRewardStashV3.sol::199 => for(uint i=0; i < tCount; i++){
  convesx-platform/PoolManagerSecondaryProxy.sol::69 => for(uint i=0; i < usedList.length; i++){

Recommendation

Remove explicit default initializations.

[G-05] Non-strict inequalities are cheaper than strict ones

Impact

Strict inequalities add a check of non equality which costs around 3 gas.

Proof of Concept

Throughout codebase.

Recommendation

Use >= or <= instead of > and < when possible.

Tools used

c4udit, manual, slither