Skip to main content
Dedaub

Yeti Finance

Smart Contract Security Assessment

Dec 21, 2021

Yeti

SUMMARY


ABSTRACT

Dedaub was commissioned to perform a security audit on a subset of Yeti Finance’s smart contracts. More specifically, the audit focused on the following files:

  • BorrowerOperations.sol
  • TroveManager.sol
  • TroveManagerLiquidations.sol
  • TroveManagerRedemptions.sol
  • StabilityPool.sol

The code was audited at commit

62ec9ceea6a1d1cac7843a4361f7bed42d79a35f (main).

The audit also examined any other functionality and contracts that were highly related with the aforementioned contracts. Two auditors worked over the codebase for a little over two weeks.

Yeti is a capital efficient lending protocol, designed to work with multiple collaterals such as Uniswap V2-style LP tokens. It achieves high capital efficiency by borrowing important design elements from Liquity. For instance, liquidations in Yeti happen using a single action that moves collateral from troves (similar to Maker CDPs) to stability providers, and burning the equivalent YUSD position that was liquidated from the stability pool. As opposed to the original Liquity protocol, Yeti allows multiple types of collaterals, with different collateralization ratios for each.


SETTING & CAVEATS

The focus of the audit was strategically limited to the core of the protocol. The time allotted was not sufficient to review the entire protocol, therefore the scope was limited (by the customer) to less than 5000 lines of code, and protocol-level documents. In order to finish the audit in the allotted time, while still maintaining a level of assurance, we:

  • reviewed core parts of the code carefully (e.g., Liquidations)
  • focused on code differences from the Liquity code
  • questioned the economic threats to the high-level protocol
  • trusted some of the protocol calculations (e.g., fees).

That is, we put more weight on reviewing threats in the code and the economic design, and less on the correct implementation of calculations of loan and fee parameters. We did this with the understanding that rigorous testing of these calculations is being carried out separately.


PROTOCOL-LEVEL CONSIDERATIONS

The Yeti protocol is more centralized than Liquity, given that the owner (of the Whitelist contract) can change safety ratios (and more). Since safety ratios affect the virtual price of different collateral types, the owner can affect liquidations, redemption order, etc. The intent is that safety ratios will not change during a deployment of the contracts, although the functionality to change them is there.


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

Sensitive function StabilityPool::receiveCollateral is unprotected

H1HIGH

Sensitive function StabilityPool::receiveCollateral is unprotected
resolved

Untrusted callers can change key parameters of the stability pool, since the receiveCollateral function does not check its caller:

function receiveCollateral(address[] memory _tokens, uint256[] memory _amounts)
external
override
{
totalColl.amounts = _leftSumColls(totalColl, _tokens, _amounts);
emit StabilityPoolBalancesUpdated(_tokens, _amounts);
}


MEDIUM SEVERITY

M1

SafeMath operation result not assigned to variable

M1MEDIUM

SafeMath operation result not assigned to variable
resolved

In BorrowerOperations::openTroveRouter, there is a SafeMath expression, whose result is not being assigned:

function openTroveRouter(...) {
...
if (params.j < _routerAVAXIndices.length && params.i ==
_routerAVAXIndices[params.j]) {

// Dedaub: Unassigned SafeMath op
params.avaxSent.sub(_routerAmounts[params.i], "BOps: Router amount is
less than avax sent");
...
}

This means that a transaction may not revert when it should. If the contract were to hold AVAX between transactions (and if the openTroveRouter functionality remains) this can become a serious issue.

M2

Function returns unconditionally on the first loop iteration

M2MEDIUM

Function returns unconditionally on the first loop iteration
resolved

In TroveManager::hasPendingRewards, the loop always returns on the first iteration, which does not seem to be the desired behavior.

function hasPendingRewards(address _borrower) public view override returns (bool) {
...
for (uint i = 0; i < assets.length; i++) {
address token = assets[i];
return (rewardSnapshots[_borrower].CollRewards[token] < L_Coll[token]);
}
return false;
}

A fix should look something like the following:

function hasPendingRewards(address _borrower) public view override returns (bool) {
for (uint i = 0; i < assets.length; i++) {
address token = assets[i];
if (rewardSnapshots[_borrower].CollRewards[token] < L_Coll[token])
return true;
}
return false;
}


LOW SEVERITY

L1

Redundant code

L1LOW

Redundant code
resolved

There are various places where there’s redundant code:

  • BorrowerOperations::_openTroveInternal:

    function _openTroveInternal(...) {
    ...
    vars.YUSDFee; // Dedaub: no-op
    ...
    }
  • TroveManagerRedemptions::_redeemCollateralFromTrove:

    function _redeemCollateralFromTrove(...) {
    ...
    newColls memory borrowerColls; // Dedaub: unused
    ...
    }
  • TroveManagerRedemptions::redeemCollateral:

    function redeemCollateral(...) {
    ...
    _requireYUSDBalanceCoversRedemption(
    contractsCache.yusdToken,
    _redeemer,
    _YUSDamount);
    ...
    _requireYUSDBalanceCoversRedemption(
    contractsCache.yusdToken,
    _redeemer,
    _YUSDamount.add(totals.YUSDfee)
    );
    // Dedaub: second check seems to subsume the first
    ...
    }
  • TroveManagerLiquidations::_sendGasCompensation:

    function _sendGasCompensation(
    IActivePool _activePool,
    address _liquidator,
    uint256 _YUSD,
    address[] memory _tokens,
    uint256[] memory _amounts
    ) internal {
    // Dedaub: Unnecessary wrapping
    newColls memory _collToSend = newColls(_tokens, _amounts);
    if (_YUSD > 0) {
    yusdTokenContract.returnFromPool(gasPoolAddress, _liquidator, _YUSD);
    }

    _activePool.sendCollateralsUnwrap(_liquidator, _collToSend.tokens, _collToSend.amounts, false);
    }
  • Function BorrowerOperations::_openTroveInternalFinish is not being used anywhere and is internal.

It is recommended that redundant code be removed from the codebase.

L2

Loop can be optimized for gas savings in TroveManagerRedemptions::_requireNonZeroRedemptionAmount

L2LOW

Loop can be optimized for gas savings in TroveManagerRedemptions::_requireNonZeroRedemptionAmount
resolved

In this function, a total is computed merely to require that it is not zero. It is easy to set a flag to true and break out of the loop early, and require the flag at the end.

function _requireNonZeroRedemptionAmount(newColls memory coll) internal pure {
uint256 total = 0;
for (uint256 i = 0; i < coll.amounts.length; i++) {
total = total.add(coll.amounts[i]);
}
require(total > 0, "must be non zero redemption amount");
}

L3

Loop can be optimized for gas savings in BorrowerOperations::_requireNoOverlapCollsWithItself

L3LOW

Loop can be optimized for gas savings in BorrowerOperations::_requireNoOverlapCollsWithItself
resolved

In this function, a doubly-nested loop can be optimized for an average of half the cost:

function _requireNoOverlapCollsWithItself(address[] memory _colls) internal pure {
for (uint256 i = 0; i < _colls.length; i++) {
for (uint256 j = 0; j < _colls.length; j++) {
// Dedaub: can start from i+1 and skip the "if (i != j)"
if (i != j) {
require(_colls[i] != _colls[j], "BorrowerOps: Collateral … overlaps");
}
}
}
}

L4

Unclear why BorrowerOperations::claimCollateral exists

L4LOW

Unclear why BorrowerOperations::claimCollateral exists
resolved

Function BorrowerOperations::claimCollateral is the only allowed caller of CollSurplusPool::claimColl, and supplies the latter with msg.sender as the argument. It is not clear why claimCollateral should even exist, instead of allowing msg.sender to claimColl on the CollSurplusPool directly.

// BorrrowerOperations
function claimCollateral() external override {
// send collateral from CollSurplus Pool to owner
collSurplusPool.claimColl(msg.sender);
}

// CollSurplusPool
function claimColl(address _account) external override {
_requireCallerIsBorrowerOperations();
...
}

L5

Some ERC20 operations may fail

L5LOW

Some ERC20 operations may fail
resolved

Some ERC20 operations (transfer, transferFrom and approve) will fail, if the underlying token is not fully compliant with the ERC20 standard, as it is possible that they do not return a boolean value for the aforementioned operations to indicate the success of the call (most notable such case is USDT on the Ethereum mainnet). The standard transfer/transferFrom is not compatible with such tokens in code produced by the current Solidity compiler, since it expects a return value.

While we are not aware of such tokens on the Avalanche network, it is recommended that at the very least the use of OpenZeppelin SafeERC20 wrappers be considered.



OTHER/ ADVISORY ISSUES

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

A1

No “emit” keyword on some event emit statements

A1ADVISORY

No “emit” keyword on some event emit statements
resolved

In ActivePool, there are some event emissions in increaseYUSDDebt and decreaseYUSDDebt that do not have the emit keyword. While this is valid Solidity, it is highly recommended that the “emit” keyword be used, for clarity and code style consistency.

A2

Use of both “now” and “block.timestamp”

A2ADVISORY

Use of both “now” and “block.timestamp”
resolved

In BorrowerOperations::setAddresses (and in files in the code base outside the scope of our audit e.g. YUSDToken), the Solidity “now” is being used instead of the “block.timestamp” style, which we found more commonly in the code.

It is good practice to stick to one of these two alternatives for a more consistent codebase.

A3

Unchecked setAddresses arguments

A3ADVISORY

Unchecked setAddresses arguments
dismissed

Throughout the code, there are many instances of contracts that utilize a one-time call setAddresses to initialize their external dependencies. Part of this process is checking whether some of the addresses which are assumed to be contracts are actually contracts. However, there are instances where this check does not take place:

  • BorrowerOperations::setAddresses::_whitelistAddress
  • ActivePool::setAddresses::_collSurplusPoolAddress
  • CollSurplusPool::setAddresses::_whitelistAddress

It is recommended that these addresses be checked, to protect against faulty initialization, especially since it’s a one-time operation.

A4

Argument used that’s expected to always be msg.sender

A4ADVISORY

Argument used that’s expected to always be msg.sender
resolved

In BorrowerOperations::_adjustTrove, the _borrower argument is always required to be msg.sender. It is, thus, both cheaper and simpler to eliminate it and access msg.sender instead.

function _adjustTrove(address _borrower, AdjustTrove_Params memory params) internal {
...
assert(msg.sender == _borrower);
...

A5

Syntax misleading: function doesn’t return a value

A5ADVISORY

Syntax misleading: function doesn’t return a value
resolved

Function TroveManager::updateTroveRewardSnapshots does not return anything but its syntax is as if it does:

function updateTroveRewardSnapshots(address _borrower) external override {
_requireCallerIsBorrowerOperations();
return _updateTroveRewardSnapshots(_borrower);
}

A6

Fields can be declared private

A6ADVISORY

Fields can be declared private
resolved

The following fields in TroveManager can be declared private:

// Dedaub: Can be made private, getters defined manually.
mapping (address => uint) public L_Coll;
mapping (address => uint) public L_YUSDDebt;

// Dedaub: Can be made private, getters already defined manually for both contents and length.
address[] public TroveOwners;

A7

Effectively duplicate fields

A7ADVISORY

Effectively duplicate fields
dismissed

There are a couple of instances in the codebase where two storage fields are being maintained with the only difference being their type. For example, in TroveManagerRedemptions:setAddresses we have:

function setAddresses(..) {
...
troveManager = ITroveManager(_troveManagerAddress);

// Dedaub: Not needed since you already have troveManager (and vice versa)
troveManagerAddress = _troveManagerAddress;
...
}

It is recommended that such field pairs be eliminated, and only one of the two be maintained, with the code modified as needed with the necessary casts.

A8

Unused storage fields

A8ADVISORY

Unused storage fields
resolved

TroveManager, TroveManagerLiquidations, TroveManagerRedemptions all share a common base contract, TroveManagerBase. They use similar functions setAddresses to initialize storage fields of that base class. However, not all three subcontracts need all storage fields of the base contract. For instance, TroveManagerLiquidations has no use for yusdTokenAddress or sortedTrovesAddress. The developers should weigh simplicity against the minor cost of having such unused fields.

A9

Event-based model unclear

A9ADVISORY

Event-based model unclear
info

In the TroveManager, it would help our understanding (and enhance checking for omissions) to have a clearer model/documentation for when event-based actions (decayBaseRateFromBorrowing, applyPendingRewards) should take place.

A10

Magic constants are used for status

A10ADVISORY

Magic constants are used for status
resolved

In BorrowerOperations, the constant 1 is being used to denote Status.active. This is an error-prone practice, since the dependency on the enum fields gets buried.

...
// _openTroveInternal
contractsCache.troveManager.setTroveStatus(_troveOwner, 1);
...
// _requireTroveisNotActive|
require(status != 1, "BorrowerOps: Trove is active");

A11

Conditions can be simplified

A11ADVISORY

Conditions can be simplified
resolved

In TroveManagerLiquidations::_liquidateRecoveryMode, there is an if-else if statement that follows the following structure:

function _liquidateRecoveryMode(...) {
if (_ICR <= _100pct) {
...
} else if ((_ICR > _100pct) && (_ICR < MCR)) {
...
} else if (
(_ICR >= MCR) && (_ICR < _TCR) && (singleLiquidation.entireTroveDebt <= _YUSDInStabPool)
) {
...
} else { ... }

...
}

The left-most logical condition in the two “else-if” branches can be safely removed, as they are implied by the negation of the conditions in the above branches.

A12

Compiler known issues

A12ADVISORY

Compiler known issues
info

The contracts were compiled with the Solidity compiler v0.6.11 which, at the time of writing, have some known bugs. We inspected the bugs listed for version 0.6.11 and concluded that the subject code is unaffected.



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