TraderJoeYieldFarmVault Manual Review Findings

TJY-01M: Significant Centralization of Contract Configuration

Type	Severity	Location
Centralization Concern		TraderJoeYieldFarmVault.sol:L390-L392, L398-L400, L406-L408, L414-L416, L422-L424, L431-L433, L439-L441

Description:

The referenced functions permit the TraderJoeYieldFarmVault contract to be arbitrarily configured by its owner.

Example:

Copy
386/**
387  * Update vault config struct
388  * @param _vaultConfig Vault config struct
389*/
390function updateVaultConfig(VaultConfig memory _vaultConfig) external onlyOwner {
391  vaultConfig = _vaultConfig;
392}
393
394/**
395  * Update management fee per second of vault
396  * @param _mgmtFeePerSecond Management fee in 1e18
397*/
398function updateMgmtFeePerSecond(uint256 _mgmtFeePerSecond) external onlyOwner {
399  mgmtFeePerSecond = _mgmtFeePerSecond;
400}
401
402/**
403  * Update performance fee of vault
404  * @param _perfFee Performance fee in 1e18
405*/
406function updatePerfFee(uint256 _perfFee) external onlyOwner {
407  perfFee = _perfFee;
408}
409
410/**
411  * Update vault's reader contract
412  * @param _reader Reader address
413*/
414function updateReader(ITraderJoeYieldFarmReader _reader) external onlyOwner {
415  reader = _reader;
416}
417
418/**
419  * Update vault's manager contract
420  * @param _manager Manager address
421*/
422function updateManager(ITraderJoeYieldFarmManager _manager) external onlyOwner {
423  manager = _manager;
424}
425
426/**
427  * Approve or revoke address to be a keeper for this vault
428  * @param _keeper Keeper address
429  * @param _approval Boolean to approve keeper or not
430*/
431function updateKeeper(address _keeper, bool _approval) external onlyOwner {
432  keepers[_keeper] = _approval;
433}
434
435/**
436  * Update max capacity value
437  * @param _capacityValue Capacity value in 1e18
438*/
439function updateCapacity(uint256 _capacityValue) external onlyOwner {
440  capacityValue = _capacityValue;
441}

Recommendation:

We advise the functions to be invoke-able by a decentralized entity, preferably sitting behind a timelock as the present implementation permits sensitive variables such as mgmtFeePerSecond to be adjusted at will (TraderJoeYieldFarmVault::updateMgmtFeePerSecond).

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

The Steadefi team has stated that they will implement a path to decentralization for all the deployed contracts within their ecosystem by utilizing a combination of a governance mechanism and a Timelock contract. As such, we consider this exhibit alleviated based on the fact that the Steadefi team will implement a decentralization path in the near future.

TJY-02M: Improper Tokenomic Share System

Type	Severity	Location
Logical Fault		TraderJoeYieldFarmVault.sol:L310, L313

Description:

The share system of TraderJoeYieldFarmVault is incorrect as the shares it mints to a deposit are proportionate to the equity the user mints. This causes the system to provide an abnormal amount of shares for small deposits when the system's targetLeverage has been adjusted.

Impact:

After multiple discussions with the Steadefi team and extensive test suites and documentation on their end, we concluded that the impact of this exhibit is inexistent as the funds that can ultimately be withdrawn from the vaules remain the same.

Example:

Copy
310uint256 _equityChange = reader.equityValue() - equityBefore;
311
312// calculate shares to users
313uint256 sharesToUser = valueToShares(_equityChange, equityBefore);
314
315_mint(msg.sender, sharesToUser);

Recommendation:

We advise adjustments of the targetLeverage to be accompanied by a rebalance operation in the same call, ensuring that the equity and debt of the vault at any given point in time reflects the desired targetLeverage.

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

The Steadefi team adjusted the updateVaultConfig function to accept multiple new arguments and perform a rebalance operation after the configuration has been updated, alleviating this exhibit in full.

TJY-03M: Improper Handling of Native Deposits

Type	Severity	Location
Language Specific		TraderJoeYieldFarmVault.sol:L165-L180

Description:

The TraderJoeYieldFarmVault::deposit function improperly handles native fund deposits. In detail, if a user submits a TraderJoeYieldFarmVault::deposit call with a non-zero msg.value but a false value for _isNative, their funds will be locked in the contract forever.

Impact:

It is currently possible to lock native funds within the contract that will be irretrievable.

Example:

Copy
165if (_isNative) {
166  require(vaultConfig.isNativeAsset, "Vault asset not native token");
167  require(_amt == msg.value, "Amt != msg.value");
168
169  token = tokenA;
170
171  IWAVAX(address(token)).deposit{ value: msg.value }();
172} else {
173  if (strategy == VaultStrategy.Neutral || strategy == VaultStrategy.Short) {
174    token = tokenB;
175  } else if (strategy == VaultStrategy.Long) {
176    token = tokenA;
177  }
178
179  token.safeTransferFrom(msg.sender, address(this), _amt);
180}

Recommendation:

We advise the code to instead mandate that vaultConfig.isNativeAsset is false in the else branch of the top-level if-else construct in the TraderJoeYieldFarmVault::deposit function, preventing native funds from being locked in the contract. Alternatively, if vaultConfig.isNativeAsset equates strategy == VaultStrategy.Long thus allowing either native funds or the wrapped token itself to be supplied for a TraderJoeYieldFarmVault::deposit call, we advise the else branch to ensure that msg.value is 0.

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

The TraderJoeYieldFarmVault::deposit flow was altered per our recommendation, making use of the vault.Config.isNativeAsset contract-level variable and preventing the lock of native funds in the contract due to misuse of the functions.

TJY-04M: Inexistent Protection Against Paused Invocation

Type	Severity	Location
Logical Fault		TraderJoeYieldFarmVault.sol:L460

Description:

The TraderJoeYieldFarmVault::rebalance function can be invoked when the contract is paused, minting a management fee incorrectly.

Impact:

A management fee collected during the system's emergency state is undesirable and can be harmful to existing users of the contract who have not withdrawn their shares yet via the TraderJoeYieldFarmVault::emergencyWithdraw function.

Example:

Copy
452function rebalance(
453  ManagerAction _action,
454  uint256 _lpAmt,
455  uint256 _borrowTokenAAmt,
456  uint256 _borrowTokenBAmt,
457  uint256 _repayTokenAAmt,
458  uint256 _repayTokenBAmt
459) external onlyKeeper {
460  _mintMgmtFee();

Recommendation:

We advise the function to be guarded by the whenNotPaused modifier, ensuring that a management fee is collected solely when the contract is in operation.

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

The TraderJoeYieldFarmVault::rebalance function properly prevents invocations when the protocol is paused via the whenNotPaused modifier, alleviating this exhibit.

TJY-05M: Inexistent Slippage Protections

Type	Severity	Location
Logical Fault		TraderJoeYieldFarmVault.sol:L184, L214

Description:

The TraderJoeYieldFarmVault::deposit and TraderJoeYieldFarmVault::withdraw functions do not apply any form of slippage protection, rendering all depositors and withdrawers prone to slippage related attacks (such as sandwich attacks).

Impact:

The current mechanisms of the contract render the users prone to arbitrage attacks, causing them to receive less shares and less funds than they expect from their deposits and withdrawals. This is especially prevalent in TraderJoeYieldFarmVault whereby the total supply of the vault is continuously increased via TraderJoeYieldFarmVault::_mintMgmtFee.

Example:

Copy
160function deposit(uint256 _amt, bool _isNative) external payable nonReentrant whenNotPaused {
161  _mintMgmtFee();
162
163  IERC20 token;
164
165  if (_isNative) {
166    require(vaultConfig.isNativeAsset, "Vault asset not native token");
167    require(_amt == msg.value, "Amt != msg.value");
168
169    token = tokenA;
170
171    IWAVAX(address(token)).deposit{ value: msg.value }();
172  } else {
173    if (strategy == VaultStrategy.Neutral || strategy == VaultStrategy.Short) {
174      token = tokenB;
175    } else if (strategy == VaultStrategy.Long) {
176      token = tokenA;
177    }
178
179    token.safeTransferFrom(msg.sender, address(this), _amt);
180  }
181
182  token.safeTransfer(address(manager), _amt);
183
184  uint256 sharesToUser = _deposit(_amt, reader.tokenValue(address(token), _amt));
185
186  emit Deposit(
187    msg.sender,
188    address(token),
189    _amt,
190    sharesToUser
191  );
192}

Recommendation:

We advise both functions to introduce a new variable representing the minimum sharesToUser and minimum withdrawAmt respectively that they wish to accept as a result of their TraderJoeYieldFarmVault::deposit / TraderJoeYieldFarmVault::withdraw transaction.

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

Both functions have had a minimum share / withdrawal amount argument introduced, ensuring that the user is able to control the slippage they render themselves prone to.

TJY-06M: Precision Loss of Convoluted Calculations

Type	Severity	Location
Mathematical Operations		TraderJoeYieldFarmVault.sol:L289-L293, L295-L299

Description:

The referenced calculations are redundant as either borrowTokenAAmt or borrowTokenBAmt will equate the input _amt fractionalized by the vaultConfig.targetLeverage. Additionally, the current vaultConfig.targetLeverage is utilized in calculations within parenthesis (()) that cause it to incur significant precision loss.

Impact:

The current calculations cause a significant precision loss that will affect how the vaultConfig.targetLeverage is set in effect within the TraderJoeYieldFarmVault contract.

Example:

Copy
283function _deposit(uint256 _amt, uint256 _depositValue) internal returns (uint256) {
284  uint256 equityBefore = reader.equityValue();
285
286  require(_amt > 0, "Amt must be > 0");
287  require(equityBefore + _depositValue <= capacityValue, "Exceeded capacity");
288
289  uint256 borrowTokenAAmt = _depositValue
290    * ((vaultConfig.targetLeverage - (1 * SAFE_MULTIPLIER)) / SAFE_MULTIPLIER )
291    * vaultConfig.tokenADebtRatio
292    / priceOracle.consult(address(tokenA))
293    / reader.to18ConversionFactor(address(tokenA));
294
295  uint256 borrowTokenBAmt = _depositValue
296    * ((vaultConfig.targetLeverage - (1 * SAFE_MULTIPLIER)) / SAFE_MULTIPLIER )
297    * vaultConfig.tokenBDebtRatio
298    / priceOracle.consult(address(tokenB))
299    / reader.to18ConversionFactor(address(tokenB));

Recommendation:

We advise the TraderJoeYieldFarmVault::_deposit call to accept an additional argument specifying which token (A or B) is the one being converted for the borrow mechanism, significantly optimizing the code's legibility as well as gas cost. Additionally, we advise the multiplication to be performed with the _depositValue prior to the division with SAFE_MULTIPLIER, ensuring the maximum precision possible for the leverage calculations.

Alleviation (4325253d6de0ea91c1e9fb9e01d2e7e98f3d83a9):

The calculations were significantly cleaned-up by introducing a new ChainLinkOracle::consultIn18Decimals function that normalizes the price to the desired 1e18 accuracy and making use of safer orders of operations, increasing the legibility and accuracy of the calculations significantly. The optimization portion of this exhibit, however, has not been applied; regardless, the vulnerability outlined has been alleviated in full rendering it addressed.