Add fee value check (#209)

* Add fee value check
omni · Jun 28, 2019 · 5e845af · 5e845af
1 parent c28770b
commit 5e845af
Show file tree

Hide file tree

Showing 5 changed files with 146 additions and 5 deletions.
diff --git a/contracts/upgradeable_contracts/BaseFeeManager.sol b/contracts/upgradeable_contracts/BaseFeeManager.sol
@@ -12,16 +12,23 @@ contract BaseFeeManager is EternalStorage, FeeTypes {
     event HomeFeeUpdated(uint256 fee);
     event ForeignFeeUpdated(uint256 fee);
 
+    // This is not a real fee value but a relative value used to calculate the fee percentage
+    uint256 internal constant MAX_FEE = 1 ether;
+
     function calculateFee(uint256 _value, bool _recover, bytes32 _feeType) public view returns(uint256) {
         uint256 fee = _feeType == HOME_FEE ? getHomeFee() : getForeignFee();
-        uint256 eth = 1 ether;
         if (!_recover) {
-            return _value.mul(fee).div(eth);
+            return _value.mul(fee).div(MAX_FEE);
         }
-        return _value.mul(fee).div(eth.sub(fee));
+        return _value.mul(fee).div(MAX_FEE.sub(fee));
+    }
+
+    modifier validFee(uint256 _fee) {
+        require(_fee < MAX_FEE);
+        _;
     }
 
-    function setHomeFee(uint256 _fee) external {
+    function setHomeFee(uint256 _fee) external validFee(_fee) {
         uintStorage[keccak256(abi.encodePacked("homeFee"))] = _fee;
         emit HomeFeeUpdated(_fee);
     }
@@ -30,7 +37,7 @@ contract BaseFeeManager is EternalStorage, FeeTypes {
         return uintStorage[keccak256(abi.encodePacked("homeFee"))];
     }
 
-    function setForeignFee(uint256 _fee) external {
+    function setForeignFee(uint256 _fee) external validFee(_fee) {
         uintStorage[keccak256(abi.encodePacked("foreignFee"))] = _fee;
         emit ForeignFeeUpdated(_fee);
     }

diff --git a/test/erc_to_erc/home_bridge.test.js b/test/erc_to_erc/home_bridge.test.js
@@ -1185,6 +1185,45 @@ contract('HomeBridge_ERC20_to_ERC20', async accounts => {
       bridgeHomeFee.should.be.bignumber.equal(newHomeFee)
       bridgeForeignFee.should.be.bignumber.equal(newForeignFee)
     })
+    it('fee should be less than 100%', async () => {
+      const feeManager = await FeeManagerErcToErcPOSDAO.new()
+      await homeBridge.rewardableInitialize(
+        rewardableValidators.address,
+        oneEther,
+        halfEther,
+        minPerTx,
+        gasPrice,
+        requireBlockConfirmations,
+        token.address,
+        foreignDailyLimit,
+        foreignMaxPerTx,
+        owner,
+        feeManager.address,
+        homeFee,
+        foreignFee,
+        blockRewardContract.address
+      ).should.be.fulfilled
+
+      // Given
+      const invalidFee = ether('1')
+      const invalidBigFee = ether('2')
+      const newHomeFee = ether('0.99')
+      const newForeignFee = ether('0.99')
+
+      // When
+      await homeBridge.setHomeFee(invalidFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      await homeBridge.setForeignFee(invalidFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+
+      await homeBridge.setHomeFee(invalidBigFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      await homeBridge.setForeignFee(invalidBigFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+
+      await homeBridge.setHomeFee(newHomeFee, { from: owner }).should.be.fulfilled
+      await homeBridge.setForeignFee(newForeignFee, { from: owner }).should.be.fulfilled
+
+      // Then
+      expect(await homeBridge.getHomeFee()).to.be.bignumber.equals(newHomeFee)
+      expect(await homeBridge.getForeignFee()).to.be.bignumber.equals(newForeignFee)
+    })
     it('should be able to get fee manager mode', async () => {
       // Given
       const feeManager = await FeeManagerErcToErcPOSDAO.new()

diff --git a/test/erc_to_native/home_bridge.test.js b/test/erc_to_native/home_bridge.test.js
@@ -542,6 +542,41 @@ contract('HomeBridge_ERC20_to_Native', async accounts => {
       const bridgeForeignFee = await homeContract.getForeignFee()
       bridgeForeignFee.should.be.bignumber.equal(newForeignFee)
     })
+    it('fee should be less than 100%', async () => {
+      await homeContract.rewardableInitialize(
+        validatorContract.address,
+        '3',
+        '2',
+        '1',
+        gasPrice,
+        requireBlockConfirmations,
+        blockRewardContract.address,
+        foreignDailyLimit,
+        foreignMaxPerTx,
+        owner,
+        feeManager.address,
+        homeFee,
+        foreignFee
+      ).should.be.fulfilled
+
+      const invalidFee = ether('1')
+      const invalidBigFee = ether('2')
+
+      await homeContract.setHomeFee(invalidFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      await homeContract.setForeignFee(invalidFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+
+      await homeContract.setHomeFee(invalidBigFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      await homeContract.setForeignFee(invalidBigFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+
+      const newHomeFee = ether('0.99')
+      const newForeignFee = ether('0.99')
+
+      await homeContract.setHomeFee(newHomeFee, { from: owner }).should.be.fulfilled
+      await homeContract.setForeignFee(newForeignFee, { from: owner }).should.be.fulfilled
+
+      expect(await homeContract.getHomeFee()).to.be.bignumber.equals(newHomeFee)
+      expect(await homeContract.getForeignFee()).to.be.bignumber.equals(newForeignFee)
+    })
   })
 
   describe('#fallback', async () => {

diff --git a/test/native_to_erc/foreign_bridge_test.js b/test/native_to_erc/foreign_bridge_test.js
@@ -961,6 +961,36 @@ contract('ForeignBridge', async accounts => {
       // Then
       expect(await foreignBridge.getHomeFee()).to.be.bignumber.equals(newHomeFee)
     })
+    it('fee should be less than 100%', async () => {
+      const feeManager = await FeeManagerNativeToErc.new()
+      await foreignBridge.rewardableInitialize(
+        rewardableValidators.address,
+        token.address,
+        oneEther,
+        halfEther,
+        minPerTx,
+        gasPrice,
+        requireBlockConfirmations,
+        homeDailyLimit,
+        homeMaxPerTx,
+        owner,
+        feeManager.address,
+        homeFee
+      ).should.be.fulfilled
+
+      // Given
+      const invalidFee = ether('1')
+      const invalidBigFee = ether('2')
+      const newHomeFee = ether('0.99')
+
+      // When
+      await foreignBridge.setHomeFee(invalidFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      await foreignBridge.setHomeFee(invalidBigFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      await foreignBridge.setHomeFee(newHomeFee, { from: owner }).should.be.fulfilled
+
+      // Then
+      expect(await foreignBridge.getHomeFee()).to.be.bignumber.equals(newHomeFee)
+    })
 
     it('should be able to get fee manager mode', async () => {
       // Given

diff --git a/test/native_to_erc/home_bridge_test.js b/test/native_to_erc/home_bridge_test.js
@@ -993,6 +993,36 @@ contract('HomeBridge', async accounts => {
       const bridgeForeignFee = await homeBridge.getForeignFee()
       bridgeForeignFee.should.be.bignumber.equal(newForeignFee)
     })
+    it('fee should be less than 100%', async () => {
+      const feeManager = await FeeManagerNativeToErc.new()
+      await homeBridge.rewardableInitialize(
+        rewardableValidators.address,
+        oneEther,
+        halfEther,
+        minPerTx,
+        gasPrice,
+        requireBlockConfirmations,
+        foreignDailyLimit,
+        foreignMaxPerTx,
+        owner,
+        feeManager.address,
+        homeFee,
+        foreignFee
+      ).should.be.fulfilled
+
+      // Given
+      const invalidFee = ether('1')
+      const invalidBigFee = ether('2')
+      const newForeignFee = ether('0.99')
+
+      // When
+      await homeBridge.setForeignFee(invalidFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      await homeBridge.setForeignFee(invalidBigFee, { from: owner }).should.be.rejectedWith(ERROR_MSG)
+      await homeBridge.setForeignFee(newForeignFee, { from: owner }).should.be.fulfilled
+
+      // Then
+      expect(await homeBridge.getForeignFee()).to.be.bignumber.equals(newForeignFee)
+    })
     it('should be able to get fee manager mode', async () => {
       // Given
       const feeManager = await FeeManagerNativeToErc.new()