[P1-L08] Add input validation throughout DVM (#1212)

* Added input validation

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

* updated multi-roll

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

* Update VoteTiming.sol

* added comment

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

* added back require

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

* Added back require

Signed-off-by: Christopher Maree <christopher.maree@gmail.com>

* Update VoteTiming.sol

* Update core/contracts/common/implementation/MultiRole.sol

Co-Authored-By: Matt Rice <matthewcrice32@gmail.com>

Co-authored-by: Matt Rice <matthewcrice32@gmail.com>

core/contracts/common/implementation/MultiRole.sol

@@ -35,6 +35,7 @@ library Shared {
     }
 
     function addMember(RoleMembership storage roleMembership, address memberToAdd) internal {
+        require(memberToAdd != address(0x0), "Cannot add 0x0 to a shared role");
         roleMembership.members[memberToAdd] = true;
     }
 

core/contracts/oracle/implementation/Store.sol

@@ -137,6 +137,7 @@ contract Store is StoreInterface, Withdrawable {
      * @param newWeeklyDelayFee fee escalation per week of late fee payment.
      */
     function setWeeklyDelayFee(FixedPoint.Unsigned memory newWeeklyDelayFee) public onlyRoleHolder(uint(Roles.Owner)) {
+        require(newWeeklyDelayFee.isLessThan(1), "weekly delay fee must be < 100%");
         weeklyDelayFee = newWeeklyDelayFee;
         emit NewWeeklyDelayFee(newWeeklyDelayFee);
     }

core/contracts/oracle/implementation/VoteTiming.sol

@@ -18,6 +18,8 @@ library VoteTiming {
      * @notice Initializes the data object. Sets the phase length based on the input.
      */
     function init(Data storage data, uint256 phaseLength) internal {
+        // This should have a require message but this results in an internal Solidity error.
+        require(phaseLength > 0);
         data.phaseLength = phaseLength;
     }
 

core/test/common/MultiRole.js

@@ -101,6 +101,9 @@ contract("MultiRole", function(accounts) {
     assert.isTrue(await multiRole.holdsRole("1", account3));
     await multiRole.revertIfNotHoldingRole("1", { from: account3 });
 
+    // Cannot set a shared role role to the 0x0 address.
+    assert(await didContractThrow(multiRole.addMember("1", zeroAddress, { from: account2 })));
+
     // Anyone who holds the role should be able to remove members.
     await multiRole.removeMember("1", account2, { from: account3 });
     assert.isFalse(await multiRole.holdsRole("1", account2));

core/test/oracle/Store.js

@@ -62,6 +62,14 @@ contract("Store", function(accounts) {
     let highFee = { rawValue: web3.utils.toWei("1", "ether") };
     assert(await didContractThrow(store.setFixedOracleFeePerSecond(highFee, { from: owner })));
 
+    // Can set weekly late fees to less than 100%.
+    await store.setWeeklyDelayFee({ rawValue: web3.utils.toWei("0.99", "ether") }, { from: owner });
+
+    // Disallow setting fees >= 100%.
+    assert(
+      await didContractThrow(store.setWeeklyDelayFee({ rawValue: web3.utils.toWei("1", "ether") }, { from: owner }))
+    );
+
     // TODO Check that only permitted role can change the fee
   });
 

core/test/oracle/VoteTiming.js

@@ -1,3 +1,5 @@
+const { didContractThrow } = require("../../../common/SolidityTestUtils.js");
+
 const VoteTimingTest = artifacts.require("VoteTimingTest");
 
 contract("VoteTiming", function(accounts) {
@@ -6,6 +8,10 @@ contract("VoteTiming", function(accounts) {
   beforeEach(async function() {
     voteTiming = await VoteTimingTest.new("100");
   });
+  it("Reject invalid init params", async function() {
+    // Should not be able to create an instance of VoteTiming with 0 phase length.
+    assert(await didContractThrow(VoteTimingTest.new("0")));
+  });
 
   it("Phasing", async function() {
     // If time % 200 is between 0 and 99 (inclusive), the phase should be commit.