`Vault.withdraw` mixes normalized and standard amounts

[code423n4]

Handle

cmichel

Vulnerability details

The Vault.balance function uses the balanceOfThis function which scales ("normalizes") all balances to 18 decimals.

for (uint8 i; i < _tokens.length; i++) {
    address _token = _tokens[i];
    // everything is padded to 18 decimals
    _balance = _balance.add(_normalizeDecimals(_token, IERC20(_token).balanceOf(address(this))));
}

Note that balance()'s second term IController(manager.controllers(address(this))).balanceOf() is not normalized, but it must be.

This leads to many issues through the contracts that use balance but don't treat these values as normalized values. For example, in Vault.withdraw, the computed _amount value is normalized (in 18 decimals). But the uint256 _balance = IERC20(_output).balanceOf(address(this)); value is not normalized but compared to the normalized _amount and even subtracted:

// @audit compares unnormalzied output to normalized output
if (_balance < _amount) {
    IController _controller = IController(manager.controllers(address(this)));
    // @audit cannot directly subtract unnormalized
    uint256 _toWithdraw = _amount.sub(_balance);
    if (_controller.strategies() > 0) {
        _controller.withdraw(_output, _toWithdraw);
    }
    uint256 _after = IERC20(_output).balanceOf(address(this));
    uint256 _diff = _after.sub(_balance);
    if (_diff < _toWithdraw) {
        _amount = _balance.add(_diff);
    }
}

Impact

Imagine in withdraw, the output is USDC with 6 decimals, then the normalized _toWithdraw with 18 decimals (due to using _amount) will be a huge number and attempt to withdraw an inflated amount. An attacker can steal tokens this way by withdrawing a tiny amount of shares and receive an inflated USDC or USDT amount (or any _output token with less than 18 decimals).

Recommended Mitigation Steps

Whenever using anything involving vault.balanceOfThis() or vault.balance() one needs to be sure that any derived token amount needs to be denormalized again before using them.

[GalloDaSballo]

An inconsistent usage of _normalizeDecimals will cause accounting issues and potentially paths for an exploit