An attacker can steal funds from multi-token vaults

[code423n4]

Handle

WatchPug

Vulnerability details

The total balance should NOT be simply added from different tokens' tokenAmounts, considering that the price of tokens may not be the same.

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

function balanceOfThis()
    public
    view
    returns (uint256 _balance)
{
    address[] memory _tokens = manager.getTokens(address(this));
    for (uint8 i; i < _tokens.length; i++) {
        address _token = _tokens[i];
        _balance = _balance.add(_normalizeDecimals(_token, IERC20(_token).balanceOf(address(this))));
    }
}

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

function harvestStrategy(
    address _strategy,
    uint256 _estimatedWETH,
    uint256 _estimatedYAXIS
)
    external
    override
    notHalted
    onlyHarvester
    onlyStrategy(_strategy)
{
    uint256 _before = IStrategy(_strategy).balanceOf();
    IStrategy(_strategy).harvest(_estimatedWETH, _estimatedYAXIS);
    uint256 _after = IStrategy(_strategy).balanceOf();
    address _vault = _vaultStrategies[_strategy];
    _vaultDetails[_vault].balance = _vaultDetails[_vault].balance.add(_after.sub(_before));
    _vaultDetails[_vault].balances[_strategy] = _after;
    emit Harvest(_strategy);
}

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

/**
 * @notice Returns the total balance of the vault, including strategies
 */
function balance()
    public
    view
    override
    returns (uint256 _balance)
{
    return balanceOfThis().add(IController(manager.controllers(address(this))).balanceOf());
}

Impact

An attacker can steal funds from multi-token vaults. Result in fund loss of all other users.

Proof of Concept

If there is a multi-token vault with 3 tokens: DAI, USDC, USDT, and their price in USD is now 1.05, 0.98, and 0.95. If the current balances are: 2M, 1M, and 0.5M.

An attacker may do the following steps:

1. Deposit 3M of USDT;
2. Withdraw 3M, receive 2M in DAI and 1M in USDC.

As 2M of DAI + 1M of USDC worth much more than 3M of USDT. The attacker will profit and all other users will be losing funds.

Recommended Mitigation Steps

Always consider the price differences between tokens.

[GalloDaSballo]

Fully agree with the finding, assuming price of tokens is the same exposes the Vault and all depositors to risk of Single Sided Exposure

This risk has been exploited multiple times, notably in the Yearn Exploit

The solution for for managing tokens with multiple values while avoiding being rekt is to have an index that ensures your LP Token maintains it's peg, curve's solution is called virtual_price

Having a virtual price would allow to maintain the Vault Architecture, while mitigating exploits that directly use balances