Seedify finance Locked-Yield-Farming

In the `SMD_v5` contract, certain functions require `isPaused` variable to be false, suggesting these functions may be paused under specific conditions. However, based on the current implementation, the contract will only ever be paused before the initial period is set. The sole function that can toggle `isPaused` to true is `__reset()`, which is called within `setNewPeriod()`. Immediately afterward, `setNewPeriod()` invokes `__setStartEnd()`, which in turn sets `isPaused` back to false. As a result, functions that require `isPaused` to be false will always proceed without interruption. Also, the pause behavior is inconsistent. For example: 1. The `viewOldRewards()` function is a view function that requires `isPaused` to be false. However, the other view functions lack this requirement. 2. The `withdraw()` does not directly require `isPaused` to be false, instead, it relies on `viewOldRewards()` which does. This implementation leads to ambiguity, causing one to question what the intended design it. 3. The `emergencyWithdraw()` function does not require `isPaused` to be false, it is recommended to check if this aligns with the intended design.

The `endAccShare` mapping in the contract captures the details of expired periods that determine the rewards users can claim from these old periods. The `endAccShare` is only updated via the `__reset()` function, which is exclusively triggered by `setNewPeriod()`. This design means that stakers must wait for a new period to start before they can claim rewards from an expired period, introducing potential delays.

The `__withdraw()` function only decreases `stakedBalance` when `isPause` is false. The `stakedBalance` represents the amount of `tokenAddress` staked in the contract for the current period, and it directly impacts the reward calculations. Therefor, It should be updated whether the contract is paused or not. Failing to do so could lead to skewed reward distributions. Specifically, the following scenario highlights the issue: 1. With `isPaused` being false, a user could invoke `emergencyWithdraw()` and reduce the actual staked balance without affecting the recorded `stakedBalance` in the contract. 2. When `isPaused` is set to true and `__updateShare()` function is triggered. When calculating the `accShare` (amount of rewards per wei), the formula uses the potentially inflated `stakedBalance` value, resulting in a smaller `accShare` than should be the case. This miscalculation means users might receive fewer rewards than they should. It's worth mentioning that, in the current design, `isPaused` remains true after the setting of the first period, thus avoiding the described situation. Nevertheless, the logic appears faulty and could lead to unintended consequences if modified in the future.

The `userDeposits()` function allows users to retrieve deposit details of a given address `from`. The `Deposits` struct contains a field name `userAccShare`, which represents the amount of rewards per wei of deposited LP token {tokenAddress} accross all periods. Notably, the `userDeposits()` function does not provide access to view the `userAccShare`, which appears to be an oversight. ```solidity=351 /// @notice get user deposit details function userDeposits( address from ) external view returns (uint256, uint256, uint256, uint256) { if (hasStaked[from]) { return ( deposits[from].amount, deposits[from].initialStake, deposits[from].latestClaim, deposits[from].currentPeriod ); } else { return (0, 0, 0, 0); } } ``` ```solidity=84 struct Deposits { uint256 amount; uint256 initialStake; uint256 latestClaim; uint256 userAccShare; uint256 currentPeriod; } ```

The `Deposits` struct maintains data related to the deposits made by a user. Within this struct, the `initialStake` field is intended to record the timestamp when a user renews their stake for new periods, as described in the comment. However, there seems to be a discrepancy. The `initialStake` gets updated even when a user re-stakes within the current period, diverging from its intended behavior as per the comment. As a result, users could be prevented from withdrawing their stakes, as withdrawals are permitted only after `lockDuration.mul(SECONDS_PER_HOUR)` from the `initialStake`." ```solidity=73 /** * @notice struct which should represent the deposit made by a wallet based on all period if the wallet * called {renew}. * * @param amount amount of LP {tokenAddress} deposited accross all period. * @param initialStake should be the timestamp at which the wallet renewed their stake for new periods. * @param latestClaim latest timestamp at which the wallet claimed their rewards. * @param userAccShare should be the amount of rewards per wei of deposited LP token {tokenAddress} * accross all periods. * @param currentPeriod should be the lastest periodCounter at which the wallet participated. */ struct Deposits { uint256 amount; uint256 initialStake; uint256 latestClaim; uint256 userAccShare; uint256 currentPeriod; } ```

The `recoverLostERC20()` function is designed to enable the `_owner` to retrieve tokens mistakenly sent to the contract. A safeguard mechanism is embedded within the function to ensure that the `_owner` cannot withdraw tokens (`tokenAddress`) that are actively staked by users. It achieves this by subtracting `stakedTotal` from the `amount` before the withdrawal. However, there's a flaw in the logic: the `stakedTotal` represents the total amount of `tokenAddress` staked in the contract over its whole existence. Since it never decreases even when users make withdrawals, it could be larger the actual current staked balance. This flawed design means that the `_owner` might not be able to recover all the excess tokens beyond the current user stakes. ```solidity=677 function recoverLostERC20(address token, address to) external onlyOwner { if (token == address(0)) revert("Token_Zero_Address"); if (to == address(0)) revert("To_Zero_Address"); uint256 amount = IERC20(token).balanceOf(address(this)); // only retrieve lost {rewardTokenAddress} if (token == rewardTokenAddress) amount -= rewardBalance; // only retrieve lost LP tokens if (token == tokenAddress) amount -= stakedTotal; IERC20(token).safeTransfer(to, amount); } ```