YieldFlow - Audit 2

CertiK12 Apr 2024Files named in the report

Audit

CertiK reviewed YieldFlow (YieldFlow - Audit 2) on 12 Apr 2024: 63 findings, 3 critical; 61 marked fixed, 2 open.

3 critical findings, fixed

Open the report Open YieldFlow’s security record

Report hosted on skynet.certik.com.

Trustblock does not review code.

Findings63

Statuses are the security firm’s own, from its report.

Severity	Open	Fixed
Critical	0	3
High	1	3
Medium	0	10
Low	1	45

CriticalIssues If NFT's Are Minted Or Burned Between DistFixed

If new NFTs are minted and sold after fees are distributed, but before they are claimed, an exploiter could purchase a new NFT and claim fees that another user was entitled to. There are also issues if an NFT is burned before fees are claimed, as any unclaimed fees may be locked in the contract.

CriticalNFTs Can Be Transferred To Repeatedly Claim FeesFixed

Fees are distributed according to the NFT balance of the holder, however, the logic does not account for the possible transfer of NFTs. As such a user could claim the fees for their NFTs, then transfer all the NFTs to another address and claim with address. They could continually transfer the NFTs to new addresses and continue claiming fees until all fees are drained from the contract.

CriticalUsers Can Claim The Most Expensive Reward Token UFixed

In the function `_distributeFees()`, the input `_amount` of the token corresponding to the input `_tokenAddress` should be distributed amongst the NFT holders. However, fees are only tracked per `tokenId` and do not also track the corresponding token. This allows for users to claim the most expensive reward token, even if their fees were from a less expensive token.

High`updateUserCakeBalance()` Uses Wrong Value When UFixed

In the function `updateUserCakeBalance()` it has the following logic: ```solidity ownerUserElement.cakeTokenBalance += sPerfFeeCake; ``` Which increases the owners `cakeTokenBalance` by the sponsors performance fee. However, it should be increased by `perfFeeCake`, which is the performance fee for the owner. If the sponsors performance fee is more than half of the performance fee, then this can cause the contract to not have enough cake tokens for the balances of all accounts, which will cause some users to lose some of the cake tokens they are entitled to. If the sponsors performance fee is less than half of the performance fee, then there will be excess cake tokens that will be locked in the contract.

HighIssues With Unclaimed Fees If NFT Is ChangedFixed

If there are fees that have not been claimed and then `changeNFT()` is called to change to a new `nft` contract, then any unclaimed fees will be lost and can then be claimed by holders of the new `nft`.

HighPotential Sandwich AttackFixed

In the function `swap()` it takes an input `slippagePercent`. It then uses the `uniswapQuoter` to determine the expected amount out and calculates the `amountOutMinimum` using the input `slippagePercent`. The `uniswapQuoter` does this by determining the value of the swap in that transaction. As there is not actions that would affect the pool between the quote and the swap, this will mean that the swap will always yield the `amountOutQuote` provided by the `uniswapQuoter`. Thus it will always pass the slippage check. If users utilize this function to make swaps, then their swaps will be vulnerable to sandwich attacks. The function `moveRange()` uses the `swap()` function so that the protocols fees are also potentially vulnerable to this sandwich attack. However, the input `amount0Min` and `amount1Min` can help mitigate against this as these values are used to check the slippage when the new liquidity position is minted. As such, if these values are carefully calculated, they can revert a transaction if there is a significant amount of slippage. In addition, when increasing or decreasing liquidity, there is a risk of sandwich attack. However, there are minimum values in these cases to protect against slippage, which should be carefully chosen.

MediumIncorrect InitializationFixed

The sited lines set initialize values to their initial state, in particular they should be set to the input values which use a trailing underscore as opposed to leading underscore. Many of these are immutable, so they can not be changed after deployment and thus would require redeploying a new contract with the corrected code.

MediumIssue If `positionManager` Is ChangedFixed

If the `positionManager` is changed, it may have a new factory and thus a new pool. However, `uniswapV3Pool` is not updated in the function `changePositionParameter`. Moreover, if the initial position NFT is minted, then changing the position manager will cause any functionality interacting with the liquidity to revert.

MediumIssue If `RangePositionManager` Was Supported AndFixed

If a user provides liquidity to a `rangePositionManager` and afterwards `updateRangePositionManagers()` is called before the user removes their liquidity and the `rangePositionManager` is not included and no longer supported, then the user will be unable to remove their liquidity from the `rangePositionManager`. Similarly if the user has uncollected fees, they will not be able to collect them.

MediumIssues If `masterChef` Is Updated After Initial PFixed

If the `masterChef` contract is changed after the initial position NFT is minted, then it will cause any function calls that interact with the liquidity to revert.

Show 10 more

MediumLack of V3 Pool Validation May Lead To RevertFixed

There is no validation that the `uniswapV3Pool` exists under the current `fee`. The contract may get `address(0)` and cause any operation that calls the pool to revert.

MediumOwner Could Remove Users TokensFixed

There are not checks that the input `_token` is `_WETH` if the `isETHNative` bool is true. This allows for the owner to withdraw tokens that are accounted for. Furthermore, note that this function may not be able to recover `WETH` from the contract, because if `withdrawExcessTokens(WETH, owner, false)` is called, then `contractBalance` will be the contracts balance of `WETH`. However, `accountedBalance` will be the accounted balance of native ETH, so that the `excessBalance` may not be correct.

MediumOwner Liquidity Only Added On First Call To `mintFixed

The function `mintOwner()` is only designed to be called once to mint the initial position nft. However, this is enforced by checking that `totalLiquidity` is zero. If it is called and then all liquidity is removed later, then it may be called again. In this case `mintOwner` can successfully be called again, but the owner will still be in `isUser` array, so they will not have their `userElement.liquidity` updated. This will cause them to be unable to access the liquidity they added.

MediumPotential To Run Out Of Gas And Cause Denial Of SFixed

If the amount of users is high enough, then this can cause the function to exceed the block gas limit. A user with a large amount of NFTs could distribute them amongst many accounts to increase the list to execute a denial of service attack. We recommend allowing this logic to be executed over multiple transactions or setting an upper limit on the amount of users to ensure that this will not run out of gas. Currently the max amount of users as 10000, however, this is not a low enough maximum. During our testing using a mocked `yieldManager` (so the gas estimate here is likely lower than the actual value) an iteration of the for-loop in collect cost at minimum `16810` and at maximum `19305`. Iterating this 10000 times this equates to a minimum of 168,100,000 which exceeds the 30 million block gas limit.

MediumTransfer Of NFT Can Be Used To Deny ServiceFixed

The function `increaseLiquidity()` makes the following check: ```solidity require(showEligible(userToIncrease), "Not eligible to enter the product"); ``` Which checks if the `NFT` balance of the `userToIncrease` is greater than or equal to the `productLock`. As such the design intent is to only allow users to participate when they hold a certain amount of NFTs. However, users can transfer their NFTs allowing other users to also participate in the protocol using the same NFT. A malicious could use `10000` different accounts and add liquidity with all of them. As there is a max amount of `10000` users, this would prevent any other users from entering the protocol.

MediumUser Never Removed From ListFixed

If a user withdraws all their liquidity, they are still included in the list of users. This contributes to the list growing large enough to hit the maximum or so that function calls will revert due to them exceeding the block limit.

Low`approve` return value not checkedFixed

Not all `IERC20` implementations `revert` when there's a failure in `approve`. The function signature has a `boolean` return value and they indicate errors that way instead. By not checking the return value, operations that should have been marked as failed may potentially go through without actually approving anything. Considering the `WETH` contract address can be changed, it may be changed to an address that does not revert on failure.

Low`for` Loop OptimizationsFixed

The following operations can be made in all for loops of the codebase: 1. The increment operation saves a small amount of gas if performed from the left (`++i`) instead of from the righthand side (`i++`). 2. The increment operation can be placed in an `unchecked` block at the end of the loop logic, since it is bounded above by the loop limit which must be a `uint256` value, avoiding overflow.

Low`getSqrtRatioAtTick()` Does Not Check That `absTiFixed

The function `getSqrtRatioAtTick()` does not check that the `absTick` is less than or equal to the `MAX_TICK`. As such this may return a value that is not between `MIN_SQRT_RATIO` and `MAX_SQRT_RATIO`.

Low`safeApprove()` Is DeprecatedFixed

The function `safeApprove()` is deprecated and in some cases the entire approval may not be used, so that there will be a nonzero approval left for the contract.

Show 10 more

LowAmbiguous EventFixed

Recovering `WETH` using `recoverERC20()` would emit `Recovered(WETH, amount, to)`, however recovering `ETH` using `recoverETH()` would also emit `Recovered(WETH, amount, to)`.

LowAvoid emitting events within loopsFixed

Emitting an event costs at least 375 base gas plus 375 gas for each indexed parameter. Instead of emitting one event for each iteration of a loop, emit one event after the loop containing all of the relevant information instead. See [***HERE***](https://www.rareskills.io/post/ethereum-events#:~:text=Gas%20cost%20to%20emit%20Solidity%20event&text=Each%20event%20costs%20at%20least,words%20written%20to%20the%20chain.) for more information about the gas cost of events.

LowCan Use Input To Save Storage ReadFixed

In the lines sited above, the event is emitted using a storage variable. However, the storage variable is set to the input value prior to emitting the event. As such, the input can be used in the emitted event to save reading from storage.

LowCan Use Single Division OperationFixed

In the lines above it divides by 100 and then divides this result by 100. Instead a single division operation by 10000 can be done instead.

LowConsider Using Custom ErrorsFixed

Throughout the codebase, `require` statements returning sting error messages are used. From Solidity `v0.8.4`, there are more gas-efficient ways to explain to users why an operation failed than through strings. Using custom errors can significantly reduce the size of the deployed bytecode and reduce the gas cost when calls revert.

LowDiscussion On `RewardNFT`Fixed

Users NFTs are marked as in use up to the `_requiredAmount` according to their index as opposed to their `nftId`. This can cause issues if there will be NFTs that are worth more than others. In particular, if a user has a high value NFT, they may wish to not have it be marked as in use as opposed to a low value NFT. If the high value NFT was the first one they purchased, then it will be at index zero and always be marked as in use provided there is a nonzero `_requiredAmount`. A user must then transfer the high value token in order to have it be reindexed to a higher index so that it would not be marked in use.

LowDiscussion On Sending/Refunding Of Tokens In `RanFixed

The function `collect()` in the `RangeMaster` contract calls `userCollect()` in the `rangePositionManager` for the `msg.sender`. The `rangePositionManager` contract will then send those tokens directly to the `msg.sender` if they are not the owner. If they are the `owner` then the appropriate amount is sent to the `feeReceiver` and the rest is sent to the `feeDistributor` to be distributed. As such, no tokens are sent to the `RangeMaster` contract, however, there is logic to then send the contracts balance of tokens to the `msg.sender`. Similarly, the function `decreaseLiquidity()` calls `rangePositionAddress.decreaseLiquidityUser()` which will refund directly to the `msg.sender`. For the function `increaseLiquidity()`, excess ETHER may have been sent as it is payable so there may be ether to refund. However, `rangePositionAddress.increaseLiquidityUser` refunds to the input `userToIncrease`, which is the `msg.sender.

LowDiscussion On WeightFixed

Can you please clarify the design intent behind `weights`. In particular, the weights are checked to sum up to `FACTOR` and then used in `increaseLiquidity()` to determine the percentage of the `weightAmount` of `weightToken` that should be set as the desired amount for each pool. One possible intent behind this is to ensure that when a user adds liquidity it is spread the liquidity in the given percentages (according to the `weightToken`) across all the supported pools. However, if this is the case, then it can be circumvented as the logic: ```solidity if (amountDesired0[i] == 0 && amountDesired1[i] == 0) { continue; } ``` Allows users to not add liquidity to a position. In particular, a user could have `amountDesired0[i] == 0 && amountDesired1[i] == 0` for all positions except one, and then choose the `weightAmount` so that the add the exact amount of liquidity they want to a single position. Furthermore, if a position is skipped the `weightAmount` must still account for the skipped position.

LowDuplicate VariableFixed

In the function `getPendingFees()`, there are two variables set for `rangePositionManagers.length`. ```solidity uint256 len = rangePositionManagers.length; uint rangeLength = rangePositionManagers.length; ```

LowEvents Do Not Emit Previous StateFixed

In the events cited above, that are emitted when state variables are changed, do not emit the previous value.

Show 10 more

LowFeeDistributor Not Compatible With All ERC721 TokFixed

The `FeeDistributor` makes the following call `nft.totalSupply()` so that it cannot support all ERC721 tokens. This is because the ERC721 standard does not require the implementation of a `totalSupply()` function. However, it will support tokens that follow the enumeration extension (see [here](https://eips.ethereum.org/EIPS/eip-721#:~:text=The%20enumeration%20extension%20is%20OPTIONAL%20for%20ERC%2D721%20smart%20contracts%20(see%20%E2%80%9Ccaveats%E2%80%9D%2C%20below).%20This%20allows%20your%20contract%20to%20publish%20its%20full%20list%20of%20NFTs%20and%20make%20them%20discoverable.))

LowFunction Can Be Modified With `pure` Or `view`Fixed

The function `onERC721Received()` does not require modifying the contract state, nor does it receive Ether. However, this function is not labeled with the `pure` or `view` modifier.

LowIncompatibility with Fee On Transfer TokensFixed

When transferring fee on transfer ERC20 tokens, the input amount may not be equal to the received amount due to the charged transaction fee. For example, if a user sends 100 deflationary tokens (with a 10% transaction fee), only 90 tokens actually arrived to the contract. However, a failure to discount such fees may allow the same user to withdraw 100 tokens from the contract, which causes the contract to lose 10 tokens in such a transaction. Reference: https://thoreum-finance.medium.com/what-exploit-happened-today-for-gocerberus-and-garuda-also-for-lokum-ybear-piggy-caramelswap-3943ee23a39f Throughout the codebase bookkeeping is performed assuming that the amount transferred will be the same as the amount received.

LowIncorrect Zero Address ValidationFixed

In the `constructor()` there is the following check: ```solidity require(_token0 != address(0) || _token1 != address(0), "token cant be 0 address"); ``` Which will pass if one of the input `_token0` or `_token1` is not the zero address. However, it will not revert if `_token0` is the zero address while `_token1` is a nonzero address or if `_token0` is a nonzero address and `token1` is the zero address.

LowInefficient Memory ParameterFixed

One or more parameters with `memory` data location are never modified in their functions and those functions are never called internally within the contract. Thus, their data location can be changed to `calldata` to avoid the gas consumption copying from calldata to memory. `setBaseURI` has memory location parameters: `baseURI`.

LowInefficient SwapFixed

The contract will swap to all of one token type and then use the `amountToSwap` to swap it back. Thus one may pay a swap fee on tokens unnecessarily.

LowLack Of Indexed Event ParametersFixed

Throughout the codebase events do not index their parameters.

LowMissing Emit EventFixed

Functions that update state variables should emit relevant events as notifications.

LowMissing Input ValidationFixed

### FeeDistributor - In the function `claimFees()`, the input `_tokenAddress` is not checked to be a registered token. - In the function `getTotalAndFeesPerNFT()`, the input `_tokenAddress` is not checked to be a registered token. - In the function `getClaimableFees()`, the input `_tokenAddress` is not checked to be a registered token. ### RangeMaster - In the `constructor()`, the input `_rangeWeights[i]` are not checked to be nonzero for all `i`. - In the function `updateRangePositionManagers()`, the input `_rangeWeights[i]` are not checked to be nonzero for all `i`. ### RangePositionManager - In the `constructor()`, the input `_distributionFee()` is not checked to be less than `10000`. We recommend determining the maximum distribution fee that would ever be charge and checking the input is less than this value. - In the function `moveRange()`, the input `amountToSwap` is not checked to be nonzero. Alternatively, logic can be added to handle when the amount is zero. - In the function `mintOwner()`, if `token0` or `token1` is WETH, it is not checked that the `msg.value` is equal to `amountDesired0` or `amountDesired1` respectively. ### SwapManager - In the function `swap()`, it is missing a check that if the `msg.value` is nonzero that it is equal to the input `amountIn ### RewardNFT - The function `batchMint()` does not check that the input amount is nonzero. This allows the function to be called successfully, when no tokens are minted.

LowMissing Zero Address ValidationFixed

### FeeDistributor - `constructor()` is missing a check that the input `_nftAddress` is not `address(0)`. - `receiveERC20Fees()` is missing a check that the input `_tokenAddress` is not `address(0)` as the contract is designed to distribute Ether through the `receive()` function. ### RangeMaster - `constructor()` is missing a check that the input `_rangePositionManagers[i]` is not `address(0)` for each `i`. - `updateRangePositionManagers()` is missing a check that the input `_rangePositionManagers[i]` is not `address(0)` for each `i`. ### RangePositionManager - `constructor()` is missing a check that the input `_positionManager` is not `address(0)`. - `constructor()` is missing a check that the input `_swapManager` is not `address(0)`. - `increaseLiquidityAuto()` is missing a check that the input `userToIncrease` is not `address(0)`. - `setMoveRangeAddress()` is missing a check that the input `moveAddress` is not `address(0)`. - `setMasterChef()` is missing a check that the input `newMasterChef` is not `address(0)`. - `setFeeDistributorSwapper()` is missing a check that the inputs `newFeeDistributor` and `newSwapManager` are not `address(0)`.

Show 10 more

LowNo Error MessageFixed

The check `require(liquidity <= userElement.liquidity);` does not provide a string error message, which can cause difficulties determining why an operation reverted.

LowNon-External Functions And Variables Do Not FolloFixed

The sited variables and functions do not conform to the solidity style guide (see [here](https://docs.soliditylang.org/en/latest/style-guide.html#underscore-prefix-for-non-external-functions-and-variables)). The style guide recommends that all non-external variable and function names have an underscore prefix.

LowOrder Of Functions Does Not Follow Style GuideFixed

The contracts/library sited above do not follow the solidity style guide (see [here](https://docs.soliditylang.org/en/latest/style-guide.html#order-of-functions)) for the order of functions. According to the style guide, the order of functions should be: - constructor - receive function (if exists) - fallback function (if exists) - external - public - internal - private Within a grouping, place the view and pure functions last.

LowOrder Of Layout Does Not Follow Style GuideFixed

The order of layout of the contracts sited above do not follow the solidity style guide (see [here](https://docs.soliditylang.org/en/latest/style-guide.html#order-of-layout)). According to the style guide inside each contract the following order should be used: - Type declarations - State variables - Events - Errors - Modifiers - Functions

LowPosition Managers Should Never Change Their `_reqFixed

If the `_requiredAmount` is changed to be a larger value for a `rangePositionManager` and a user had already had their NFTs marked as in use, then the NFTs that were already marked as in use will have their `countNFTUsing` incremented again and be pushed a second time to the `userRangePositionManagerNFTs` array unnecessarily. Currently the `_requiredAmount` is given by the immutable `_productLock` in `RangePositionManager`, so that it will not be changed.

LowPotential Locked Tokens And EtherFixed

The function `distributeFees()` distributes an `_amount` of ETH or ERC20 token that was received using the following logic: ```solidity feeInfo[_tokenAddress].feesPerNFT += _amount / totalNFTs; feeInfo[_tokenAddress].totalFees += _amount; ``` However, the `feesPerNFT` is rounded down so that `feesPerNFT * totalNFTs` may be less than the provided `_amount`. The tokens are distributed according to the `feesPerNFT`, so the difference will not be distributed and locked in the contract. --- Similarly the function `collect()` distributes an `amount0` and `amount1` of ETH or ERC20 token using the following logic: ```solidity uint share0 = amount0 * userElement.liquidity / totalLiquidity; uint share1 = amount1 * userElement.liquidity / totalLiquidity; ``` However, these share values are rounded down so that they may add up to less than the collected amount. The difference will be locked in the contract. Moreover, depending on the frequency of this call and the token decimals users with a small amount of liquidity may receive 0 shares of the collected fees. It should be carefully checked that there is enough precision so that during the normal operations of the protocol users with a small amount of liquidity still receive their fair share of the fees. Similarly for the performance fees, which are taken from the calculated shares.

LowPotential ReentrancyFixed

Reentrancy can occur when the contract creates a function that makes an external call to another untrusted contract before resolving any effects. If an attacker can control the untrusted contract, they can make a recursive call back to the contract, repeating interactions that would have otherwise not run after the external call resolved the effects. If a token using hooks is used, then when the token is transferred the hook may be used to reenter the contract. Note that this is given an *informational* severity as in the current codebase this only emits events out of order.

LowPotential Swept Or Unwrapped Tokens Not AccountedFixed

In the function `collect()`, it makes the following calls: ```solidity positionManager.unwrapWETH9(0, address(this)); IWETH9(WETH).withdraw(IERC20(WETH).balanceOf(address(this))); positionManager.sweepToken(token0, 0, address(this)); positionManager.sweepToken(token1, 0, address(this)); ``` These will sweep any WETH, token0, or token1 tokens in the `positionManager` contract, and will convert any WETH held by the contract to ETH. However, only the output `amount0` and `amount1` from the `positionManager.collect()` are handled in the logic. If there are any tokens accidentally sent to the `positionManager` then more tokens may be received and locked in the contract.

LowProtocol Only Supports Native ETH And Does Not SuFixed

The contracts assume that if `WETH` is used as an input, that the user will be supplying native ETH and not WETH. As such the protocol does not support users utilizing WETH.

LowRangePositionManager Can Receive Any ERC721Fixed

`RangePositionManger` includes an `onERC721Received()` function so that it can receive ERC721 tokens using the `safeTransferFrom()` functionality. However, this allows it to accept any ERC721 tokens, when it is only designed to handle those from the `positionManager`. If another ERC721 token is accidentally sent to the contract it will be locked in the contract.

Show 10 more

LowRedundant And Unused Struct ElementsFixed

In the struct `RangePosition`, the elements `added` and `updated` are redundant as they are both updated to be `block.timestamp`. In the struct `RangePosition`, the element `locked` is only ever set to `false` and is not used in the logic of the contract.

LowSpecific Imports Are Not UsedFixed

Specific imports are recommended to be used to avoid unpredictable pollution of the namespace. For example: import {A, B, C} from "X";

LowSwap Using a Single Fee May Be SuboptimalFixed

Uniswapv3swapper uses hardcoded `fee` to make a swap. This can result in a swap being conducted in a pool with little liquidity and with a high price impact.

LowTypos and InconsistenciesFixed

### RangeMaster - The error message in the `constructor()` for the `yieldManager` references the `owner` as opposed to the `yieldManager`. - The inputs `amountDesired0` and `amountDesired1` in the function `decreaseLiquidity()` do not reflect that they are minimum values. ### SwapManager - The comments for the `constructor()` only states "0 representing Uniswap" and does not state that 1 represents PancakeSwap. - The comment "// Perform the token swap using Uniswap V3 SwapRouter (example code, comment only" does not seem to apply. ### RangePositionManager - The `moveRangeAddresses` mapping also gives access to call `increaseLiquidityAuto`. We recommend changing the name to reflect this additional privilege.

LowUnnecessary `onERC721Received()` FunctionFixed

The `onERC721Received()` function is only called during safe operations, which first checks if it is being sent to a contract. If it is a contract, then it attempts to call `onERC721Received()` of that contract and will revert if this does not return the `onERC721Received.selector`. As such, the ability to receive NFTs through the use of safe operations can be removed by not including the `onERC721Received()` function as opposed to directly disabling it. Note, that NFTs can still be transferred to the contract if the safe operations are not used.

LowUnnecessary ApprovalFixed

In the WETH contract at address [0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2](https://etherscan.io/token/0xc02aaa39b223fe8d0a0e5c4f27ead9083c756cc2#code), the function `withdraw()` uses the balance of the `msg.sender` and does not use any approval. If the WETH contracts that are to be used will always follow this convention, then the approval can be removed.

LowUnnecessary CastingFixed

In the function `setMasterChef()`, `newMasterChef` is cast as an `address`, however, it is already of type `address`.

LowUnnecessary Condition In `_onlyOwner()`Fixed

The function `_onlyOwner()` in the contract `FeeDistributor`, `RangeMaster`, `RangePositionManager`, and `SwapManager` requires that either the `msg.sender` is the `owner` or `address(this)`. However, the `onlyOwner` modifier is used on functions that are never called by the contract itself. As such the `msg.sender` will never be `address(this)`.

LowUnused EventFixed

The event `NewMaxUsers` is never emitted.

LowUse `uint256` Instead of `uint`Fixed

Throughout the codebase `uint` and `uint256` are used interchangeably.

Show 1 more

LowWrong Parameter Emitted In EventFixed

In the function `acceptFeeReceiver()` it emits the event ```solidity emit NewFeeReceiver(owner); ``` However, the input should be the new `feeReceiver` not the `owner`.