Resolve audit feedback and convert SavingsVault to initializable

contracts/rewards/InitializableRewardsDistributionRecipient.sol

@@ -0,0 +1,47 @@
+pragma solidity 0.5.16;
+
+import { InitializableModule2 } from "../shared/InitializableModule2.sol";
+import { IERC20 } from "@openzeppelin/contracts/token/ERC20/IERC20.sol";
+import { IRewardsDistributionRecipient } from "../interfaces/IRewardsDistributionRecipient.sol";
+
+/**
+ * @title  RewardsDistributionRecipient
+ * @author Originally: Synthetix (forked from /Synthetixio/synthetix/contracts/RewardsDistributionRecipient.sol)
+ *         Changes by: Stability Labs Pty. Ltd.
+ * @notice RewardsDistributionRecipient gets notified of additional rewards by the rewardsDistributor
+ * @dev    Changes: Addition of Module and abstract `getRewardToken` func + cosmetic
+ */
+contract InitializableRewardsDistributionRecipient is IRewardsDistributionRecipient, InitializableModule2 {
+
+    // @abstract
+    function notifyRewardAmount(uint256 reward) external;
+    function getRewardToken() external view returns (IERC20);
+
+    // This address has the ability to distribute the rewards
+    address public rewardsDistributor;
+
+    /** @dev Recipient is a module, governed by mStable governance */
+    function _initialize(address _nexus, address _rewardsDistributor) internal {
+        rewardsDistributor = _rewardsDistributor;
+        InitializableModule2._initialize(_nexus);
+    }
+
+    /**
+     * @dev Only the rewards distributor can notify about rewards
+     */
+    modifier onlyRewardsDistributor() {
+        require(msg.sender == rewardsDistributor, "Caller is not reward distributor");
+        _;
+    }
+
+    /**
+     * @dev Change the rewardsDistributor - only called by mStable governor
+     * @param _rewardsDistributor   Address of the new distributor
+     */
+    function setRewardsDistribution(address _rewardsDistributor)
+        external
+        onlyGovernor
+    {
+        rewardsDistributor = _rewardsDistributor;
+    }
+}

contracts/savings/BoostedSavingsVault.sol

@@ -2,8 +2,9 @@ pragma solidity 0.5.16;
 
 // Internal
 import { IBoostedVaultWithLockup } from "../interfaces/IBoostedVaultWithLockup.sol";
-import { RewardsDistributionRecipient } from "../rewards/RewardsDistributionRecipient.sol";
+import { InitializableRewardsDistributionRecipient } from "../rewards/InitializableRewardsDistributionRecipient.sol";
 import { BoostedTokenWrapper } from "./BoostedTokenWrapper.sol";
+import { Initializable } from "@openzeppelin/upgrades/contracts/Initializable.sol";
 
 // Libs
 import { IERC20, SafeERC20 } from "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";
@@ -22,7 +23,12 @@ import { StableMath, SafeMath } from "../shared/StableMath.sol";
  *          - Struct packing of common data
  *          - Searching for and claiming of unlocked rewards
  */
-contract BoostedSavingsVault is IBoostedVaultWithLockup, BoostedTokenWrapper, RewardsDistributionRecipient {
+contract BoostedSavingsVault is
+    IBoostedVaultWithLockup,
+    Initializable,
+    InitializableRewardsDistributionRecipient,
+    BoostedTokenWrapper
+{
 
     using StableMath for uint256;
     using SafeCast for uint256;
@@ -67,19 +73,22 @@ contract BoostedSavingsVault is IBoostedVaultWithLockup, BoostedTokenWrapper, Re
         uint128 rate;
     }
 
-    // TODO - add constants to bytecode at deployTime to reduce SLOAD cost
-    /** @dev StakingRewards is a TokenWrapper and RewardRecipient */
-    constructor(
+    /**
+     * @dev StakingRewards is a TokenWrapper and RewardRecipient
+     * Constants added to bytecode at deployTime to reduce SLOAD cost
+     */
+    function initialize(
         address _nexus, // constant
         address _stakingToken, // constant
         address _stakingContract, // constant
         address _rewardsToken, // constant
         address _rewardsDistributor
     )
-        public
-        RewardsDistributionRecipient(_nexus, _rewardsDistributor)
-        BoostedTokenWrapper(_stakingToken, _stakingContract)
+        external
+        initializer
     {
+        InitializableRewardsDistributionRecipient._initialize(_nexus, _rewardsDistributor);
+        BoostedTokenWrapper._initialize(_stakingToken, _stakingContract);
         rewardsToken = IERC20(_rewardsToken);
     }
 
@@ -142,7 +151,7 @@ contract BoostedSavingsVault is IBoostedVaultWithLockup, BoostedTokenWrapper, Re
         _;
     }
 
-    /** @dev Updates the reward for a given address, before executing function */
+    /** @dev Updates the boost for a given address, after the rest of the function has executed */
     modifier updateBoost(address _account) {
         _;
         _setBoost(_account);
@@ -314,6 +323,8 @@ contract BoostedSavingsVault is IBoostedVaultWithLockup, BoostedTokenWrapper, Re
         internal
     {
         require(_amount > 0, "Cannot stake 0");
+        require(_beneficiary != address(0), "Invalid beneficiary address");
+
         _stakeRaw(_beneficiary, _amount);
         emit Staked(_beneficiary, _amount, msg.sender);
     }

contracts/savings/BoostedTokenWrapper.sol

@@ -6,7 +6,7 @@ import { IIncentivisedVotingLockup } from "../interfaces/IIncentivisedVotingLock
 
 // Libs
 import { SafeERC20, IERC20 } from "@openzeppelin/contracts/token/ERC20/SafeERC20.sol";
-import { ReentrancyGuard } from "@openzeppelin/contracts/utils/ReentrancyGuard.sol";
+import { InitializableReentrancyGuard } from "../shared/InitializableReentrancyGuard.sol";
 import { SafeMath } from "@openzeppelin/contracts/math/SafeMath.sol";
 import { StableMath } from "../shared/StableMath.sol";
 import { Root } from "../shared/Root.sol";
@@ -20,7 +20,7 @@ import { Root } from "../shared/Root.sol";
  *          - Adding `_boostedBalances` and `_totalBoostedSupply`
  *          - Implemting of a `_setBoost` hook to calculate/apply a users boost
  */
-contract BoostedTokenWrapper is ReentrancyGuard {
+contract BoostedTokenWrapper is InitializableReentrancyGuard {
 
     using SafeMath for uint256;
     using StableMath for uint256;
@@ -45,9 +45,13 @@ contract BoostedTokenWrapper is ReentrancyGuard {
      * @param _stakingToken Wrapped token to be staked
      * @param _stakingContract mStable MTA Staking contract
      */
-    constructor(address _stakingToken, address _stakingContract) internal {
+    function _initialize(
+        address _stakingToken,
+        address _stakingContract
+    ) internal {
         stakingToken = IERC20(_stakingToken);
         stakingContract = IIncentivisedVotingLockup(_stakingContract);
+        InitializableReentrancyGuard._initialize();
     }
 
     /**
@@ -138,7 +142,7 @@ contract BoostedTokenWrapper is ReentrancyGuard {
 
         // Check whether balance is sufficient
         // is_boosted is used to minimize gas usage
-        if(rawBalance > MIN_DEPOSIT) {
+        if(rawBalance >= MIN_DEPOSIT) {
             uint256 votingWeight = stakingContract.balanceOf(_account);
             boost = _computeBoost(rawBalance, votingWeight);
         }
@@ -158,10 +162,8 @@ contract BoostedTokenWrapper is ReentrancyGuard {
     function _computeBoost(uint256 _deposit, uint256 _votingWeight)
         private
         pure
-        returns (uint256)
+        returns (uint256 boost)
     {
-        require(_deposit >= MIN_DEPOSIT, "Requires minimum deposit value");
-
         if(_votingWeight == 0) return MIN_BOOST;
 
         // Compute balance to the power 7/8
@@ -175,12 +177,10 @@ contract BoostedTokenWrapper is ReentrancyGuard {
                                 denominator)))))
             );
         denominator = denominator.div(1e3);
-        uint256 boost = _votingWeight.mul(BOOST_COEFF).div(10).divPrecisely(denominator);
+        boost = _votingWeight.mul(BOOST_COEFF).div(10).divPrecisely(denominator);
         boost = StableMath.min(
             MIN_BOOST.add(boost),
             MAX_BOOST
         );
-
-        return boost;
     }
 }

contracts/savings/SavingsContract.sol

@@ -6,7 +6,7 @@ import { ISavingsManager } from "../interfaces/ISavingsManager.sol";
 // Internal
 import { ISavingsContractV1, ISavingsContractV2 } from "../interfaces/ISavingsContract.sol";
 import { InitializableToken } from "../shared/InitializableToken.sol";
-import { InitializableModule } from "../shared/InitializableModule.sol";
+import { InitializableModule2 } from "../shared/InitializableModule2.sol";
 import { IConnector } from "./peripheral/IConnector.sol";
 import { Initializable } from "@openzeppelin/upgrades/contracts/Initializable.sol";
 
@@ -30,7 +30,7 @@ contract SavingsContract is
     ISavingsContractV2,
     Initializable,
     InitializableToken,
-    InitializableModule
+    InitializableModule2
 {
     using SafeMath for uint256;
     using StableMath for uint256;
@@ -79,7 +79,7 @@ contract SavingsContract is
     uint256 constant private MAX_APY = 2e18;
     uint256 constant private SECONDS_IN_YEAR = 365 days;
 
-    // TODO - Add these constants to bytecode at deploytime
+    // Add these constants to bytecode at deploytime
     function initialize(
         address _nexus, // constant
         address _poker,
@@ -91,7 +91,7 @@ contract SavingsContract is
         initializer
     {
         InitializableToken._initialize(_nameArg, _symbolArg);
-        InitializableModule._initialize(_nexus);
+        InitializableModule2._initialize(_nexus);
 
         require(address(_underlying) != address(0), "mAsset address is zero");
         underlying = _underlying;
@@ -250,6 +250,7 @@ contract SavingsContract is
         returns (uint256 creditsIssued)
     {
         require(_underlying > 0, "Must deposit something");
+        require(_beneficiary != address(0), "Invalid beneficiary address");
 
         // Collect recent interest generated by basket and update exchange rate
         IERC20 mAsset = underlying;
@@ -350,9 +351,9 @@ contract SavingsContract is
         returns (uint256 creditsBurned, uint256 massetReturned)
     {
         // Centralise credit <> underlying calcs and minimise SLOAD count
-        uint256 credits_ = 0;
-        uint256 underlying_ = 0;
-        uint256 exchangeRate_ = 0;
+        uint256 credits_;
+        uint256 underlying_;
+        uint256 exchangeRate_;
         // If the input is a credit amt, then calculate underlying payout and cache the exchangeRate
         if(_isCreditAmt){
             credits_ = _amt;
@@ -545,8 +546,8 @@ contract SavingsContract is
             //      Always expect the collateral in the connector to increase in value
             require(connectorBalance >= lastBalance_, "Invalid yield");
             if(connectorBalance > 0){
-                //  Validate the collection by ensuring that the APY is not ridiculous (forked from SavingsManager)
-                _validateCollection(lastBalance_, connectorBalance.sub(lastBalance_), timeSinceLastPoke);
+                //  Validate the collection by ensuring that the APY is not ridiculous
+                _validateCollection(connectorBalance, connectorBalance.sub(lastBalance_), timeSinceLastPoke);
             }
 
             // 3. Level the assets to Fraction (connector) & 100-fraction (raw)
@@ -569,6 +570,9 @@ contract SavingsContract is
                 }
             }
             //     Else ideal == connectorBalance (e.g. 0), do nothing
+            // TODO - consider if this will actually work.. maybe check that new rawBalance is
+            // fully withdrawn?
+            require(connector_.checkBalance() >= ideal, "Enforce system invariant");
 
             // 4i. Refresh exchange rate and emit event
             lastBalance = ideal;
@@ -685,11 +689,11 @@ contract SavingsContract is
         pure
         returns (uint256 _exchangeRate)
     {
-        return _totalCollateral.divPrecisely(_totalCredits.sub(1));
+        _exchangeRate = _totalCollateral.divPrecisely(_totalCredits.sub(1));
     }
 
     /**
-     * @dev Converts masset amount into credits based on exchange rate
+     * @dev Converts credit amount into masset based on exchange rate
      *               m = credits * exchangeRate
      */
     function _creditsToUnderlying(uint256 _credits)