QA Report

[code423n4]

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:     }