From b8a70f0e559189d79c3892a5273eb7d93f5d8d80 Mon Sep 17 00:00:00 2001
From: Leo Arias <leo@zeppelin.solutions>
Date: Wed, 5 Sep 2018 15:34:21 -0600
Subject: [PATCH] Improve encapsulation on Crowdsales (#1268)

* Improve encapsulation on Crowdsales

* add missing tests

* remove only

* Do not prefix getters
---
 contracts/crowdsale/Crowdsale.sol             | 58 ++++++++++++++-----
 .../distribution/FinalizableCrowdsale.sol     | 13 ++++-
 .../distribution/PostDeliveryCrowdsale.sol    | 15 +++--
 .../distribution/RefundableCrowdsale.sol      | 17 ++++--
 .../crowdsale/emission/AllowanceCrowdsale.sol | 15 +++--
 .../crowdsale/emission/MintedCrowdsale.sol    |  3 +-
 .../price/IncreasingPriceCrowdsale.sol        | 26 +++++++--
 .../crowdsale/validation/CappedCrowdsale.sol  | 15 +++--
 .../IndividuallyCappedCrowdsale.sol           | 18 +++---
 test/crowdsale/AllowanceCrowdsale.test.js     |  4 ++
 .../IncreasingPriceCrowdsale.test.js          |  5 ++
 test/crowdsale/PostDeliveryCrowdsale.test.js  |  2 +
 12 files changed, 141 insertions(+), 50 deletions(-)

diff --git a/contracts/crowdsale/Crowdsale.sol b/contracts/crowdsale/Crowdsale.sol
index 8bf055ada..0b133c0aa 100644
--- a/contracts/crowdsale/Crowdsale.sol
+++ b/contracts/crowdsale/Crowdsale.sol
@@ -22,19 +22,16 @@ contract Crowdsale {
   using SafeERC20 for IERC20;
 
   // The token being sold
-  IERC20 public token;
+  IERC20 private token_;
 
   // Address where funds are collected
-  address public wallet;
+  address private wallet_;
 
   // How many token units a buyer gets per wei.
-  // The rate is the conversion between wei and the smallest and indivisible token unit.
-  // So, if you are using a rate of 1 with a ERC20Detailed token with 3 decimals called TOK
-  // 1 wei will give you 1 unit, or 0.001 TOK.
-  uint256 public rate;
+  uint256 private rate_;
 
   // Amount of wei raised
-  uint256 public weiRaised;
+  uint256 private weiRaised_;
 
   /**
    * Event for token purchase logging
@@ -52,6 +49,9 @@ contract Crowdsale {
 
   /**
    * @param _rate Number of token units a buyer gets per wei
+   * @dev The rate is the conversion between wei and the smallest and indivisible
+   * token unit. So, if you are using a rate of 1 with a ERC20Detailed token
+   * with 3 decimals called TOK, 1 wei will give you 1 unit, or 0.001 TOK.
    * @param _wallet Address where collected funds will be forwarded to
    * @param _token Address of the token being sold
    */
@@ -60,9 +60,9 @@ contract Crowdsale {
     require(_wallet != address(0));
     require(_token != address(0));
 
-    rate = _rate;
-    wallet = _wallet;
-    token = _token;
+    rate_ = _rate;
+    wallet_ = _wallet;
+    token_ = _token;
   }
 
   // -----------------------------------------
@@ -76,6 +76,34 @@ contract Crowdsale {
     buyTokens(msg.sender);
   }
 
+  /**
+   * @return the token being sold.
+   */
+  function token() public view returns(IERC20) {
+    return token_;
+  }
+
+  /**
+   * @return the address where funds are collected.
+   */
+  function wallet() public view returns(address) {
+    return wallet_;
+  }
+
+  /**
+   * @return the number of token units a buyer gets per wei.
+   */
+  function rate() public view returns(uint256) {
+    return rate_;
+  }
+
+  /**
+   * @return the mount of wei raised.
+   */
+  function weiRaised() public view returns (uint256) {
+    return weiRaised_;
+  }
+
   /**
    * @dev low level token purchase ***DO NOT OVERRIDE***
    * @param _beneficiary Address performing the token purchase
@@ -89,7 +117,7 @@ contract Crowdsale {
     uint256 tokens = _getTokenAmount(weiAmount);
 
     // update state
-    weiRaised = weiRaised.add(weiAmount);
+    weiRaised_ = weiRaised_.add(weiAmount);
 
     _processPurchase(_beneficiary, tokens);
     emit TokensPurchased(
@@ -113,7 +141,7 @@ contract Crowdsale {
    * @dev Validation of an incoming purchase. Use require statements to revert state when conditions are not met. Use `super` in contracts that inherit from Crowdsale to extend their validations.
    * Example from CappedCrowdsale.sol's _preValidatePurchase method:
    *   super._preValidatePurchase(_beneficiary, _weiAmount);
-   *   require(weiRaised.add(_weiAmount) <= cap);
+   *   require(weiRaised().add(_weiAmount) <= cap);
    * @param _beneficiary Address performing the token purchase
    * @param _weiAmount Value in wei involved in the purchase
    */
@@ -152,7 +180,7 @@ contract Crowdsale {
   )
     internal
   {
-    token.safeTransfer(_beneficiary, _tokenAmount);
+    token_.safeTransfer(_beneficiary, _tokenAmount);
   }
 
   /**
@@ -191,13 +219,13 @@ contract Crowdsale {
   function _getTokenAmount(uint256 _weiAmount)
     internal view returns (uint256)
   {
-    return _weiAmount.mul(rate);
+    return _weiAmount.mul(rate_);
   }
 
   /**
    * @dev Determines how ETH is stored/forwarded on purchases.
    */
   function _forwardFunds() internal {
-    wallet.transfer(msg.value);
+    wallet_.transfer(msg.value);
   }
 }
diff --git a/contracts/crowdsale/distribution/FinalizableCrowdsale.sol b/contracts/crowdsale/distribution/FinalizableCrowdsale.sol
index 651069d41..693e0b498 100644
--- a/contracts/crowdsale/distribution/FinalizableCrowdsale.sol
+++ b/contracts/crowdsale/distribution/FinalizableCrowdsale.sol
@@ -13,22 +13,29 @@ import "../validation/TimedCrowdsale.sol";
 contract FinalizableCrowdsale is Ownable, TimedCrowdsale {
   using SafeMath for uint256;
 
-  bool public isFinalized = false;
+  bool private finalized_ = false;
 
   event CrowdsaleFinalized();
 
+  /**
+   * @return true if the crowdsale is finalized, false otherwise.
+   */
+  function finalized() public view returns(bool) {
+    return finalized_;
+  }
+
   /**
    * @dev Must be called after crowdsale ends, to do some extra finalization
    * work. Calls the contract's finalization function.
    */
   function finalize() public onlyOwner {
-    require(!isFinalized);
+    require(!finalized_);
     require(hasClosed());
 
     _finalization();
     emit CrowdsaleFinalized();
 
-    isFinalized = true;
+    finalized_ = true;
   }
 
   /**
diff --git a/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol b/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol
index 5d3c75e81..aca5e5e82 100644
--- a/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol
+++ b/contracts/crowdsale/distribution/PostDeliveryCrowdsale.sol
@@ -12,7 +12,7 @@ import "../../math/SafeMath.sol";
 contract PostDeliveryCrowdsale is TimedCrowdsale {
   using SafeMath for uint256;
 
-  mapping(address => uint256) public balances;
+  mapping(address => uint256) private balances_;
 
   /**
    * @dev Withdraw tokens only after crowdsale ends.
@@ -20,12 +20,19 @@ contract PostDeliveryCrowdsale is TimedCrowdsale {
    */
   function withdrawTokens(address _beneficiary) public {
     require(hasClosed());
-    uint256 amount = balances[_beneficiary];
+    uint256 amount = balances_[_beneficiary];
     require(amount > 0);
-    balances[_beneficiary] = 0;
+    balances_[_beneficiary] = 0;
     _deliverTokens(_beneficiary, amount);
   }
 
+  /**
+   * @return the balance of an account.
+   */
+  function balanceOf(address _account) public view returns(uint256) {
+    return balances_[_account];
+  }
+
   /**
    * @dev Overrides parent by storing balances instead of issuing tokens right away.
    * @param _beneficiary Token purchaser
@@ -37,7 +44,7 @@ contract PostDeliveryCrowdsale is TimedCrowdsale {
   )
     internal
   {
-    balances[_beneficiary] = balances[_beneficiary].add(_tokenAmount);
+    balances_[_beneficiary] = balances_[_beneficiary].add(_tokenAmount);
   }
 
 }
diff --git a/contracts/crowdsale/distribution/RefundableCrowdsale.sol b/contracts/crowdsale/distribution/RefundableCrowdsale.sol
index b27d1e3d3..6279fbbde 100644
--- a/contracts/crowdsale/distribution/RefundableCrowdsale.sol
+++ b/contracts/crowdsale/distribution/RefundableCrowdsale.sol
@@ -15,7 +15,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
   using SafeMath for uint256;
 
   // minimum amount of funds to be raised in weis
-  uint256 public goal;
+  uint256 private goal_;
 
   // refund escrow used to hold funds while crowdsale is running
   RefundEscrow private escrow_;
@@ -26,8 +26,15 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
    */
   constructor(uint256 _goal) public {
     require(_goal > 0);
-    escrow_ = new RefundEscrow(wallet);
-    goal = _goal;
+    escrow_ = new RefundEscrow(wallet());
+    goal_ = _goal;
+  }
+
+  /**
+   * @return minimum amount of funds to be raised in wei.
+   */
+  function goal() public view returns(uint256) {
+    return goal_;
   }
 
   /**
@@ -35,7 +42,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
    * @param _beneficiary Whose refund will be claimed.
    */
   function claimRefund(address _beneficiary) public {
-    require(isFinalized);
+    require(finalized());
     require(!goalReached());
 
     escrow_.withdraw(_beneficiary);
@@ -46,7 +53,7 @@ contract RefundableCrowdsale is FinalizableCrowdsale {
    * @return Whether funding goal was reached
    */
   function goalReached() public view returns (bool) {
-    return weiRaised >= goal;
+    return weiRaised() >= goal_;
   }
 
   /**
diff --git a/contracts/crowdsale/emission/AllowanceCrowdsale.sol b/contracts/crowdsale/emission/AllowanceCrowdsale.sol
index 4e48638ab..54db50af1 100644
--- a/contracts/crowdsale/emission/AllowanceCrowdsale.sol
+++ b/contracts/crowdsale/emission/AllowanceCrowdsale.sol
@@ -14,7 +14,7 @@ contract AllowanceCrowdsale is Crowdsale {
   using SafeMath for uint256;
   using SafeERC20 for IERC20;
 
-  address public tokenWallet;
+  address private tokenWallet_;
 
   /**
    * @dev Constructor, takes token wallet address.
@@ -22,7 +22,14 @@ contract AllowanceCrowdsale is Crowdsale {
    */
   constructor(address _tokenWallet) public {
     require(_tokenWallet != address(0));
-    tokenWallet = _tokenWallet;
+    tokenWallet_ = _tokenWallet;
+  }
+
+  /**
+   * @return the address of the wallet that will hold the tokens.
+   */
+  function tokenWallet() public view returns(address) {
+    return tokenWallet_;
   }
 
   /**
@@ -30,7 +37,7 @@ contract AllowanceCrowdsale is Crowdsale {
    * @return Amount of tokens left in the allowance
    */
   function remainingTokens() public view returns (uint256) {
-    return token.allowance(tokenWallet, this);
+    return token().allowance(tokenWallet_, this);
   }
 
   /**
@@ -44,6 +51,6 @@ contract AllowanceCrowdsale is Crowdsale {
   )
     internal
   {
-    token.safeTransferFrom(tokenWallet, _beneficiary, _tokenAmount);
+    token().safeTransferFrom(tokenWallet_, _beneficiary, _tokenAmount);
   }
 }
diff --git a/contracts/crowdsale/emission/MintedCrowdsale.sol b/contracts/crowdsale/emission/MintedCrowdsale.sol
index 9f3a8b4b1..1b676e077 100644
--- a/contracts/crowdsale/emission/MintedCrowdsale.sol
+++ b/contracts/crowdsale/emission/MintedCrowdsale.sol
@@ -23,6 +23,7 @@ contract MintedCrowdsale is Crowdsale {
     internal
   {
     // Potentially dangerous assumption about the type of the token.
-    require(ERC20Mintable(address(token)).mint(_beneficiary, _tokenAmount));
+    require(
+      ERC20Mintable(address(token())).mint(_beneficiary, _tokenAmount));
   }
 }
diff --git a/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol b/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol
index 841f09bc3..7cc07f083 100644
--- a/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol
+++ b/contracts/crowdsale/price/IncreasingPriceCrowdsale.sol
@@ -13,8 +13,8 @@ import "../../math/SafeMath.sol";
 contract IncreasingPriceCrowdsale is TimedCrowdsale {
   using SafeMath for uint256;
 
-  uint256 public initialRate;
-  uint256 public finalRate;
+  uint256 private initialRate_;
+  uint256 private finalRate_;
 
   /**
    * @dev Constructor, takes initial and final rates of tokens received per wei contributed.
@@ -24,8 +24,22 @@ contract IncreasingPriceCrowdsale is TimedCrowdsale {
   constructor(uint256 _initialRate, uint256 _finalRate) public {
     require(_finalRate > 0);
     require(_initialRate >= _finalRate);
-    initialRate = _initialRate;
-    finalRate = _finalRate;
+    initialRate_ = _initialRate;
+    finalRate_ = _finalRate;
+  }
+
+  /**
+   * @return the initial rate of the crowdsale.
+   */
+  function initialRate() public view returns(uint256) {
+    return initialRate_;
+  }
+
+  /**
+   * @return the final rate of the crowdsale.
+   */
+  function finalRate() public view returns (uint256) {
+    return finalRate_;
   }
 
   /**
@@ -37,8 +51,8 @@ contract IncreasingPriceCrowdsale is TimedCrowdsale {
     // solium-disable-next-line security/no-block-members
     uint256 elapsedTime = block.timestamp.sub(openingTime);
     uint256 timeRange = closingTime.sub(openingTime);
-    uint256 rateRange = initialRate.sub(finalRate);
-    return initialRate.sub(elapsedTime.mul(rateRange).div(timeRange));
+    uint256 rateRange = initialRate_.sub(finalRate_);
+    return initialRate_.sub(elapsedTime.mul(rateRange).div(timeRange));
   }
 
   /**
diff --git a/contracts/crowdsale/validation/CappedCrowdsale.sol b/contracts/crowdsale/validation/CappedCrowdsale.sol
index 1df0e3eb3..99b188a98 100644
--- a/contracts/crowdsale/validation/CappedCrowdsale.sol
+++ b/contracts/crowdsale/validation/CappedCrowdsale.sol
@@ -11,7 +11,7 @@ import "../Crowdsale.sol";
 contract CappedCrowdsale is Crowdsale {
   using SafeMath for uint256;
 
-  uint256 public cap;
+  uint256 private cap_;
 
   /**
    * @dev Constructor, takes maximum amount of wei accepted in the crowdsale.
@@ -19,7 +19,14 @@ contract CappedCrowdsale is Crowdsale {
    */
   constructor(uint256 _cap) public {
     require(_cap > 0);
-    cap = _cap;
+    cap_ = _cap;
+  }
+
+  /**
+   * @return the cap of the crowdsale.
+   */
+  function cap() public view returns(uint256) {
+    return cap_;
   }
 
   /**
@@ -27,7 +34,7 @@ contract CappedCrowdsale is Crowdsale {
    * @return Whether the cap was reached
    */
   function capReached() public view returns (bool) {
-    return weiRaised >= cap;
+    return weiRaised() >= cap_;
   }
 
   /**
@@ -42,7 +49,7 @@ contract CappedCrowdsale is Crowdsale {
     internal
   {
     super._preValidatePurchase(_beneficiary, _weiAmount);
-    require(weiRaised.add(_weiAmount) <= cap);
+    require(weiRaised().add(_weiAmount) <= cap_);
   }
 
 }
diff --git a/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol b/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol
index 3c7884c65..f0c3fdd4c 100644
--- a/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol
+++ b/contracts/crowdsale/validation/IndividuallyCappedCrowdsale.sol
@@ -12,8 +12,8 @@ import "../../ownership/Ownable.sol";
 contract IndividuallyCappedCrowdsale is Ownable, Crowdsale {
   using SafeMath for uint256;
 
-  mapping(address => uint256) public contributions;
-  mapping(address => uint256) public caps;
+  mapping(address => uint256) private contributions_;
+  mapping(address => uint256) private caps_;
 
   /**
    * @dev Sets a specific user's maximum contribution.
@@ -21,7 +21,7 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale {
    * @param _cap Wei limit for individual contribution
    */
   function setUserCap(address _beneficiary, uint256 _cap) external onlyOwner {
-    caps[_beneficiary] = _cap;
+    caps_[_beneficiary] = _cap;
   }
 
   /**
@@ -37,7 +37,7 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale {
     onlyOwner
   {
     for (uint256 i = 0; i < _beneficiaries.length; i++) {
-      caps[_beneficiaries[i]] = _cap;
+      caps_[_beneficiaries[i]] = _cap;
     }
   }
 
@@ -47,7 +47,7 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale {
    * @return Current cap for individual user
    */
   function getUserCap(address _beneficiary) public view returns (uint256) {
-    return caps[_beneficiary];
+    return caps_[_beneficiary];
   }
 
   /**
@@ -58,7 +58,7 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale {
   function getUserContribution(address _beneficiary)
     public view returns (uint256)
   {
-    return contributions[_beneficiary];
+    return contributions_[_beneficiary];
   }
 
   /**
@@ -73,7 +73,8 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale {
     internal
   {
     super._preValidatePurchase(_beneficiary, _weiAmount);
-    require(contributions[_beneficiary].add(_weiAmount) <= caps[_beneficiary]);
+    require(
+      contributions_[_beneficiary].add(_weiAmount) <= caps_[_beneficiary]);
   }
 
   /**
@@ -88,7 +89,8 @@ contract IndividuallyCappedCrowdsale is Ownable, Crowdsale {
     internal
   {
     super._updatePurchasingState(_beneficiary, _weiAmount);
-    contributions[_beneficiary] = contributions[_beneficiary].add(_weiAmount);
+    contributions_[_beneficiary] = contributions_[_beneficiary].add(
+      _weiAmount);
   }
 
 }
diff --git a/test/crowdsale/AllowanceCrowdsale.test.js b/test/crowdsale/AllowanceCrowdsale.test.js
index b0d68a9b1..ee6a063f2 100644
--- a/test/crowdsale/AllowanceCrowdsale.test.js
+++ b/test/crowdsale/AllowanceCrowdsale.test.js
@@ -25,6 +25,10 @@ contract('AllowanceCrowdsale', function ([_, investor, wallet, purchaser, tokenW
   });
 
   describe('accepting payments', function () {
+    it('should have token wallet', async function () {
+      (await this.crowdsale.tokenWallet()).should.be.equal(tokenWallet);
+    });
+
     it('should accept sends', async function () {
       await this.crowdsale.send(value);
     });
diff --git a/test/crowdsale/IncreasingPriceCrowdsale.test.js b/test/crowdsale/IncreasingPriceCrowdsale.test.js
index e68d451bd..c62c17d89 100644
--- a/test/crowdsale/IncreasingPriceCrowdsale.test.js
+++ b/test/crowdsale/IncreasingPriceCrowdsale.test.js
@@ -55,6 +55,11 @@ contract('IncreasingPriceCrowdsale', function ([_, investor, wallet, purchaser])
         await this.token.transfer(this.crowdsale.address, tokenSupply);
       });
 
+      it('should have initial and final rate', async function () {
+        (await this.crowdsale.initialRate()).should.be.bignumber.equal(initialRate);
+        (await this.crowdsale.finalRate()).should.be.bignumber.equal(finalRate);
+      });
+
       it('at start', async function () {
         await increaseTimeTo(this.startTime);
         await this.crowdsale.buyTokens(investor, { value, from: purchaser });
diff --git a/test/crowdsale/PostDeliveryCrowdsale.test.js b/test/crowdsale/PostDeliveryCrowdsale.test.js
index 918c916c3..e95c5a1f4 100644
--- a/test/crowdsale/PostDeliveryCrowdsale.test.js
+++ b/test/crowdsale/PostDeliveryCrowdsale.test.js
@@ -47,6 +47,7 @@ contract('PostDeliveryCrowdsale', function ([_, investor, wallet, purchaser]) {
       });
 
       it('does not immediately assign tokens to beneficiaries', async function () {
+        (await this.crowdsale.balanceOf(investor)).should.be.bignumber.equal(value);
         (await this.token.balanceOf(investor)).should.be.bignumber.equal(0);
       });
 
@@ -61,6 +62,7 @@ contract('PostDeliveryCrowdsale', function ([_, investor, wallet, purchaser]) {
 
         it('allows beneficiaries to withdraw tokens', async function () {
           await this.crowdsale.withdrawTokens(investor);
+          (await this.crowdsale.balanceOf(investor)).should.be.bignumber.equal(0);
           (await this.token.balanceOf(investor)).should.be.bignumber.equal(value);
         });