Luckchemy Smart Contract Audit
Luckchemy engaged New Alchemy to audit the smart contracts that implement their crowd sale and associated Luckchemy token (LUK). The engagement was technical in nature and focused on identifying security flaws in the design and implementation of the contracts. This included finding differences between the contracts' implementation and their behaviour as described in public documentation, as well as discovering any issues with the contracts that may impact their trustworthiness.
New Alchemy performed the audit over three days in April 2018. This document describes the issues discovered in the audit. An initial version of this document was provided to Luckchemy, who made various changes to their contracts based on New Alchemy’s findings; this document was subsequently updated to reflect the changes.
Files Audited
The code reviewed by New Alchemy was provided by Luckchemy using Google drive. New Alchemy’s audit was additionally guided by the Luckchemy whitepaper.
Re-test notes: the code audited during the re-test is found in https://github.com/Luckchemy/Luckchemy, at commit hash fe8380c3e65251bf1bb90ea880fa5852893b9d0d.
New Alchemy’s review of the Luckchemy contracts did not uncover any issues of critical severity.
The audit identified a variety of issues with impacts ranging from very minor to moderate. The most significant findings address concerns related to the level of trust that users must place in the contract owner when investing.
General Discussion
The Luckchemy contracts implement a token sale for the LUK token.
The code uses Solidity version 0.4.15 and 0.4.19, both of which are older than the recommended latest version at the time of audit, which is 0.4.21. Luckchemy should use the latest version of Solidity in order to take advantage of the latest features and improvements that Solidity has to offer.
The code uses the well-known OpenZeppelin library to implement much of the critical token functionality, utilizing the StandardToken and BurnableToken ERC20 contracts.
The code places a large amount of trust in the Service Agent address, allowing it to purchase arbitrary amounts of LUKtokens, with no ETH cost. This was done in order to facilitate Bitcoin purchases off-chain, but represents a large risk to potential investors.
Re-test results: As of the re-test, Luckchemy has updated the contracts to 0.4.21. This was the most recent version at the time of testing, however 0.4.22 was released during the re-test.
Moderate Issues
Fixed: Token sale hard cap is not defined
The Luckchemy whitepaper calls for a ‘hard cap’ of 22,000 ETH sold, at which the sale will end. No code in the Luckchemy contracts explicitly enforces this cap, though there is code that enforces a maximum crowdsale supply, of 700000000 tokens.
The Luckchemy contracts should explicitly enforce a hard cap for the crowdsale, as described in the whitepaper.
Re-test results: As of the latest revision of the contracts provided to New Alchemy, a hard cap has been defined and enforced in the smart contracts. The white paper and contracts now descibe and implement the same exact following values:
- hard cap: 45360 ether
- soft cap: 2000 ether
- Conversion rate: 1 ether = 12500 LUK
- private presale token cap : 70000000 tokens
- pre sale token cap: 105000000 tokens
- crowdsale stage 1 token cap : 175000000 tokens
- crowdsale stage 2 token cap: 350000000 tokens
- private pre sale discount: 80% (this value is not indicated in the white paper)
- pre sale discount: 40% (this value is named differently in the contract but effectively implements the discount as described in the white paper)
- crowdsale stage 1 discount: 20% (this value is named differently in the contract but effectively implements the discount as described in the white paper)
- crowdsale stage 2 discount: 0% (this value is named differently in the contract but effectively implements value as described in the white paper)
Not-Fixed: “Service Agent” can buy arbitrary amounts of LUK for free
The LuckchemyCrowdSale contract defines an onlyServiceAgent modifier. This modifier is used in payFiat, a function which can be used to transfer an arbitrary amount of tokens from the current pool to a specified beneficiary. This function requires no Ethereum tokens to be sent, so the contract is effectively trusting the service agent with the ability to buy an arbitrary amount of tokens from the crowd sale at no cost. This behavior could be used to manipulate the supply of tokens, end the token sale early, exhaust all the tokens, or change the stage of the sale.
Re-test results: The Service Agent is still given access to the payFiat function.
Minor Issues
Fixed: Token is not ERC223
The Luckchemy whitepaper calls for an ERC223 token, however, the contracts provided for audit implement an ERC20 token. Luckchemy should update the contracts to implement the ERC223 token.
Re-test results: While the token remains an ERC20 token, Luckchemy changed its whitepaper to accurately describe the LUK token as being ERC20 compliant. This effectively fixes this contract/documentation discrepancy issue.
Not-Fixed: Crowdsale logic depends on Ethereum block timestamp
The logic for determining the stage of the token sale and whether the sale has ended is made by using now, an alias for block.timestamp. Per the Ethereum technical specification, this value can be manipulated by miners up to 900 seconds per block. While unlikely, it is possible that a miner could maliciously alter the behavior of the token sale by altering the block timestamp. See https://github.com/ethereum/wiki/wiki/Safety#timestamp-dependence for more information.
To fix this issue, consider defining the duration for the token sale in terms of block height, instead of timestamp values.
Re-test results: Luckchemy opted not to address this issue, due to its minor impact and the complexity of determining sale boundaries using block numbers.
Fixed: Wei refunds at the end of stage are slightly too low due to truncation
For each stage, if the user attempts to buy more tokens than are available in that stage, they will be refunded the amount of Wei for the tokens which they attempted to purchase but did not purchase successfully. The logic that carries out this behavior is slightly flawed, and transfers an amount lower than the actual amount, due to order of operations and multiple remainder truncation. This affects both processPrivatePurchase and processPublicPurchase. The affected code is below:
// example taken from processPrivatePurchase. Logic is similar in processPublicPurchase.
// snip...
//pool doesn't have enough tokens
uint leftTokens = tokensToBuy.sub(tokenPools[stage]);
//left wei
uint leftWei = leftTokens.mul(1 ether).div(stageRates[stage]);
uint usedWei = weiAmount.sub(leftWei);
tokensSold = tokensSold.add(tokenPools[stage]);
tokenPools[stage] = 0;
deposits[beneficiary] = deposits[beneficiary].add(usedWei);
//change stage to Public Sale
currentStage = Stage.Discount40; // tokensToBuy - leftTokens == tokens in pool
token.transfer(beneficiary, tokensToBuy.sub(leftTokens));
TokenETHPurchase(msg.sender, beneficiary, usedWei, tokensToBuy.sub(leftTokens));
//return left wei to beneficiary and change stage
beneficiary.transfer(leftWei);
Here we can see the leftWei is calculated by recomputing the values from tokensToBuy, a value which has already potentially had remainder truncation occur. The code could be changed to the following to minimize truncation issues:
//pool doesn't have enough tokens
tokensToBuy = tokenPools[stage];
//left wei
uint256 usedWei = (tokensToBuy.mul(1 ether)).div(stageRates[stage]);
uint256 leftWei = weiAmount.sub(usedWei);
tokensSold = tokensSold.add(tokenPools[stage]);
tokenPools[stage] = 0;
deposits[beneficiary] = deposits[beneficiary].add(usedWei);
//change stage to Public Sale
currentStage = Stage.Discount40; // tokensToBuy - leftTokens == tokens in pool
token.transfer(beneficiary, tokensToBuy);
TokenETHPurchase(msg.sender, beneficiary, usedWei, tokensToBuy);
//return left wei to beneficiary and change stage
beneficiary.transfer(leftWei);
Re-test results: This issue has been fixed.
Fixed: Inconsistent integer types
The Luckchemy contracts use uint and uint256 interchangably. For consistency and safety, uint256 should be solely used, matching the underlying OpenZeppelin ERC20 interfaces.
Re-test results: This issue has been fixed.
Not-Fixed: Short-address attack protections
Some Ethereum clients may create malformed messages if a user is persuaded to call a method on a contract with an address that is not a full 20 bytes long. In such a “short-address attack”, an attacker generates an address whose last byte is 0x00, then sends the first 19 bytes of that address to a victim. When the victim makes a contract method call, it appends the 19-byte address to msg.data followed by a value. Since the high-order byte of the value is almost certainly 0x00, reading 20 bytes from the expected location of the address in msg.data will result in the correct address. However, the value is then left-shifted by one byte, effectively multiplying it by 256 and potentially causing the victim to transfer a much larger number of tokens than intended. msg.data will be one byte shorter than expected, but due to how the EVM works, reads past its end will just return 0x00.
This attack effects methods that transfer tokens to destination addresses, where the method parameters include a destination address followed immediately by a value. In the Luckchemy contracts, such methods include
LuckchemyToken.transferLuckchemyToken.transferFromLuckchemyToken.approveLuckchemyToken.increaseApprovalLuckchemyToken.decreaseApprovalLuckchemyCrowdsale.payFiat
While the root cause of this flaw is buggy serializers and how the EVM works, it can be easily mitigated in contracts. When called externally, an affected method should verify that msg.data.length is at least the minimum length of the method's expected arguments (for instance, msg.data.length for an external call to LuckchemyToken.transfer should be at least 68: 4 for the hash, 32 for the address (including 12 bytes of padding), and 32 for the value; some clients may add additional padding to the end). This can be implemented in a modifier. External calls can be detected in the following ways:
- Compare the first four bytes of
msg.dataagainst the method hash. If they don't match, then the call is internal and no short-address check is necessary. - Avoid creating
publicmethods that may be subject to short-address attacks; instead create onlyexternalmethods that check for short addresses as described above.publicmethods can be simulated by having the external methods callprivateorinternalmethods that perform the actual operations and that do not check for short-address attacks.
Whether or not it is appropriate for contracts to mitigate the short-address attack is a contentious issue among smart-contract developers. Many, including those behind the OpenZeppelin project, have explicitly chosen not to do so. While it is New Alchemy’s position that there is value in protecting users by incorporating low-cost mitigations into likely target functions, Luckchemy would not stand out from the community if they also choose not to do so.
Re-test results: Luckchemy opted not to address this issue, since OpenZeppelin has decided not to address it.
Fixed: Lack of two-phase ownership transfer
In contracts that inherit the common Ownable contract from the OpenZeppelin project^3, a contract has a single owner. That owner can unilaterally transfer ownership to a different address. However, if the owner of a contract makes a mistake in entering the address of an intended new owner, then the contract can become irrecoverably unowned.
In order to preclude this, New Alchemy recommends implementing two-phase ownership transfer. In this model, the original owner designates a new owner, but does not actually transfer ownership. The new owner then accepts ownership and completes the transfer. This can be implemented as follows:
contract Ownable {
address public owner;
address public newOwner
event OwnershipTransferred(address indexed oldOwner, address indexed newOwner);
function Ownable() public {
owner = msg.sender;
newOwner = address(0);
}
modifier onlyOwner() {
require(msg.sender == owner);
_;
}
function transferOwnership(address _newOwner) public onlyOwner {
require(address(0) != _newOwner);
newOwner = _newOwner;
}
function acceptOwnership() public {
require(msg.sender == newOwner);
OwnershipTransferred(owner, msg.sender);
owner = msg.sender;
newOwner = address(0);
}
}Re-test results: Luckchemy switched its LuckchemyToken so that it now inherits from the openZeppelin Claimablecontract which implements a safe two-phase ownership transfer system.
Line by line comments
This section lists comments on design decisions and code quality made by New Alchemy during the review. They are not known to represent security flaws.
LuckchemyCrowdSale.sol
Fixed: Lines 228–263: processPrivatePurchase
For a better defense-in-depth strategy, consider adding an additional require(currentStage == stage) to this function to prevent callers from buying from the private pool after the stage has incremented.
Re-test notes: This issue has been fixed.
Addressed: Lines 408–416: refund
Note that the logic defined here requires the contract owner call forwardFunds() within 7 days of the sale ending, otherwise depositors will be able to refund their deposits, even if the goal has been reached.
Re-test notes: The requirement has been switched to within 30 days of the sale ending.
Addressed: Lines 52–64, 178–186
Note that these multiplication and division operators are not done using SafeMath, and may result in truncated remainders.
Re-test notes: Luckchemy noted that these values are constant and they checked them for correctness.
LuckchemyToken.sol
Lines 111–121: addBlacklistItem and removeBlacklistItem
Though these functions and underlying mappings are defined, they are not used anywhere in either LuckchemyCrowdSale.sol or LuckchemyToken.sol
Re-test results: The client noted that although these functions and mappings are not used in the provided contracts, they are using them in external code.
Disclaimer
The audit makes no statements or warranties about utility of the code, safety of the code, suitability of the business model, regulatory regime for the business model, or any other statements about fitness of the contracts to purpose, or their bug-free status. The audit documentation is for discussion purposes only.
New Alchemy is a leading blockchain strategy and technology consulting group specializing in tokenization. One of the only companies to offer a full spectrum of guidance from tactical technical execution to high-level theoretical modeling, New Alchemy provides C-level strategy, smart contract development, project management, token design, marketing services, and security audits to the most innovative startups worldwide. Get in touch with us at Hello@NewAlchemy.io
