Read the findings of the 2022 Web3 Security Report

The contract has unlocked compiler version. An unlocked compiler version in the source code of the contract permits the user to compile it at or above a particular version. This, in turn, leads to differences in the generated bytecode between compilations due to differing compiler version numbers. This can lead to an ambiguity when debugging as compiler specific bugs may occur in the codebase that would be hard to identify over a span of multiple compiler versions rather than a specific one.

Unlocked Compiler Version

In the contract `Manager`, the role `contractOwner` has authority over the following functions:
- setMember(): the `contractOwner` can set member address
- setUserPermit(): the `contractOwner` can set the permissions of the account

In the contract `Member`, the role `contractOwner` has authority over the following functions:
- setManager(): the contractOwner can update the `manager` contract address

In the contract `Token`, the role `owner` has authority over the following functions:
- issue(): the owner can mint `MP` token to the owner's account

In the contract `TokenStake`, the member `mpToken\nftMasterChef\nft\updatecard\owner` have authority over the following functions:
- update(): add reward from `funder` member to `TokenStake` contract

In the contract `TokenStake`, the member `owner` has authority over the following functions:
- timeLockChange(): the owner member account can set the value of `timeLock`

Any compromise to any privileged account may allow a hacker to take advantage of this authority.

Centralization Related Risks

There should always be events emitted in the sensitive functions that are controlled by centralization roles.

Missing Emit Events

`public` functions that are never called by the contract could be declared as `external`. `external` functions are more efficient than `public` functions.

Improper Usage Of `public` And `external` Type

According to the logic, if users wants to withdraw their deposit, they need to call `withdrawRequest()` first and they need to wait for `timeLock`. Only then they can call `withdraw()` to withdraw their deposit.  
```solidity=115
function withdraw() public {
  require(lockRequest[msg.sender] !=0 
        && block.timestamp >= lockRequest[msg.sender].add(timeLock), "locked");
  ...
}
```
The initial value of `timeLock` is 15 minutes in the `TokenStake` contract, but the `owner` member can update `timeLock` value without any restriction. 
```solidity=110
    function timeLockChange(uint256 _period) public {
        require(msg.sender == manager.members("owner"), "onlyOwner");
        timeLock = _period;
    }
```
If the private key of the `owner` account was hacked or leaked, the attacker can update the `timeLock` to a large value, it may cause that users can not withdraw their deposit. Although the `contractOwner` can overwrite the `owner` address and update the value of `timeLock`, we still recommend adding a reasonable upper limit to `timeLock` to avoid this from happening.

`timeLock` Lacks The Upper Limit

When the user sends a withdrawal request, the contract will calculate the user's reward and transfer the reward to the user. 
```solidity=
  function withdrawRequest() public {
        require(lockRequest[msg.sender] == 0, "is in pending");
        getReward();
        ...
    }
```
If the reward in the contract is insufficient, the withdrawal request will fail and the user cannot withdraw.

Withdrawal May Fail Due To Insufficient Rewards

Through communication, we known that `TokenStake` contract is a single-currency pledge pool, that is, `stakeToken` and `MP` are the same token, we think there may be risks.
The `validSender` contains many privileged roles, these privileged roles can call the `update()` function to add rewards to the `TokenStake` contract. If these privileged roles are hacked, the attacker maliciously calls update() or the privileged role operates incorrectly, without transferring the actual reward to the contract. This will cause users to receive the principal pledged by other users when they receive rewards. For example:
1. The attacker pledges 10,000 MP tokens, the total pledge amount of all users is 20,000 MP tokens, and the reward for the pledge round is 20,000. According to the design, it is estimated that the attacker can receive 50% of the reward in this round.
2. The attacker stole the privileged account and called `update()` to add a reward of 20,000 (the reward was not actually transferred to the contract). At this time, the reward for this round is 40,000.
3. The attacker withdraws the deposit and rewards, reward=40000*(10000/20000)=20000, deposit=10000, fee=10000*2%=200, a total of 29800 MP. The attacker stole 10000 MP tokens. Due to insufficient rewards, the stolen part was actually deposits of other users.

Missing Input Validation

According to the logic, when users withdraw their deposit, they will be charged 2% of their deposit as fee.
```solidity=118
function withdraw() public {
  require(lockRequest[msg.sender] !=0 
        && block.timestamp >= lockRequest[msg.sender].add(timeLock), "locked");
  uint256 pendingWithdraw = userInfo[msg.sender].pendingWithdraw;
  uint256 fee = pendingWithdraw.mul(2).div(100);
  IERC20(stakeToken).transfer(msg.sender, pendingWithdraw.sub(fee));
  IERC20(stakeToken).transfer(address(manager.members("OfficalAddress")), fee);
  ...
}
```
We have some questions:  
1. What is `stakeToken`?  
2. Since users will be charged a 2% fee when withdrawing, if the users' deposit time is not enough, the value of the fee may be greater than the value of the users' reward?

Discussion For `withdraw()`

There is a mathematical calculation model in the contract. Although it is not complicated, there may be problems such as loss of precision. When the last person withdraws, the calculated amount may not match the balance in the contract (the error is very small), or the reward was not transferred to the contract before calling `update()` function(unable to confirm calling process), the user may not be able to withdraw due to insufficient contract balance.

Transfers May Fail Due To Insufficient Rewards

According to the code, the duration of `roundTime` is `24 minutes`.
```solidity=70
        if(block.timestamp >= roundTime[round] + 24 minutes) {
            round++;
            roundTime[round] = block.timestamp;
            ...
        }
```
Should it be `24 hours`?

Discussion For `roundTime`

When transferring standard ERC20 deflationary tokens, the input amount may not be equal to the received amount due to the charged transaction fee.  

For example, if a user stakes 100 deflationary tokens (with a 10% transaction fee) in `TokenStake`, only 90 tokens actually arrived in the contract. However, the user can still withdraw 100 tokens from the contract, which causes the contract to lose 10 tokens in such a transaction.  

It may cause the user's withdrawal failure due to insufficient `stakeToken` balance in the contract.

Incompatibility With Deflationary Tokens

The order of external call/transfer and storage manipulation should follow the check-effect-interaction pattern.

Check Effect Interaction Pattern Violated

The **require** can be used to check for conditions and throw an exception if the condition is not met. It is better to provide a string message containing details about the error that will be passed back to the caller.

Missing Error Messages

The given input is missing the check for the non-zero address.

All of the MP tokens are sent to the contract deployer when deploying the contract. This could be a centralization risk as the deployer can distribute MP tokens without obtaining the consensus of the community.

Initial Token Distribution

MetaPirate

Highlights & Alerts

This is just a placeholder

highlight placeholder lorem Ipsum...

highlight lorem

This is a alert demo

Alert demo

Code Audit History

15

0

5

0

10