Badger-Vested-Aura contest - kirk-baird's results

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().

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L428-L431 https://github.com/Badger-Finance/badger-vaults-1.5/blob/3c96bd83e9400671256b235422f63644f1ae3d2a/contracts/BaseStrategy.sol#L346-L353

Vulnerability details

Impact

The function _sendBadgerToTree() will transfer the amount to the tree twice.

The impact depends on the calling function.

When the callpath is sweepRewardToken() -> _handleRewardTransfer() -> _sendBadgerToTree(). The result is the transaction will always revert for the BADGER token. The reason it reverts is that for each transfer the amount transferred is the initial balance i.e. from line #111 of MyStrategy.sol.

uint256 toSend = IERC20Upgradeable(token).balanceOf(address(this));

Since the contract only has toSend balance trying to transfer toSend twice will result in the second transfer failing due to insufficient funds and therefore the entire transaction reverts. Hence it is impossible to call sweepRewardToken() when the token is BADGER. Therefore all BADGER received as rewards will be locked in the contract.

When the call path is claimBribesFromHiddenHand() -> _handleRewardTransfer() -> _sendBadgerToTree() then the transaction will send twice the rewards obtained to the tree. One batch through the vault which will extract fees and the other directly to the tree. The impact is that either the transaction will revert due to insufficient balance or that the twice the rewards will be transferred from the contract both situations are high risk as they interfere with the core accounting of the protocol.

Proof of Concept

The first transfer occurs in MyStrategy._sendBadgerToTree()

    function _sendBadgerToTree(uint256 amount) internal {
        IERC20Upgradeable(BADGER).safeTransfer(BADGER_TREE, amount);
        _processExtraToken(address(BADGER), amount);
    }

The second transfer occurs in BaseStrategy._processExtraToken() since token = BADGER and the amount remains unchanged from that passed to _sendBadgerToTree(uint256 amount).

    function _processExtraToken(address _token, uint256 _amount) internal {
        require(_token != want, "Not want, use _reportToVault");
        require(_token != address(0), "Address 0");
        require(_amount != 0, "Amount 0");

        IERC20Upgradeable(_token).safeTransfer(vault, _amount);
        IVault(vault).reportAdditionalToken(_token);
    }

Recommended Mitigation Steps

To resolve this issue one of the steps should be removed from _sendBadgerToTree().

To account for governance and other fees it is recommended to transfer this via the vault by removing

IERC20Upgradeable(BADGER).safeTransfer(BADGER_TREE, amount);` on line #429 of `MyStrategy.sol

Alternatively removing the internal call _processExtraToken(address(BADGER), amount) on line #430 of MyStrategy.sol will also resolve the issue and avoid paying vault fees.