iosiro
blockchain security

TRT Crowdsale Audit

Audit Results

 

Disclaimer

This is a limited report on our findings based on our analysis, in accordance with good industry practice as at the date of this report, in relation to: (i) cybersecurity vulnerabilities and issues in the smart contract source code analysed, the details of which are set out in this report, (Source Code); and (ii) the Source Code compiling, deploying and performing the intended functions. In order to get a full view of our findings and the scope of our analysis, it is crucial for you to read the full report. While we have done our best in conducting our analysis and producing this report, it is important to note that you should not rely on this report and cannot claim against us on the basis of what it says or doesn’t say, or how we produced it, and it is important for you to conduct your own independent investigations before making any decisions. We go into more detail on this in the below disclaimer below – please make sure to read it in full.

DISCLAIMER: By reading this report or any part of it, you agree to the terms of this disclaimer. If you do not agree to the terms, then please immediately cease reading this report, and delete and destroy any and all copies of this report downloaded and/or printed by you. This report is provided for information purposes only and on a non-reliance basis, and does not constitute investment advice. No one shall have any right to rely on the report or its contents, and Zenoic Proprietary Limited trading as “Iosiro” and its affiliates (including holding companies, shareholders, subsidiaries, employees, directors, officers and other representatives) (Iosiro) owe no duty of care towards you or any other person, nor does Iosiro make any warranty or representation to any person on the accuracy or completeness of the report. The report is provided "as is", without any conditions, warranties or other terms of any kind except as set out in this disclaimer, and Iosiro hereby excludes all representations, warranties, conditions and other terms (including, without limitation, the warranties implied by law of satisfactory quality, fitness for purpose and the use of reasonable care and skill) which, but for this clause, might have effect in relation to the report. Except and only to the extent that it is prohibited by law, Iosiro hereby excludes all liability and responsibility, and neither you nor any other person shall have any claim against Iosiro, for any amount or kind of loss or damage that may result to you or any other person (including without limitation, any direct, indirect, special, punitive, consequential or pure economic loss or damages, or any loss of income, profits, goodwill, data, contracts, use of money, or business interruption, and whether in delict, tort (including without limitation negligence), contract, breach of statutory duty, misrepresentation (whether innocent or negligent) or otherwise under any claim of any nature whatsoever in any jurisdiction) in any way arising from or connected with this report and the use, inability to use or the results of use of this report, and any reliance on this report.

Table of Contents

1. Introduction    
2. Executive Summary    
3. Audit Details    
3.1 Scope    
3.1.1 therentalstoken    
3.2  Methodology    
3.2.1 Dynamic Analysis    
3.2.2 Automated Analysis    
3.2.3 Code Review    
3.3  Risk Ratings    
4. Design Specification    
4.1 The Rentals Token    
4.2 Crowdsale   
5. Detailed Findings    
5.1 High Risk    
5.2 Medium Risk
5.3 Low Risk
5.3.1 Refunded Tokens Are Locked       
5.3.2 Specification Discrepancies   
5.3.3 Potential To Bypass Escrow Limits
5.4 Informational    
5.4.1 Design Comments    
5.5 Closed    
5.5.1 transferOrigin Introduces Potential Phishing-Style Attack (High)    
5.5.2 Able to Withdraw Funds from Escrow without Reaching Soft Limit (High)    
5.5.3 Escrow Incorrectly Tracks Remaining Balances (High)    
5.5.4 Potential Race Condition (Medium) 
5.5.5 Specification Discrepancies (Low)
5.5.6 Design Comments (Informational)    

1. Introduction

iosiro was commissioned by LaComunity to conduct an audit on their ERC-20 The Rentals Token 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: A high-level description of the findings of the audit.
  • Section 3 - Audit Details: A description of the scope and methodology of the audit.
  • Section 4 - Design Specification: An outline of the intended functionality of the smart contracts.
  • Section 5 - Detailed Findings:  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.

2. Executive Summary

This report presents the findings of an audit performed by iosiro on The Rentals 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.

3. Audit Details

3.1 Scope

The source code considered in-scope for the assessment is described below. Code from any other files is considered to be out-of-scope.

3.1.1 therentalstoken

Project: TokenSale

Link: 6ffde1b

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. No tests were included in the project by the team.

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 from solc and linters were used to identify potential 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.

4. Design Specification

The following section outlines the intended functionality of the smart contracts.

4.1 The Rentals Token

The Rentals Token (TRT) token specification is given below.

ERC-20 Standard
The token should implement the ERC-20 standard. The following features should be used:

  • Token Name: The Rentals Token
  • Token Symbol: TRT
  • 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 specification is given 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 introduced the ability to have more than 3 ICO rounds. Information on additional rounds was not available at the time of the review.

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:

distribution

Caps
The following shows should be followed:

Screen Shot 2018-06-12 at 17.10.50.png

Update: The requirement for a 2nd Tier softcap was removed by the team and implemented in 33148e8.

Discounts
The following discounts should be given:

  • Presale: 45% 
  • Tier 1:  20%
  • Tier 2:  0%

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 open at the conclusion of the review.

5.2 Medium Risk

No medium risk issues were open at the conclusion of the review.

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

Description

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 release schedule is given below.

issuing-schedule

The specification did not mention when tokens should be released to the addresses at each stage, however, the table appears to suggest that a specific amount of tokens should be released during phase.

5.3.3 Potential To Bypass Escrow Limits

Escrow.sol

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 description outlines actions to improve the performance, 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 address 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 is removed. Depending on the desired functionality, alternative standards, such as ERC-223 or ERC-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, ffc7bcf, and 1e66bc3. 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.

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.

5.5.4 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.

Update: Addressed in 88334eb. 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 2a635f68c6443b, and 3f7dd7f.

5.5.6 Design Comments (Informational)

General

The following description outlines actions to improve the performance, functionality, and readability of the codebase.

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. 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.

Update SafeMath
A slight modification in the way multiplication is performed in SafeMath results in reduced gas costs. It is recommended that a newer version of SafeMath is used to incorporate this change.

Update: Addressed in 769929d.

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.

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.

Update: Addressed in 4a0dcfd.

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.

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.