From 94f04aede8876113c7547c1348f769ca7d81e334 Mon Sep 17 00:00:00 2001 From: alsco77 Date: Thu, 14 Jan 2021 14:47:31 +0100 Subject: [PATCH] Resolve audit feedback and convert SavingsVault to initializable --- ...tializableRewardsDistributionRecipient.sol | 47 ++++++ contracts/savings/BoostedSavingsVault.sol | 29 ++-- contracts/savings/BoostedTokenWrapper.sol | 20 +-- contracts/savings/SavingsContract.sol | 26 ++-- contracts/shared/InitializableModule2.sol | 134 ++++++++++++++++++ test/savings/TestSavingsContract.spec.ts | 2 + test/savings/TestSavingsVault.spec.ts | 44 +++--- 7 files changed, 256 insertions(+), 46 deletions(-) create mode 100644 contracts/rewards/InitializableRewardsDistributionRecipient.sol create mode 100644 contracts/shared/InitializableModule2.sol diff --git a/contracts/rewards/InitializableRewardsDistributionRecipient.sol b/contracts/rewards/InitializableRewardsDistributionRecipient.sol new file mode 100644 index 00000000..dccd78d1 --- /dev/null +++ b/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; + } +} diff --git a/contracts/savings/BoostedSavingsVault.sol b/contracts/savings/BoostedSavingsVault.sol index b25ae68e..db95bb4a 100644 --- a/contracts/savings/BoostedSavingsVault.sol +++ b/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); } diff --git a/contracts/savings/BoostedTokenWrapper.sol b/contracts/savings/BoostedTokenWrapper.sol index 3854f8ca..ec310d66 100644 --- a/contracts/savings/BoostedTokenWrapper.sol +++ b/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; } } \ No newline at end of file diff --git a/contracts/savings/SavingsContract.sol b/contracts/savings/SavingsContract.sol index bc9d427d..413b2161 100644 --- a/contracts/savings/SavingsContract.sol +++ b/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) diff --git a/contracts/shared/InitializableModule2.sol b/contracts/shared/InitializableModule2.sol new file mode 100644 index 00000000..1f40af4e --- /dev/null +++ b/contracts/shared/InitializableModule2.sol @@ -0,0 +1,134 @@ +pragma solidity 0.5.16; + +import { ModuleKeys } from "./ModuleKeys.sol"; +import { INexus } from "../interfaces/INexus.sol"; + +/** + * @title InitializableModule + * @author Stability Labs Pty. Ltd. + * @dev Subscribes to module updates from a given publisher and reads from its registry. + * Contract is used for upgradable proxy contracts. + */ +contract InitializableModule2 is ModuleKeys { + + INexus public nexus; + + /** + * @dev Modifier to allow function calls only from the Governor. + */ + modifier onlyGovernor() { + require(msg.sender == _governor(), "Only governor can execute"); + _; + } + + /** + * @dev Modifier to allow function calls only from the Governance. + * Governance is either Governor address or Governance address. + */ + modifier onlyGovernance() { + require( + msg.sender == _governor() || msg.sender == _governance(), + "Only governance can execute" + ); + _; + } + + /** + * @dev Modifier to allow function calls only from the ProxyAdmin. + */ + modifier onlyProxyAdmin() { + require( + msg.sender == _proxyAdmin(), "Only ProxyAdmin can execute" + ); + _; + } + + /** + * @dev Modifier to allow function calls only from the Manager. + */ + modifier onlyManager() { + require(msg.sender == _manager(), "Only manager can execute"); + _; + } + + /** + * @dev Initialization function for upgradable proxy contracts + * @param _nexus Nexus contract address + */ + function _initialize(address _nexus) internal { + require(_nexus != address(0), "Nexus address is zero"); + nexus = INexus(_nexus); + } + + /** + * @dev Returns Governor address from the Nexus + * @return Address of Governor Contract + */ + function _governor() internal view returns (address) { + return nexus.governor(); + } + + /** + * @dev Returns Governance Module address from the Nexus + * @return Address of the Governance (Phase 2) + */ + function _governance() internal view returns (address) { + return nexus.getModule(KEY_GOVERNANCE); + } + + /** + * @dev Return Staking Module address from the Nexus + * @return Address of the Staking Module contract + */ + function _staking() internal view returns (address) { + return nexus.getModule(KEY_STAKING); + } + + /** + * @dev Return ProxyAdmin Module address from the Nexus + * @return Address of the ProxyAdmin Module contract + */ + function _proxyAdmin() internal view returns (address) { + return nexus.getModule(KEY_PROXY_ADMIN); + } + + /** + * @dev Return MetaToken Module address from the Nexus + * @return Address of the MetaToken Module contract + */ + function _metaToken() internal view returns (address) { + return nexus.getModule(KEY_META_TOKEN); + } + + /** + * @dev Return OracleHub Module address from the Nexus + * @return Address of the OracleHub Module contract + */ + function _oracleHub() internal view returns (address) { + return nexus.getModule(KEY_ORACLE_HUB); + } + + /** + * @dev Return Manager Module address from the Nexus + * @return Address of the Manager Module contract + */ + function _manager() internal view returns (address) { + return nexus.getModule(KEY_MANAGER); + } + + /** + * @dev Return SavingsManager Module address from the Nexus + * @return Address of the SavingsManager Module contract + */ + function _savingsManager() internal view returns (address) { + return nexus.getModule(KEY_SAVINGS_MANAGER); + } + + /** + * @dev Return Recollateraliser Module address from the Nexus + * @return Address of the Recollateraliser Module contract (Phase 2) + */ + function _recollateraliser() internal view returns (address) { + return nexus.getModule(KEY_RECOLLATERALISER); + } +} diff --git a/test/savings/TestSavingsContract.spec.ts b/test/savings/TestSavingsContract.spec.ts index 98f01946..abbabbf6 100644 --- a/test/savings/TestSavingsContract.spec.ts +++ b/test/savings/TestSavingsContract.spec.ts @@ -1087,6 +1087,8 @@ contract("SavingsContract", async (accounts) => { "Not enough time elapsed", ); }); + // TODO - handle 2 scenarios - 1 with Lending market, 1 with yVault + // Run fully through each scenario context("with a connector", () => { beforeEach(async () => { await createNewSavingsContract(); diff --git a/test/savings/TestSavingsVault.spec.ts b/test/savings/TestSavingsVault.spec.ts index 06a4b93e..e761a929 100644 --- a/test/savings/TestSavingsVault.spec.ts +++ b/test/savings/TestSavingsVault.spec.ts @@ -15,6 +15,7 @@ const { expect } = envSetup.configure(); const MockERC20 = artifacts.require("MockERC20"); const SavingsVault = artifacts.require("BoostedSavingsVault"); const MockStakingContract = artifacts.require("MockStakingContract"); +const MockProxy = artifacts.require("MockProxy"); interface StakingBalance { raw: BN; @@ -114,13 +115,20 @@ contract("SavingsVault", async (accounts) => { rewardToken = await MockERC20.new("Reward", "RWD", 18, rewardsDistributor, 10000000); imUSD = await MockERC20.new("Interest bearing mUSD", "imUSD", 18, sa.default, 1000000); stakingContract = await MockStakingContract.new(); - return SavingsVault.new( - nexusAddress, - imUSD.address, - stakingContract.address, - rewardToken.address, - rewardsDistributor, - ); + + const proxy = await MockProxy.new(); + const impl = await SavingsVault.new(); + const data: string = impl.contract.methods + .initialize( + nexusAddress, + imUSD.address, + stakingContract.address, + rewardToken.address, + rewardsDistributor, + ) + .encodeABI(); + await proxy.methods["initialize(address,address,bytes)"](impl.address, sa.dummy4, data); + return SavingsVault.at(proxy.address); }; const snapshotStakingData = async ( @@ -794,8 +802,6 @@ contract("SavingsVault", async (accounts) => { await imUSD.transfer(staker2, staker2Stake); await imUSD.transfer(staker3, staker3Stake); }); - // TODO - add boost for Staker 1 and staker 2 - // - reward accrual rate only changes AFTER the action it("should accrue rewards on a pro rata basis", async () => { /* * 0 1 2 <-- Weeks @@ -916,13 +922,19 @@ contract("SavingsVault", async (accounts) => { rewardToken = await MockERC20.new("Reward", "RWD", 12, rewardsDistributor, 1000000); imUSD = await MockERC20.new("Interest bearing mUSD", "imUSD", 16, sa.default, 1000000); stakingContract = await MockStakingContract.new(); - savingsVault = await SavingsVault.new( - systemMachine.nexus.address, - imUSD.address, - stakingContract.address, - rewardToken.address, - rewardsDistributor, - ); + const proxy = await MockProxy.new(); + const impl = await SavingsVault.new(); + const data: string = impl.contract.methods + .initialize( + systemMachine.nexus.address, + imUSD.address, + stakingContract.address, + rewardToken.address, + rewardsDistributor, + ) + .encodeABI(); + await proxy.methods["initialize(address,address,bytes)"](impl.address, sa.dummy4, data); + savingsVault = await SavingsVault.at(proxy.address); }); it("should not affect the pro rata payouts", async () => { // Add 100 reward tokens