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:
Issue resolution includes “dismissed”, by the client, or “resolved”, per the auditors.
CRITICAL SEVERITY
[No critical severity issues]
HIGH SEVERITY
Swaps unprotected, no minimum
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
Sub-contracts of Router vulnerable if they expose swap publicly or hold balances
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 non-view function fetch price fails to update price.
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 non-view function fetchPrice fails to update the price and retrieve the current price.
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
No Chainlink staleness check
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, _compound() has no return value
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.
Misleading comments
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
Vault Initialization strategy unclear
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.
Inconsistent initialization strategy for CRVTokenOracle
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.
TransparentProxy pattern not fully implemented
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
Does WAVAX need two definitions?
The Router contract has a private constant WAVAX field, while its subcontract Vault has a public (initializer-settable) WAVAX. Is this accidental?
Unused functions
In Router, _getAmountPairOut and _getAmountPairIn are currently unused.
Can nodeType become an enum?
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
Use of safeApprove recommended
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?
Redundant require() in LPTokenPriceFeed
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);
}
}
Asymmetrical require() in Vault deposit() and redeem() functions
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.
Redundant import in QiTokenOracle
In QiTokenOracle, IPriceFeed.sol is imported twice.
Commented constructors in integrations contracts
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.
Compiler bugs
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.