Merge pull request #2279 from 0xProject/fix/3.0-audit/staking/assert-valid-params-in-MixinParams

Assert storage params when calling `MixinParams.setParams()`.

contracts/staking/CHANGELOG.json

@@ -13,6 +13,10 @@
             {
                 "note": "Removed protocol fee != 0 assertion.",
                 "pr": 2278
+            },
+            {
+                "note": "Call `StakingProxy.assertValidStorageParams()` in `MixinParams.setParams()`",
+                "pr": 2279
             }
         ]
     },

contracts/staking/contracts/src/StakingProxy.sol

@@ -157,8 +157,8 @@ contract StakingProxy is
     //       Asserts that a stake weight is <= 100%.
     //       Asserts that pools allow >= 1 maker.
     //       Asserts that all addresses are initialized.
-    function _assertValidStorageParams()
+    function assertValidStorageParams()
-        internal
+        public
         view
     {
         // Epoch length must be between 5 and 30 days long
@@ -216,6 +216,6 @@ contract StakingProxy is
         }
 
         // Assert initialized storage values are valid
-        _assertValidStorageParams();
+        assertValidStorageParams();
     }
 }

contracts/staking/contracts/src/interfaces/IStakingProxy.sol

@@ -55,4 +55,13 @@ contract IStakingProxy {
         external
         view
         returns (IStructs.ReadOnlyState memory);
+
+    /// @dev Asserts that an epoch is between 5 and 30 days long.
+    //       Asserts that 0 < cobb douglas alpha value <= 1.
+    //       Asserts that a stake weight is <= 100%.
+    //       Asserts that pools allow >= 1 maker.
+    //       Asserts that all addresses are initialized.
+    function assertValidStorageParams()
+        external
+        view;
 }

contracts/staking/contracts/src/sys/MixinParams.sol

@@ -22,6 +22,7 @@ import "@0x/contracts-utils/contracts/src/LibRichErrors.sol";
 import "../immutable/MixinStorage.sol";
 import "../immutable/MixinConstants.sol";
 import "../interfaces/IStakingEvents.sol";
+import "../interfaces/IStakingProxy.sol";
 import "../libs/LibStakingRichErrors.sol";
 
 
@@ -53,6 +54,10 @@ contract MixinParams is
             _cobbDouglasAlphaNumerator,
             _cobbDouglasAlphaDenominator
         );
+
+        // Let the staking proxy enforce that these parameters are within
+        // acceptable ranges.
+        IStakingProxy(address(this)).assertValidStorageParams();
     }
 
     /// @dev Retrieves all configurable parameter values.

contracts/staking/contracts/test/TestAssertStorageParams.sol

@@ -49,7 +49,7 @@ contract TestAssertStorageParams is
         minimumPoolStake = params.minimumPoolStake;
         cobbDouglasAlphaNumerator = params.cobbDouglasAlphaNumerator;
         cobbDouglasAlphaDenominator = params.cobbDouglasAlphaDenominator;
-        _assertValidStorageParams();
+        assertValidStorageParams();
     }
 
     function _attachStakingContract(address)

contracts/staking/contracts/test/TestMixinParams.sol

@@ -0,0 +1,47 @@
+/*
+
+  Copyright 2019 ZeroEx Intl.
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+
+*/
+
+pragma solidity ^0.5.9;
+pragma experimental ABIEncoderV2;
+
+import "../src/sys/MixinParams.sol";
+
+
+contract TestMixinParams is
+    MixinParams
+{
+    bool public shouldFailAssertValidStorageParams;
+
+    /// @dev Set `shouldFailAssertValidStorageParams`
+    function setShouldFailAssertValidStorageParams(bool shouldFail)
+        external
+    {
+        shouldFailAssertValidStorageParams = shouldFail;
+    }
+
+    /// @dev `IStakingProxy.assertValidStorageParams()` that reverts if
+    ///      `shouldFailAssertValidStorageParams` is true.
+    function assertValidStorageParams()
+        public
+        view
+    {
+        if (shouldFailAssertValidStorageParams) {
+            revert("ASSERT_VALID_STORAGE_PARAMS_FAILED");
+        }
+    }
+}

contracts/staking/contracts/test/TestStakingProxy.sol

@@ -35,8 +35,8 @@ contract TestStakingProxy is
         )
     {}
 
-    function _assertValidStorageParams()
+    function assertValidStorageParams()
-        internal
+        public
         view
     {
         require(

contracts/staking/package.json

@@ -37,7 +37,7 @@
     },
     "config": {
         "abis:comment": "This list is auto-generated by contracts-gen. Don't edit manually.",
-        "abis": "./generated-artifacts/@(IStaking|IStakingEvents|IStakingProxy|IStorage|IStorageInit|IStructs|IZrxVault|LibCobbDouglas|LibFixedMath|LibFixedMathRichErrors|LibProxy|LibSafeDowncast|LibStakingRichErrors|MixinAbstract|MixinConstants|MixinCumulativeRewards|MixinDeploymentConstants|MixinExchangeFees|MixinExchangeManager|MixinFinalizer|MixinParams|MixinScheduler|MixinStake|MixinStakeBalances|MixinStakeStorage|MixinStakingPool|MixinStakingPoolRewards|MixinStorage|ReadOnlyProxy|Staking|StakingProxy|TestAssertStorageParams|TestCobbDouglas|TestCumulativeRewardTracking|TestDelegatorRewards|TestExchangeManager|TestFinalizer|TestInitTarget|TestLibFixedMath|TestLibProxy|TestLibProxyReceiver|TestLibSafeDowncast|TestMixinStake|TestMixinStakeStorage|TestProtocolFees|TestStaking|TestStakingNoWETH|TestStakingProxy|TestStorageLayoutAndConstants|ZrxVault|ZrxVaultBackstop).json"
+        "abis": "./generated-artifacts/@(IStaking|IStakingEvents|IStakingProxy|IStorage|IStorageInit|IStructs|IZrxVault|LibCobbDouglas|LibFixedMath|LibFixedMathRichErrors|LibProxy|LibSafeDowncast|LibStakingRichErrors|MixinAbstract|MixinConstants|MixinCumulativeRewards|MixinDeploymentConstants|MixinExchangeFees|MixinExchangeManager|MixinFinalizer|MixinParams|MixinScheduler|MixinStake|MixinStakeBalances|MixinStakeStorage|MixinStakingPool|MixinStakingPoolRewards|MixinStorage|ReadOnlyProxy|Staking|StakingProxy|TestAssertStorageParams|TestCobbDouglas|TestCumulativeRewardTracking|TestDelegatorRewards|TestExchangeManager|TestFinalizer|TestInitTarget|TestLibFixedMath|TestLibProxy|TestLibProxyReceiver|TestLibSafeDowncast|TestMixinParams|TestMixinStake|TestMixinStakeStorage|TestProtocolFees|TestStaking|TestStakingNoWETH|TestStakingProxy|TestStorageLayoutAndConstants|ZrxVault|ZrxVaultBackstop).json"
     },
     "repository": {
         "type": "git",

contracts/staking/src/artifacts.ts

@@ -47,6 +47,7 @@ import * as TestLibFixedMath from '../generated-artifacts/TestLibFixedMath.json'
 import * as TestLibProxy from '../generated-artifacts/TestLibProxy.json';
 import * as TestLibProxyReceiver from '../generated-artifacts/TestLibProxyReceiver.json';
 import * as TestLibSafeDowncast from '../generated-artifacts/TestLibSafeDowncast.json';
+import * as TestMixinParams from '../generated-artifacts/TestMixinParams.json';
 import * as TestMixinStake from '../generated-artifacts/TestMixinStake.json';
 import * as TestMixinStakeStorage from '../generated-artifacts/TestMixinStakeStorage.json';
 import * as TestProtocolFees from '../generated-artifacts/TestProtocolFees.json';
@@ -101,6 +102,7 @@ export const artifacts = {
     TestLibProxy: TestLibProxy as ContractArtifact,
     TestLibProxyReceiver: TestLibProxyReceiver as ContractArtifact,
     TestLibSafeDowncast: TestLibSafeDowncast as ContractArtifact,
+    TestMixinParams: TestMixinParams as ContractArtifact,
     TestMixinStake: TestMixinStake as ContractArtifact,
     TestMixinStakeStorage: TestMixinStakeStorage as ContractArtifact,
     TestProtocolFees: TestProtocolFees as ContractArtifact,

contracts/staking/src/wrappers.ts

@@ -45,6 +45,7 @@ export * from '../generated-wrappers/test_lib_fixed_math';
 export * from '../generated-wrappers/test_lib_proxy';
 export * from '../generated-wrappers/test_lib_proxy_receiver';
 export * from '../generated-wrappers/test_lib_safe_downcast';
+export * from '../generated-wrappers/test_mixin_params';
 export * from '../generated-wrappers/test_mixin_stake';
 export * from '../generated-wrappers/test_mixin_stake_storage';
 export * from '../generated-wrappers/test_protocol_fees';

contracts/staking/test/migration_test.ts

@@ -228,7 +228,7 @@ blockchainTests('Migration tests', env => {
             const expectedError = new StakingRevertErrors.InvalidParamValueError(
                 StakingRevertErrors.InvalidParamValueErrorCodes.InvalidEpochDuration,
             );
-            expect(tx).to.revertWith(expectedError);
+            return expect(tx).to.revertWith(expectedError);
         });
         it('reverts if epoch duration is > 30 days', async () => {
             const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({
@@ -238,21 +238,21 @@ blockchainTests('Migration tests', env => {
             const expectedError = new StakingRevertErrors.InvalidParamValueError(
                 StakingRevertErrors.InvalidParamValueErrorCodes.InvalidEpochDuration,
             );
-            expect(tx).to.revertWith(expectedError);
+            return expect(tx).to.revertWith(expectedError);
         });
         it('succeeds if epoch duration is 5 days', async () => {
             const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({
                 ...stakingConstants.DEFAULT_PARAMS,
                 epochDurationInSeconds: fiveDays,
             });
-            expect(tx).to.be.fulfilled('');
+            return expect(tx).to.be.fulfilled('');
         });
         it('succeeds if epoch duration is 30 days', async () => {
             const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({
                 ...stakingConstants.DEFAULT_PARAMS,
                 epochDurationInSeconds: thirtyDays,
             });
-            expect(tx).to.be.fulfilled('');
+            return expect(tx).to.be.fulfilled('');
         });
         it('reverts if alpha denominator is 0', async () => {
             const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({
@@ -262,7 +262,7 @@ blockchainTests('Migration tests', env => {
             const expectedError = new StakingRevertErrors.InvalidParamValueError(
                 StakingRevertErrors.InvalidParamValueErrorCodes.InvalidCobbDouglasAlpha,
             );
-            expect(tx).to.revertWith(expectedError);
+            return expect(tx).to.revertWith(expectedError);
         });
         it('reverts if alpha > 1', async () => {
             const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({
@@ -273,7 +273,7 @@ blockchainTests('Migration tests', env => {
             const expectedError = new StakingRevertErrors.InvalidParamValueError(
                 StakingRevertErrors.InvalidParamValueErrorCodes.InvalidCobbDouglasAlpha,
             );
-            expect(tx).to.revertWith(expectedError);
+            return expect(tx).to.revertWith(expectedError);
         });
         it('succeeds if alpha == 1', async () => {
             const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({
@@ -281,7 +281,7 @@ blockchainTests('Migration tests', env => {
                 cobbDouglasAlphaNumerator: new BigNumber(1),
                 cobbDouglasAlphaDenominator: new BigNumber(1),
             });
-            expect(tx).to.be.fulfilled('');
+            return expect(tx).to.be.fulfilled('');
         });
         it('succeeds if alpha == 0', async () => {
             const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({
@@ -289,7 +289,7 @@ blockchainTests('Migration tests', env => {
                 cobbDouglasAlphaNumerator: constants.ZERO_AMOUNT,
                 cobbDouglasAlphaDenominator: new BigNumber(1),
             });
-            expect(tx).to.be.fulfilled('');
+            return expect(tx).to.be.fulfilled('');
         });
         it('reverts if delegation weight is > 100%', async () => {
             const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({
@@ -299,14 +299,14 @@ blockchainTests('Migration tests', env => {
             const expectedError = new StakingRevertErrors.InvalidParamValueError(
                 StakingRevertErrors.InvalidParamValueErrorCodes.InvalidRewardDelegatedStakeWeight,
             );
-            expect(tx).to.revertWith(expectedError);
+            return expect(tx).to.revertWith(expectedError);
         });
         it('succeeds if delegation weight is 100%', async () => {
             const tx = proxyContract.setAndAssertParams.awaitTransactionSuccessAsync({
                 ...stakingConstants.DEFAULT_PARAMS,
                 rewardDelegatedStakeWeight: new BigNumber(stakingConstants.PPM),
             });
-            expect(tx).to.be.fulfilled('');
+            return expect(tx).to.be.fulfilled('');
         });
     });
 });

contracts/staking/test/unit_tests/params_test.ts

@@ -3,20 +3,20 @@ import { AuthorizableRevertErrors, BigNumber } from '@0x/utils';
 import { TransactionReceiptWithDecodedLogs } from 'ethereum-types';
 import * as _ from 'lodash';
 
-import { artifacts, IStakingEventsParamsSetEventArgs, MixinParamsContract } from '../../src/';
+import { artifacts, IStakingEventsParamsSetEventArgs, TestMixinParamsContract } from '../../src/';
 
 import { constants as stakingConstants } from '../utils/constants';
 import { StakingParams } from '../utils/types';
 
 blockchainTests('Configurable Parameters unit tests', env => {
-    let testContract: MixinParamsContract;
+    let testContract: TestMixinParamsContract;
     let authorizedAddress: string;
     let notAuthorizedAddress: string;
 
     before(async () => {
         [authorizedAddress, notAuthorizedAddress] = await env.getAccountAddressesAsync();
-        testContract = await MixinParamsContract.deployFrom0xArtifactAsync(
+        testContract = await TestMixinParamsContract.deployFrom0xArtifactAsync(
-            artifacts.MixinParams,
+            artifacts.TestMixinParams,
             env.provider,
             env.txDefaults,
             artifacts,
@@ -66,6 +66,12 @@ blockchainTests('Configurable Parameters unit tests', env => {
             return expect(tx).to.revertWith(expectedError);
         });
 
+        it('throws if `assertValidStorageParams()` throws`', async () => {
+            await testContract.setShouldFailAssertValidStorageParams.awaitTransactionSuccessAsync(true);
+            const tx = setParamsAndAssertAsync({});
+            return expect(tx).to.revertWith('ASSERT_VALID_STORAGE_PARAMS_FAILED');
+        });
+
         it('works if called by owner', async () => {
             return setParamsAndAssertAsync({});
         });

contracts/staking/tsconfig.json

@@ -45,6 +45,7 @@
         "generated-artifacts/TestLibProxy.json",
         "generated-artifacts/TestLibProxyReceiver.json",
         "generated-artifacts/TestLibSafeDowncast.json",
+        "generated-artifacts/TestMixinParams.json",
         "generated-artifacts/TestMixinStake.json",
         "generated-artifacts/TestMixinStakeStorage.json",
         "generated-artifacts/TestProtocolFees.json",