Controller.withdraw(...) User may lose funds when withdraw wantToken from the underlying contract

[code423n4]

Handle

WatchPug

Vulnerability details

The wantToken of the strategy may be different from the _token argument of Controller.withdraw(address _token, uint256 _amount) according to code at line 469-474 of Controller.sol.

https://github.com/code-423n4/2021-09-yaxis/blob/cf7d9448e70b5c1163a1773adb4709d9d6ad6c99/contracts/v3/controllers/Controller.sol#L469-L474

if (_want != _token) {
    address _converter = _vaultDetails[msg.sender].converter;
    IERC20(_want).safeTransfer(_converter, _amounts[i]);
    // TODO: do estimation for received
    IConverter(_converter).convert(_want, _token, _amounts[i], 1);
}

However, the token address of _vaultDetails[_vault].balances[_strategy] will always be the strategy's wantToken address according to code at line 635.

https://github.com/code-423n4/2021-09-yaxis/blob/cf7d9448e70b5c1163a1773adb4709d9d6ad6c99/contracts/v3/controllers/Controller.sol#L635

_vaultDetails[_vault].balances[_strategy] = IStrategy(_strategy).balanceOf();

https://github.com/code-423n4/2021-09-yaxis/blob/cf7d9448e70b5c1163a1773adb4709d9d6ad6c99/contracts/v3/strategies/BaseStrategy.sol#L212

return balanceOfWant().add(balanceOfPool());

At line 610-620 of Controller.getBestStrategyWithdraw(address _token, uint256 _amount), it just plainly compare and compute with _vaultDetails[_vault].balances[_strategy] (in strategy's wantToken) and _amount (in _token argument). Result in miscalculation here without necessary conversion.

https://github.com/code-423n4/2021-09-yaxis/blob/cf7d9448e70b5c1163a1773adb4709d9d6ad6c99/contracts/v3/controllers/Controller.sol#L610-L620

_balance = _vaultDetails[_vault].balances[_strategy];
// if the strategy doesn't have the balance to cover the withdraw
if (_balance < _amount) {
    // withdraw what we can and add to the _amounts
    _amounts[i] = _balance;
    _amount = _amount.sub(_balance);
} else {
    // stop scanning if the balance is more than the withdraw amount
    _amounts[i] = _amount;
    break;
}

And line 473-478 of Controller.withdraw(address _token, uint256 _amount), it will convert amounts[i] of wantToken to _token, and transfer the converted _token to msg.sender.

If the token price of wantToken is lower than _token, Controller.withdraw(...) will transfer insufficient amount of _token to msg.sender (Vault contract);

If the token price of wantToken is higher than _token, Controller.withdraw(...) will transfer overmuch _token to msg.sender (Vault contract).

https://github.com/code-423n4/2021-09-yaxis/blob/cf7d9448e70b5c1163a1773adb4709d9d6ad6c99/contracts/v3/controllers/Controller.sol#L473-L478

        IConverter(_converter).convert(_want, _token, _amounts[i], 1);
    }
}
_amount = IERC20(_token).balanceOf(address(this));
_vaultDetails[msg.sender].balance = _vaultDetails[msg.sender].balance.sub(_amount);
IERC20(_token).safeTransfer(msg.sender, _amount);

Therefore after Vault.withdraw(...) called controller.withdraw(_output, _toWithdraw), the received output token amount _diff may be less than expected _toWithdraw.

This causes the _amount of _output transferred to the user to be less than the amount it should have been. This is not because there is not enough balance in the strategy, but because there is a miscalculation and not enough want tokens are withdrawn from the underlying.

https://github.com/code-423n4/2021-09-yaxis/blob/cf7d9448e70b5c1163a1773adb4709d9d6ad6c99/contracts/v3/Vault.sol#L250-L263

if (_balance < _amount) {
    IController _controller = IController(manager.controllers(address(this)));
    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);
    }
}

IERC20(_output).safeTransfer(msg.sender, _amount);

Impact

Users may get a fewer amount of token when call vault.withdraw(...) while the user's stored balance gets deducted for a larger amount.

Proof of Concept

If there is a BTCB vault with BNB as wantToken, and we assume that there is no wallet balance of BTCB token in the vault.

An user may do the following steps:

1. Call btcbVault.withdraw(112e18, btcbToken);
2. Call controller.withdraw(btcbToken, 112e18)
   • controller.getBestStrategyWithdraw returns ([bnbWantedStrategy], [112e18]);
   • controller.withdraw(...) L469-474 will swap 112e18 of bnbToken to 1e18 of btcbToken;
   • controller.withdraw(...) L476-478 will transfer 1e18 of btcbToken to msg.sender (btcbVault);
   • btcbVault recives 1e18 of btcbToken;
   • Vault.withdraw(...) L259 will update the _amount to 1e18.
   • Only 1e18 of btcbToken will be received.

As a result, the user will lose funds.

Recommended Mitigation Steps

Make sure to swap tokens when converting from tokenA to tokenB.

[GainsGoblin]

Duplicate of #8

[GalloDaSballo]

Duplicate of #28