QA Report

[code423n4]

Low & QA report

Relevant portions of code are marked with @audit tags.

Low

---

Low-01: No check for interface implementation for newly added extraRewards in BaseRewardPool.sol

Description:

The rewardManager of a BaseRewardPool can add new tokens to be extraRewards of a pool that are distributed to users in addition to the normal stakingToken. However since there is no check that the newly added token address implements the expected interface IReward - notably, the withdraw and stake functions amoung others - it's possible for the admin to mistakenly add an incompatible address to the extraRewards list, and for subsequent external calls to it to fail.

Since the handling of the extraRewards tokens is processed along with stakingToken, users will not be able to perform critical operations on the pool like withdrawing or staking if one of the extra rewards tokens is not implemented correctly.

The rewardManager can only call clearExtraRewards() which deletes the entire array, and they will have to re-add all of them again.

POC:

Function to add new extra rewards

    function addExtraReward(address _reward) external returns(bool){
        require(msg.sender == rewardManager, "!authorized");
        require(_reward != address(0),"!reward setting");

        extraRewards.push(_reward);
        // @audit there is no check that a newly added aribitary token addresses implements the IReward interface as expected 
        return true;
    }

Function to withdraw stakingToken and extraRewards if available:

    function withdraw(uint256 amount, bool claim)
        public
        updateReward(msg.sender)
        returns(bool)
    {
        require(amount > 0, 'RewardPool : Cannot withdraw 0');

        //also withdraw from linked rewards
        for(uint i=0; i < extraRewards.length; i++){
            IRewards(extraRewards[i]).withdraw(msg.sender, amount); // @audit calling .withdraw on the extraReward token address
        }
        ...
        stakingToken.safeTransfer(msg.sender, amount); // @audit note that operations for the stakingToken are in the same functions as the extraRewards
        emit Withdrawn(msg.sender, amount);
        ...

Mitigation steps

Consider using ERC165 to check that the address implements the expected interface, or allow the rewardManager to remove specific tokens from the extraRewards list by using swap and pop or another data structure.

Low-02: Calls to AuraVestedEscrow:fund() can be frontrun with bogus values

Description:
The fund() function is used to initialize the contract after creation with the appropriate amount of rewardTokens. Since there are no permissions required to call it, a malicious user can listen for calls to fund() and frontrun it with their own transaction. They can pass in two empty arrays which will require them to transfer 0 rewardTokens to the contract. Thus, the initialized flag is set to true and no one else can call fund() again.

The contract is unusable and it was at 0 cost to the attacker. Now the team must pay txn fees for redeployment.

POC

    function fund(address[] calldata _recipient, uint256[] calldata _amount) external nonReentrant {
        require(!initialised, "initialised already");

        uint256 totalAmount = 0;
        for (uint256 i = 0; i < _recipient.length; i++) {
            uint256 amount = _amount[i];

            totalLocked[_recipient[i]] += amount;
            totalAmount += amount;

            emit Funded(_recipient[i], amount);
        }
        // @audit if empty recipient and amount arrays, totalAmount == 0
        rewardToken.safeTransferFrom(msg.sender, address(this), totalAmount);
        initialised = true;
    }

Mitigation
Consider only allowing admins to fund the contract, or choose to deploy and fund using private relayers.

QA

---

QA-01: Emit event for adding a new rewardsToken to AuraLocker

    function addReward(address _rewardsToken, address _distributor) external onlyOwner {
        require(rewardData[_rewardsToken].lastUpdateTime == 0, "Reward already exists");
        require(_rewardsToken != address(stakingToken), "Cannot add StakingToken as reward");
        rewardTokens.push(_rewardsToken);
        rewardData[_rewardsToken].lastUpdateTime = uint32(block.timestamp);
        rewardData[_rewardsToken].periodFinish = uint32(block.timestamp);
        rewardDistributors[_rewardsToken][_distributor] = true;
        // @audit QA - Add event here for new reward token added
    }

Add a NewRewardToken event here to be in line other emitted events in the rest of the codebase.

Specific code link here.

QA-02: Move AuraLocker.sol:setApprovals() from the ADMIN section to the Actions section as it is not priviledged.

    // @audit QA - should not be in the ADMIN section - code organization
    function setApprovals() external {
        IERC20(cvxCrv).safeApprove(cvxcrvStaking, 0);
        IERC20(cvxCrv).safeApprove(cvxcrvStaking, type(uint256).max);
    }

    /***************************************
                    ACTIONS
    ****************************************/

QA-03: No setter function for penaltyForwarder in AuraMerkleDrop.sol

There is no function allowing the penaltyForwarder to be changed after contract creation. Additionally, there is no check in the constructor that the supplied penaltyForwarder is not the 0 address. Thus, if the value is incorrectly set on creation, forwardPenalty() will revert and there will not be a way to fix the issue without redployment. It's helpful to always have a way to change state variables that are critical to the contract's functionality (with the correct permissioning of course).

QA-05: Emit event for new extraRewards added in BaseRewardPool.sol

    function addExtraReward(address _reward) external returns(bool){
        ...

        extraRewards.push(_reward);
        // @audit QA - emit event here for new extraRewardToken added
        return true;
    }

Keep it inline with the rest of the codebase where events are emitted with important state changes. Also will help in transparency for users.

QA-06: Emit event for extraReward withdraw in BaseRewardPool.sol:withdraw()

        ...
        //also withdraw from linked rewards
        for(uint i=0; i < extraRewards.length; i++){
            IRewards(extraRewards[i]).withdraw(msg.sender, amount);
            // @audit QA: emit event here for extraReward withdraw
        }
        ...

QA-07: Check return value for stake(balance) in BaseRewardPool.sol:stakeAll()

    function stakeAll() external returns(bool){
        uint256 balance = stakingToken.balanceOf(msg.sender);
        // @audit QA: if stake() will expected to return false in the future, it pays to check it here before it is missed
        stake(balance);
        return true;
    }

If stake() will expected to return false in the future, it pays to check it here before it is missed. Or do return stake(balance).

QA-07: donate() function in BaseRewardPool.sol does not return a value despite function signature

    function donate(uint256 _amount) external returns(bool){
        IERC20(rewardToken).safeTransferFrom(msg.sender, address(this), _amount);
        queuedRewards = queuedRewards.add(_amount);
        // @audit does not return anything despite func sig return(bool)
    }

return true to follow the pattern in the rest of the codebase.

QA-08: @param for seal missing in constructor in BoosterOwner.sol

Real nitpicky but

    /**
     * @param _owner         Owner (e.g. CVX multisig)
     * @param _poolManager   PoolManager (e.g. PoolManagerSecondaryProxy or 0xD20904e5916113D11414F083229e9C8C6F91D1e1)
     * @param _booster       The booster (e.g. 0xF403C135812408BFbE8713b5A23a04b3D48AAE31)
     * @param _stashFactory  Creates stashes (e.g. 0x884da067B66677e72530df91eabb6e3CE69c2bE4)
     * @param _rescueStash   Rescues tokens for subsequent vlCVX redistribution (e.g. 0x01140351069af98416cC08b16424b9E765436531)
     // @audit QA - missing isSealed param annotation
     */
    constructor(
        address _owner,
        address _poolManager,
        address _booster,
        address _stashFactory,
        address _rescueStash,
        bool _seal
    )

QA-09: Change events emitted for ownership transfer in BoosterOwner.sol

Currently the contract emits TransferOwnership when the pendingOwner is updated, and AcceptedOwnership when the pendingOwner accepts it.

I think a more logical nomenclature would be to emit PendingOwnerUpdated when transferOwnership() is called, and emit AcceptedOwnership when the pendingOwner is accepted as well as PendingOwnerUpdated(0x0) afterwards.

(This is the same pattern used in Seaport)

    function transferOwnership(address _owner) external onlyOwner{
        pendingowner = _owner;
        emit TransferOwnership(_owner); // @audit change to emit PendingOwnerUpdated(_owner);
    }

    function acceptOwnership() external {
        require(pendingowner == msg.sender, "!pendingowner");
        owner = pendingowner;
        pendingowner = address(0);
        // @audit add emit PendingOwnerUpdated(0);
        emit AcceptedOwnership(owner);
    }