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.
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.
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.
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.
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.
The source code considered in-scope for the assessment is described below. Code from any other files are considered to be out-of-scope.
Project Name: TokenSale
A variety of techniques were used to perform the audit, these are outlined below.
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.
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.
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.
Each Issue identified during the audit is assigned a risk rating. The rating is dependent on the criteria outlined below..
The following section outlines the intended functionality of the smart contracts.
The token functionality is described below.
The token implements the ERC20 standard.
|Name||The Rentals Token|
It should be possible for the token contract owner to enable and disable transfers.
All of the tokens should be held in an escrow contract initially.
The crowdsale functionality is described below.
It should be possible to have multiple rounds, namely:
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.
Each round has three stages these are described below.
A minimum contribution limit should be set for each round of the crowdsale.
A time lock is set for tokens, freezing token transfers until the timelock passes.
If the round softcap is not reached, the round should become refundable, allowing contributors to withdraw their funds.
Only participants who have been whitelisted should be able to contribute to the ICO.
The exchange rate should be set to 7500 TRT per ETH.
Funds should be released equally to three separate wallets upon successful completion of a round.
While the specification did not specify the minimum round contributions, the following values were found:
The following distribution table should be adhered to:
|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)|
The following shows should be followed::
|Round||Softcap (ETH)||Hardcap (ETH)|
Update: The requirement for a 2nd Tier softcap was removed by the team and implemented in 33148e8.
The following discounts should be given:
The following section includes in depth descriptions of the findings of the audit.
No high risk issues were present at the conclusion of the audit.
No medium risk issues were present at the conclusion of the audit.
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.
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.
A number of discrepancies between the specification and the implementation were identified. These are outlined below.
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.
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.
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.
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.
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.
The following describes possible actions to improve the functionality and readability of the codebase.
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.
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.
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.
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 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:
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.
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.
tokens.sol: line 139-149
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.
Malicious actor (MA) deposits ether into an exchange.
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.
When MA asks to withdraw their ether from the exchange, the exchange sends ether to the contract, calling the fallback function and triggering
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.
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.
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.
It is recommended that the following line is added to
require(address(this).balance == 0);
This would only allow updates until the stage that the RefundVault started to receive funds.
Update: Addressed in f0caecb.
escrow.sol: line 47-53
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.
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.
tokens.sol: line 121-126
A potential race condition exists in the current implementation of the
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.
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
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.
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.
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
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.
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
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: Addressed in 769929d.
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
0x0 to indicate that the tokens are being created, rather than transferred.
Update: Addressed in 769929d.
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 can lead to wasted gas and increased costs.
Update: Addressed in 9db3db6.
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.
Interested in getting your smart contracts audited? Request a Quote
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.