Too centralized pending reward info updating

Critical  | Invalid

Only devaddr can call updatePendingInfo() function where reward multiplier is changed. When in some cases this function won’t call or will call with some delays it will lead to outdated info that triggers incorrect reward distribution so the user will be able to lose his accumulated reward.

Recommendation:

Consider fixing the updating reward multiplier in a more trusted and decentralized way. Or add some validation to prevent user deposit or withdrawal before the pending reward info won’t be updated.

Re-Audit:

Explanation from the POP Network team: “We are logging masternode running time on server and update staking contract on the server-side. We are updating these infos every some period and we let users know when we update infos. We know it's too centralized but it's a good choice for now.”

‍

Incorrect logic in emergencyWithdraw() function

Critical  |  Resolved

In the emergencyWithdraw() function user should be able to withdraw the available deposited amount. In the current logic before transferring the user’s funds user.amount value is set to zero in line 119 and only after that appears transfer where the passed amount is variable user.amount which was set to zero before. In this case, the user can’t be able to withdraw his funds and after a call of emergencyWithdraw() function user’s funds will be locked in the contract balance without any chance to withdraw it.

function emergencyWithdraw() public {
    UserInfo storage user = userInfo[msg.sender];
    user.amount = 0;
    user.lastRewardBlock = block.number;
    user.rewardMultiplier = 0;
    pop.transfer(address(msg.sender), user.amount);
    emit EmergencyWithdraw(msg.sender, user.amount);
}

Recommendation:

Consider fix logic of the emergencyWithdraw() function.

‍

Potential loss of accumulated user’s reward

Critical |  Resolved

The reward is transferred to the user when the user deposits and withdraws. To transfer rewards used safePopTransfer() function. When the staking contract won’t have enough pop tokens to cover the reward distribution all available tokens will be transferred to the user which will be less than the actually accumulated reward. There is no tracking of the reward transferring so if the user withdraws staked tokens as it is (with fewer rewards amount) he won’t be able to withdraw the missing amount.

Recommendation:

Consider managing case when there are not enough tokens to withdraw for reward in that case so the user will not loss.

‍

Unchecked transfer return value

Low |  Resolved

Ignored return value of transferFrom and transfer in the functions deposit, withdraw, emergencyWithdraw, safePopTransfer in lines 94, 112, 122, 131, 133.

Recommendation:

Consider validating return value by wrapping transfer/transferFrom in require or using SafeERC20 library.

Re-Audit:

In line 13 there is defined library usage using SafeERC20Upgradeable for IERC20; but  functionality from SafeERC20Upgradeable library is not used in the codebase. For transfer tokens used simple transfer/transferFrom but not safeTransfer/safeTransferFrom.

Re-Audit 2:

Fixed.

‍

Lack of validation in deposit() function

Low  |  Resolved

There is no check if the given param _amount is available for the deposit as the available amount for staking starts from 50K POP tokens.

Recommendation:

Consider adding a check for the local variable “amount” to check if the depositing amount isn’t zero.

Re-Audit:

Comment from POP Network team: “We use deposit function for claim rewards. So if we input amount as zero, then it only claims rewards.”

We may suggest you add separate function for claiming rewards, so user can spend less gas for calling claiming transaction.

Re-Audit 2:

Fixed.

‍

No check for zero value in claimable rewards

Low  |  Resolved

In line 94 of function deposit() and in line 109 of function withdraw() there is no check if claimable user rewards are greater than 0.

Recommendation:

Consider adding a check for claimable amounts to not perform token transfer if the amount of rewards is equal to 0.

‍

Floating pragma

Low  |  Resolved

The current version of solc is ^0.6.12 and it is better to lock the pragma to a specific version.

Contracts should be deployed with the same compiler version and flags that they have been tested with thoroughly. Locking the pragma helps to ensure that contracts do not accidentally get deployed using, for example, an outdated compiler version that might introduce bugs that affect the contract system negatively.

Recommendation:

Lock pragma to a specific version.

‍

Unspecified state variable visibility

Low  |  Resolved

It is best practice to set the visibility of state variables explicitly. The default visibility for cycleLen in line 28 is internal.

Recommendation:

Explicitly specify the visibility of state variables.

‍

Lack of zero address check

Low  |  Resolved

In functions initialize() and dev() there is no check if the _devaddr address isn't a zero address.

Recommendation:

Consider adding a check for zero address for functions initialize() and dev().

‍

Functions visibility optimization

Low  |  Resolved

Functions getMultiplier(), deposit(), withdraw(), emergencyWithdraw(), updatePendingInfo() and dev() can be marked as external as they are not used in the contract.

Recommendation:

Consider changing visibility from public to external to safe gas usage on calling these functions.

‍

Function state visibility optimization

Low  |  Resolved

Functions getMultiplier() state visibility can be restricted to pure.

Recommendation:

Consider changing state visibility in getMultiplier() from view to pure.

‍

Lack of validation in withdraw() function

Low  |  Resolved

There is no check if the given param _amount isn’t zero.

Recommendation:

Consider adding a check for zero value.

‍

Lack of validation in emergencyWithdraw() function

Low  |  Resolved

There is no check if the user has any funds to withdraw.

Recommendation:

Consider adding a check for zero value.

‍

Lack of validation in UpdatePopPerBlock() function

Low  |  Resolved

There is no check if the input param _popPerBlock isn’t zero.

Recommendation:

Consider adding a check for zero value.

‍

Division by zero in updatePendingInfo() funciton

Low  |  Resolved

In line 141 getPopPerBlock() function can return 0.

Recommendation:

Consider managing case when function getPopPerBlock() return zero while calculating value of divider.

‍

Lack of NatSpec annotations

Informational  |  Resolved

Smart contract Stakel is not covered by NatSpec annotations.

Recommendation:

Consider covering by NatSpec all contract methods.

Re-Audit :

Defined only @notice section but @param and @return sections are not disclosed.

Re-Audit 2:

Fixed.

‍

Commented import statement

Informational  |  Resolved

In line 5 in the codebase there commented import statement for SafeERC20.sol, but in line 6 there used SafeERC20Upgradeable.sol.

Recommendation:

Remove redundant import statement.

‍

Naming style

Informational  |  Resolved

Constant stakeUnit is not in UPPER_CASE_WITH_UNDERSCORES .

Recommendation:

Consider fixing the naming style according to the solidity style guide.

‍

Lack of documentation

Informational  |  Resolved

The POP Network team didn’t share any type of documentation with the Vidma team.

Recommendation:

Consider providing some documentation.

The rich text element allows you to create and format headings, paragraphs, blockquotes, images, and video all in one place instead of having to add and format them individually. Just double-click and easily create content.

Static and dynamic content editing

A rich text element can be used with static or dynamic content. For static content, just drop it into any page and begin editing. For dynamic content, add a rich text field to any collection and then connect a rich text element to that field in the settings panel. Voila!

How to customize formatting for each rich text

Headings, paragraphs, blockquotes, figures, images, and figure captions can all be styled after a class is added to the rich text element using the "When inside of" nested selector system.