Jake's Audit Notes: Starfleet

# Jake's Audit Notes: Starfleet Client: Starfleet https://tech.origintrail.io/protocol https://github.com/OriginTrail/starfleet-boarding-contract/blob/main/contracts/ `595a75` ## Whitepaper & specification about the project Separate documentation provided declaring intent and implementation. Project is a logistics platform which uses staked TRAC tokens to allow writes to the OriginTrail Decentralised Network. ## Review of the protocol/implementation [1] **Tests Not Running** **Files Affected:** `./test` **Severity: Low** Tests are not able to be run via `npm run test`. Runable tests are important to determine the implementation of the project is correct and functions provide the expected outputs. **Recommendations:** [a] Provide instructions on running tests and ensure they are executable. [2] **Unlocked Pragma** Files Affected: `Migrations.sol`, `StarfleetStake.sol`, `TracToken.sol`. **Severity: Low** The version of the Solidity defined in the code is not fixed. This can lead to unintended or unexpected behaviours in the implementation due to different compiler versions between what is intented in the code and the output at compile time. **Recommendations:** [a] Statically define the version of the Solidity compiler using `pragma solidity 0.X.0`, where the desired release of Solidity is denoted. Suggested to use Solidity 8 which has protection for integer over/underflows and other security mechanisms. [3] **Validation of Constructor Arguments** Files Affected: `StarfleetStake.sol`. **Severity: Low** No checking is performed on the arguments presented to the constructor at deploy time to validate the boarding time is sane (not in the past, too far in the future). Additionally the token address for TRAC is fixed, and the token at `0xaA7a9CA87d3694B5755f213B5D04094b8d0F0A6F` is not upgradable. In the event of a token migration/swap/upgrade, this would bind the upgrade of the TRAC token to this deployment of Starfleet. **Recommendations:** [a] Validate the start time of the staking is not in the past or too far in the future. [b] Consider the implications of not being to change the TRAC token. https://github.com/OriginTrail/starfleet-boarding-contract/blob/595a752c62466786f837889496f957b2110f0476/contracts/StarfleetStake.sol#L42 [4] **Incomplete Implementation from Specification** Files Affected: `Migrations.sol`, `TracToken.sol`, `StarfleetStake.sol`. **Severity: High** FR3 requires that `Token holders SHALL NOT be able to withdraw TRAC tokens during the Starfleet launch window and Bridge launch window phases.` Tracking the launch and bridge window phases is not implemented in the [`withdrawTokens()`](https://github.com/OriginTrail/starfleet-boarding-contract/blob/595a752c62466786f837889496f957b2110f0476/contracts/StarfleetStake.sol#L100), which means this functional requirement has not been met. FR7 requires `The contract manager SHALL be able to change ownership of the contract to another address for bridge deployment or security reasons`. No functionality of transfer of the contract ownership has been implemented. **Recommendations:** [a] Ensure the implementation matches the functional requirement specifications. [5] **Unsafe Transfer Call** Files Affected: `StarfleetStake.sol` **Severity: Medium** [`transferTokens()`](https://github.com/OriginTrail/starfleet-boarding-contract/blob/595a752c62466786f837889496f957b2110f0476/contracts/StarfleetStake.sol#L148) uses `transfer()` to move funds from the contract to a custodian during the bridge period. This call is unsafe as it uses a fixed amount of gas to execute, and future forks including changes of the gas mechanism in Ethereum may result in this function being unusable or very expensive. Also affects [`fallbackWithdrawTokens()`](https://github.com/OriginTrail/starfleet-boarding-contract/blob/595a752c62466786f837889496f957b2110f0476/contracts/StarfleetStake.sol#L120). **Recommendations:** [a] Move to using `call()` to transfer the assets. [6] **Missing Access Control Modifier** Files Affected: `StarfleetStake.sol`. **Severity: High** [`accountStarTRAC()`](https://github.com/OriginTrail/starfleet-boarding-contract/blob/595a752c62466786f837889496f957b2110f0476/contracts/StarfleetStake.sol#L127) and [`transferTokens()`](https://github.com/OriginTrail/starfleet-boarding-contract/blob/595a752c62466786f837889496f957b2110f0476/contracts/StarfleetStake.sol#L142) use a modifier to control access for who may call these functions. The modifier is not defined in the code base so these functions will never be able to be called. **Recommendations:** [a] Implement the correct access control modifier to set the `onlyOwner` user. [7] **Missing Error Messages in Require** Files Affected: `StarfleetStake.sol`. **Severity: Informational** The use of `require()` is used throughout the project for the evaluation of conditions before function logic can be executed. In the event these are not met there is no error message returned with the revert to inform the user of why the call fails. **Recommendations:** [a] Implement an error message with each require clause such as `require(msg.sender == _account, "Sender not authorized.");` [8] **Unimplementable Functional Requirement** Files Affected: `StarfleetStake.sol`. **Severity: Medium** It is not possible to prevent Ether from being sent to a smart contract. This is a requirement in FR8. Even with a `revert()` in a fallback function preventing Ether being received, it is still possible for a contract to receive Ether. If a contract is created and `SELFDESTRUCT` is used to destroy the instance, this contract can be populated with the Ether from the destroyed contract via `selfdestruct(addr);`, where `addr` is the address of the Starfleet contract. **Recommendations:** [a] Ensure that all Ether balance operations account for the total amount of deposits.