Merge pull request #24 from GalloDaSballo/fix-quantstamp-fixes

Fix quantstamp fixes

README.md

@@ -40,3 +40,7 @@ Solution:
 Accrue the contract and calculate the totalSupply - thisContract Supply
 
 Status: Mitigated
+
+
+## Potential Attack Vectors to Explore
+- Can you cause the Points to be greater than TotalPoints, allowing to extract more than the rewards for one epoch for one vault?

scripts/claim_simulation.py

@@ -54,7 +54,7 @@ def test_full_deposit_claim_one_year_of_rewards_with_optimization():
   print(initialized_contract.currentEpoch())
 
   ## Claim Bulk
-  tx = initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorage([1, EPOCHS, fake_vault, tokens], {"from": user})
+  tx = initialized_contract.reap([1, EPOCHS, fake_vault, tokens], {"from": user})
 
   print("Gas Cost to add")
   print(EPOCHS)

tests/attacks/test_balance_in_future_reward_steal.py

@@ -39,7 +39,8 @@ def test_accrue_future_balance_out_of_whack(initialized_contract, user, fake_vau
   assert initialized_contract.getBalanceAtEpoch(EPOCH, fake_vault, user)[0] == 0
 
   ## Future epoch baalnce is INITIAL_DEPOSIT ## fixed because can't accrue future
-  assert initialized_contract.getBalanceAtEpoch(EPOCH + 1, fake_vault, user)[0] == 0
+  with brownie.reverts():
+    assert initialized_contract.getBalanceAtEpoch(EPOCH + 1, fake_vault, user)[0] == 0
 
   ## Go next epoch
   chain.sleep(initialized_contract.SECONDS_PER_EPOCH() + 1)
@@ -77,8 +78,9 @@ def test_accrue_future_breaks_time_left_to_accrue(initialized_contract, user, fa
   ## Current epoch balance is 0
   assert initialized_contract.getBalanceAtEpoch(EPOCH, fake_vault, user)[0] == 0
 
-  ## Future epoch baalnce is INITIAL_DEPOSIT
-  assert initialized_contract.getBalanceAtEpoch(EPOCH + 1, fake_vault, user)[0] == 0
+  ## Future epoch baalnce is INITIAL_DEPOSIT | Reverts
+  with brownie.reverts():
+    assert initialized_contract.getBalanceAtEpoch(EPOCH + 1, fake_vault, user)[0] == 0
 
   ## Go next epoch
   chain.sleep(initialized_contract.SECONDS_PER_EPOCH() + 1)

tests/attacks/test_bug_without_storage_looses_shares.py

@@ -0,0 +1,22 @@
+import brownie
+from brownie import *
+from helpers.utils import (
+    approx,
+)
+AddressZero = "0x0000000000000000000000000000000000000000"
+MaxUint256 = str(int(2 ** 256 - 1))
+
+"""
+  Tests for QSP-2
+  Optimized functions will burn shares if user uses them with 1 epoch range (same epoch start and end)
+  -> Post-FIX -> Test Passes because of overflow protection
+"""
+
+def test_can_get_rewards_for_zero_due_to_overflow(initialized_contract, deployer, fake_vault, token):
+  ## Before fix, no revert, rewards are set, but no tokens are transfered | After fix, reverts
+  with brownie.reverts():
+    initialized_contract.addBulkRewards(3, 4, fake_vault, token,[MaxUint256, 1], {"from": deployer})
+
+
+
+

tests/attacks/test_overflow_risk.py

@@ -0,0 +1,59 @@
+import brownie
+from brownie import *
+from helpers.utils import (
+    approx,
+)
+AddressZero = "0x0000000000000000000000000000000000000000"
+MaxUint256 = str(int(2 ** 256 - 1))
+
+"""
+  Tests for QSP-5
+
+  - Overflowing Total Supply cannot happen
+  - Overflowing of points cannot happen
+"""
+
+def test_cannot_overflow_total_supply(initialized_contract, deployer, user, fake_vault, token):
+  initialized_contract.notifyTransfer(AddressZero, user, MaxUint256, {"from": fake_vault})
+
+  ## Adding above total supply will overflow
+  with brownie.reverts():
+    initialized_contract.notifyTransfer(AddressZero, user, 1, {"from": fake_vault})
+
+def test_cannot_overflow_points(initialized_contract, deployer, user, fake_vault, token):
+
+  EPOCH = initialized_contract.currentEpoch()
+
+  ## Deposit max
+  initialized_contract.notifyTransfer(AddressZero, user, MaxUint256, {"from": fake_vault})
+
+  chain.sleep(2)
+  ## Will revert after 1 second has elapsed
+  with brownie.reverts():
+    initialized_contract.accrueVault(EPOCH, fake_vault, {"from": user})
+
+  ## Any accrual of Vault must revert
+  chain.sleep(initialized_contract.SECONDS_PER_EPOCH() + 1)
+  chain.mine()
+
+  ## Will also revert at end of epoch because seconds are > 1
+  with brownie.reverts():
+    initialized_contract.accrueVault(EPOCH, fake_vault, {"from": user})
+
+  ## Additionally any function for claiming rewards should also revert as while the revert above prevents the storage to be changed
+  ## The other functions that minimize storage usage are not going
+
+  with brownie.reverts():
+    initialized_contract.claimRewardReference(EPOCH, fake_vault, token, user, {"from": user})
+
+  with brownie.reverts():
+    initialized_contract.claimReward(EPOCH, fake_vault, token, user, {"from": user})
+
+  with brownie.reverts():
+    initialized_contract.claimBulkTokensOverMultipleEpochs(EPOCH, EPOCH, fake_vault, [token], user, {"from": user})
+
+  with brownie.reverts():
+    initialized_contract.reap([EPOCH, EPOCH, fake_vault, [token]], {"from": user})
+
+  with brownie.reverts():
+    initialized_contract.tear([EPOCH, EPOCH, fake_vault, [token]], {"from": user})

tests/conftest.py

@@ -39,9 +39,8 @@ def second_user():
 def token(user, deployer, second_user):
     whale = accounts.at("0xD0A7A8B98957b9CD3cFB9c0425AbE44551158e9e", force=True)
     t = interface.IERC20("0x3472A5A71965499acd81997a54BBA8D852C6E53d")
-    t.transfer(user, t.balanceOf(whale) // 3, {"from": whale})
-    t.transfer(deployer, t.balanceOf(whale) // 3, {"from": whale})
-    t.transfer(second_user, t.balanceOf(whale) // 3, {"from": whale})
+    t.transfer(user, t.balanceOf(whale) // 2, {"from": whale})
+    t.transfer(deployer, t.balanceOf(whale) // 2, {"from": whale})
     return t
 
 @pytest.fixture

tests/lens/test_lens_is_equivalent.py

@@ -88,9 +88,57 @@ def test_claimBulkTokensOverMultipleEpochsOptimized_basic(initialized_contract,
 
   ## Claim rewards via the bulk function
   ## Btw only you can claim for yourself
-  tx = initialized_contract.claimBulkTokensOverMultipleEpochsOptimized(EPOCH, EPOCH, fake_vault, [token], {"from": user})
+  tx = initialized_contract.reap([EPOCH, EPOCH, fake_vault, [token]], {"from": user})
 
   ## Verify you got the entire reward amount
   assert token.balanceOf(user) == initial_reward_balance + REWARD_AMOUNT
 
-  assert calculated_amount == REWARD_AMOUNT
+  assert calculated_amount == REWARD_AMOUNT
+
+def test_getClaimableBulkRewards_coverage(initialized_contract, user, fake_vault, token, second_user):
+  INITIAL_DEPOSIT = 1e18
+  REWARD_AMOUNT = 1e20
+  EPOCH = initialized_contract.currentEpoch()
+
+  ## Add rewards here
+  token.approve(initialized_contract, MaxUint256, {"from": user})
+  initialized_contract.addReward(EPOCH, fake_vault, token, REWARD_AMOUNT, {"from": user})
+
+  ## User didn't deposited, they have 0 points
+  assert initialized_contract.points(EPOCH, fake_vault, user) == 0
+
+  ## Because user has the tokens too, we check the balance here
+  initial_reward_balance = token.balanceOf(user)
+
+  ## Test revert cases: require(params.epochStart <= params.epochEnd);
+  claimParams = [EPOCH + 1, EPOCH, fake_vault.address, [token.address]] 
+  with brownie.reverts():
+       quote = initialized_contract.getClaimableBulkRewards(claimParams, user)
+
+  ## Test revert cases: require(params.epochEnd < currentEpoch());
+  claimParams = [EPOCH, EPOCH + 1000, fake_vault.address, [token.address]]
+  with brownie.reverts():
+       quote = initialized_contract.getClaimableBulkRewards(claimParams, user)       
+
+  ## Wait the epoch to end
+  chain.sleep(initialized_contract.SECONDS_PER_EPOCH() + 1)
+  chain.mine()
+  claimParams = [EPOCH, EPOCH, fake_vault.address, [token.address]]
+  quote = initialized_contract.getClaimableBulkRewards(claimParams, user)
+  after_reward_balance = token.balanceOf(user)
+  assert after_reward_balance == initial_reward_balance ## no reward here since no deposit
+
+  ## Only deposit so we get 100% of rewards and claim midway
+  initialized_contract.notifyTransfer(AddressZero, user, INITIAL_DEPOSIT, {"from": fake_vault})
+  nxtEPOCH = initialized_contract.currentEpoch()
+  initialized_contract.addReward(nxtEPOCH, fake_vault, token, REWARD_AMOUNT, {"from": user})
+  chain.sleep(initialized_contract.SECONDS_PER_EPOCH() + 1)
+  chain.mine()
+  initialized_contract.claimReward(nxtEPOCH, fake_vault, token, user, {"from": user})
+
+  ## Test revert cases: require(pointsWithdrawn[epochId][params.vault][user][token] == 0);
+  claimParams = [nxtEPOCH, nxtEPOCH, fake_vault.address, [token.address]]
+  with brownie.reverts():
+       quote = initialized_contract.getClaimableBulkRewards(claimParams, user) 
+
+

tests/lifecycle/test_no_storage_claim.py

@@ -48,7 +48,7 @@ def test_claim_non_emitting(initialized_contract, user, fake_vault, token):
   assert userEpochTotalPoints == userPointsWithdrawn and userPointsWithdrawn == userTimeToAccure * userBalanceAtEpochId
 
 """
-  Test claimBulkTokensOverMultipleEpochsOptimizedWithoutStorageNonEmitting
+  Test tear
 """
 def test_bulk_claim_non_emitting(initialized_contract, user, fake_vault, token):
   INITIAL_DEPOSIT = 1e18
@@ -74,7 +74,7 @@ def test_bulk_claim_non_emitting(initialized_contract, user, fake_vault, token):
   ## Claim rewards over multiple epochs
   balBefore = token.balanceOf(user.address)
   claimParams = [EPOCH, lastEPOCH, fake_vault.address, [token.address]]
-  claimTx = initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorageNonEmitting(claimParams, {"from": user})
+  claimTx = initialized_contract.tear(claimParams, {"from": user})
 
   ## Verify all rewards for multiple epochs have been claimed  
   balAfter = token.balanceOf(user.address)
@@ -88,7 +88,7 @@ def test_bulk_claim_non_emitting(initialized_contract, user, fake_vault, token):
   assert pointsLast == 0
 
 """
-  Test claimBulkTokensOverMultipleEpochsOptimizedWithoutStorage
+  Test reap
 """
 def test_bulk_claim(initialized_contract, user, fake_vault, token):
   INITIAL_DEPOSIT = 1e18
@@ -114,7 +114,7 @@ def test_bulk_claim(initialized_contract, user, fake_vault, token):
   ## Claim rewards over multiple epochs
   balBefore = token.balanceOf(user.address)
   claimParams = [EPOCH, lastEPOCH, fake_vault.address, [token.address]]
-  claimTx = initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorage(claimParams, {"from": user})
+  claimTx = initialized_contract.reap(claimParams, {"from": user})
 
   ## Verify all rewards for multiple epochs have been claimed  
   balAfter = token.balanceOf(user.address)
@@ -125,40 +125,4 @@ def test_bulk_claim(initialized_contract, user, fake_vault, token):
   shareLast = initialized_contract.shares(lastEPOCH, fake_vault.address, user.address)
   assert shareLast == INITIAL_DEPOSIT
   pointsLast = initialized_contract.points(lastEPOCH, fake_vault.address, user.address)
-  assert pointsLast == 0
-
-"""
-  Test claimBulkTokensOverMultipleEpochsOptimized
-"""
-def test_bulk_claim_optimized(initialized_contract, user, fake_vault, token):
-  INITIAL_DEPOSIT = 1e18
-  REWARD_AMOUNT = 1e19
-  ## Multiple epochs to claim rewards
-  EPOCH = initialized_contract.currentEpoch()
-  nextEPOCH = EPOCH + 1
-  lastEPOCH = EPOCH + 2
-
-  ## Add rewards for next two epochs
-  token.approve(initialized_contract, MaxUint256, {"from": user})
-  initialized_contract.addReward(nextEPOCH, fake_vault, token, REWARD_AMOUNT, {"from": user})
-  initialized_contract.addReward(lastEPOCH, fake_vault, token, REWARD_AMOUNT, {"from": user})
-
-  ## Only deposit so we get 100% of rewards
-  initialized_contract.notifyTransfer(AddressZero, user, INITIAL_DEPOSIT, {"from": fake_vault})
-
-  ## Wait the all epochs to end
-  advanceTime = initialized_contract.SECONDS_PER_EPOCH() + 1
-  chain.sleep(advanceTime * 3)
-  chain.mine()
-
-  ## Claim rewards over multiple epochs
-  balBefore = token.balanceOf(user.address)
-  claimTx = initialized_contract.claimBulkTokensOverMultipleEpochsOptimized(EPOCH, lastEPOCH, fake_vault.address, [token.address], {"from": user})
-
-  ## Verify all rewards for multiple epochs have been claimed  
-  balAfter = token.balanceOf(user.address)
-  assert balAfter - balBefore == REWARD_AMOUNT * 2
-  shareLast = initialized_contract.shares(lastEPOCH, fake_vault.address, user.address)
-  assert shareLast == INITIAL_DEPOSIT
-  pointsLast = initialized_contract.points(lastEPOCH, fake_vault.address, user.address)
   assert pointsLast == 0

tests/lifecycle/test_reverts.py

@@ -35,7 +35,7 @@ def test_duplicates(initialized_contract, user, fake_vault, token):
   duplicateArrays = [fake_vault.address, token.address, token.address]
   claimParams = [EPOCH, lastEPOCH, fake_vault.address, duplicateArrays]
   with brownie.reverts():
-       claimTx = initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorageNonEmitting(claimParams, {"from": user})
+       claimTx = initialized_contract.tear(claimParams, {"from": user})
 
 """
   Test revert on validity checks against given epochId
@@ -78,7 +78,7 @@ def test_invalid_epochId(initialized_contract, user, fake_vault, token):
 
 """
   Test revert on epoch which already claimed thus can't be optimized 
-       claimBulkTokensOverMultipleEpochsOptimizedWithoutStorage()
+       reap()
 """
 def test_bulk_claim_over_already_claimed(initialized_contract, user, fake_vault, token):
   INITIAL_DEPOSIT = 1e18
@@ -112,22 +112,22 @@ def test_bulk_claim_over_already_claimed(initialized_contract, user, fake_vault,
   ## Claim rewards over multiple epochs will revert due to epochStart > epochEnd
   claimParams = [lastEPOCH, EPOCH, fake_vault.address, [token.address]]
   with brownie.reverts():
-       initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorage(claimParams, {"from": user}) 
+       initialized_contract.reap(claimParams, {"from": user}) 
 
   ## Claim rewards over multiple epochs will revert due to epochEnd >= currentEpoch()
   claimParams = [EPOCH, invalidEPOCH, fake_vault.address, [token.address]]
   with brownie.reverts():
-       initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorage(claimParams, {"from": user}) 
+       initialized_contract.reap(claimParams, {"from": user}) 
 
   ## Claim rewards over multiple epochs will revert due to one of the given epochs has been claimed
   claimParams = [EPOCH, EPOCH, fake_vault.address, [token.address]]
   with brownie.reverts():
-       initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorage(claimParams, {"from": user})
+       initialized_contract.reap(claimParams, {"from": user})
 
   ## Claim rewards over rest epochs will be good
   balBefore = token.balanceOf(user.address)
   claimParams = [nextEPOCH, lastEPOCH, fake_vault.address, [token.address]]  
-  claimTx = initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorage(claimParams, {"from": user})
+  claimTx = initialized_contract.reap(claimParams, {"from": user})
   balAfter = token.balanceOf(user.address)
   assert balAfter > balBefore ## should get some reward before withdrwal
   startAccrueTimestamp = initialized_contract.lastUserAccrueTimestamp(nextEPOCH, fake_vault.address, user.address)
@@ -136,7 +136,7 @@ def test_bulk_claim_over_already_claimed(initialized_contract, user, fake_vault,
 
 """
   Test revert on epoch which already claimed thus can't be optimized 
-       claimBulkTokensOverMultipleEpochsOptimizedWithoutStorageNonEmitting()
+       tear()
 """
 def test_bulk_claim_non_emitting_over_already_claimed(initialized_contract, user, fake_vault, token):
   INITIAL_DEPOSIT = 1e18
@@ -170,22 +170,22 @@ def test_bulk_claim_non_emitting_over_already_claimed(initialized_contract, user
   ## Claim rewards over multiple epochs will revert due to epochStart > epochEnd
   claimParams = [lastEPOCH, EPOCH, fake_vault.address, [token.address]]
   with brownie.reverts():
-       initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorageNonEmitting(claimParams, {"from": user}) 
+       initialized_contract.tear(claimParams, {"from": user}) 
 
   ## Claim rewards over multiple epochs will revert due to epochEnd >= currentEpoch()
   claimParams = [EPOCH, invalidEPOCH, fake_vault.address, [token.address]]
   with brownie.reverts():
-       initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorageNonEmitting(claimParams, {"from": user}) 
+       initialized_contract.tear(claimParams, {"from": user}) 
 
   ## Claim rewards over multiple epochs will revert due to one of the given epochs has been claimed
   claimParams = [EPOCH, EPOCH, fake_vault.address, [token.address]]
   with brownie.reverts():
-       initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorageNonEmitting(claimParams, {"from": user})
+       initialized_contract.tear(claimParams, {"from": user})
 
   ## Claim rewards over rest epochs will be good
   balBefore = token.balanceOf(user.address)
   claimParams = [nextEPOCH, lastEPOCH, fake_vault.address, [token.address]]  
-  claimTx = initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorageNonEmitting(claimParams, {"from": user})
+  claimTx = initialized_contract.tear(claimParams, {"from": user})
   balAfter = token.balanceOf(user.address)
   assert balAfter > balBefore ## should get some reward before withdrwal
   startAccrueTimestamp = initialized_contract.lastUserAccrueTimestamp(nextEPOCH, fake_vault.address, user.address)

tests/simulation/one_week_of_accrual.py

@@ -121,7 +121,7 @@ def test_full_deposit_one_week_bulk_function(initialized_contract, user, fake_va
   chain.mine()
 
   ## Claim rewards here
-  tx = initialized_contract.claimBulkTokensOverMultipleEpochsOptimizedWithoutStorageNonEmitting([1, 1, fake_vault, [token]], {"from": user})
+  tx = initialized_contract.tear([1, 1, fake_vault, [token]], {"from": user})
 
   ## Verify you got the entire amount
   assert token.balanceOf(user) == initial_reward_balance + REWARD_AMOUNT