Code review

contracts/AllocatedCappedCrowdsale.sol

@@ -1,6 +1,6 @@
+///// [review] Лучше поднять версию до текущей релизной
 pragma solidity ^0.4.8;
 
-
 import "./validation/ValidationUtil.sol";
 import "./Haltable.sol";
 import "./token/BurnableCrowdsaleToken.sol";
@@ -50,6 +50,7 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
     uint public endsAt;
 
     /* Кол-во проданных токенов*/
+    ///// [review] Размерность - (10 ^ token.decimals())
     uint public tokensSold;
 
     /* Если не набрали минимальной суммы, то инвесторы могут запросить refund */
@@ -85,6 +86,8 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
     mapping (address => uint) public tokenAmountOf;
 
     /** Мапа, адрес инвестора - кол-во эфира */
+
+    ///// [review] В отличии от этого поля, deposited в FundsVault может уменьшаться
     mapping (address => uint) public investedAmountOf;
 
     /** Адреса, куда будут распределены токены */
@@ -102,6 +105,10 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
      * - Finalized: Сработал финализатор
      * - Refunding: Возвращаем эфир, который загружен на контракт
      */
+
+    ///// [review] Меня ввело в заблуждение название PreFunding - 
+    ///// [review] С англ. это можно перевести как предпродажи
+    ///// [review] Я бы переименовал в Init или Stopped чтобы обозначить, что токены еще нельзя начислять/продавать
     enum State{PreFunding, Funding, Success, Failure, Finalized, Refunding}
 
     // Событие покупки токена
@@ -272,7 +279,9 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
             revert();
         }
 
+        ///// [review] (10 ^ 18) * (10 ^ 18) / (10 ^ 18) = 10 ^ 18
         uint resultValue = weiAmount.mul(multiplier).div(getOneTokenInWei());
+        ///// [review] 10 ^ 18
         uint tokenAmount = getAvailableTokens(resultValue.mul(bonusPercentage.add(100)).div(100));
 
         // Кол-во токенов к выдаче = 0?, делаем откат
@@ -303,6 +312,10 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
         address receiver = msg.sender;
         uint weiAmount = msg.value;
 
+        ///// [review] 1. Если здесь человек купит по одной цене, то может забрать себе токены из следующий "когорты" токенов по этой цене
+        ///// [review] Об этом нужно не забыть сказать в условиях покупки
+        ///// [review] 2. Не уверен, но если такое произойдет - ваша математика точно сойдется?
+        ///// [review] то есть будет ли валидный hard cap, если люди получат чуть больше токенов по чуть меньшей цене?
         uint tokenAmount = calcTokenAmount(receiver, weiAmount);
 
         internalAssignTokens(receiver, weiAmount, tokenAmount);
@@ -312,6 +325,8 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
             internalPresaleDeposit(presaleWallet, weiAmount);
         } else if (block.timestamp >= startsAt && block.timestamp <= endsAt){
             // TGE
+
+            ///// [review] Параметр destinationWallet не используется
             internalSaleDeposit(destinationWallet, weiAmount);
         }
 
@@ -323,6 +338,10 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
      * Покупка токенов через бэкенд, кидаем токены на адрес отправителя
      * Эта функция нужна в случае перевода альтернативных валют через бэкенд
      */
+    ///// [review] Очень опасная функция, которая может не понравится инвесторам)
+    ///// [review] 1. У них не будет никакой возможности проверить вашу честность
+    ///// [review] 2. Если ключик owner украдут -> будет плохо. Рекомендую здесь сделать отдельный аккаунт onlyBackend
+    ///// [review] (чтобы если уж украли, то смогли только эти функции вызывать, а не все и сразу, а вы бы смогли запаузить контракт с owner)
     function backendBuy(address receiver, uint weiAmount, uint8 currencyType, uint currencyAmount) external onlyOwner stopInEmergency isEtherRateAndTokenPriceAssigned canReceivePayments {
         uint tokenAmount = calcTokenAmount(receiver, weiAmount);
 
@@ -333,6 +352,7 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
     }
 
     /* Включение режима возвратов */
+    ///// [review] Это должно вызываться только если soft cap is not reached (state==Failure)
     function enableRefunds() external onlyOwner {
         require(!isRefunding);
 
@@ -344,10 +364,15 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
     /**
      * Спец. функция, которая позволяет продавать токены вне ценовой политики, доступка только владельцу
      */
+    ///// [review] Очень опасная функция, которая может не понравится инвесторам)
+    ///// [review] Лучше ее вызывать не явно, а автоматически из конструктора или перед началом Presale/ICO
+    ///// [review] Всегда лучше написать контракт так, чтобы если вы потеряете ключик owner - то деньги и все остальное
+    ///// [review] можно было бы как-то получить
     function preallocate(address receiver, uint weiAmount, uint tokenAmount) external onlyOwner {
         internalAssignTokens(receiver, weiAmount, getAvailableTokens(tokenAmount));
 
         // Вызываем событие
+        ///// [review] почему Invested? 
         Invested(receiver, weiAmount, tokenAmount);
     }
 
@@ -442,10 +467,15 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
     /**
      * Финализатор, вызвать может только владелец и только в случае успеха
      */
+    ///// [review] Лучше ее вызывать не явно, а автоматически из конструктора или перед началом Presale/ICO
+    ///// [review] Всегда лучше написать контракт так, чтобы если вы потеряете ключик owner - то деньги и все остальное
+    ///// [review] можно было бы как-то получить
     function finalize() external onlyOwner inState(State.Success) {
         // Продажи должны быть не завершены
         require(!isFinalized);
 
+        ///// [review] Здесь вызывается метод internalFinalize() из файла RefundableAllocatedCappedCrowdsale.sol (переопределение)
+        ///// [review] Который в свою очередь вызывает internalFinalize() из ТЕКУЩЕГО файла 
         internalFinalize();
     }
 
@@ -454,6 +484,7 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
      */
     function internalAssignTokens(address receiver, uint weiAmount, uint tokenAmount) internal {
         // Новый инвестор?
+        ///// [review] Лучше это перенести в updateStat
         if (investedAmountOf[receiver] == 0 && presaleInvestedAmountOf[receiver] == 0) {
             investorCount++;
         }
@@ -518,6 +549,7 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
         uint referalTokenTransferAmount = referalTokenAmount.mul(10 ** token.decimals());
         uint reserveTokenTransferAmount = reserveTokenAmount.mul(10 ** token.decimals());
 
+        ///// [review] Заменить на require 
         if (!token.transfer(teamWallet, teamTokenTransferAmount)) revert();
         if (!token.transfer(advisorsWallet, advisorsTokenTransferAmount)) revert();
         if (!token.transfer(referalWallet, referalTokenTransferAmount)) revert();
@@ -585,4 +617,4 @@ contract AllocatedCappedCrowdsale is Haltable, ValidationUtil {
 
         _;
     }
-}
+}

contracts/RefundableAllocatedCappedCrowdsale.sol

@@ -1,3 +1,4 @@
+///// [review] Лучше поднять версию до текущей релизной
 pragma solidity ^0.4.8;
 
 import "./AllocatedCappedCrowdsale.sol";
@@ -53,11 +54,13 @@ contract RefundableAllocatedCappedCrowdsale is AllocatedCappedCrowdsale {
     }
 
     function internalEnableRefunds() internal {
+        ///// [review] Если я все правильно понял, то после refunds невозможно завершение  
         fundsVault.enableRefunds();
     }
 
     /** Переопределение функции принятия допозита на счет, в данном случае, идти будет через vault
     */
+    ///// [review] Параметр receiver не используется
     function internalSaleDeposit(address receiver, uint weiAmount) internal {
         // Шлем на кошелёк эфир
         fundsVault.deposit.value(weiAmount)(msg.sender);
@@ -68,13 +71,16 @@ contract RefundableAllocatedCappedCrowdsale is AllocatedCappedCrowdsale {
     function internalSetDestinationWallet(address destinationAddress) internal {
         fundsVault.setWallet(destinationAddress);
 
+        ///// [review] Почему нужно вызывать метод базового класса? Во многих других переопределениях такого нет (см. например выше)
         super.internalSetDestinationWallet(destinationAddress);
     }
 
     /** Переопределение функции возврата, возврат можно сделать только раз
     */
+    ///// [review, critical] При refund не вызывается burn или передача токенов
     function internalRefund(address receiver) internal {
         // Поддерживаем только 1 возврат
+        ///// [review] Заменить на require 
         if (refundedInvestors[receiver]) revert();
 
         // Получаем значение, которое нам было переведено в эфире

contracts/token/BurnableCrowdsaleToken.sol

@@ -1,3 +1,4 @@
+///// [review] Лучше поднять версию до текущей релизной
 pragma solidity ^0.4.8;
 
 import "./BurnableToken.sol";

contracts/token/BurnableToken.sol

@@ -1,3 +1,4 @@
+///// [review] Лучше поднять версию до текущей релизной
 pragma solidity ^0.4.8;
 
 import "../validation/ValidationUtil.sol";
@@ -11,9 +12,12 @@ import '../zeppelin/contracts/math/SafeMath.sol';
  *
  */
 
+///// [review] В OpenZeppelin есть BurnableToken, возможно можно заменить на него 
+///// [review] https://github.com/OpenZeppelin/zeppelin-solidity/blob/release/v1.5.0/contracts/token/BurnableToken.sol
 contract BurnableToken is StandardToken, Ownable, ValidationUtil {
     using SafeMath for uint;
 
+    ///// [review] Лучше всегда явно ставить значение по умолчанию, для наглядности
     address public tokenOwnerBurner;
 
     /** Событие, сколько токенов мы сожгли */
@@ -23,6 +27,11 @@ contract BurnableToken is StandardToken, Ownable, ValidationUtil {
         // Проверка, что адрес не пустой
         requireValidAddress(_tokenOwnerBurner);
 
+        ///// [review] Мне кажется, что лучше убрать requireValidAddress и делать просто:
+        /*
+        require(!isAddressEmpty(_tokenOwnerBurner));
+        */
+
         tokenOwnerBurner = _tokenOwnerBurner;
     }
 
@@ -56,11 +65,17 @@ contract BurnableToken is StandardToken, Ownable, ValidationUtil {
         // Проверка, что адрес не пустой
         requireValidAddress(tokenOwnerBurner);
 
+        ///// [review] Я бы убрал requireValidAddress вообще и сделал бы так:
+        /*
+        require(isAddressNotEmpty(tokenOwnerBurner));
+        */
+
         _;
     }
 
     modifier invalidOwnerBurner() {
         // Проверка, что адрес не пустой
+        ///// [review] Правильный комментарий: "Проверка, что адрес пустой"
         require(!isAddressNotEmpty(tokenOwnerBurner));
 
         _;

contracts/token/CrowdsaleToken.sol

@@ -1,3 +1,4 @@
+///// [review] Лучше поднять версию до текущей релизной
 pragma solidity ^0.4.8;
 
 import '../zeppelin/contracts/token/StandardToken.sol';
@@ -8,6 +9,7 @@ import "../zeppelin/contracts/ownership/Ownable.sol";
  *
  * ERC-20 токен, для ICO
  *
+///// [review] Не понятно, как это условие выполняется. Думаю, лучше убрать этот комментарий или поменять логику.
  * - Токен может быть с верхним лимитом или без него
  *
  */
@@ -41,6 +43,10 @@ contract CrowdsaleToken is StandardToken, Ownable {
         symbol = _symbol;
         decimals = _decimals;
 
+        ////// [review] Вот здесь странно, что totalSupply устновлен, но никому не начислен баланс. 
+        ////// [reivew] То есть уже по стандарту расхождение. Я бы переделал семантику так, чтобы либо: 
+        ////// [review] 1) убрать initialSupply и делать все в конструкторе; 
+        ////// [review] 2) либо делать все в initialSupply
         totalSupply = _initialSupply;
     }
 
@@ -59,6 +65,8 @@ contract CrowdsaleToken is StandardToken, Ownable {
     /**
      * Владелец может обновить инфу по токену
      */
+    ////// [review] Странный функционал, если честно. Это точно необходимо?
+    ////// [review] Тогда не понятно, почему decimals не меняются втч.
     function setTokenInformation(string _name, string _symbol) external onlyOwner {
         name = _name;
         symbol = _symbol;

contracts/validation/ValidationUtil.sol

@@ -12,4 +12,11 @@ contract ValidationUtil {
     function isAddressNotEmpty(address value) internal returns (bool result){
         return value != 0;
     }
+
+    ///// [review] Я бы убрал второй метод и сделал так:
+    /*
+    function isAddressEmpty(address _value) internal constant returns (bool result){
+          return (0x0!=value);
+    }
+    */
 }

contracts/vault/FundsVault.sol

@@ -1,3 +1,4 @@
+///// [review] Лучше поднять версию до текущей релизной
 pragma solidity ^0.4.8;
 
 import "../zeppelin/contracts/ownership/Ownable.sol";
@@ -16,6 +17,7 @@ contract FundsVault is Ownable, ValidationUtil {
     using SafeMath for uint;
     using Math for uint;
 
+    ///// [review] Если я все правильно понял, то после Refunding невозможен переход в Active/Closed опять.
     enum State {Active, Refunding, Closed}
 
     State public state;
@@ -46,6 +48,8 @@ contract FundsVault is Ownable, ValidationUtil {
         wallet = _wallet;
         sump = _sump;
 
+        ///// [review] Crowdsale будет активен начиная с 1ой секунды, как закончится деплой?
+        ///// [review] Может быть нужно добавить методы по запуску/паузе?
         state = State.Active;
     }
 
@@ -121,4 +125,6 @@ contract FundsVault is Ownable, ValidationUtil {
         _;
     }
 
+    ///// [review] Сюда лучше добавить пустую function(){}
+
 }