Adding assymetric liquidity in _addLiquidity results in fewer LP tokens minted than what should be wanted

Handle tensors Vulnerability details Impact Because the call in addLiquidity forwards the entire balances of the 3 stablecoins without checking the ratio between the 3, less liquidity is minted than what should be wanted. Furthermore, an attacker can abuse this arbitrage the forwarded balances if...

_addLiquidity will lose user funds due to frontrunning.

Handle tensors Vulnerability details Impact If addLiquidity is ever called with funds at stake anything more than a few thousand dollars it becomes profitable for MEV bots and other frontrunners to frontrun the addLiquidity call by skewing the pool reserves lowering the amount of LP tokens return...

Carefully add tokens to the list that the protocol uses

Handle tensors Vulnerability details Impact As of right now I believe the only outside tokens the protocol uses are DAI, USDC, USDT and WETH. If other tokens are added, make sure to check that they have no callbacks on transfer. For example, CREAM protocol added the AMP token which has a callback...

Anyone can call harvestNextStrategy with a very low amount of _estimated tokens

Handle tensors Vulnerability details Impact The amounts estimatedWETH and estimatedYAXIS are lower bounds that the protocol expects to recieve. An attacker can call havervestNextStrategyvault, 1, 1 after manipulating the pools called in harvest and swap. The protocol sees nothing wrong with only...

Use mutex lock on VaultHelper.sol

Handle tensors Vulnerability details Impact I strongly recommend adding a nonreentrant modifier on the functions within VaultHelper.sol The contract makes a bunch of unsafe external calls to the user submitted addresses vault and gauge. Also, add some checks to make sure vault and gauge are...

missing access control in basket.sol

Handle jah Vulnerability details Impact function mint and function minTo are not protected so anyone can mint Proof of Concept Tools Used manual analysis Recommended Mitigation Steps use modifier to check who can call the function --- The text was updated successfully, but these errors were...

Certain view functions should never be used in code, only UI. They are easily manipulated.

Handle tensors Vulnerability details Impact The view functions in StablesConverter.sol can be manipulated to give incorrect answers by flashloan attacks. Using them within the code in a naive way can lead to lost funds. Example Recommendations Make sure the functions are only used as estimates fo...

_harvest and _swap

Handle tensors Vulnerability details Impact The minimum amount out on the implemented harvest and swap methods means that attackers can manipulate the price with flashloans/frontrun before calling harvest to actually force the output to be small, pocketing the difference for themselves when they...

Halting the protocol should be onlyGovernance and not onlyStrategist

Handle 0xRajeev Vulnerability details Impact A malicious strategist can halt the entire protocol and force a permanent shutdown once they observe that governance is trying to set a new strategist and they do not agree with that decision. They may use the 7 day window to halt the protocol. The...

There is no corresponding setResume() for setHalted()

Handle 0xRajeev Vulnerability details Impact There is no function for setting halted to false, i.e. to resume the halted protocol, unlike pause/unpause. It appears that halted is actually permanent shutdown here. If so, this should be documented clearly. If not, setHalted should take a boolean to...

Vault.withdraw can be unfair

Handle 0xsanson Vulnerability details Impact In the Vault.withdraw function an user burns shares quantity of VaultTokens to get amount of outputTokens back from the vault. If the vault doesn't have enough tokens, even after withdrawing from the controller, they receive less tokens than they shoul...

The function addToken does not check if the token was already added

Handle hrkrshnn Vulnerability details addToken does not check if the token was already added The function addToken does not check if the token was already present. function addToken address vault, address token external override notHalted onlyStrategist requireallowedTokenstoken, "!allowedTokens"...

VaultHelper deposits don't work with fee-on transfer tokens

Handle cmichel Vulnerability details There are ERC20 tokens that may make certain customizations to their ERC20 contracts. One type of these tokens is deflationary tokens that charge a certain fee for every transfer or transferFrom. Others are rebasing tokens that increase in value over time like...

Vault.balance() mixes normalized and standard amounts

Handle cmichel Vulnerability details The Vault.balance function uses the balanceOfThis function which scales "normalizes" all balances to 18 decimals. for uint8 i; i tokens.length; i++ address token = tokensi; // everything is padded to 18 decimals balance = balance.addnormalizeDecimalstoken,...

Incorrect access control on Harvester add/remove strategy functions

Handle 0xRajeev Vulnerability details Impact The documentation comments indicate that addStrategy and removeStrategy are gov/strategist only functions which is true for setHarvester and setSlippage but add/remove strategy have the incorrect onlyController modifier instead of onlyStrategist. Proof...

Vault.withdraw sometimes burns too many shares

Handle cmichel Vulnerability details The Vault.withdraw function attempts to withdraw funds from the controller if there are not enough in the vault already. In the case the controller could not withdraw enough, i.e., where diff toWithdraw, the user will receive less output tokens than their fair...

Vault.withdraw mixes normalized and standard amounts

Handle cmichel Vulnerability details The Vault.balance function uses the balanceOfThis function which scales "normalizes" all balances to 18 decimals. for uint8 i; i 0 controller.withdrawoutput, toWithdraw; uint256 after = IERC20output.balanceOfaddressthis; uint256 diff = after.subbalance; if dif...

Controller does not raise an error when there's insufficient liquidity

Handle jonah1005 Vulnerability details Impact When a user tries to withdraw the token from the vault, the vault would withdraw the token from the controller if there's insufficient liquidity in the vault. However, the controller does not raise an error when there's insufficient liquidity in the...

An attacker can steal funds from multi-token vaults

Handle WatchPug Vulnerability details The total balance should NOT be simply added from different tokens' tokenAmounts, considering that the price of tokens may not be the same. function balanceOfThis public view returns uint256 balance address memory tokens = manager.getTokensaddressthis; for...

PostAuctionLauncher can be manipulated by a caller other than the owner

Handle tensors Vulnerability details Impact A comment in the code asks whether it is safe to allow anyone to call PostAuctionLauncher, finalize. In the case that an attacker can get even a few wei of the auction token, it is not safe. Suppose an attacker somehow gets a small amount of the auction...

Missing check for duplicate token in addToken

Handle 0xRajeev Vulnerability details Impact addToken does not check for token being added a duplicate of what was already added. If a duplicate token is added, removeToken only removes the first matching token and the later duplicates still remain. With the vaulttoken deleted, this may lead to...

use of transfer() instead of call() to send eth

Handle JMukesh Vulnerability details Impact Use of transfer might render ETH impossible to withdraw becuase after istanbul hardfork , there is increases in the gas cost of the SLOAD operation and therefore breaks some existing smart contracts.Those contracts will break because their fallback...

Vault.balanceOfThis values all tokens equally

Handle cmichel Vulnerability details The Vault.balanceOfThis function values all tokens equally. They are normalized to 18 decimals and then simply added up: for uint8 i; i tokens.length; i++ address token = tokensi; // adds up different tokens here, treating them as exactly equal value 1.0 A = 1...

Vault may not have enough tokens for withdraw

Handle 0xRajeev Vulnerability details Impact There is an assumption in LegacyController.vault that the vault will have enough tokens0 to cover the balance difference. If not, the user may receive less than amount requested and balance funds get lost/locked unless the vault withdraws from the...

token -> vault mapping can be overwritten

Handle cmichel Vulnerability details One vault can have many tokens, but each token should only be assigned to a single vault. The Manager contract keeps a mapping of tokens to vaults in the vaultstoken = vault map, and a mapping of vault to tokens in tokensvault = token. The addToken function ca...

SushiToken transfers are broken due to wrong delegates accounting on transfers

Handle cmichel Vulnerability details When minting / transferring / burning tokens, the SushiToken.beforeTokenTransfer function is called and supposed to correctly shift the voting power due to the increase/decrease in tokens for the from and two accounts. However, it does not correctly do that, i...

Controller.withdrawAll sets wrong vault balance

Handle cmichel Vulnerability details The Controller.withdrawAll decreases the vault balance by amount, the want token amount that has been withdrawn from the strategy and transferred to the vault. Note that amount gets overwritten in the convert != address0 branch and is a convert token value...

YAxisVotePower.balanceOf can be manipulated

Handle cmichel Vulnerability details The YAxisVotePower.balanceOf contract uses the Uniswap pool reserves to compute a lpStakingYax reward: uint256 yaxReserves,, = yaxisEthUniswapV2Pair.getReserves; int256 lpStakingYax = yaxReserves .mulstakeAmount .divsupply .addrewardsYaxisEth.earnedvoter; The...

Vault: Swaps at parity with swap fee = withdrawal fee

Handle hickuphh3 Vulnerability details Impact The vault treats all assets to be of the same price. Given that one can also deposit and withdraw in the same transaction, this offers users the ability to swap available funds held in the vault at parity, with the withdrawal protection fee 0.1%...

Vault: Withdrawal amount isn't un-normalized

Handle hickuphh3 Vulnerability details Impact In withdraw, the withdrawal amount is the proportion of the normalized amounts of all the tokens in the vault and its strategies. However, this amount isn't un-normalized to the output token's decimals, thus leading to an erroneous token amount being...

convert fails for fee-on-transfer tokens

Handle 0xsanson Vulnerability details Impact The Controller contract can call converter.convert inside earn and withdraw functions, after transferring amount of tokens to the Converter contract. This contract assumes that it has received exactly amount tokens, however this isn't true for...

Last person to withdraw his tokens might not be able to do this, in Crowdsale (edge case)

Handle gpersoon Vulnerability details Impact Suppose a Crowdsale is successful and enough commitments are made before the marketInfo.endTime. Suppose marketStatus.commitmentsTotal == marketInfo.totalTokens -1 // note this is an edge case, but can be constructed by an attacker Then the function...

manager.allowedVaults check missing for add/remove strategy

Handle 0xRajeev Vulnerability details Impact The manager.allowedVaults check is missing for add/remove strategy like how it is used in reorderStrategies. This will allow a strategist to accidentally/maliciously add/remove strategies on unauthorized vaults. Given the critical access control that i...

PostAuctionLauncher's liquidity provision can be exploited

Handle cmichel Vulnerability details The PostAuctionLauncher.finalize function takes the raised payment token amounts and uses previously provided auction token amounts to provide liquidity to a Sushiswap pool after an auction has successfully been finalized. It provides this liquidity at a...

Controller.inCaseStrategyGetStuck does not update balance

Handle cmichel Vulnerability details The Controller.inCaseStrategyGetStuck withdraws from a strategy but does not call updateBalancevault, strategy afterwards. Impact The vaultDetailsvault.balancesstrategy variable does not correctly track the actual strategy balance anymore. I'm not sure what...

Issue in balance update in setCap

Handle 0xsanson Vulnerability details Impact During Controller.setCap we change vaultDetailsvault.balance to vaultDetailsvault.balance.subbalance. This is wrong, and the correct value should be vaultDetailsvault.balance.subdiff, because diff is the value withdrawn from the strategy. High risk...

Harvest can be frontrun

Handle 0xsanson Vulnerability details Impact In the NativeStrategyCurve3Crv.harvest there are two instances that a bad actor could use to frontrun the harvest. First, when we are swapping WETH to a stablecoin by calling swapTokensweth, stableCoin, remainingWeth, 1 the function isn't checking the...

Controller.setCap sets wrong vault balance

Handle cmichel Vulnerability details The Controller.setCap function sets a cap for a strategy and withdraws any excess amounts diff. The vault balance is decreased by the entire strategy balance instead of by this diff: // @audit why not sub diff? vaultDetailsvault.balance =...

Vault: Withdrawals can be frontrun to cause users to burn tokens without receiving funds in return

Handle hickuphh3 Vulnerability details Impact Let us assume either of the following cases: 1. The vault / protocol is to be winded down or migrated, where either the protocol is halted and withdrawAll has been called on all active strategies to transfer funds into the vault. 2. There are 0...

ERC20 return values not checked

Handle cmichel Vulnerability details The ERC20.transfer and ERC20.transferFrom functions return a boolean value indicating success. This parameter needs to be checked for success. Some tokens do not revert if the transfer failed but return false instead. The Manager.recoverToken function does not...

Vault does not normalize decimal on withdrawing

Handle jonah1005 Vulnerability details Impact The vault does not normalize decimals when a user withdraws the token. When a user has 100e18 shares, he can withdraw all usdc/ usdt from the token. The liquidity of USDC/USDC would be drained. I consider this is a high-risk issue. Proof of Concept...

wrong YAXIS estimates

Handle cmichel Vulnerability details The Harvester.getEstimates contract tries to estimate a YAXIS amount but uses the wrong path and/or amount. It currently uses a WETH input amount to compute a YAXIS - WETH trade. address memory path; path0 = IStrategystrategy.want; path1 =...

Controller.withdraw(...) User may lose funds when withdraw wantToken from the underlying contract

Handle WatchPug Vulnerability details The wantToken of the strategy may be different from the token argument of Controller.withdrawaddress token, uint256 amount according to code at line 469-474 of Controller.sol. if want != token address converter = vaultDetailsmsg.sender.converter;...

finalize is susceptible to front-running leading to DoS and contract redeployment

Handle 0xRajeev Vulnerability details Impact PostAuctionLauncher finalize has removed the requirement of admin-only finalize as noted in the function comment and lets anyone call it. This makes it susceptible to front-running by anyone when tokens token1 or token2 are yet to be added to pool. The...

User may receive less than the eligible amount per the shares being withdrawn

Handle 0xRajeev Vulnerability details Impact User may receive less than the eligible amount per the shares being withdrawn. It is not clear under what conditions this happens but needs to be documented and user warned. Proof of Concept Tools Used Manual Analysis Recommended Mitigation Steps...

PostAuctionLauncher.sol#finalize() Adding liquidity to an existing pool may allows the attacker to steal most of the tokens

Handle WatchPug Vulnerability details PostAuctionLauncher.finalize can be called by anyone, and it sends tokens directly to the pair pool to mint liquidity, even when the pair pool exists. An attacker may control the LP price by creating the pool and then call finalize to mint LP token with unfai...

Reordering of strategies on Controller does not reorder strategies in Harvester

Handle itsmeSTYJ Vulnerability details Impact The harvester might harvest the wrong strategy because the array of strategies that it stored in its storage is not in the same order as the one stored in the controller. Recommended Mitigation Steps Add a similar reorder strategy function in...

harvestNextStrategy never executes because lastCalled is 0 and stays 0

Handle gpersoon Vulnerability details Impact The function harvestNextStrategy of Harvester.sol checks canHarvest to make sure it can harvest. Initially strategy.lastCalled will be 0 so canHarvest will return false. Thus the require in harvestNextStrategy fails And it never reaches the point where...

Deposits into vaults are not tracked at a token level

Handle itsmeSTYJ Vulnerability details Impact If the strategy used accepts multiple tokens, a user can deposit in a cheaper token and withdraw in a more expensive token because the vault only tracks ownership based on how many shares they own. Proof of Concept 1. An approved strategist adds 2...

Possibility to Stake Twice

Handle verifyfirst Vulnerability details Impact Potential for some users to double-stake their Yaxis. Proof of Concept // SPDX-License-Identifier: MIT pragma solidity 0.6.12; import "@openzeppelin/contracts/token/ERC20/ERC20.sol"; import "@openzeppelin/contracts/token/ERC20/SafeERC20.sol"; // yAx...