Merge pull request #3 from poanetwork/audit-fixes

Audit fixes
poanetwork · May 1, 2020 · 5d6c91d · 5d6c91d
2 parents cdc6793 + 19450cb
commit 5d6c91d
Show file tree

Hide file tree

Showing 9 changed files with 146 additions and 25 deletions.
diff --git a/README.md b/README.md
@@ -73,3 +73,6 @@ The owner can only change the game parameters.
 The Proxy Admin can upgrade the logic of the contracts. This role will be abolished after an audit and some testing time.
 
 *`Note: All methods are described in code.`*
+
+## Dependencies
+The project uses the modified library `SortitionSumTreeFactory` with added `total()` and `numberOfNodes()` functions. You can find the modified implementation here: [@poanetwork/kleros](https://github.com/poanetwork/kleros). These changes were not audited but they add minimal risk.
diff --git a/contracts/DrawManager.sol b/contracts/DrawManager.sol
@@ -1,4 +1,4 @@
-pragma solidity ^0.5.16;
+pragma solidity 0.5.16;
 
 import "@kleros/kleros/contracts/data-structures/SortitionSumTreeFactory.sol";
 import "@openzeppelin/contracts-ethereum-package/contracts/math/SafeMath.sol";

diff --git a/contracts/IPOSDAORandom.sol b/contracts/IPOSDAORandom.sol
@@ -1,4 +1,4 @@
-pragma solidity ^0.5.16;
+pragma solidity 0.5.16;
 
 interface IPOSDAORandom {
     function collectRoundLength() external view returns(uint256);

diff --git a/contracts/PoaMania.sol b/contracts/PoaMania.sol
@@ -1,4 +1,4 @@
-pragma solidity ^0.5.16;
+pragma solidity 0.5.16;
 
 import "@openzeppelin/contracts-ethereum-package/contracts/ownership/Ownable.sol";
 import "@openzeppelin/contracts-ethereum-package/contracts/math/SafeMath.sol";
@@ -155,6 +155,8 @@ contract PoaMania is Ownable {
         require(_owner != address(0), "zero address");
         // initializes all params
         Ownable.initialize(_owner);
+        random.init(_randomContract);
+        _setBlockTime(_blockTime);
         _setRoundDuration(_roundDuration);
         _setFee(_fee);
         _setFeeReceiver(_feeReceiver);
@@ -163,11 +165,9 @@ contract PoaMania is Ownable {
         _setExecutorShare(_executorShare);
         _validateSumOfShares();
         _setPrizeSizes(_prizeSizes);
-        _setBlockTime(_blockTime);
-        _setMinDeposit(_minDeposit);
         _setMaxDeposit(_maxDeposit);
+        _setMinDeposit(_minDeposit);
         jackpot = 0;
-        random.init(_randomContract);
         // creates a tree of nodes
         drawManager.create();
         // starts the first round
@@ -418,8 +418,16 @@ contract PoaMania is Ownable {
      * @return The timestamp when deposits and withdrawals will be locked
      */
     function getLockStart() public view returns (uint256) {
+        uint256 lockDuration = getLockDuration();
+        return startedAt.add(roundDuration).sub(lockDuration);
+    }
+
+    /**
+     * @dev Returns lock duration
+     */
+    function getLockDuration() public view returns (uint256) {
         uint256 randomUpdateInterval = random.getUpdateInterval();
-        return startedAt.add(roundDuration).sub(randomUpdateInterval.mul(2).mul(blockTime));
+        return randomUpdateInterval.mul(2).mul(blockTime);
     }
 
     /**
@@ -472,11 +480,12 @@ contract PoaMania is Ownable {
 
     /**
      * @dev Sets the round duration
-     * Reverts if the value is zero
+     * Reverts if the value is less than 2 lock durations
      * @param _roundDuration The round duration (in seconds)
      */
     function _setRoundDuration(uint256 _roundDuration) internal {
-        require(_roundDuration > 0, "should be greater than 0");
+        uint256 lockDuration = getLockDuration();
+        require(_roundDuration >= lockDuration.mul(2), "should be greater than or equal to 2 lock durations");
         roundDuration = _roundDuration;
     }
 
@@ -531,7 +540,12 @@ contract PoaMania is Ownable {
      */
     function _setPrizeSizes(uint256[2] memory _prizeSizes) internal {
         uint256 sum = _prizeSizes[0].add(_prizeSizes[1]);
-        require(sum > 0 && sum <= 1 ether, "should be less than or equal to 1 ether");
+        require(sum > 0 && sum < 1 ether, "should be greater than 0 and less than 1 ether");
+        uint256 thirdPrize = uint256(1 ether).sub(sum);
+        require(
+            _prizeSizes[0] >= _prizeSizes[1] && _prizeSizes[1] >= thirdPrize,
+            "should satisfy: 1st prize >= 2nd prize >= 3rd prize"
+        );
         prizeSizes = _prizeSizes;
     }
 
@@ -550,6 +564,7 @@ contract PoaMania is Ownable {
      * @param _minDeposit The minimum deposit
      */
     function _setMinDeposit(uint256 _minDeposit) internal {
+        require(_minDeposit <= maxDeposit, "should be less than or equal to max deposit");
         minDeposit = _minDeposit;
     }
 
@@ -558,6 +573,7 @@ contract PoaMania is Ownable {
      * @param _maxDeposit The maximum deposit
      */
     function _setMaxDeposit(uint256 _maxDeposit) internal {
+        require(_maxDeposit >= minDeposit, "should be greater than or equal to min deposit");
         maxDeposit = _maxDeposit;
     }
 

diff --git a/contracts/Random.sol b/contracts/Random.sol
@@ -1,4 +1,4 @@
-pragma solidity ^0.5.16;
+pragma solidity 0.5.16;
 
 import "./IPOSDAORandom.sol";
 

diff --git a/contracts/Sacrifice.sol b/contracts/Sacrifice.sol
@@ -1,4 +1,4 @@
-pragma solidity ^0.5.16;
+pragma solidity 0.5.16;
 
 contract Sacrifice {
     constructor(address payable _recipient) public payable {

diff --git a/contracts/mocks/RandomMock.sol b/contracts/mocks/RandomMock.sol
@@ -1,4 +1,4 @@
-pragma solidity ^0.5.16;
+pragma solidity 0.5.16;
 
 import '../IPOSDAORandom.sol';
 

diff --git a/contracts/mocks/ReceiverMock.sol b/contracts/mocks/ReceiverMock.sol
@@ -1,4 +1,4 @@
-pragma solidity ^0.5.16;
+pragma solidity 0.5.16;
 
 import "../PoaMania.sol";
 

diff --git a/test/PoaMania.test.js b/test/PoaMania.test.js
@@ -10,7 +10,7 @@ const ReceiverMock = artifacts.require('ReceiverMock');
 
 contract('PoaMania', accounts => {
   const [owner, firstParticipant, secondParticipant, thirdParticipant, fourthParticipant, fifthParticipant] = accounts;
-  const roundDuration = new BN(600);                       // in seconds
+  const roundDuration = new BN(800);                       // in seconds
   const blockTime = new BN(5);                             // in seconds
   const minDeposit = ether('10');                  // 10 POA
   const maxDeposit = ether('500000');              // 500,000 POA
@@ -151,7 +151,7 @@ contract('PoaMania', accounts => {
           jackpotShare.toString(),
           jackpotChance.toString(),
         ),
-        'should be greater than 0'
+        'should be greater than or equal to 2 lock durations'
       );
       await expectRevert(
         initialize(
@@ -170,6 +170,23 @@ contract('PoaMania', accounts => {
         ),
         'should be greater than 0'
       );
+      await expectRevert(
+        initialize(
+          owner,
+          randomContract.address,
+          roundDuration.toString(),
+          blockTime.toString(),
+          minDeposit.toString(),
+          maxDeposit.toString(),
+          [0, 0],
+          fee.toString(),
+          feeReceiver,
+          roundCloserShare.toString(),
+          jackpotShare.toString(),
+          jackpotChance.toString(),
+        ),
+        'should be greater than 0 and less than 1 ether'
+      );
       await expectRevert(
         initialize(
           owner,
@@ -185,7 +202,41 @@ contract('PoaMania', accounts => {
           jackpotShare.toString(),
           jackpotChance.toString(),
         ),
-        'should be less than or equal to 1 ether'
+        'should be greater than 0 and less than 1 ether'
+      );
+      await expectRevert(
+        initialize(
+          owner,
+          randomContract.address,
+          roundDuration.toString(),
+          blockTime.toString(),
+          minDeposit.toString(),
+          maxDeposit.toString(),
+          [ether('0.4'), ether('0.5')].map(item => item.toString()),
+          fee.toString(),
+          feeReceiver,
+          roundCloserShare.toString(),
+          jackpotShare.toString(),
+          jackpotChance.toString(),
+        ),
+        'should satisfy: 1st prize >= 2nd prize >= 3rd prize'
+      );
+      await expectRevert(
+        initialize(
+          owner,
+          randomContract.address,
+          roundDuration.toString(),
+          blockTime.toString(),
+          minDeposit.toString(),
+          maxDeposit.toString(),
+          [ether('0.3'), ether('0.2')].map(item => item.toString()),
+          fee.toString(),
+          feeReceiver,
+          roundCloserShare.toString(),
+          jackpotShare.toString(),
+          jackpotChance.toString(),
+        ),
+        'should satisfy: 1st prize >= 2nd prize >= 3rd prize'
       );
       await expectRevert(
         initialize(
@@ -238,6 +289,23 @@ contract('PoaMania', accounts => {
         ),
         'should be less than or equal to 1 ether'
       );
+      await expectRevert(
+        initialize(
+          owner,
+          randomContract.address,
+          roundDuration.toString(),
+          blockTime.toString(),
+          ether('10').toString(),
+          ether('9').toString(),
+          prizeSizes.map(item => item.toString()),
+          fee.toString(),
+          feeReceiver,
+          roundCloserShare.toString(),
+          jackpotShare.toString(),
+          jackpotChance.toString(),
+        ),
+        'should be less than or equal to max deposit'
+      );
     });
   });
   describe('deposit', () => {
@@ -297,39 +365,39 @@ contract('PoaMania', accounts => {
     });
     it('should withdraw specified amount', async () => {
       await poaMania.deposit({ from: firstParticipant, value: ether('5') });
-      const receipt = await poaMania.withdraw(ether('5'), { from: firstParticipant });
+      const receipt = await poaMania.methods['withdraw(uint256)'](ether('5'), { from: firstParticipant });
       expect(await poaMania.numberOfParticipants()).to.be.bignumber.equal(new BN(1));
       expect(await poaMania.totalDepositedBalance()).to.be.bignumber.equal(minDeposit);
       expect(await poaMania.balanceOf(firstParticipant)).to.be.bignumber.equal(minDeposit);
       expectEvent(receipt, 'Withdrawn', { user: firstParticipant, amount: ether('5') });
     });
     it('fails if zero value', async () => {
       await expectRevert(poaMania.methods['withdraw()']({ from: secondParticipant }), 'zero value');
-      await expectRevert(poaMania.withdraw(0, { from: firstParticipant }), 'zero value');
+      await expectRevert(poaMania.methods['withdraw(uint256)'](0, { from: firstParticipant }), 'zero value');
     });
     it('fails if less than min deposit', async () => {
       await expectRevert(
-        poaMania.withdraw(ether('5'), { from: firstParticipant }),
+        poaMania.methods['withdraw(uint256)'](ether('5'), { from: firstParticipant }),
         'should be greater than or equal to min deposit'
       );
     });
     it('fails if greater than user deposit', async () => {
       await expectRevert(
-        poaMania.withdraw(ether('11'), { from: firstParticipant }),
+        poaMania.methods['withdraw(uint256)'](ether('11'), { from: firstParticipant }),
         'SafeMath: subtraction overflow'
       );
     });
     it('fails if locked', async () => {
       await poaMania.deposit({ from: firstParticipant, value: ether('1') });
-      await poaMania.withdraw(ether('1'), { from: firstParticipant });
+      await poaMania.methods['withdraw(uint256)'](ether('1'), { from: firstParticipant });
       const lockStart = await poaMania.getLockStart();
       await time.increaseTo(lockStart);
       await expectRevert(
         poaMania.methods['withdraw()']({ from: firstParticipant }),
         'locked'
       );
       await expectRevert(
-        poaMania.withdraw(minDeposit, { from: firstParticipant }),
+        poaMania.methods['withdraw(uint256)'](minDeposit, { from: firstParticipant }),
         'locked'
       );
     });
@@ -635,8 +703,13 @@ contract('PoaMania', accounts => {
     it('fails if wrong value', async () => {
       await expectRevert(
         poaMania.setRoundDuration(0, { from: owner }),
-        'should be greater than 0'
+        'should be greater than or equal to 2 lock durations'
       );
+      await expectRevert(
+        poaMania.setRoundDuration(799, { from: owner }),
+        'should be greater than or equal to 2 lock durations'
+      );
+      await poaMania.setRoundDuration(800, { from: owner });
     });
   });
   describe('setFee', () => {
@@ -751,8 +824,21 @@ contract('PoaMania', accounts => {
     it('fails if wrong value', async () => {
       await expectRevert(
         poaMania.setPrizeSizes([ether('0.8'), ether('0.25')], { from: owner }),
-        'should be less than or equal to 1 ether'
+        'should be greater than 0 and less than 1 ether'
+      );
+      await expectRevert(
+        poaMania.setPrizeSizes([0, 0], { from: owner }),
+        'should be greater than 0 and less than 1 ether'
+      );
+      await expectRevert(
+        poaMania.setPrizeSizes([ether('0.4'), ether('0.5')], { from: owner }),
+        'should satisfy: 1st prize >= 2nd prize >= 3rd prize'
+      );
+      await expectRevert(
+        poaMania.setPrizeSizes([ether('0.3'), ether('0.3')], { from: owner }),
+        'should satisfy: 1st prize >= 2nd prize >= 3rd prize'
       );
+      await poaMania.setPrizeSizes([ether('0.4'), ether('0.3')], { from: owner });
     });
   });
   describe('setBlockTime', () => {
@@ -786,6 +872,14 @@ contract('PoaMania', accounts => {
         'Ownable: caller is not the owner'
       );
     });
+    it('fails if greater than max deposit', async () => {
+      await poaMania.setMaxDeposit(ether('100'));
+      await expectRevert(
+        poaMania.setMinDeposit(ether('101')),
+        'should be less than or equal to max deposit'
+      );
+      await poaMania.setMinDeposit(ether('100'));
+    });
   });
   describe('setMaxDeposit', () => {
     it('should set', async () => {
@@ -799,5 +893,13 @@ contract('PoaMania', accounts => {
         'Ownable: caller is not the owner'
       );
     });
+    it('fails if less than min deposit', async () => {
+      await poaMania.setMinDeposit(ether('100'));
+      await expectRevert(
+        poaMania.setMaxDeposit(ether('99')),
+        'should be greater than or equal to min deposit'
+      );
+      await poaMania.setMaxDeposit(ether('100'));
+    });
   });
 });