Any User May Transfer Rewards From AuraLocker To MyStrategy These Rewards Are Unaccounted For and Stuck

[code423n4]

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L220-L228

Vulnerability details

Impact

Harvesting the rewards from AuraLocker incorrectly assumes that rewards are only transferred during harvest when LOCKER.getRewards(address(this)) is called. However it is possible for anyone to call AuraLocker.getRewards(address(MyStrategy) and transfer the rewards to the contract.

The impact of this is that when a user calls AuraLocker.getRewards(MyStrategy) the rewards are transferred to MyStrategy. They are not accounted for in _harvest() which does a before and after balance check around LOCKER.getRewards(), since the rewards will already be in the contract before _harvest() is called.

AURABAL is the reward token, hence it is protected token it cannot be withdrawn by other means. Therefore the reward tokens maybe stuck in the contract.

Proof of Concept

MyStrategy _harvest() performs a before and after balance check around LOCKER.getReward(). Only auraBalEarned will be harvested and transferred to the vault.

        uint256 auraBalBalanceBefore = AURABAL.balanceOf(address(this));

        // Claim auraBAL from locker
        LOCKER.getReward(address(this));

        harvested = new TokenAmount[](1);
        harvested[0].token = address(AURA);

        uint256 auraBalEarned = AURABAL.balanceOf(address(this)).sub(auraBalBalanceBefore);

AuraLocker getRewards() can be called by anyone and will transfer the rewards to the passed _account.

    function getReward(address _account, bool _stake) public nonReentrant updateReward(_account) {
        uint256 rewardTokensLength = rewardTokens.length;
        for (uint256 i; i < rewardTokensLength; i++) {
            address _rewardsToken = rewardTokens[i];
            uint256 reward = userData[_account][_rewardsToken].rewards;
            if (reward > 0) {
                userData[_account][_rewardsToken].rewards = 0;
                if (_rewardsToken == cvxCrv && _stake && _account == msg.sender) {
                    IRewardStaking(cvxcrvStaking).stakeFor(_account, reward);
                } else {
                    IERC20(_rewardsToken).safeTransfer(_account, reward);
                }
                emit RewardPaid(_account, _rewardsToken, reward);
            }
        }
    }

Recommended Mitigation Steps

Consider treating all AURABAL tokens as harvestable rewards. That is in _harvest() use the entire contract balance rather than just the amount received from the external call LOCKER.getReward().

[KenzoAgada]

Duplicate of #129