`Controller.withdrawAll` sets wrong vault balance

[code423n4]

Handle

cmichel

Vulnerability details

The Controller.withdrawAll decreases the vault balance by _amount, the want token amount that has been withdrawn from the strategy and transferred to the vault. Note that _amount gets overwritten in the _convert != address(0) branch and is a _convert token value instead of a _want token value:

if (_convert != address(0)) {
    // @audit _amount is overwritten here with the traded return amount of _convert token!
    _amount = _converter.convert(_want, _convert, _amount, 1);
    IERC20(_convert).safeTransfer(_vault, _amount);
} else {
    IERC20(_want).safeTransfer(_vault, _amount);
}
if (_balance >= _amount) {
    // @audit they subtract the wrong _amount here if _convert was set
    _vaultDetails[_vault].balance = _vaultDetails[_vault].balance.sub(_amount);
}

Impact

The _vaultDetails[_vault].balance variable does not correctly track the actual vault balances anymore as the traded amount of _convert token could have a completely different value and decimals than the want token. It could heavily over-or-underestimate the actual withdrawn want amount.

This variable is used in Controller.balanceOf(), which in turn is used in Vault.balance(), which in turn is used to determine how many shares to mint / amount to receive when redeeming shares. If the value is less, users will lose money as they can redeem fewer tokens. If the value is more, users can redeem their existing shares for more value, leading to a bankrun until the vault is emptied and other users cannot redeem anymore.

Recommended Mitigation Steps

Do not overwrite the _amount variable in the if branch.

[GainsGoblin]

Duplicate of #8

[GalloDaSballo]

Duplicate of #77

Also adding slippage checks in controller would mitigate the specific attack mentioned here