Skip to main content
Dedaub

Yeti Finance Contracts

Smart Contract Security Assessment

May 06, 2022

Yeti

SUMMARY


ABSTRACT

Dedaub was commissioned to perform an audit on selected contracts from Yeti Finance’s VaultTokenRepo and YetiFinanceContracts repositories. In the past, we have audited other parts of the Yeti protocol (although not yet its entirety) and such past reports should be consulted jointly with the current.

The audited contracts list is the following:

VaultTokenRepo

** ** Commit 31ade50c7a24e315faa26880e22b747b3b933073

contracts/src/Lever.sol contracts/src/Router.sol contracts/src/Vault.sol contracts/src/VaultProxy.sol contracts/src/integrations/CRVVault.sol contracts/src/integrations/JLPVault.sol contracts/src/integrations/aaveV3Vault.sol contracts/src/integrations/aaveVault.sol contracts/src/integrations/compVault.sol contracts/src/integrations/sJOEVault.sol

YetiFinanceContracts

commit 3a72d2b8fb184dfa32da01fdc5b92d52853509e2

contracts/Core/PriceFeed.sol contracts/Oracles/CRVTokenOracle.sol contracts/Oracles/LPTokenPriceFeed.sol contracts/Oracles/QiTokenOracle.sol contracts/Oracles/VaultOracle.sol

Two auditors worked on the codebase over two weeks.


SETTING & CAVEATS

The audit’s main target is security threats, i.e., what the community understanding would likely call "hacking", rather than regular use of the protocol. Functional correctness (i.e., issues in "regular use") is a secondary consideration. Typically it can only be covered if we are provided with unambiguous (i.e., full-detail) specifications of what is the expected, correct behavior. In terms of functional correctness, we often trusted the code’s calculations and interactions, in the absence of any other specification. Functional correctness relative to low-level calculations (including units, scaling, quantities returned from external protocols) is generally most effectively done through thorough testing rather than human auditing.


VULNERABILITIES & FUNCTIONAL ISSUES

This section details issues that affect the functionality of the contract. Dedaub generally categorizes issues according to the following severities, but may also take other considerations into account such as impact or difficulty in exploitation:

Category
Description
CRITICAL
Can be profitably exploited by any knowledgeable third-party attacker to drain a portion of the system’s or users’ funds OR the contract does not function as intended and severe loss of funds may result.
HIGH
Third-party attackers or faulty functionality may block the system or cause the system or users to lose funds. Important system invariants can be violated.
MEDIUM
Examples:
  • User or system funds can be lost when third-party systems misbehave.
  • DoS, under specific conditions.
  • Part of the functionality becomes unusable due to a programming error.
LOW
Examples:
  • Breaking important system invariants but without apparent consequences.
  • Buggy functionality for trusted users where a workaround exists.
  • Security issues which may manifest when the system evolves.

Issue resolution includes “dismissed”, by the client, or “resolved”, per the auditors.


CRITICAL SEVERITY

[No critical severity issues]


HIGH SEVERITY

H1

Swaps unprotected, no minimum

H1HIGH

Swaps unprotected, no minimum
open

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


MEDIUM SEVERITY

M1

Sub-contracts of Router vulnerable if they expose swap publicly or hold balances

M1MEDIUM

Sub-contracts of Router vulnerable if they expose swap publicly or hold balances
open

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.

M2

In CRVTokenOracle, the non-view function fetch price fails to update price.

M2MEDIUM

In CRVTokenOracle, the non-view function fetch price fails to update price.
open

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

M3

In QiTokenOracle, the non-view function fetchPrice fails to update the price and retrieve the current price.

M3MEDIUM

In QiTokenOracle, the non-view function fetchPrice fails to update the price and retrieve the current price.
open

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


LOW SEVERITY

L1

L1LOW

No Chainlink staleness check
open

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

L2

In Vault, _compound() has no return value

L2LOW

In Vault, _compound() has no return value
open

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.



OTHER/ADVISORY ISSUES

This section details issues that are not thought to directly affect the functionality of the project, but we recommend considering them.

A1

Misleading comments

A1ADVISORY

Misleading comments
open

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

A2

Vault Initialization strategy unclear

A2ADVISORY

Vault Initialization strategy unclear
open

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.

A3

Inconsistent initialization strategy for CRVTokenOracle

A3ADVISORY

Inconsistent initialization strategy for CRVTokenOracle
open

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.

A4

TransparentProxy pattern not fully implemented

A4ADVISORY

TransparentProxy pattern not fully implemented
open

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:

https://docs.openzeppelin.com/contracts/4.x/api/proxy#TransparentUpgradeableProxy

A5

Does WAVAX need two definitions?

A5ADVISORY

Does WAVAX need two definitions?
open

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

A6

Unused functions

A6ADVISORY

Unused functions
open

In Router, _getAmountPairOut and _getAmountPairIn are currently unused.

A7

Can nodeType become an enum?

A7ADVISORY

Can nodeType become an enum?
open

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

A8

A8ADVISORY

Use of safeApprove recommended
open

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?

A9

Redundant require() in LPTokenPriceFeed

A9ADVISORY

Redundant require() in LPTokenPriceFeed
open

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

A10

Asymmetrical require() in Vault deposit() and redeem() functions

A10ADVISORY

Asymmetrical require() in Vault deposit() and redeem() functions
open

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.

A11

Redundant import in QiTokenOracle

A11ADVISORY

Redundant import in QiTokenOracle
open

In QiTokenOracle, IPriceFeed.sol is imported twice.

A12

Commented constructors in integrations contracts

A12ADVISORY

Commented constructors in integrations contracts
open

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.

A13

Compiler bugs

A13ADVISORY

Compiler bugs
info

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



DISCLAIMER

The audited contracts have been analyzed using automated techniques and extensive human inspection in accordance with state-of-the-art practices as of the date of this report. The audit makes no statements or warranties on the security of the code. On its own, it cannot be considered a sufficient assessment of the correctness of the contract. While we have conducted an analysis to the best of our ability, it is our recommendation for high-value contracts to commission several independent audits, a public bug bounty program, as well as continuous security auditing and monitoring through Dedaub Security Suite.


ABOUT DEDAUB

Dedaub offers significant security expertise combined with cutting-edge program analysis technology to secure some of the most prominent protocols in DeFi. The founders, as well as many of Dedaub's auditors, have a strong academic research background together with a real-world hacker mentality to secure code. Protocol blockchain developers hire us for our foundational analysis tools and deep expertise in program analysis, reverse engineering, DeFi exploits, cryptography and financial mathematics.

Dedaub