yAxis contest - cmichel's results

Handle

cmichel

Vulnerability details

The Controller.setCap function sets a cap for a strategy and withdraws any excess amounts (_diff). The vault balance is decreased by the entire strategy balance instead of by this _diff:

// @audit why not sub _diff?
_vaultDetails[_vault].balance = _vaultDetails[_vault].balance.sub(_balance);

Impact

The _vaultDetails[_vault].balance variable does not correctly track the actual vault balances anymore, it will usually underestimate the vault balance. This variable is used in Controller.balanceOf(), which in turn is used in Vault.balance(), which in turn is used to determine how many shares to mint / amount to receive when redeeming shares. If the value is less, users will lose money as they can redeem fewer tokens. Also, an attacker can deposit and will receive more shares than they should receive. They can then wait until the balance is correctly updated again and withdraw their shares for a higher amount than they deposited. This leads to the vault losing tokens.

Recommended Mitigation Steps

Sub the _diff instead of the balance: _vaultDetails[_vault].balance = _vaultDetails[_vault].balance.sub(_diff);

Handle

cmichel

Vulnerability details

The Vault.balanceOfThis function values all tokens equally. They are normalized to 18 decimals and then simply added up:

for (uint8 i; i < _tokens.length; i++) {
    address _token = _tokens[i];
    // adds up different tokens here, treating them as exactly equal value 1.0 A = 1.0 B
    _balance = _balance.add(_normalizeDecimals(_token, IERC20(_token).balanceOf(address(this))));
}

Impact

The balanceOfThis function does not take into account the actual market prices these tokens trade at. It would treat 1.0 USDC as 1.0 WETH and simply add these values. Even if only stablecoins are used, they can still differ in value, especially with non-reserve-backed algorithmic stablecoins like DAI.

If DAI is valued at 0.99$ but USDC is valued at 1.00$, then arbitragers will deposit(DAI, amount), receive shares, withdraw the shares for USDC (withdraw(shares, USDC)) and make a profit (assuming the _withdrawalProtectionFee is set to an adequate value). The contract will run out of the higher-valued USDC asset immediately, and overall lose total value.

Recommended Mitigation Steps

Use a price oracle for each token with a common denominating asset, iterate over the tokens, convert them with the oracle price, and then add them up.

Alternatively, set a high withdrawal fee _withdrawalProtectionFee but this is bad for the user and could turn off users from depositing anything. Assume one gets 12% APY but the withdrawal fee is set to 2%, a user will lose money if they withdraw before 2 months.

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. The code is adding a non-normalized amount (for example 6 decimals only for USDC) to a normalized (18 decimals).

Impact

The result is that the balance() will be under-reported. This leads to receiving wrong shares when depositing tokens, and a wrong amount when redeeming tokens.

Recommended Mitigation Steps

The second term IController(manager.controllers(address(this))).balanceOf() must also be normalized before adding it. IController(manager.controllers(address(this))).balanceOf() uses _vaultDetails[msg.sender].balance which directly uses the raw token amounts which are not normalized.

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.

Handle

cmichel

Vulnerability details

The Controller.withdrawAll decreases the vault balance by _amount, the want token amount that has been withdrawn from the strategy and transferred to the vault. Note that _amount gets overwritten in the _convert != address(0) branch and is a _convert token value instead of a _want token value:

if (_convert != address(0)) {
    // @audit _amount is overwritten here with the traded return amount of _convert token!
    _amount = _converter.convert(_want, _convert, _amount, 1);
    IERC20(_convert).safeTransfer(_vault, _amount);
} else {
    IERC20(_want).safeTransfer(_vault, _amount);
}
if (_balance >= _amount) {
    // @audit they subtract the wrong _amount here if _convert was set
    _vaultDetails[_vault].balance = _vaultDetails[_vault].balance.sub(_amount);
}

Impact

The _vaultDetails[_vault].balance variable does not correctly track the actual vault balances anymore as the traded amount of _convert token could have a completely different value and decimals than the want token. It could heavily over-or-underestimate the actual withdrawn want amount.

This variable is used in Controller.balanceOf(), which in turn is used in Vault.balance(), which in turn is used to determine how many shares to mint / amount to receive when redeeming shares. If the value is less, users will lose money as they can redeem fewer tokens. If the value is more, users can redeem their existing shares for more value, leading to a bankrun until the vault is emptied and other users cannot redeem anymore.

Recommended Mitigation Steps

Do not overwrite the _amount variable in the if branch.

Handle

cmichel

Vulnerability details

There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer() or transferFrom(). Others are rebasing tokens that increase in value over time like Aave's aTokens (balanceOf changes over time).

Impact

The VaultHelper's depositVault() and depositMultipleVault functions transfer _amount to this contract using IERC20(_token).safeTransferFrom(msg.sender, address(this), _amount);. This could have a fee, and less than _amount ends up in the contract. The next actual vault deposit using IVault(_vault).deposit(_token, _amount); will then try to transfer more than the this contract actually has and will revert the transaction.

Recommended Mitigation Steps

One possible mitigation is to measure the asset change right before and after the asset-transferring routines. This is already done correctly in the Vault.deposit function.

Handle

cmichel

Vulnerability details

The ERC20.transfer() and ERC20.transferFrom() functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead.

The Manager.recoverToken function does not check the return value of this function.

Impact

Tokens that don't actually perform the transfer and return false are still counted as a correct transfer. Furthermore, tokens that do not correctly implement the EIP20 standard, like USDT which does not return a success boolean, will revert.

Recommended Mitigation Steps

We recommend using OpenZeppelin’s SafeERC20 versions with the safeTransfer and safeTransferFrom functions that handle the return value check as well as non-standard-compliant tokens.

Handle

cmichel

Vulnerability details

The Vault.withdraw function attempts to withdraw funds from the controller if there are not enough in the vault already. In the case the controller could not withdraw enough, i.e., where _diff < _toWithdraw, the user will receive less output tokens than their fair share would entitle them to (the initial _amount).

if (_diff < _toWithdraw) {
    // @audit burns too many shares for a below fair-share amount
    _amount = _balance.add(_diff);
}

Impact

The withdrawer receives fewer output tokens than they were entitled to.

Recommended Mitigation Steps

In the mentioned case, the _shares should be recomputed to match the actual withdrawn _amount tokens:

if (_diff < _toWithdraw) {
    _amount = _balance.add(_diff);
    // recompute _shares to burn based on the lower payout
    // should be something like this, better to cache balance() once at the start and use that cached value
    _shares = (totalSupply().mul(_amount)).div(_balance);
}

Only these shares should then be burned.

Handle

cmichel

Vulnerability details

The Controller.inCaseStrategyGetStuck withdraws from a strategy but does not call updateBalance(_vault, _strategy) afterwards.

Impact

The _vaultDetails[_vault].balances[_strategy] variable does not correctly track the actual strategy balance anymore. I'm not sure what exactly this field is used for besides getting the withdraw amounts per strategy in getBestStrategyWithdraw. As the strategy contains a lower amount than stored in the field, Controller.withdraw will attempt to withdraw too much.

Recommended Mitigation Steps

Call updateBalance(_vault, _strategy) in inCaseStrategyGetStuck.

Handle

cmichel

Vulnerability details

One vault can have many tokens, but each token should only be assigned to a single vault. The Manager contract keeps a mapping of tokens to vaults in the vaults[_token] => _vault map, and a mapping of vault to tokens in tokens[vault] => token[].

The addToken function can assign any token to a single vault and allows overwriting an existing vaults[_token] map entry with a different vault. This indirectly disassociates the previous vault for the token. Note that the previous vault's tokens[_previousVault] map still contains the token.

Impact

The token disappears from the system for the previous vault but the actual tokens are still in there, getting stuck. Only the new vault is considered for the token anymore, which leads to many issues, see Controller.getBestStrategyWithdraw and the onlyVault modifier that doesn't work correctly anymore.

Recommended Mitigation Steps

It should check if the token is already used in a map, and either revert or correctly remove the token from the vault - from the tokens array. It should do the same cleanup procedure as in removeToken:

if (found) {
    // remove the token from the vault
    tokens[_vault][index] = tokens[_vault][k-1];
    tokens[_vault].pop();
    delete vaults[_token];
    emit TokenRemoved(_vault, _token);
}

addToken should also check that the token is not already in the tokens[_vault] array.

Handle

cmichel

Vulnerability details

The YAxisVotePower.balanceOf contract uses the Uniswap pool reserves to compute a _lpStakingYax reward:

(uint256 _yaxReserves,,) = yaxisEthUniswapV2Pair.getReserves();
int256 _lpStakingYax = _yaxReserves
      .mul(_stakeAmount)
      .div(_supply)
      .add(rewardsYaxisEth.earned(_voter));

The pool can be temporarily manipulated to increase the _yaxReserves amount.

Impact

If this voting power is used for governance proposals, an attacker can increase their voting power and pass a proposal.

Recommended Mitigation Steps

One could build a TWAP-style contract that tracks a time-weighted-average reserve amount (instead of the price in traditional TWAPs). This can then not be manipulated by flashloans.

Handle

cmichel

Vulnerability details

The Harvester.getEstimates contract tries to estimate a YAXIS amount but uses the wrong path and/or amount.

It currently uses a WETH input amount to compute a YAXIS -> WETH trade.

address[] memory _path;
_path[0] = IStrategy(_strategy).want();
_path[1] = IStrategy(_strategy).weth();
// ...

_path[0] = manager.yaxis();
// path is YAXIS -> WETH now
// fee is a WETH precision value
uint256 _fee = _estimatedWETH.mul(manager.treasuryFee()).div(ONE_HUNDRED_PERCENT);
// this will return wrong trade amounts
_amounts = _router.getAmountsOut(_fee, _path);
_estimatedYAXIS = _amounts[1];

Impact

The estimations from getEstimates are wrong. They seem to be used to provide min. amount slippage values (_estimatedWETH, _estimatedYAXIS) for the harvester when calling Controller._estimatedYAXIS. These are then used in BaseStrategy._payHarvestFees and can revert the harvest transactions if the wrongly computed _estimatedYAXIS value is above the actual trade value. Or they can allow a large slippage if the _estimatedYAXIS value is below the actual trade value, which can then be used for a sandwich attack.

Recommended Mitigation Steps

Fix the estimations computations.

As the estimations are used in BaseStrategy._payHarvestFees, the expected behavior seems to be trading WETH to YAXIS. The path should therefore be changed to path[0] = WETH; path[1] = YAXIS in Harvester.getEstimates.