Film.io security record

In function `unstaketokens()`, there is an attempt to check that `stakeid` input matches the recorded parameter `stakeid` for a struct corresponding to the `stakeid` in the `stake` arrays `stakeofaccounts` and `objectStakes`. In such a case, the goal is to either delete the corresponding information or update the recorded `quantity` parameter. However, there is no reason that the `stakeid` parameter at `stakeofaccounts[stakeid - 1].stakeid` or `objectStakes[stakeid - 1].stake.id` should be equal to the input `stakeid`, and in most cases where multiple users are staking multiple times and in between one another, this is likely false. This means this check in the if-block conditional statement will cause issues in the function logic. ### Scenario Say 1000 users have already staked so that storage variable `stakeindex` is currently value 1000. Now assume that a user, Alice, wishes to stake for the first time. Previously for Alice, her corresponding `stake` array, `stakeofaccounts = []`, is an empty array. Alice successfully calls `staketokens()` and now `stakeIndex = 1001`, and her `stakeId` will also be `1001`. However, Alice has only staked once, so she has one entry in her array `stakeofaccounts`: a `stake` struct with `stakeId` of 1001. Thus `stakeofaccounts[0].stakeid` is 1001 for Alice, while a check to `stakeofaccounts[stakeid -1].stakeid` is a check of the 1000th entry of this array. This entry does not exist for Alice, and will cause a revert. In this way, if more than one user attempts to stake with this contract, all but the first staker will be kept from unstaking because of the revert caused by this logical issue. The same issue occurs with `objectStakes` logic.

Any rating entry in the mapping `projectEvaluation` can be overwritten with a new chosen rating by any user that has previously created their own evaluation through the logic in `modifyEvaluation()`. The function checks that an input `prj_id` matches the parameter in `projectLockDetails[prj_id]` and `projectEvaluation[prj_id], that `prj_id` is nonzero, that the input `que_id` matches the parameter `projectEvaluation[prj_id].que_id`, and finally that `userEvaluation[_msgSender()].account` is the `_msgSender()`. If two distinct users have used function `createEvaluation()` for two distinct `prj_id` values, then either can update the `projectEvaluation` mapping of the other's `prj_id` to their own chosen `rating` value.

Function `getlockedAmount()` is used to calculate the `lockedAmount` for a sending address in the internal `_transfer()` function of the token. While this function uses the length of `userLocks[_user]` as an upper bound in a for loop, to sum up `lockedAmount`, there are several issues with the logic of this function. 1. The `lockDuration` and `lockAmount` at `lockDetails[i]` are used in the function logic, where `i` is the iteration number of the for-loop. This entry of the mapping `lockDetails` may or may not correspond to the `_user` address, so the resulting `lockedAmount` that is calculated in the for-loop may or may not be equal to the actual total amount locked for the `_user` address. The value can be less or more than `_user` address's actual total locked amount. This is a critical vulnerability, as it may allow users to transfer funds that are meant to be locked, it may keep users from transferring amounts meant to be unlocked, and it may result in a `lockedAmount` that is larger than `balanceOf(from)` in `_transfer()`, causing a revert due to underflow. 2. The for-loop includes the value `userLocks[_user].length` as an iteration value `i` even though `userLocks[_user][length]` is a not an entry of the array `userLocks[_user]`. The function does not revert in this case though, because, again, `lockDetails[i]` is read instead of getting the `lockId` value held at `userLocks[_user][i]`, and `lockDetails` is a mapping while `userLocks[_user]` is an array. These two issues together allow this function to execute inconsistently and cause critical issues with the locking logic of the contract ### Scenario As a simplified example, Alice and Bob are set up with a locked amount of tokens through the `lockUp()` function. Alice is given 10 tokens first, so she gets lockId = 1, and Bob is given two locked amounts over lockId = 2 (say amount is 10), and lockId = 3 (say amount is 11). When Alice calls public view function `getlockedAmount()` to see her total locked amount, the `userLocks[_user].length` is 1, so the for-loop will iterate through i = 0 and i = 1. For i = 0, `lockDetails[0].lockDuration` is 0, so nothing is counted (this will always be true). When i =1, assuming `lockDetails[1].lockDuration` is larger than the current timestamp, then `lockedAmount` is updated to be `lockedDetails[i].lockAmount`, which is 10. It happens that in this case, i = 1 matches Alice's lockId that she owns, so the return value is accurate. Now consider Bob's case. Again, assuming both his locks are still upheld for the current `block.timestamp`, assume he calls the `getlockedAmount()` function with his address. Then his value of `userLocks[_user].length` is 2, and the for-loop will iterate through i = 0, 1, 2. As before, `lockDetails[0]` will not be counted. However, `lockedAmount` will get `lockDetails[1].lockAmount` (amount is 10), and `lockDetails[2].lockAmount` (amount is again 10) added to it. The total returned `lockedAmount` will be 20, even though for Bob, it is supposed to be 21. This is because two of the `lockDetails[i].lockAmount` values (i = 0, and i =1) did not belong to Bob, resulting in a miscalculation.

If a user calls `createEvaluation()` more than once on two different `prj_id` inputs, then the first correspondence for `userEvaluation` mapping will be overwritten with the second, losing the first `EvaluateDetails` struct correspondence. However, the `EvaluateDetails` struct in the `projectEvaluation` mapping will persist for the first `prj_id`.

When users call the function `staketokens()`, they specify an input address `to` which can be any address they choose as long as it is not the one they are making the call from. The `staketokens()` function uses this address to send tokens from the `_msgSender()` to the target `to` address. This creates recorded stake information within the contract that can then be used in the function `unstaketokens()` to transfer tokens from the `escrow_account` to the `_msgSender()`, regardless of where the tokens were first transferred. A user can have a boundless number of their own accounts. So a user can input one of their own addresses for the `to` input, transfer this `quantity` of tokens from their first account to their other owned account, then immediately call `unstaketokens()` to have an equivalent amount of tokens transferred from the `escrow_account` to their original account. Now they own twice as much of the token as they originally did across two accounts. This pattern can be repeated by the attacker until they have drained all available funds from the escrow account.

All `FAN` tokens are sent to the `_msgSender()` address when initializing the contract. This is a potential centralization risk as the deployer can distribute all `FAN` tokens without the consensus of the community.

Function `createEvaluation()` has the following requirement: ```solidity require(rating>=100 && rating <=2000,"Rating should be between 100 to 2000"); ``` while function `modifyEvaluation()` has the requirement: ```solidity require(rating >= 10 && rating <= 2000,"Rating should be between 10 to 2000"); //rating is between 1 t0 20 ``` Based on the comment made next to the latter requirement, the rating should be scaled down by a factor of 100 to be 1 to 20, meaning that the rating requirement in function `createEvaluation()` is correct, while the rating requirement in `modifyEvaluation()` does not fit accurately reflect the scale. Any updates in `modifyEvaluation()` below the value of 100 may be incorrectly converted to a value of 0.

Since the `escrow_account` is the funding account address for unstaking in the `StakeV1` contract, the address should be made public, so that users can see the balance of this address and the allowances of the `StakeV1` address for the `escrow_account`. That way, they are able to make an informed decision about whether to add their tokens to the `StakeV1` contract.

The order of external call/transfer and storage manipulation must follow the check-effect-interaction pattern. To follow this pattern, each call to `tokenInterface.transferFrom()` should occur at the very end of the function in which it is used.

In the function `createRating()` the input `userrating` must be strictly greater than 0 and no more than 50. The comment in the requirement states "Rating needs to be between 1 to 5". Considering the `userrating` is allowed to be as high as 50, it seems that `userrating` is supposed to be scaled by a factor of 10. However, any inputs for `userrating` that are lower than 10 in that case will be read as 0 when the value is scaled down, due to truncating decimals.