The Rentals Token Smart Contract Audit
# 1. Introduction
iosiro was commissioned by [LaComunity](https://www.lacomunity.com/) to conduct an audit on their ERC-20 [The Rentals Token](https://www.rentalstoken.io/en) and crowdsale smart contracts. The audit was performed between 30 May 2018 and 05 June 2018. A review of the issues was performed between 06 June 2018 and 07 June 2018.
This report is organized into the following sections.
* **[Section 2 - Executive Summary:](#section-2)** A high-level description of the findings of the audit.
* **[Section 3 - Audit Details:](#section-3)** A description of the scope and methodology of the audit.
* **[Section 4 - Design Specification:](#section-4)** An outline of the intended functionality of the smart contracts.
* **[Section 5 - Detailed Findings:](#section-5)** Detailed descriptions of the findings of the audit.
The information in this report should be used to understand the risk exposure of the smart contracts, and as a guide to improve the security posture of the smart contracts by remediating the issues that were identified. The results of this audit are only a reflection of the source code reviewed at the time of the audit and of the source code that was determined to be in-scope.
<a name="section-2"></a>
# 2. Executive Summary
This report presents the findings of an audit performed by iosiro on their token and crowdsale smart contracts. The purpose of the audit was to achieve the following.
* Ensure that the smart contracts functioned as intended.
* Identify potential security flaws.
Due to the unregulated nature and ease of transfer of cryptocurrencies, operations that store or interact with these assets are considered very high risk with regards to cyber attacks. As such, the highest level of security should be observed when interacting with these assets. This requires a forward-thinking approach, which takes into account the new and experimental nature of blockchain technologies. There are a number of techniques that can help to achieve this, some of which are described below.
* Security should be integrated into the development lifecycle.
* Defensive programming should be employed to account for unforeseen circumstances.
* Current best practices should be followed wherever possible.
At the conclusion of the audit review process, no high or medium risk issues were open. The audit originally identified three high and one medium risk issues that could lead to the loss of funds or tokens. Design recommendations were also provided to improve the functionality and performance of the smart contracts. Despite the findings, the code quality was of a relatively high standard. It prioritized clarity over performance and was modularized. As this appears to be a completely new implementation of a token and crowdsale, and that several issues were identified, it is recommended that another full audit is performed to allow a second iteration of tests to be performed.
The risk posed by the smart contracts can be further mitigated by using the following controls prior to releasing the contracts to a production environment.
* Use a public bug bounty program to identify security vulnerabilities.
* Develop a comprehensive test suite for the codebase.
<a name="section-3"></a>
# 3. Audit Details
## 3.1 Scope
The source code considered in-scope for the assessment is described below. Code from any other files are considered to be out-of-scope.
### 3.1.1 The Rentals Token
**Project Name:** TokenSale<br/>
**Commit:** [6ffde1b](https://github.com/therentalstoken/TokenSale/tree/6ffde1bc216ffd8a225a6123b311c325616f7b2a/contracts)<br/>
## 3.2 Methodology
A variety of techniques were used to perform the audit, these are outlined below.
### 3.2.1 Dynamic Analysis
The contracts were compiled, deployed, and tested using both Truffle tests and manually on a local test network. A number of pre-existing tests were included in the project.
### 3.2.2 Automated Analysis
Tools were used to automatically detect the presence of potential vulnerabilities, such as reentrancy, timestamp dependency bugs, transaction-ordering dependency bugs, and so on. Static analysis was conducted using Mythril and Oyente. Additional tools, such as the Remix IDE, compilation output and linters were used to identify potential security flaws.
### 3.2.3 Code Review
Source code was manually reviewed to identify potential security flaws. This type of analysis is useful for detecting business logic flaws and edge-cases that may not be detected through dynamic or static analysis.
## 3.3 Risk Ratings
Each Issue identified during the audit is assigned a risk rating. The rating is dependent on the criteria outlined below..
* **High Risk** - The issue could result in a loss of funds for the contract owner or users.
* **Medium Risk** - The issue results in the code specification operating incorrectly.
* **Low Risk** - A best practice or design issue that could affect the security standard of the contract.
* **Informational** - The issue addresses a lapse in best practice or a suboptimal design pattern that has a minimal risk of affecting the security of the contract.
* **Closed** - The issue was identified during the audit and has since been addressed to a satisfactory level to remove the risk that it posed.
<a name="section-4"></a>
# 4. Design Specification
The following section outlines the intended functionality of the smart contracts.
## 4.1 The Rentals Token
The token functionality is described below.
#### ERC20 Token
The token implements the ERC20 standard.
Field | Value
------------ | -------------
Symbol | TRT
Name | The Rentals Token
Decimals | 18
Total Supply | 1,350,000,000
#### Haltable
It should be possible for the token contract owner to enable and disable transfers.
#### Escrow
All of the tokens should be held in an escrow contract initially.
## 4.2 Crowdsale
The crowdsale functionality is described below.
#### Multiple Rounds
It should be possible to have multiple rounds, namely:
* Presale
* 1st Tier
* 2nd Tier
* It should be possible to set various variables, such as dates, prices, caps, and minimum investments per round.
*Update: [9324d28](https://github.com/therentalstoken/TokenSale/commit/9324d2870264ab6f29d07baea2e9c9644993fdc2#diff-e9839443e2c0b012386b0555ffe30f98) introduced the ability to have more than 3 ICO rounds. Information on additional rounds was not available at the time of the review.*
#### Stages
Each round has three stages these are described below.
* Setup - The round details (i.e. minimum contribution amount, maximum contribution amount, hard cap, and exchange rate per round) can be set at this stage.
* Started - It is possible to purchase tokens during this stage.
* Ended - At this stage, it is possible to proceed to the next round.
#### Minimum Contribution Limit
A minimum contribution limit should be set for each round of the crowdsale.
#### Token Release Time
A time lock is set for tokens, freezing token transfers until the timelock passes.
#### Refundable
If the round softcap is not reached, the round should become refundable, allowing contributors to withdraw their funds.
#### Whitelist
Only participants who have been whitelisted should be able to contribute to the ICO.
#### Exchange Rate
The exchange rate should be set to 7500 TRT per ETH.
#### Fund Distribution
Funds should be released equally to three separate wallets upon successful completion of a round.
#### Minimum Round Contributions
While the specification did not specify the minimum round contributions, the following values were found:
* Presale: 1 ether
* Tier 1: 1 ether
#### Distribution
The following distribution table should be adhered to:
Role | Percentage
------------ | -------------
Public Sale | 50% (675,000,000)
Community Reserve | 20% (270,000,000)
Company Reserve | 14% (189,000,000)
Team and Advisors | 15% (202,500,000)
Bounty Program | 1% (13,500,000)
#### Caps
The following shows should be followed::
Round | Softcap (ETH) | Hardcap (ETH)
-------- | ----------------- | -----------------
Presale | 750 | 8,067
1st Tier | 0 | 8,000
2nd Tier | 0 | 65,333
*Update: The requirement for a 2nd Tier softcap was removed by the team and implemented in [33148e8](https://github.com/therentalstoken/TokenSale/commit/33148e8321be57a71fa48088b7a2ffc60c590dac).*
#### Discounts
The following discounts should be given:
* Presale: 45%
* Tier 1: 20%
* Tier 2: 0%
<a name="section-5"></a>
# 5. Detailed Findings
The following section includes in depth descriptions of the findings of the audit.
## 5.1 High Risk
No high risk issues were present at the conclusion of the audit.
## 5.2 Medium Risk
No medium risk issues were present at the conclusion of the audit.
## 5.3 Low Risk
### 5.3.1 REFUNDED TOKENS ARE LOCKED
*Tokens.sol: General*
#### Description
In the event of an unsuccessful ICO round, the tokens returned to the ICO contract by the contributors during the refund process would be locked in the ICO contract. While it was possible to withdraw unsold tokens in the finalise function, there was no functionality to withdraw tokens sent to the contract after that stage. As a result, a potentially large portion of tokens could be locked indefinitely in the event of an unsuccessful round.
As only the Presale round enforced a softcap, the severity of the issue was limited. In the event of a failed Presale and the tokens being locked, a completely new token and crowdsale could be launched. If additional rounds are launched in the future, they should also have a softcap of 0, otherwise refunded tokens could be lost in the event of not reaching the goal.
#### Remedial Action
It is recommended that when tokens are returned during the refund process that they are sent directly to the Escrow contract in order to be used in further rounds.
### 5.3.2 SPECIFICATION DISCREPANCIES
*General*
A number of discrepancies between the specification and the implementation were identified. These are outlined below.
#### Tokens easily lockable
It was found that due to the ICO having weak validation of parameters on deployment, it was easily possible to lock tokens in the contract. For example, if the RefundVault was set to an invalid address on deployment, it would still be possible to fund the ICO with tokens, but it would be impossible to move or withdraw the tokens. It is recommended that a method to withdraw the tokens back into the escrow is added in the event of an emergency.
#### Tokens not transferable after 24 hours
The specification detailed that “tokens are not tradable until 24 hours after the end of first ICO level”. The timelock was found to be set to 45 days after deployment, on line 63 of tokens.sol, while the Presale round lasted 24 days and Tier 1 lasted 14 days. As functionality was available to modify the timelock, the impact of the issue is limited.
#### Limited token issuing functionality implemented on-chain
The escrow contract was found to not implement any release schedule. Rather, the full balance of each address was available to be withdrawn by the owner. The owner had to manually withdraw the correct amount of tokens at each stage.
The specification did not specify the amount of tokens to be released to each addresses at each stage.
### 5.3.3 POTENTIAL TO BYPASS ESCROW LIMITS
*Escrow.sol: General*
#### Description
The `deposit(...)` function in Escrow.sol relied on the ICO contract to indicate the amount of tokens being returned by the ICO to the escrow in the event of an unsuccessful round. As a result, it would be possible to deploy an ICO contract that arbitrarily called the deposit function to increase the withdrawal allowance of the ICO, bypassing the 50% limit in the specification.
#### Remedial Action
It is recommended that the Escrow contract calculate the number of tokens based on the current balance minus the balance required for the non-ICO allowances. The result would be equal to the number of tokens held by the contract available for the ICO.
## 5.4 Informational
### 5.4.1 Design Comments
*General*
The following describes possible actions to improve the functionality and readability of the codebase.
#### No method of removing from whitelist
While it was possible to add addresses to the whitelist, there was no option to remove them. This would mean that if an address was accidentally added or if KYC regulations changed, there would be no way to revoke a previously added investor. It is recommended that a `removeInvestor(...)` function is added to remove an address from the whitelist.
#### Expensive method of setting wallet values
In order to ensure that all of the contracts used the correct wallet addresses, each contract that required the addresses inherited from the HardcodedWallets contract. This would add to the deployment cost, as each instance had its own instance of HardcodedWallets. A more efficient implementation would be to simply pass the wallet addresses into the constructor of each of the contracts.
#### No functionality to update start time of ICO
While functionality was available to update the end time of the ICO, there was no functionality available to update the start time. It may be desirable to include this functionality in the event that the ICO times need to be adjusted after deployment. This functionality was not included in the specification.
#### Use absolute dates
The contract was found to use dates relative to the deployment date, for example, the token timelock duration was specified as 45 days from the deployment of the token. Rather, absolute dates should be set using a unix timestamp, specified in seconds, to ensure that times are adhered to correctly. For example, `require(now < 1528121677)` will determine if the current time is before 04 June 2018.
#### No contract validation
No validation was performed when setting contract addresses to ensure that the address was a valid implementation of the contract. For example, when setting the token smart contract address in the escrow contract, an invalid instance of the contract, such as 0x01, would be undetected.
It is recommended that whenever an instance of a contract is specified, that the instance is validated to ensure that the address is a valid implementation of the contract. An example of how this can be achieved is to have a validation function, such as `isToken()`, in the token contract that returns true. After line 93 in escrow.sol, a statement can be added:
`require(SCTokens.isToken());`
#### No tests provided
No tests were provided with the supplied smart contract. Tests would help developers to ensure that tokens are allocated correctly, dates are adhered to, and otherwise determine whether the contract operates as intended.
It is recommended that a framework, such as Truffle, is used to develop and test the smart contracts. Truffle provides the ability to build tests in both JavaScript and Solidity. For JavaScript tests, Truffle uses the Mocha testing framework and Chai for assertions. More information can be found here. The coverage of the tests can then be determined by using a tool such as solidity-coverage to ensure that the tests cover a broad range of code paths.
#### Add a fallback revert function to RefundVault
As the RefundVault contract is intended to receive ether for a specific period of time, it is recommended that a fallback function is added that simply reverts. This requires funds to be sent through only the `deposit(...)` function, avoiding the issue of accidentally locking funds in the contract once it is out of the Active state.
## 5.5 Closed
### 5.5.1 TRANSFERORIGIN INTRODUCES POTENTIAL PHISHING-STYLE ATTACK (HIGH)
*tokens.sol: line 139-149*
#### Description
The `transferOrigin(...)` function facilitated token transfers based on the value of `tx.origin`, rather than the usual `msg.sender`. The purpose of this was to allow external contracts to transfer tokens on behalf of a user. This function is incredibly dangerous as it introduces the ability to trivially steal tokens from users. An example of how the pattern can be exploited is outlined below.
1. Malicious actor (MA) deposits ether into an exchange.
2. MA sets their exchange withdrawal address as a contract. The contract implements a fallback function that calls `transferOrigin(...)` with the total supply of the tokens on the exchange.
3. When MA asks to withdraw their ether from the exchange, the exchange sends ether to the contract, calling the fallback function and triggering `transferOrigin(...)` with `tx.origin` as the exchange address, transferring all of the tokens out of the exchange.
If a standard transfer were being used, the `msg.sender` would appear as the malicious contract, not allowing the MA to affect the exchanges’ funds.
#### Remedial Action
It is highly recommended that the `transferOrigin(...)` function be removed. Depending on the desired functionality, alternative standards, such as [ERC-223](https://github.com/Dexaran/ERC223-token-standard/tree/master/token/ERC223) or [ERC-827](https://github.com/ethereum/EIPs/issues/827) may want to be considered.
Additionally, the `onlyOwnerOrigin` modifier should be considered a high risk design decision, as the same attack outlined above could be used to phish the owner account to call functions protected by this modifier. It is recommended that this modifier be removed, in favour of the traditional `onlyOwner` modifier. This would require more transactions to perform the same task, but would greatly improve the security of the contracts. In the event that this change is implemented, it is recommended that the `onlyOwner` modifier is added to `fundICO(...)`, as there would be no need to allow any address to call the function.
*Update: Addressed in [fd7d4b1](https://github.com/therentalstoken/TokenSale/commit/fd7d4b168da610b76c19cd43954be56a01cd9a03#diff-e9839443e2c0b012386b0555ffe30f98), [ffc7bcf](https://github.com/therentalstoken/TokenSale/commit/ffc7bcf7f5e68ecedd7dc3bd99a23091ffc6ebcf#diff-e9839443e2c0b012386b0555ffe30f98), and [1e66bc3](https://github.com/therentalstoken/TokenSale/commit/1e66bc3b023bee954b802799e6b7f8a568965a99). The implemented fix required `msg.sender` to be the ICO contract, preventing regular users from being able to exploit the vulnerability. However, theoretically the contract owner would still be able to execute the attack, as they would be able to control the ICO contract address arbitrarily.*
*It should be noted that if a round is unsuccessful, moving to the next round (changing the ICO address of the token) would prevent users from being able to claim their refund.*
### 5.5.2 ABLE TO WITHDRAW FUNDS FROM ESCROW WITHOUT REACHING SOFT LIMIT (HIGH)
*Escrow.sol: General*
#### Description
A potential bypass to the soft cap withdrawal limit was identified during the audit. As the ICO contract notified the RefundVault whether the ICO was successful, it would be possible to change the ICO address of the RefundVault, before closing the ICO, to a contract that always returned successful. In such an event, it would be possible to withdraw the funds from the RefundVault without actually reaching the target.
#### Remedial Action
It is recommended that the following line is added to `setMyICOContract(address _SCICO)`:
`require(address(this).balance == 0)`;
This would only allow updates until the stage that the RefundVault started to receive funds.
*Update: Addressed in [f0caecb](https://github.com/therentalstoken/TokenSale/commit/f0caecb06d8a8c7e318430a2e82a7ca1085d83d7#diff-e9839443e2c0b012386b0555ffe30f98).*
### 5.5.3 ESCROW INCORRECTLY TRACKS REMAINING BALANCES (HIGH)
*escrow.sol: line 47-53*
#### Description
It was found that the `withdraw(...)` function in the Escrow contract did not deduct the withdrawn tokens from the balance of the account. As a result, it would be possible to withdraw the entire escrow to a single account, as long as it was performed in batches of less than the accounts total allocation.
#### Remedial Action
It is recommended that the following line is added to the `withdraw(...)` function to correctly track the remaining balances of an account.
`deposited[_address] = deposited[_address].sub(_amount);`
*Update: Addressed in [5cd8afd](https://github.com/therentalstoken/TokenSale/commit/5cd8afda442b1220490eabdd79791ef5126feda6#diff-e9839443e2c0b012386b0555ffe30f98).*
### POTENTIAL RACE CONDITION (MEDIUM)
*tokens.sol: line 121-126*
#### Description
A potential race condition exists in the current implementation of the `approve()` and `transferFrom()` functions. As the `approve()` function overrides the value of the allowance, if a call to `transferFrom()` is made and then a call to the `approve()` function is called, it may be possible to call `transferFrom()` again allowing the spender to transfer more tokens than the owner intended. This attack is described in-depth here.
#### Remedial Action
One suggested workaround is to require that the allowance of a spender first be set to 0 before changing the allowance. An alternative pattern is to use `increaseApproval()` and `decreaseApproval()` functions, an example can be found in the OpenZeppelin implementation found [here](https://github.com/OpenZeppelin/zeppelin-solidity/blob/a7e91856f3e275668b4a4c55cbd14864aa61b100/contracts/token/ERC20/StandardToken.sol).
*Update: Addressed in [88334eb](https://github.com/therentalstoken/TokenSale/commit/88334ebf11883723dbd256388449358f0a3f2447#diff-e9839443e2c0b012386b0555ffe30f98). It is still possible for the race condition to occur, but alternative safe functions are given.*
### 5.5.5 SPECIFICATION DISCREPANCIES (LOW)
*General*
#### Tier 2 not implemented
Only the Presale and Tier 1 rounds were implemented at the time of the audit. Tier 2 was not available and was therefore out of scope. It is recommended that this code is included in a future audit before deploying it to a production environment.
*Update: Addressed in [2a635f6](https://github.com/therentalstoken/TokenSale/commit/2a635f6b86650192494e688405b1429ee60b664f#diff-e9839443e2c0b012386b0555ffe30f98), [8c6443b](https://github.com/therentalstoken/TokenSale/commit/8c6443b36ca0be34ae1da8fdfc03e5dc36cbf69d#diff-e9839443e2c0b012386b0555ffe30f98), and [3f7dd7f](https://github.com/therentalstoken/TokenSale/commit/3f7dd7f6b753143a2aa0ab6673cf1ce39f461e77#diff-e9839443e2c0b012386b0555ffe30f98).*
### 5.5.6 DESIGN COMMENTS (INFORMATIONAL)
*Escrow.sol: General*
#### Returning false instead of reverting
The code was found to return `false` in many instances when it would have been safer to revert the state. When performing input validation it is generally better to use the `require` or `revert` keywords to enforce restrictions, as they will rollback any changes to the state. If a `false` return value is not reverted by the calling function, changes to the state could take effect without the call succeeding. No vulnerabilities were identified in the code related to this, however, as a best practice it is recommended that validation code is changed from if statements to require.
Most instances of this issue were related to the `error(...)` function using the `Error` event, logging the error, rather than reverting the state. While this pattern is useful for debugging purposes during development, it is recommended that this functionality is changed to revert before going into production.
*Update: Addressed in [8f91835](https://github.com/therentalstoken/TokenSale/commit/8f91835e1e20f206590fdcc6ae55bc4e1877f522#diff-e9839443e2c0b012386b0555ffe30f98). After identifying a dangerous edge case related to issuing refunds, the team was instructed to fix the issue.*
#### Incorrect variable names used
The following discrepancies between the ERC-20 standard and the code were identified. This can cause problems when interfacing with third party applications, such as wallets and exchanges. It is recommended that the following changes are made to tokens.sol.
* `balanceOf(address _someone)` function must be `balanceOf(address _owner)`
* `allowance(address _someone, ...)` function must be function `allowance(address _owner, ...)`
* `Approval(address indexed _someone, ...)` event must be `Approval(address indexed _owner, ...)`
*Update: Addressed in [f7dc4c8](https://github.com/therentalstoken/TokenSale/commit/f7dc4c80cc9627aaa3857870d57dfaa5cff7242e#diff-e9839443e2c0b012386b0555ffe30f98).*
#### Update SafeMath
A [slight modification](https://github.com/OpenZeppelin/openzeppelin-solidity/commit/5ad07e1892a08e8485d0ba6752eeb67fbd4182b2#diff-b7935a40e05eeb5fe9024dc210c8ad8a) in the way multiplication is performed in SafeMath results in reduced gas costs. It is recommended that a [newer version](https://github.com/OpenZeppelin/openzeppelin-solidity/blob/ad12381549c4c0711c2f3310e9fb1f65d51c299c/contracts/math/SafeMath.sol) of SafeMath is used to incorporate this change.
*Update: Addressed in [769929d](https://github.com/therentalstoken/TokenSale/commit/769929d61dc6b53a8efa2358aae6053573c39ece#diff-e9839443e2c0b012386b0555ffe30f98).*
#### No token creation event
The ERC-20 standard specifies that a Transfer event should be emitted when tokens are created with a from address of 0x0. As such, it is recommended that line 68 of tokens has its from address changed from `this` to `0x0` to indicate that the tokens are being created, rather than transferred.
*Update: Addressed in [769929d](https://github.com/therentalstoken/TokenSale/commit/769929d61dc6b53a8efa2358aae6053573c39ece#diff-e9839443e2c0b012386b0555ffe30f98).*
#### Improper casing
It is recommended that the filenames of the smart contracts are named using a CapWords styling, e.g. “SimpleToken.sol”, to match the contract naming style specified in the solidity [style guide](https://solidity.readthedocs.io/en/v0.4.24/style-guide.html).
*Update: Addressed in [4a0dcfd](https://github.com/therentalstoken/TokenSale/commit/4a0dcfd89ecc72c2148a4d4ef0be6eb807a68202#diff-e9839443e2c0b012386b0555ffe30f98).*
#### Unnecessary inheritance and imports
Unnecessary inheritance and imports can lead to wasted gas and increased costs.
* The HardcodedWallets contract did not need to inherit from Haltable as it did not make use of any of its functions. It did not need to import system.sol for the same reason.
* The Whitelist contract did not need to inherit from HardcodedWallets, as the wallet addresses were not used in the Whitelist contract.
*Update: Addressed in [9db3db6](https://github.com/therentalstoken/TokenSale/commit/9db3db6f1b0fa95cb5f419f5449dd1a2dc8f57c6#diff-e9839443e2c0b012386b0555ffe30f98).*
#### Contract tokens burned during unsuccessful ICO round
It was found that if an ICO round failed, either by the softcap not being reached or if refunds were forcibly enforced by the contract owner, the tokens held by the contract were still burned on line 238 of ICO.sol. This behaviour was not identified in the specification. It is recommended that functionality is added to withdraw the tokens from the ICO in order to use them in a different ICO.
*Update: Addressed in [4e7d632](https://github.com/therentalstoken/TokenSale/commit/4e7d632d82ab456749a8fdeea219076fe015d805).*
