Badger-Vested-Aura contest - kenzo's results

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L249 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L275

Vulnerability details

The harvest function executes swaps without minimum amount out specified.

Impact

MEV searchers can sandwich harvest calls to an unlimited degree, thereby making Badger get less funds than it deserves.

Proof of Concept

The _harvest function executes swaps without specifiying minAmountOut:

            uint256 balEthBptEarned = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max);

The third parameter, 0 is the minAmountOut - see in Balancer documentation.

And similarly here:

            harvested[0].amount = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max);

Recommended Mitigation Steps

Before executing the harvest transaction, query balancer for the expected amount out - see here for Balancer guide on how to do so. Then, pass this amount to the harvest function, and check that the slippage is whatever percentage is acceptable for you.

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L428

Vulnerability details

_sendBadgerToTree will send BADGER twice and therefore fail. It is sending it to the BADGER_TREE in _sendBadgerToTree, and then continues to send the same amount to the vault in _processExtraToken.

Impact

BADGER rewards cannot be claimed. The contract is trying to send same amount twice - therefore the second transfer will fail. (Unless there is extra BADGER in the contract, in which case there will be accounting errors.)

Proof of Concept

This is _sendBadgerToTree:

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

And this is BaseStrategy's _processExtraToken:

    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);
    }

As we can see in the safeTransfer lines of the two functions, the BADGER amount is being sent in _sendBadgerToTree to BADGER_TREE and then it is being sent in _processExtraToken to the vault.

Recommended Mitigation Steps

I am not sure, but to my understanding you don't need to call _processExtraToken in _sendBadgerToTree, but only call IVault(vault).reportAdditionalToken(_token).