Introduction
Coinfabrik has been hired to audit the INLOCK token sale smart contracts.
We started this audit report writing a summary of the smart contracts provided by the client and a list of the analysis methods used to audit the contracts. Next, we detailed our findings ordering the issues by severity, followed by all the observations we considered important to add. We ended this audit with a conclusion explaining how do the auditors value the code reviewed, and what are the most important things that need to be corrected to it to make it work flawlessly and securely.
Summary
The contracts audited have been taken from the INLOCK repository at https://github.com/IncomeLocker/ilktoken-contract. The audit is based on the commit 8b76c6f03b5efdc3e80f362efae5b016f1fa5c9d, and updated to reflect changes at commit f12a931d29b6d8659ecd01ccbe52050d2aa420ee.
Both the token and the ICO use code delegation, which means the code for every call gets delegated to another contract that can be replaced in the future by the INLOCK team. This allows to fix and/or upgrade the contracts if needed but ultimately it requires trust on the INLOCK team.
The audited contracts are these:
-
- contract/ico.sol: Proxy contract for the ICO
-
- contract/icoLib.sol: Code for the ICO accessed from its proxy using delegate.
-
- contract/multiOwnerWallet.sol: A multiple owner wallet requiring voting for actions.
-
- contract/owned.sol: Contract ownership privilege call modifier implementation.
-
- contract/safeMath.sol: Overflow checked integer operations.
-
- contract/token.sol: Proxy contract for the Token
-
- contract/tokenLib.sol: Code for the token accessed from its proxy using delegate.
- contract/tokenDB.sol: Storage for the token accessed using calls.
The following analyses were performed:
-
- Misuse of the different call methods: call.value(), send() and transfer().
-
- Integer rounding errors, overflow, underflow and related usage of SafeMath functions.
-
- Old compiler version pragmas.
-
- Race conditions such as reentrancy attacks or front running.
-
- Misuse of block timestamps, assuming anything other than them being strictly increasing.
-
- Contract softlocking attacks (DoS).
-
- Potential gas cost of functions being over the gas limit.
-
- Missing function qualifiers and their misuse.
-
- Fallback functions with a higher gas cost than the one that a transfer or send call allows.
-
- Fraudulent or erroneous code.
-
- Code and contract interaction complexity.
-
- Wrong or missing error handling.
-
- Overuse of transfers in a single transaction instead of using withdrawal patterns.
- Insufficient analysis of the function input requirements.
File summary
SafeMath.sol
No vulnerabilities were found. This is a common straightforward implementation of overflow checked integer operations.
Owned.sol
No vulnerabilities were found. This is a very simple straightforward implementation of contract ownership.
multiOwnerWallet.sol
Two minor vulnerabilities were found, see the “Detailed findings” section for details. The wallet has a voting system in which certain actions can be performed by accumulating votes. In this system, for each specific call data received by the contract you can have an action, and this action accumulates votes if said call data is received by the contract by multiple owners. This includes adding and removing owners.
ico.sol
A medium vulnerability was found, see the “Detailed findings” section for details. This is a proxy contract for the ICO. It has all the entry points and storage of the ICO, but the actual code is delegated to the contract icoLib, which can be changed after deployment.
icoLib.sol
No vulnerabilities were found. This is the code for the ICO. It’s an ICO with vesting and multiple phases each having different rewards. It also has a variable rate for rewards that it is set by an external entity. All phases require whitelisting for buying (KYC).
token.sol
A medium vulnerability was found, see the “Detailed findings” section for details. This is a proxy contract for the ERC20 token. It has all the entry points of the token, but the actual code is delegated to the contract tokenLib, which can be changed after deployment.
tokenLib.sol
No vulnerabilities were found. This is the code for the token, it delegates the main storage to the contract tokenDB which gets called every time a transaction or approval needs to be made. It’s a straightforward implementation of an ERC20 token, and also has a function for bulk transfers.
tokenDB.sol
No vulnerabilities were found. This is the storage for the token, it has all the balance and approval data from users. It also supports upgrading to a newer contract, and it automatically restores the balance for each party each time a transaction is made, even back to the first iteration.
Detailed findings
Critical severity
No critical severity issues were found.
Medium severity
Invalid address load in inline assembly
All delegated functions at the proxy contracts use the following scheme:
Function approve(address _spender, uint256 _value) external returns (bool _success) {
address _trg = libAddress;
assembly {
let m := mload(0x20)
calldatacopy(m, 0, calldatasize)
let success := delegatecall(gas, _trg, m, calldatasize, m, 0x20)
switch success case 0 {
revert(0, 0)
} default {
return(m, 0x20)
}
}
}
However, this scheme incurs in an error: 0x20 isn’t a valid documented address to take a memory pointer from. The solidity memory map is described here, specifically, the 0x20 address is part of the scratch space, which means a valid address will not reside there. This will likely load a 0, which will end up overwriting the reserved solidity memory with the call data and then the return data. The resulting behavior can vary from working just fine to being vulnerable to a full attack depending on the compiler generated code.
The proper address to load from is 0x40, which is documented there as a free memory pointer, in other words, memory not reserved by solidity. We recommend changing the load address to 0x40, as it is documented by solidity.
This has been fixed in commit d1aacc9702cd424fad1a3185735d68ac9601c2fe
Minor severity
Owner of Multi-owned wallet can deny service
If any owner initiates a vote they are the only one who may revoke it. This means an owner may deny any action by simple revoking and initiating said vote repeatedly, as long as they keep getting the ownership of the vote the action could get interrupted forever. If revoking is not as desirable with this issue in mind removing it will fix the issue, otherwise, it may need a redesign.
This was fixed in commit f12a931d29b6d8659ecd01ccbe52050d2aa420ee
Owner of Multi-owned wallet can create locked actions
Since saving the voters for each action requires a mapping, it cannot be deleted, which potentially means the same action cannot be done twice. In this case, the developers solved this by saving the block number in each action when it is created as a form of a fingerprint. Each vote will have this block number assigned, if owners want to vote for a second action, the only requirement is for the action to have a different block number than the one they have saved.
function doVote(bytes32 _hash) internal returns (bool _voted) {
require( owners[msg.sender] );
if ( actions[_hash].origin == 0x00 ) {
actions[_hash].origin = msg.sender;
actions[_hash].voteCounter = 1;
actions[_hash].startBlock = block.number;
} else if ( ( actions[_hash].voters[msg.sender] != actions[_hash].startBlock ) && actions[_hash].origin != msg.sender ) {
actions[_hash].voters[msg.sender] = actions[_hash].startBlock;
actions[_hash].voteCounter = actions[_hash].voteCounter.add(1);
emit vote(_hash, msg.sender);
}
if (actions[_hash].voteCounter.mul(100).div(ownerCounter) >= actionVotedRate) {
_voted = true;
emit votedAction(_hash);
delete actions[_hash];
}
}
function revokeAction(bytes32 _hash) internal {
require( actions[_hash].origin == msg.sender );
delete actions[_hash];
emit revokedAction(_hash);
}
There is a problem with this, each block can have multiple transactions which will share the same block number. An evil owner then may create these transactions in order at the same block:
-
- the transaction which creates an action.
-
- N vote transactions for said action: The victims and/or complices of the attack.
-
- A transaction which revokes or executes the action which causes its deletion.
- A transaction which creates the same action as 1.
If this happens, since the new action at 4 shares the same block number with the first action at 1 it cannot be voted for again by the same owners who voted in the previous action. If the remaining voters are less than the required for an action to be executed, the action cannot be ever done again unless the owner that proposed the action revokes it. It can also be used to restrain an owner from voting certain actions, by creating the same action before the owner votes for it and then revoking it, all in the same block. Note that this can also happen accidentally without malice with a small number of voters.
This attack is obviously very expensive but it is possible to realize. One way of removing this attack is to have a counter each time a new action is created, and using that counter as an ID instead of block numbers.
This was fixed in commit a0cead290dc933b16dabb0696d7a84df1c957d9c
Enhancements
Decouple ICO from the Token
Right now the token needs the ICO to know when a user is allowed to transfer:
Function allowTransfer(address _owner) public view returns (bool _success, bool _allow) {
return ( true, _owner == address(this) || transferRight[_owner] || currentPhase == phaseType.preFinish || currentPhase == phaseType.finish );
}
This is not needed on the ICO as all this information could be stored in the token itself. If the owner is shared between the ICO and the token it is already in the token storage, otherwise a separate one could be added. The transferRight mapping can be saved on the token, along with its setter function. The currentPhase conditions can be replaced by a boolean value at the token that gets set by the ICO once the preFinish phase starts.
This would ensure that the token does not depend on the ICO once it is finished, it reduces gas costs on transactions as no call is needed and simplifies deployment as no ICO address needs to be set in the contract.
This was acknowledged by the INLOCK team
Conclusion
While the contracts had an inline assembly for delegation, there are not that many lines of code which always helps to audit. The developers also have a good enough understanding of Solidity, as evidenced by the voting code. The contracts also used up to date features and practices, which isn’t a minor thing considering the fast pace at which solidity improves.
Disclaimer: This audit report is not a security warranty, investment advice, or an approval of the Inlock project since Coinfabrik has not reviewed its platform. Moreover, it does not provide a smart contract code faultlessness guarantee.
Buenos Aires, Argentina @2018 by Coinfabrik technologies
