OriginTrail Starfleet Staking

Integer overflow/underflow occur when an integer hits its bit-size limit. Every integer has a set range; when that range is passed, the value loops back around. A clock is a good analogy: at 11:59, the minute hand goes to 0, not 60, because 59 is the largest possible minute. The following instance were detected in the contracts: 1. The `require` statement on L66 uses the primitive addition operator to add a user provided input `amount` to the balance of the contract, which could result in an integer overflow. 2. There are several locations where `tZero` is added to one of the 3 period length constants defined in the contract using the `+` operator: * `depositTokens` on L65 * `fallbackWithdrawTokens` on L114 * `accountStarTRAC` on L128 * `transferTokens` on L142

Most functions will return a `true` or `false` value upon success. Some functions, like `send()`, are more crucial to check than others. According to the ERC20 standard the `transfer` and `transferFrom` functions return `true` if the transfer was successful and `false` otherwise. These functions do not need to revert if the transfer is not successful. However, there are several places in the contract where these return values are ignored: 1. `depositTokens` ignores return value by `token.transferFrom(msg.sender,address(this),amount)` on L70 2. `withdrawTokens` ignores return value by `token.transfer(msg.sender,amount)` on L107 3. `fallbackWithdrawTokens()` ignores return value by `token.transfer(msg.sender,amount)` L120 4. `transferTokens` ignores return value by `token.transfer(custodian,balanceTransferred)` on L148 If the `token` contract signals an error by returning `false` instead of reverting, the functions above do not detect any error and proceed with their execution.

Once the total token balance of the contract is higher or equal to `MIN_THRESHOLD`, the `depositTokens` function will emit the `MinThresholdReached` event on each subsequent deposit. This might be confusing for any listener of this event, in case they are expecting this event to be emitted only once.

In the constructor, `tZero` could be set to the past, i.e., any timestamp lower than `now` if `startTime > 0`. It is unclear whether this is a desired behavior or if it is a bug.

The list of `participants` tracks addresses with positive balance of `token` deposited in the contract. If the contributor withdraws their balance by calling the `withdrawTokens()` function, that contributor is not removed from the list of `participants`.

Projects may rely on block timestamps for various purposes. However, it's important to realize that miners individually set the timestamp of a block, and attackers may be able to manipulate timestamps for their own purposes by up to 900 seconds. The smart contract relies the value of the block timestamp for almost every function and is therefore subject to this attack.

As required by functional requirement FR8 the staking contract should not be allowed to receive any ETH transfers. The default behaviour (if no fallback function is explicitly given) is to throw an exception. For unwilling EOA transfers this is sufficient. However, according to the official Solidity documentation (https://docs.soliditylang.org/en/v0.6.0/contracts.html#receive-ether-function): > "A contract without a receive Ether function can receive Ether as a recipient of a **coinbase** transaction (aka miner block reward) or as a destination of a `selfdestruct`."

Presently, the contract owner could renounce his ownership, leaving the contract without any owner. If that happens, `onlyOwner` operations will not longer be possible to be executed, which could cause the contract to be partially inoperable.

On L52 and 55, the input token address is converted to `ERC20`, which may be an unsafe operation if the target contract does not inherit from OpenZeppelin’s implementation.