This is the report from a security audit performed on ETC TokenMint by yuriy77k.
TokenMint is a service that offers to launch an ICO and deploy a custom token contract easily.
Smart-contracts at this repo: https://github.com/ethereumproject/TokenMint/tree/master/contracts
- https://github.com/ethereumproject/TokenMint/blob/master/contracts/metaTokenmint.sol (commit hash: d6409c1)
- https://github.com/ethereumproject/TokenMint/blob/master/contracts/registry.sol (commit hash: 32a7f1a)
- https://github.com/ethereumproject/TokenMint/blob/master/contracts/deployed/crowdsale.sol (commit hash: 1fe1f58)
- https://github.com/ethereumproject/TokenMint/blob/master/contracts/deployed/icoMachine.sol (commit hash: 035c918)
- https://github.com/ethereumproject/TokenMint/blob/master/contracts/deployed/manReg.sol (commit hash: 0d8d839)
- https://github.com/ethereumproject/TokenMint/blob/master/contracts/deployed/ERC223.sol
- https://github.com/ethereumproject/TokenMint/blob/master/contracts/deployed/safeMath.sol
In total, 17 issues were reported including:
-
6 high severity issues.
-
6 medium severity issues.
-
3 low severity issues.
-
2 minor observation.
Issues for metaTokenmint.sol.
The modifier onlyAdmin use an uninitialized variable aCount to enumerate adminList. In this case it always exit without execution a function code.
Change the implementation of onlyAdmin modifier.
This condition run the function code all time when caller in not admin.
In case of the issue 1 this issue does not work, but it will if fix the issue 1.
Change the implementation to if (adminCheck == 1){_;} and move it out of for() {} body.
The function modAdmin not correct add new admin after deleting one.
For example, there are 2 admins: adminList[1].adminAddr = 1..., adminList[2].adminAddr = 2..., adminCount = 2.
After deleting first admin, we get: adminList[2].adminAddr = 2..., adminCount = 1.
And now if add new admin with address 3..., we get: adminList[2].adminAddr = 3..., adminCount = 2.
So admin 2 was replaced with admin 3, and we get only one active admin in the list instead of two.
Change the implementation of modAdmin function. Notice, that delete just remove records from storage, but not make reindexing.
Issues for registry.sol.
The definitions of struct Token of registry.sol contract made the Remix compiler unable of generating bytecode of the contract.
This is not a security issue, but a code flaw. This can not affect a deployed contract, but this made compiler behave in unintended way. This hurts contract's reusability property as well.
Change the implementation of registry.sol.
Modifier onlyOwner run the function code if caller is not owner.
Change the implementation of condition to: if (msg.sender == owner) {
or change the implementation of whole modifier onlyOwner to this one.
The function register has the modifier costs which works in other way, as planned. It run the function code when price was not paid.
Change the implementation of condition in the modifier costs to:
if (msg.value < price)
throw;
_;
The function modAdmin not correct add new admin after deleting one.
This issue similar with issue 3.
Change the implementation of modAdmin function. Notice, that delete just remove records from storage, but not make reindexing.
The function modAdmin allow contract owner to overflow adminCount variable when adding more then 256 admins.
Change type of variable adminCount from uint8 to uint and in linked places (mapping and the variable i.
This condition run function code when caller in not admin.
Change the implementation to if (adminCheck) {.
Issues for crowdsale.sol.
The function safeWithdrawal alow an user to make withdraw his payment when funding goal is not reached.
When user buy tokens, his original payment value stored in the array balanceOf. But when same user make more then one payment, the next payment replace previous value in that array (in functions () and buyTokens). So when user make withdraw, he withdrow only last payment, but not all the amount that he paid.
Change the implementation in the function () to balanceOf[msg.sender] += amount; and in the function buyTokens to balanceOf[buyer] += amount;.
If an user call function safeWithdrawal when funding goal is not reached, he received back his payment, but bought tokens remain in his account.
Implement the return of tokens from the user to the crowdsale address.
When funding goal reached, the beneficiary will not receive money if someone has made a withdrawal before, because the value of variable amountRaised will be bigger then current contract balance.
Implement in the function safeWithdrawal decrease value of variable amountRaised on withdrawal amount. For example, add amountRaised -= amount; after this row and add amountRaised += amount; after this row.
Issues for icoMachine.sol.
If the token creator call the function createSale more then one time, it replace original crowdsale address in the Ico struct to the new crowdsale address with no tokens on it (all tokens moved only on the first crowdsale address).
Implement checking that crowdsale address was added. For example, add assert(tokens[msg.sender][index].saleAddress == 0x0); after this string.
Issues for manReg.sol.
The variable choice unused in this project. This is not a security issue, but a code flaw. This can not affect a deployed contract.
Delete this variable.
The function modAdmin not correct add the new admin after deleting one.
This issue similar with issue 3.
Change the implementation of modAdmin function. Notice, that delete just remove records from storage, but not make reindexing.
The function modAdmin allow to the contract owner to overflow adminCount variable when adding more then 256 admins.
Change type of the variable adminCount from uint8 to uint and argument uint8 _index to uint _index.
The modifier onlyAdmin use an uninitialized variable aCount to enumerate adminList. In this case it always exit without execution the function code.
Also if (adminCheck != 1){throw; _;} conditon never run function code.
Change the implementation of the onlyAdmin modifier.
This project has the several identical contracts (owned, priced) and I would recommend to write them in a separate .sol file and import it where necessary.
The functions in the files metaTokenmint.sol , registry.sol, manReg.sol are very similar, and I would recommend merging them into the one contract. Also pay attention to the variable price (registry.sol and manReg.sol). In the comments it is stated that the price is in ethers, although in the contract it is used as the price in the Wei. This can cause misunderstanding when the contract is used.
6 critical vulnerabilities were detected.
Smart-contract must be fixed before launch.