code423n4 commented 2 years ago

Lines of code

https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L182-L190 https://github.com/Badger-Finance/badger-vaults-1.5/blob/0.1.0/contracts/BaseStrategy.sol#L237

Vulnerability details

Impact

_withdrawAll() should check that all of your positions are unwinded. It does check balanceOfPool() and LOCKER.balanceOf. Then withdrawToVault() in BaseStrategy.sol can transfer all want to the vault. But it doesn’t check reward tokens. want could also be one of the reward tokens.

Proof of Concept

withdrawToVault() in BaseStrategy.sol calls _withdrawAll() to make sure that all of your positions are unwinded. Then it can transfer all want to the vault. https://github.com/Badger-Finance/badger-vaults-1.5/blob/0.1.0/contracts/BaseStrategy.sol#L237

    function withdrawToVault() external {
        _onlyVault();

        _withdrawAll();

        uint256 balance = IERC20Upgradeable(want).balanceOf(address(this));
        _transferToVault(balance);
    }

_withdrawAll() only confirms balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0 https://github.com/Badger-Finance/vested-aura/blob/v0.0.2/contracts/MyStrategy.sol#L182-L190

    function _withdrawAll() internal override {
        //NOTE: This probably will always fail unless we have all tokens expired
        require(
            balanceOfPool() == 0 && LOCKER.balanceOf(address(this)) == 0,
            "You have to wait for unlock or have to manually rebalance out of it"
        );

        // Make sure to call prepareWithdrawAll before _withdrawAll
    }

Tools Used

None

Recommended Mitigation Steps

Also check reward tokens in _withdrawAll()

GalloDaSballo commented 2 years ago

_withdrawAll is a migration only function that is called exclusively when changing strategy from the vault.

Any time a strategy needs to be changed, this is considered "emergency mode" and stopping for rewards would be extremely ill-advised.

Additionally, balanceOfRewards is a function that we added to vaults 1.5 with the goal of allowing off-chain monitoring of the harvest health and the amount of rewards accrued, to compare second by second APY with the harvest values.

For those reasons, I must disagree with the finding