Masa - Staking #1

The function `rescueERC20Token` can be used by any address with the `DEFAULT_ADMIN_ROLE` role to transfer tokens from the contract. This includes the staking and reward token `masaToken`. As such, this privileged role is able to steal users' stakes and rewards.

The variable `blockTime` is meant to denote the amount of time between blocks and is used to determine the number of blocks a staking period is for. ```solidity=351 uint256 stakingPeriodBlocks = (stakeData.period * 30 days) / blockTime; ``` However, `blockTime` can be changed by `setBlockTime`, which can lead to inaccurate staking periods. For example, suppose the staking period is 1, i.e. 30 days, and the starting `blockTime` is 1 day. Then after 15 blocks, half of the staking period, which is 15 days, have elapsed. However, if after these 15 days `blockTime` is updated to 2 days, the staking period would be considered over, since each of the 15 blocks that have **already** elapsed are now considered as 2 days each, for a total of 30 days. This allows a user to unstake earlier than the 30 days staking period. Similarly, decreasing the `blockTime` part way through a stake would increase the amount of time required to unstake.

There is currently no guarantee of rewards for stakers. Since the staking token and reward token are the same, there is a possibility that rewards are paid by stakers, leading to a ponzi-scheme.

Documentation and comments for the staking contract state that there are three specialized roles: - `DEFAULT_ADMIN_ROLE` - `PAUSER_ROLE` - `INTEREST_RATE_ROLE` However, only the `DEFAULT_ADMIN_ROLE` role is currently implemented.

Performing division by zero would raise an error and revert the transaction. ```solidity=325 uint256 elapsedFraction = (blocksElapsed * 1e18) / periodInBlocks; // Multiply by 1e18 for precision ``` The expression `(blocksElapsed * 1e18) / periodInBlocks` may divide by zero.

The following functions access storage variables without writing to them: - `getUserStake` - `getUserStakesWithInterest` - `canWithdrawStake` Since storage reads are much more expensive than memory reads, it would be more efficient to use `memory` instead of `storage`.