Closed kkx closed 7 years ago
Hello Kkx, Thank you for your concern.
The baseEthCapPerAddress is 0 by default to no permit to send ethereum to the contract if we did nothing on it. (dead contract)
We decided that 24H before the sale was enough.
onlyBeforeSale() come from the Zeppelin contract : https://github.com/OpenZeppelin/zeppelin-solidity/blob/master/contracts/crowdsale/Crowdsale.sol. In order to modify the less possible, we decided to let it.
Best,
so require(_startTime >= now); is wrong check. And you just closed issues like everything is perfect in your contracts. i don't see any sense to make a bounty program if you guys are not wiling to admit mistakes.
Hey Kkx, a bug bounty is to find vulnerabilities, we are glad that you do recommandations but this is not in the scope of the program. You can check the conditions on https://bounty.ethereum.org/ and ask yourself : what is the severity of this bug, how important is the impact if exploited and what is the likelihood that it is exploited.
This part of the code comes from Open Zeppelin and has been audited by many companies, I don't think we should modify it as it does not introduce any risk.
Also the architecture we chose makes it easy to reuse these contracts in an independant way. Changing this would broke the architecture.
Found some small issues regarding to your contracts, if you have planed completely all the steps in your ico, these issues may not be any problems, but in the contract perspective, i think these issues can lead some posible errors.
baseEthCapPerAddress should be bigger than 0. otherwise, getCurrentEthCapPerAddress will always return a 0. so in https://github.com/RequestNetwork/RequestTokenSale/blob/ddf5b022bcba83aca3adf4078bf63ba4fbded947/contracts/ProgressiveIndividualCappedCrowdsale.sol#L39 i would add this check in this function:
require(_baseEthCapPerAddress > 0)
Personally, i think this kind of parameters should be hard coded in the contract instead of making it configurable. This can prevent human errors like forgetting to call this function before the ico started.Another check is to startTime. as you use modifier
only24HBeforeSale
in various functions used before the ico started, the starttime should be set at least one day afternow
(i think 2 days at least to allow some operation space). so i would change this check https://github.com/RequestNetwork/RequestTokenSale/blob/d493261158543e9d4c42da3825340ea2511a76af/contracts/StandardCrowdsale.sol#L47 to this:require(now < startTime.sub(2 days));
onlyBeforeSale mofidier is not used in anywhere, should be removed to save some gas https://github.com/RequestNetwork/RequestTokenSale/blob/9fb1cdec478416d71d65503d394d3e6d800da9f0/contracts/StandardCrowdsale.sol#L127