Yield may be stolen by MEV bot by sandwiching harvest()

[code423n4]

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L219 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L249 https://github.com/Badger-Finance/vested-aura/blob/d504684e4f9b56660a9e6c6dfb839dcebac3c174/contracts/MyStrategy.sol#L275 https://github.com/Badger-Finance/badger-vaults-1.5/blob/655e7545e74ed225c91f5a280f4ee22611517cef/contracts/BaseStrategy.sol#L391-L394

Vulnerability details

Impact

Yield may be stolen by MEV bot by sandwiching harvest(). Because of minimum output amount of swapping is set to 0. Which mean MEV bot can pump price of AURA token to the highest price before your strategy swap to let you buy AURA token at an incredibly high price then create another transaction immediately to sell pumped AURA token back at greater price.

Proof of Concept

    function harvest() external whenNotPaused returns (TokenAmount[] memory harvested) {
        _onlyAuthorizedActors();
        return _harvest();
    }

function harvest just check for authorized actors, doing _harvest() and return the harvested amount without any check on minimum output amount.

function _harvest() internal override returns (TokenAmount[] memory harvested) { doesn't receive minOutput parameter

uint256 balEthBptEarned = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max); swap AURABAL -> BALETH_BPT without any slippage prevention

harvested[0].amount = BALANCER_VAULT.swap(singleSwap, fundManagement, 0, type(uint256).max); swap ETH -> AURA without any slippage prevention

No minimum output amount check is performed

Tools Used

Manual

Recommended Mitigation Steps

You should add minimum output amount check to harvest()

    function harvest(TokenAmount[] calldata minOutput) external whenNotPaused returns (TokenAmount[] memory harvested) {
        _onlyAuthorizedActors();
        harvested = _harvest();
        uint256 harvestedLength = harvested.length;
        require(minOutput.length == harvestedLength, "Invalid length");
        unchecked {
        for (uint256 i = 0; i < harvestedLength; ++i) {
            require(harvested[i] >= minOutput[i], "Insufficient Output");
        }
        }
    }

[GalloDaSballo]

Disagree with severity, also we can run the same check from a caller contract, no need to add to the vault codebase. To mitigate we use Private Transactions

[KenzoAgada]

Duplicate of #155