DigiVerse Staking - Audit

The logic related to calculating reward differs in function `unstake` and `claimEarned` in the following way: 1. The refer reward is calculated in function `claimEarned` but not in `unstake`. It is possible for the user to not giving his referral any reward by calling `unstake` and setting the amount to 0. In this way, he can still get all of his reward, but the referrer cannot get any token as reward. 2. When a user calls `claimEarned`, `stakes[_stakingId][msg.sender][i].totalClaim` is updated. However, in `unstake` function, when the reward is calculated, `stakes[_stakingId][msg.sender][i].totalClaim` is not updated. 3. `claimEarned` is protected by modifier `checkPools` so that user deposits cannot be claimed as reward, but `unstake` does not have such restrictions, so that it is possible to claim other user's deposits as reward.

In commit https://github.com/DigiverseProject/Blockchain/commit/ef556727f9b28b2210f7d277fc0bdd6e8ac672aa, function `claimEarned` is modified. The modification introduces a new issue: In the for loop starting line 209, `_eAmount` is deducted by the proportion of the requested amount to claim from each staking. At the end of the for loop, `_eAmount` equals 0, making users unable to withdraw rewards from the contract.

In commit https://github.com/DigiverseProject/Blockchain/commit/659ea6759bfccd228391c8d675cc9d9497a370c5, function `claimEarned` is modified. The modification introduces a new issue: The `claimEarned()` function is responsible for withdrawing user rewards. During iteration over the staking array, rewards are claimed proportionally from each staked amount. However, the total of unclaimed rewards (`totalUnclaimed`) does not get updated accordingly, leading to inaccurate calculations of subsequent reward claims.

When a user calls unstake, the contract will iterate through all of the staking records, update them accordingly, and transfer tokens to user. When a staking record will be charged penalty, and the iteration will break. So it is possible that the staking amount deducted from the staking record is smaller than the amount user requested. However, when transfer tokens to user, it does not consider this scenario. This will allow users to withdraw more token than he supposed to.

A user's staking record is saved at `stakes[_stakingId][msg.sender]`. When a user calls `unstake`, it will iterate through all of the record stored in `stakes[_stakingId][msg.sender]` and compare `block.timestamp` to `_staking.endstakeAt`. If `block.timestamp < _staking.endstakeAt`, penalty is applied. When a penalty is detected, it will break the for loop. The issue here is that the array `stakes[_stakingId][msg.sender]` is not sorted by time, as the stake duration could be different for different deposits(see the Scenario section for more detail). This will cause two issues: 1. penalty is falsely applied to user even if there is staking record that has reached the end. 2. User cannot withdraw the amount he supposed to be able to withdraw. There are two cases where the staking record is not sorted by time: - The admin changes the staking duration - When removing staking record, The last staking record is put to the first element, disrupting the sorting.

The abstract contract `IERC20Staking` is named like an interface. However, it contains code logic and variable declaration, which is extremely confusing.

The contract inherits Openzeppelin library Ownable, but did not initialize the owner.

The `checkPools()` modifier in the smart contract is currently used to check the token balance before certain actions (e.g., `unstake`, `claimEarned`, `claimReward`) are performed. It confirms that the contract has enough tokens to cover the staked amount. However, it does not verify if the balance remains sufficient after these actions are completed. This oversight could allow a situation where users are able to claim rewards greater than the available balance, which may include tokens from other users' stakes. ```solidity=343 modifier checkPools() { uint256 totalsPools = IERC20(stakingToken).balanceOf(address(this)); require(totalsPools > totalStaked, "Insufficient balance pools need to refill token into contract"); _; } ```

The user `currentUpline0`/`currentUpline` could have a non-existent referrer, which defaults to the `address(0)`. During the referrer reward calculation process, this leads to referrer rewards being incorrectly attributed to the `address(0)`. ```solidity=93 address currentUpline0 = _referrer; for (uint i = 0; i < refPercent.length; ++i) { user[currentUpline0].totalDownline += 1; currentUpline0 = user[currentUpline0].invitedBy; // Move to next referrer } ``` ```solidity=322 function updateReferralEarnings(uint256 amount) internal { address currentUpline = user[msg.sender].invitedBy; for (uint256 i = 0; i < refPercent.length; ++i) { uint256 bonusInvite = (amount * refPercent[i]) / 100; user[currentUpline].totalEarning += bonusInvite; user[currentUpline].claimableEarning += bonusInvite; currentUpline = user[currentUpline].invitedBy; // Move to next referrer } } ```

The `SafeMath` library is used unnecessarily. With Solidity compiler versions 0.8.0 or newer, arithmetic operations will automatically revert in case of integer overflow or underflow. ```solidity=51 using SafeMath for uint256; ``` - `SafeMath` library is used for `uint256` type in `DigiStake` contract. ```solidity=154 _earned = _earned.add(calculateEarned(withdrawAmount, _staking.lastClaim, plan.apr)); ``` - `SafeMath.add` is called in `unstake` function of `DigiStake` contract. *Note: Only a sample of 2 `SafeMath` library usage in this contract (out of 21) are shown above.*