Yeti Finance Contracts

Swaps of yield/reward in Vault::_compound are exploitable via sandwich attacks. There is no minimum amount-out specified. An attacker can tilt the swap pool arbitrarily and profit by nearly the full amount of the swap, for amounts swapped above a certain level (roughly one-way swap fees).

Furthermore, the swaps can be triggered whenever an attacker wants, by calling the public compound function. This means that the attack does not need to front-run an actual transaction (which may or may not be profitable for the attacker), it can be performed whenever the attacker considers it profitable.

function compound() external nonReentrant returns (uint256) {
    	return _compound();
}
function _compound() internal returns (uint256) {
  ...
    	for (uint256 i = 0; i < rewardTokens.length; i++) {
  ...
          swap(
           address(WAVAX),
           _underlyingAddress,
           nativeBalance,
           0  // Dedaub: no minimum
          );
  ...
          swap(
           rewardTokens[i],
           _underlyingAddress,
           rewardBalance,
           0  // Dedaub: no minimum
          );
      }
}

The structure of the swap logic in Router is susceptible to mistakes. Overall there is a computation of the funds before a swap and those after, with a check that the difference is more than expected.

 uint256 initialOutAmount =
  IERC20(_endingTokenAddress).balanceOf(address(this));
 ...
 for (uint256 i; i < path.length; i++) {
      	if (path[i].nodeType == 1) {
         	_amount = swapJoePair(...)...
      	} else if (path[i].nodeType == 2) { ...
             	_amount = swapLPToken(...)...
      	}
 }
 uint256 outAmount =
  IERC20(_endingTokenAddress).balanceOf(address(this)) - initialOutAmount;
 require(outAmount >= _minSwapAmount,
         "Did not receive enough tokens to account for slippage");

However, at each individual swap step there is no such check. Instead, all individual swaps are performed unconditionally, swapping the entire available balance in the token at hand (and with no provision for a fair swap price, but this is entirely secondary).

E.g., to swap an LP token:

function swapLPToken(...) ... {
...
   swapJoePair(
        	_pair,
           	token1,
           	token0,
           	IERC20(token1).balanceOf(address(this))
             // Dedaub: entire balance
             );
...
}

The result is that any token (including native funds) held by a contract that exposes this swap publicly is likely stealable. For instance, the Lever contract exposes the swap through functions route, unRoute, and fullTx. E.g.,

function route(
    	address _toUser,
    	address _startingTokenAddress,
    	address _endingTokenAddress,
    	uint256 _amount,
    	uint256 _minSwapAmount
	) external nonReentrant returns (uint256 amountOut) {
    	amountOut = swap(
        	_startingTokenAddress,
        	_endingTokenAddress,
        	_amount,
        	_minSwapAmount
    	);
    	SafeTransferLib.safeTransfer(ERC20(_endingTokenAddress), _toUser,
                                   amountOut);
}

(These functions are claimed in comments to be only callable by BorrowerOperations, with _toUser being the active pool, but no such check is performed. Furthermore, the route and unRoute functions are identical, and even fullTx has significant similarity.)

Scenario: Lever happens to hold a balance in the USDC token. An attacker finds a swap path that involves USDC in some intermediate step. E.g., cUSDC <> USDC <> YUSD . The first step on that path is a Compound unwrap, the second is a TraderJoe swap. The attacker calls swap and passes in a very low amount of cUSDC (compound USDC), in this way they also collect all the USDC that the contract happens to hold, because all of those get swapped in the second step.

The issue can be avoided with cautious use (e.g., Lever never holds funds between transactions) but there are fixes that will mitigate the overall threat more fundamentally. Swaps should not be exposed to possible attackers. Further, the ideal swap logic would be to compute increments of balances inside every swap step, not just at the beginning and end.

In CRVTokenOracle, the function fetchPrice() calls fetchPrice_v() on each of the underlying feeds, instead of fetchPrice(). The price of the underlying feeds thus fails to be updated.

function fetchPrice() external override returns (uint) {
	// MAX_INT
	uint cheapestToken = MAX_UINT;
	for (uint i; i < underlyingFeeds.length; ++i) {
  	  uint underlyingPrice = underlyingFeeds[i].fetchPrice_v();
  	  if (underlyingPrice < cheapestToken) {
    	    cheapestToken = underlyingPrice;
  	  }
	}
	require(cheapestToken != MAX_UINT, "No underlying price feed
      available");
	return curvePool.get_virtual_price().mul(cheapestToken).div(1e18);
}

In QiTokenOracle, the function fetchPrice() calls fetchPrice_v() on the underlying feed, instead of fetchPrice(). The price of the underlying feed thus fails to be updated.

The function also calls exchangeRateStored() rather than exchangeRateCurrent() on the QiToken contract.

function fetchPrice_v() external override view returns (uint) {
	Return IQIToken(qiToken).exchangeRateStored()
      .mul(underlyingFeed.fetchPrice_v()).div(wad);
}

In PriceFeed::_chainlinkIsBroken (or similar) we also expected a staleness check, since Chainlink oracles provide the block timestamp information.

function _chainlinkIsBroken(ChainlinkResponse memory _response) internal view
 returns (bool) {
    	// Check for response call reverted
    	if (!_response.success) {
        	return true;
    	}
    	// Check for an invalid roundId that is 0
    	if (_response.roundId == 0) {
        	return true;
    	}
    	// Check for an invalid timeStamp that is 0, or in the future
    	if (_response.timestamp == 0 || _response.timestamp > block.timestamp) {
        	return true;
    	}
    	// Check for non-positive price
    	if (_response.answer <= 0) {
        	return true;
    	}

    	return false;
}

In Vault, the signature of _compound() indicates that it should return a value of type uint256, but there is no corresponding return statement in the method.

Some comments are wrong. We note the ones that are actively misleading/confusing.

The top comment in aaveVault is the result of a copy-paste mistake.

/**
 * @notice aaveVault is the vault token for compound-like tokens such as Banker
 * Joe jTokens and Benqi qiTokens. It collects rewards from the rewardController
 * and distributes them to the swap so that it can autocompound.
 */

In Vault::redeemFor:

// Below line should throw if allowance is not enough, or if from is the
// caller itself.
if (allowed != type(uint256).max && msg.sender != _from) {
      	allowance[_from][msg.sender] = allowed - _amt;
}
// Dedaub: second clause is false. No throwing if msg.sender == from

It is not entirely clear to us why the initialization strategy employs initializers (which are susceptible to races, and prevent storage fields from being immutable, for gas savings and clarity), instead of setting values at construction.

For instance, in the Vault contract we cannot see mutual dependencies or other factors that prevent initialization-by-constructor.

CRVTokenOracle.sol is Ownable, unlike the other oracles. Initialisation is carried out using an external onlyOwner getParam() function which sets the contract's initial state and then transfers ownership to address(0). This is used instead of a constructor as is the case for all the other oracles.

VaultProxy is a TransparentProxy, but the codebase does not fully follow the TransparentProxy pattern, which requires the use of a ProxyAdmin contract to act as the admin interface of the VaultProxy. This would cleanly separate the proxy’s admin account from other accounts intended to access Vault functionality and would avoid calls from the proxy admin’s account failing unexpectedly.

See the following for more information:

The Router contract has a private constant WAVAX field, while its subcontract Vault has a public (initializer-settable) WAVAX. Is this accidental?

In Router, _getAmountPairOut and _getAmountPairIn are currently unused.

In Router, the description of nodeType strongly suggests an enum. Is there a reason why this is not possible?

	// nodeType
	// 1 = Trader Joe Swap
	// 2 = Joe LP Token
	// 3 = curve pool
	// 4 = convert between native balance and ERC20
	// 5 = comp-like Token for native
	// 6 = aave-like Token
	// 7 = comp-like Token

In Router::setApprovals a regular approve is used instead of safeApprove (which is the convention elsewhere in the code for ERC20 calls). It is not clear if there is any significance, or even if there are any Avalanche tokens that do not conform to the return value requirement of ERC20.

 IERC20(_token).approve(_who, _amount);  // Dedaub: why not safeApprove?

In LPTokenPriceFeed's constructor, there is a check require(decimalSum.div(2) != (decimalSum.add(2)).div(2). However this will always pass because the second operand of != will always be 1 greater than the first one.

constructor(IPriceFeed _base0, uint32 _base0Decimals, IPriceFeed _base1, uint32 _base1Decimals, address _pair, string memory _name) public {
	name = _name;
	base0 = _base0;
	base1 = _base1;
	pair = _pair;
	uint256 decimalSum = _base0Decimals + _base1Decimals;
	require(decimalSum.div(2) != (decimalSum.add(2)).div(2), "Decimals
      must be even");
	// Since sqrtK is actually the shifted decimals, we will need to
      shift by sqrt(shift) => decimalSum / 2
	decimalSum = decimalSum.div(2);
	if (decimalSum > 18) {
  	  shiftByMul = false;
  	  shift = 10 ** (decimalSum - 18);
	}
      else {
  	  shiftByMul = true;
  	  shift = 10 ** (18 - decimalSum);
	}
  }

In the Vault contract, calls to deposit() must pass the constraint require(_amt > 0), but in redeem, require(_amt > 0) has been commented out. This allows any caller to cause the Vault to compound by calling redeem(), but not by calling deposit(). Unless there is a reason behind this asymmetry, both could be guarded by a require(). A user who wants to claim the compounding reward without depositing or withdrawing can call compound() directly.

In QiTokenOracle, IPriceFeed.sol is imported twice.

The contracts aaveV3Vault, aaveVault, compVault, JLPVault and sJOEVault all have constructors which are commented out. Consider cleaning the source files to avoid doubts about the intended functionality of the contracts.

The contracts are compiled with the Solidity compiler v0.8.10 which, at the time of writing, has no known issues.