yAxis contest - WatchPug's results

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.

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.