Disclaimer
Single code audit does not guarantee its security. We recommend proceeding with several independent audits and a public bug bounty program to ensure smart contract security. A security audit should not be regarded as investment advice.
Introduction
General Provisions
The LTO Network team asked MixBytes Team to audit their token sale contracts. The code is located in the hidden GitHub repository.
The scope of the Audit
The primary scope of the audit is smart contracts at https://github.com/legalthings/tokensale/tree/02fa2620aef4c854675230b6544461961d47f968/contracts. Migrations at https://github.com/legalthings/tokensale/tree/02fa2620aef4c854675230b6544461961d47f968/migrationswere also included in the scope as they contain mission critical deployment and configuration code which ties the system together. Audited commit is 02fa2620aef4c854675230b6544461961d47f968.
Subsequently, we were also asked to audit the revised token at https://github.com/legalthings/tokensale/blob/98ba921251fb4989d18f8b99a2bfc732f2056937/contracts/LTOToken.sol.
Security Assessment Principles
Classification of Issues
- CRITICAL: Bugs that enable theft of ether/tokens, lock access to funds without a possibility to restore it, or lead to any other loss of ether/tokens to be transferred to any party (for example, dividends).
- MAJOR: Bugs that can trigger a contract failure, with further recovery only possible through manual modification of the contract state or contract replacement altogether.
- WARNINGS: Bugs that can break the intended contract logic or enable a DoS attack on the contract.
- COMMENTS: All other issues and recommendations.
Security Assessment Methodology
The audit was performed with triple redundancy by three auditors.
Stages of the audit were as follows:
- “Blind” manual check of the code and model behind the code
- “Guided” manual check of the code
- Check of math balance
- Check of adherence of the code to requirements of the client
- Automated security analysis using internal solidity security checker
- Automated security analysis using public analyzers
- Manual by-checklist inspection of the system
- Discussion and merge of independent audit results
- Report execution
Detected Issues
CRITICAL
None found
MAJOR
None found
WARNING
Generation of two events in the form emit Transfer(address(0), receiver address, amount); should be added. Otherwise, some client software won’t recognize the arrival of the tokens to msg.senderand _bridgeAddressrecipients.
Fixed in 686101d
The recipient address specified in the Transferevent does not match the factual address of the recipient (which can be seen here). In case this was made on purpose we should note that it’ll hinder diagnosing of the contract operation. We recommend to do the actual transfer using super.transferfunction.
Fixed in 686101d
In case _intermediate is being used at the moment by some user addIntermediateAddress call will cause all token transfers to this user to fail to reach them. It’ll be difficult to diagnose this situation because of the problem with the Transferevent described above. Access to the addIntermediateAddressfunction is limited to the address stored in the bridgeAddressfield, but for the moment, code that will be used is unavailable to the audit. It’s a common security approach to assume that the problem described above will take place, accidentally or as a result of a more complex attack vector.
A minimal, but not complete, the solution is to add a check that the token balance of _intermediate address is zero at the moment of addIntermediateAddress call. A definitive solution can be designed only with the bridge mechanics in mind.
Fixed in 686101d
Obviously, totalSupply_ is being maintained as a difference between the total amount of tokens ( internalTotalSupply) and amount of tokens in bridgeAddress possession. However, totalSupply_value won’t be properly updated in case of transferFrom function usage, which is inherited from the StandardToken.
Fixed in 686101d
There are mechanics which allows users to transfer tokens from some predefined addresses (enumerated in intermediateAddress) to the bridge balance. However, mechanics can be bypassed, and tokens can be transferred, to an address. This applies even if this address is listed in intermediate addresses in case of transferFrom function usage, which is inherited from the StandardToken.
Fixed in 686101d
Users sending ether from contracts which default function performs nontrivial computations (e.g. writing value to a new storage variable) won’t be able to receive their tokens, while the project won’t be able to receive transferred ether. This is caused by limited gas stipend of the transferfunction.
It’s recommended to use withdrawal pattern to send change and to untie sending of change from other actions. I.e. we recommend offering change to users during withdrawalor withdrawalFor transaction processing via sendor, failing that, with a separate transaction via a dedicated function call.
Fixed in PR #18
Malicious contract (or contracts which default function performs nontrivial computations) can block a change to it and as a result any transaction which calls _withdawalfor this contract will be reverted. Therefore withdrawalwill never reach purchaserList.length and the clearfunction will be blocked, which will result in blocking some tokens and ether on the balance of LTOTokenSale. The ether vulnerable to this attack includes only ether deposited to the contract bypassing buy() and default functions, which is why it’s not a serious problem. It is worth noting that tokens transferred to the contract by creators exceeding totalSaleAmountare vulnerable to the attack.
It’s recommended using withdrawal pattern to send change and to untie sending of change from other actions. I.e. we recommend to send change to users during withdrawalor withdrawalFor transaction processing via sendor, failing that, with a separate transaction via a dedicated function call.
Fixed in PR #14
Payments are not expected to be sent to this function. We, therefore, recommend removing the payable keyword to prevent accidental ether transfer to the contract which won’t be processed by the token sale logic.
Fixed in PR #14
Payments are not expected to be sent to this function. We, therefore, recommend removing payable keyword to prevent accidental ether transfer to the contract which won’t be processed by the token sale logic.
Fixed in PR #14
Payments are not expected to be sent to this function. We, therefore, recommend removing payable keyword to prevent accidental ether transfer to the contract which won’t be processed by the token sale logic.
Fixed in PR #14
An attacker can send some value of ether in the first transaction, receive a maximum bonus and then send 99 transactions of minimal value minimumAmount, depriving other token sale participants of their bonuses. Essentially this is griefing of a transaction counter. This makes economic sense given some conditions, because the fewer participants receive their bonuses, the lower the price is to the attacker compared to the average price, making the following sale more profitable for the attacker.
Acknowledged
Individual cap mechanics are subject to Sybil attack. A user can distribute his N ethers to unit(N / maximumCapAmount) + 1 distinct addresses, and perform purchase from each of these addresses, effectively bypassing maximumCapAmountlimit.
Acknowledged
13. The burning of unsold tokens was mentioned by the client. However, in the contracts code, there is no sign of such mechanics and LTOTokenis not burnable.
Fixed in PR #16
14. LTOToken.sol#L19
Generation of an event in the form emit Transfer(address(0), _bridgeAddress, _bridgeSupply) ; should be added. Otherwise, some client software won’t recognize the arrival of the tokens to the _bridgeAddress.
Fixed in PR #18
COMMENT
The only value besides 0, which a key X can have in the intermediateAddreesmapping, is the Х itself. It means that logically this mapping has boolean value type, i.e. mapping (address => bool). We recommend using this type explicitly and making appropriate code changes to simplify reasoning about the code.
We recommend using SafeMath.sub here because this code fragment has a high probability of underflow error.
The only value besides 0, which a key X can have in the capFreeAddressesmapping, is the Х itself. It means that logically this mapping has boolean value type, i.e. mapping (address => bool). We recommend using this type explicitly and making appropriate code changes to simplify reasoning about the code.
This branch is unachievable in practice (it requires ~5e14 ether to achieve), we recommend replacing it with assert(false);.
The expression totalWannaBuyAmount == 0here is unachievable, we recommend removing it from if-operator and introduce assert(totalWannaBuyAmount > 0); instead.
In startSale function, it would be helpful to ensure that the token balance of the contract is equal to totalSaleAmount. Otherwise, if the token balance is less than totalSaleAmount some ether will be locked in the contract forever.
Right after deployment of the contract isEndedfunction will return true. This won’t cause any troubles in this version of the code, but it’s better to return false in case the sale was never started (similar check is present in isStarted).
The same holds for isUserWithdrawalTime, isClearTime functions.
It’s better to use the ethDecimalsconstant, instead of the 1 ethervalue to prevent possible inconsistencies.
Condition <= is excessive and can be replaced with a strict comparison ( <).
Keeping a purchasers list in purchaserListand further processing in withdrawalFor incurs a high gas consumption. At this moment, calling withdrawalFor is required to get all possible ether from the contract and to send tokens and ether to purchasers. These two processes can be separated and gas spendings by the project can be minimized. Amount of ether to be withdrawn by the project can be calculated based on globalAmount, totalSaleAmount and TotalWannaBuyAmount and withdrawn in one transaction.
We recommend creating the explicit view function isSaleInitializedand use it here to prevent double initialization, as well as throughout isStarted, isEndedand other state querying functions
We recommend checking the result of token transfer call.
There are a lot of states and state transitions involved in the token sale contract, e.g. sale started, sale ended, user withdrawal time etc. We recommend using state machine to remove some code complexity and ensure correct operation, as can be seen in this example.
13. LTOToken.sol#L35
Tokens which were accidentally sent to the bridge address could not be recovered. We recommend adding require(to != bridgeAddress); check.
14. LTOToken.sol#L38
If value is expected to be greater than bridgeBalancein some cases we recommend adding check require(value <= bridgeBalance);. The reason is that assertinside the bridgeBalance.subshould never be reached during an expected path of execution of the code.
15. LTOToken.sol#L56
The sum of balances is not equal to totalSupply. This behavior could be unexpected for some tools or clients.
CONCLUSION
The overall security level of the system was rated “High”. No major flaws were found. However, there were many issues about which we warned the client. Some of them can be accepted as a known expected behavior, but some, in our opinion, required fixes, e.g., ones related to withdrawal functions. All necessary changes were made and existed in pool request #19 branch.
At the time of writing, LTO Network has successfully finished token sale using the audited version of the contracts, raising $4 million.
The audit was performed by MixBytes team. In case you need a security audit for your project please submit an application at the official website or via Telegram.
