yAxis contest - hickuphh3's results

Handle

hickuphh3

Vulnerability details

Impact

In withdraw(), the withdrawal amount is the proportion of the normalized amounts of all the tokens in the vault and its strategies. However, this amount isn't un-normalized to the output token's decimals, thus leading to an erroneous token amount being withdrawn.

Proof of Concept

We take USDC as an example, since it has 6 decimals.

1. deposit(USDC, 1000e6): Deposit 1000 USDC into the vault. The user receives 1000 shares.

2. Attempt a full withdrawal withdraw(1000e18, USDC)

   _amount
   = (balance().mul(_shares)).div(totalSupply());
   = (1000e18) * 1000e18 / 1000e18
   = 1000e18

The expected _amount is 1000e6 as per the deposited amount. While the withdrawal will technically work because of the following lines:

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);
  }
}

we note that this logic should only be applied under normal circumstances, where the _amount is assumed to be in the token decimals.

Recommended Mitigation Steps

_amount needs to be un-normalised back to its native token decimals.

Handle

hickuphh3

Vulnerability details

Impact

The vault treats all assets to be of the same price. Given that one can also deposit and withdraw in the same transaction, this offers users the ability to swap available funds held in the vault at parity, with the withdrawal protection fee (0.1%) effectively being the swap fee.

Due care and consideration should therefore be placed if new stablecoins are to be added to the vault (eg. algorithmic ones that tend to occasionally be off-peg), or when lowering the withdrawal protection fee.

Recommended Mitigation Steps

• Prevent users from depositing and withdrawing in the same transaction. This would help prevent potential flash loan attacks as well
• setWithdrawalProtectionFee() could have a requirement for the value to be non-zero. Zero withdrawal fee could be set in setHalted() whereby only withdrawals will be allowed.
• Use price oracles to accurately calculate the shares to be transferred to users for deposits, or token amounts to be sent for withdrawals

Handle

hickuphh3

Vulnerability details

Impact

Let us assume either of the following cases:

1. The vault / protocol is to be winded down or migrated, where either the protocol is halted and withdrawAll() has been called on all active strategies to transfer funds into the vault.
2. There are 0 strategies. Specifically, _controller.strategies() = 0

Attempted withdrawals can be frontrun such that users will receive less, or even no funds in exchange for burning vault tokens. This is primarily enabled by the feature of having deposits in multiple stablecoins.

Proof of Concept

1. Assume getPricePerFullShare() of 1e18 (1 vault token = 1 stablecoin). Alice has 1000 vault tokens, while Mallory has 2000 vault tokens, with the vault holdings being 1000 USDC, 1000 USDT and 1000 DAI.
2. Alice attempts to withdraw her deposit in a desired stablecoin (Eg. USDC).
3. Mallory frontruns Alice's transaction and exchanges 1000 vault tokens for the targeted stablecoin (USDC). The vault now holds 1000 USDT and 1000 DAI.
4. Alice receives nothing in return for her deposit because the vault no longer has any USDC. getPricePerFullShare() now returns 2e18.
5. Mallory splits his withdrawals evenly, by burning 500 vault tokens for 1000 USDT and the other 500 vault tokens for 1000 DAI.

Hence, Mallory is able to steal Alice's funds by frontrunning her withdrawal transaction.

Recommended Mitigation Steps

The withdrawal amount could be checked against getPricePerFullShare(), perhaps with reasonable slippage.