Owners can prevent withdrawals by halting + Several others

z-zap commented 7 years ago

Major Issue

Currently, your contract JincorTokenICO.sol has a function refund() that other accounts call to get a refund if the softcap is not met. They cannot call this function if the ICO is not ended (thanks to icoEnded). The function withdraw() callable by the owners cannot withdraw unless the ICO has reached its soft cap. Presumably this logic protects investors; they will always be able to withdraw if the softcap is not reached.

However, by setting halted to true, the owners can actually disable the refund() functionality and freeze the contents of the contract indefinitely if the softcap is not reached. Because they cannot themselves withdraw the funds if the softcap is not reached, there is no incentive to not do this. Investors trust owners to not call halt() to prevent withdrawals. This is a severe problem for trustworthiness. I would not consider investing in any ICO that had this loophole, even if I believed wholeheartedly in the trustworthiness of the team (I'm sure you guys are above board!)

The fix is simple. Make halt() and unhalt() require that the crowdsale be active, and reassure investors!

Medium Issues

The way your hardcap mechanism works simply requires (collected.add(msg.value) <= hardCap);. This is not ideal because if you have lets say 199 tokens left and someone tries to buy 200, their transaction will simply throw. The more complete way to implement a hardcap would be to only partially fill their transaction, with

if (collected.add(msg.value) <= hardCap); {
    uint tokens = hardCap.mul(price);
}

And appropriate surrounding adjustments. This can be fixed as per above in JincorTokenPreSale.sol, but a different fix for the same issue will have to be made in JincorTokenICO.sol, as you use newTokensSold for this check. This is an issue that is easy to overlook but will be very irritating to the investors trying to contribute towards the end of your sale.

Minor Issues

issue 1

In refund(), the following block of code is a classic example of reentrancy vulnerability:

msg.sender.transfer(refund);
deposited[msg.sender] = 0;

In this case, this does not introduce a security flaw, but should be changed regardless

issue 2

Similarly, in doPurchase(), put the following line before token.transfer() for similar reasons:

    deposited[msg.sender] = deposited[msg.sender].add(msg.value);

issue 3

calculateBonus(uint tokens) in JincorTokenICO.sol should be private.

issue 4

Line 36 in JincorToken.sol: "released" is spelled incorrectly. Line 50 in JincorTokensol: "Constructor" is spelled incorrectly.

z-zap commented 7 years ago

Excited to see your project guys! Your codebase looks good for the most part. I like the referral program, that's new for ICOs I've seen.

artemii235 commented 7 years ago

Hello @zlgrube!

Thank you for creating an issue.

Major issue This is good suggestion, the only thing to adjust - we should allow to unhalt independently on ICO active state because we can get stuck - if ICO ends and it's halted - we won't be able to unhalt it at all. We will implement this change.

Medium issues Same suggestion was discussed in #10, we decided to leave it as is because it might not look correct for investor to receive less tokens than he expected even if transaction will be partially refunded https://github.com/JincorTech/ico/issues/10#issuecomment-327773477.

Minor issues refund reentrancy is already mentioned in #46, we will fix this. doPurchase reentrancy - will fix. calculateBonus is internal already, I think we should leave it like this so this function will be available in derived contracts. Spelling errors - will fix.

We will make a decision regarding your bounty after we implement accepted fixes and comment the value here. Thanks again!

z-zap commented 6 years ago

@artemii235 any update on this?

JincorTech / ico