Aura Finance contest - p_crypt0's results

Floating compiler versions:

Use stated compiler version: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraLocker.sol#L2

Use fixed compiler version: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraBalRewardPool.sol#L2

Use fixed compiler version: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraVestedEscrow.sol#L2

Use fixed compiler version: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraVestedEscrow.sol#L2

Use fixed compiler version: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/ClaimFeesHelper.sol#L2

Use fixed compiler version: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraMinter.sol#L2

Use fixed compiler version: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraMath.sol#L2

Use fixed compiler version: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraMerkleDrop.sol#L2

Use fixed compiler version: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraPenaltyForwarder.sol#L2

Naming Convention and style:

Inconsistent naming convention for MaxFees as constant. Should be MAX_FEES in the same vein as FEE_DENOMINATOR https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/Booster.sol#L30-L31

Inconsistent naming convention for MAXTIME. Should be MAX_TIME in the same vein as FEE_DENOMINATOR and still in contrast to MaxFees from Booster.sol https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/CrvDepositor.sol#L26

Inconsistent naming convention for denominator. Should be DENOMINATOR or FEE_DENOMINATOR as per other contracts: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraStakingProxy.sol#L45

Consider using constants for min _outputBps and max _outputBps rather than magic numbers: https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/contracts/AuraStakingProxy.sol#L90

Typos:

'Reponsible' -> Responsible (s) https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/Booster.sol#L569

'Reponsible' -> Responsible (s) https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/Booster.sol#L631

'staler' -> 'staker' https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/CrvDepositor.sol#L14

'ot' -> 'on' https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/Booster.sol#L372

"curves" -> " curve's " https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/CrvDepositor.sol#L16

'Can locking immediately…' -> 'Can lock immediately or defer locking to someone else by paying a fee. While users can choose to lock or defer, this is mostly in place so that the cvx reward contract isn't costly when claiming rewards.' https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/CrvDepositor.sol#L161

Missing Comments

Missing comment:

/**
     * @notice Set the stash accessibility of a stashPool.
     * @param _stash Address of the specific stashPool.
     * @param_status boolean enforcing a specific stashPool is accessible or not.
     */

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/VoterProxy.sol#L121-L128

Low / Informational

Use of openzeppelin safeApprove() over IncreasedAllowance.

Standard approach is to increase/decrease allowance as described in the OpenZeppelin manual.

2022-05-aura/Booster.sol at 4989a2077546a5394e3650bf3c224669a0f7e690 · code-423n4/2022-05-aura · GitHub

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/VoterProxy.sol#L193-L194

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/VoterProxy.sol#L244-L245

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/VoterProxy.sol#L255-L256

Owner (multi-sig) can arbitrarily change rewards withdrawal address.

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/VoterProxy.sol#L83-L87

Consider adding timelock functionality whilst transitioning to a DAO framework, or mandating that both multi-sig wallets require approval before changing withdrawal address.

Operator can reach over then their designated prerogative in withdrawing tokens from VoterProxy.sol

The operator can perform the same action as the withdrawer in VoterProxy.sol, in that they can withdraw up to 100.0% of any ERC20 tokens from the contract, straight to their address.

Withdrawer can only return it to the withdrawal address.

/**
     * @notice  Withdraw LP tokens from a gauge 
     * @dev     Only callable by the operator 
     * @param _token    LP token address
     * @param _gauge    Gauge for this LP token
     * @param _amount   Amount of LP token to withdraw
     */
    function withdraw(address _token, address _gauge, uint256 _amount) public returns(bool){
        require(msg.sender == operator, "!auth");
        uint256 _balance = IERC20(_token).balanceOf(address(this));
        if (_balance < _amount) {
            _amount = _withdrawSome(_gauge, _amount.sub(_balance));
            _amount = _amount.add(_balance);
        }
        IERC20(_token).safeTransfer(msg.sender, _amount);
        return true;
    }

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/VoterProxy.sol#L199-L215

The reason why this appears to be non-desired code, is that the "_withdrawSome()" function withdraws tokens from the curve gauge specifically, whereas the "withdraw" function just transfers from a given address of a token submitted by the operator.

In the following function, only the "withdrawer" permission is granted access

    /**
     * @notice  Withdraw ERC20 tokens that have been distributed as extra rewards
     * @dev     Tokens shouldn't end up here if they can help it. However, dao can
     *          set a withdrawer that can process these to some ExtraRewardDistribution.
     */
    function withdraw(IERC20 _asset) external returns (uint256 balance) {
        require(msg.sender == withdrawer, "!auth");
        require(protectedTokens[address(_asset)] == false, "protected");


        balance = _asset.balanceOf(address(this));
        _asset.safeApprove(rewardDeposit, 0);
        _asset.safeApprove(rewardDeposit, balance);
        IRewardDeposit(rewardDeposit).addReward(address(_asset), balance);
        return balance;
    }

https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/VoterProxy.sol#L183-L197

As a thought, would it be possible to transfer ownership of a curve gauge token, by using this (https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/VoterProxy.sol#L199-L215) function?

Within _withdrawSome(), it is specified that the address

is the address of the curve gauge token (ERC 20, ICurveGauge), whereas (https://github.com/code-423n4/2022-05-aura/blob/4989a2077546a5394e3650bf3c224669a0f7e690/convex-platform/contracts/contracts/VoterProxy.sol#L199-L215) specifies that any ERC20 can be transferred (which is presumed to be LP tokens…).

Why not use ICurveGauge(_gauge).withdraw(_amount); in withdraw() instead of IERC20(_token).safeTransfer(msg.sender, _amount);

Curve Gauge tokens being transferred would not be desired code and would be critical, according to the team.