Yeti Finance

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

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.

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

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.

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

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

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

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.

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.

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.

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.

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

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

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;

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.

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.

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.

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

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.

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.