fix(StakeManager): add checks for whether lockup period is in range (#39

)

This commit introduces `MIN_LOCKUP_PERIOD` and `MAX_LOCKUP_PERIOD` and
makes use of them within `StakeManager.stake()` and
`StakeManager.lock()` accordingly.

When users deposit tokens into their vault via `stake()`, they can
provide an optional lockup time. If the value is `0` it implies users do
not want to lock their stake.

If the value is `> 0` it has to be within the range of
`MIN_LOCKUP_PERIOD` and `MAX_LOCKUP_PERIOD`.

Properly addresses #15

.gas-snapshot

@@ -1,24 +1,26 @@
 CreateVaultTest:testDeployment() (gas: 9774)
-CreateVaultTest:test_createVault() (gas: 650992)
-ExecuteAccountTest:testDeployment() (gas: 26400)
-ExecuteAccountTest:test_RevertWhen_InvalidLimitEpoch() (gas: 991602)
-LeaveTest:testDeployment() (gas: 26172)
-LeaveTest:test_RevertWhen_NoPendingMigration() (gas: 678051)
-LeaveTest:test_RevertWhen_SenderIsNotVault() (gas: 10562)
-LockTest:testDeployment() (gas: 26400)
-LockTest:test_RevertWhen_DecreasingLockTime() (gas: 994528)
-LockTest:test_RevertWhen_SenderIsNotVault() (gas: 10607)
-MigrateTest:testDeployment() (gas: 26172)
-MigrateTest:test_RevertWhen_NoPendingMigration() (gas: 677890)
+CreateVaultTest:test_createVault() (gas: 650970)
+ExecuteAccountTest:testDeployment() (gas: 26421)
+ExecuteAccountTest:test_RevertWhen_InvalidLimitEpoch() (gas: 992511)
+LeaveTest:testDeployment() (gas: 26193)
+LeaveTest:test_RevertWhen_NoPendingMigration() (gas: 678007)
+LeaveTest:test_RevertWhen_SenderIsNotVault() (gas: 10540)
+LockTest:testDeployment() (gas: 26421)
+LockTest:test_RevertWhen_DecreasingLockTime() (gas: 995651)
+LockTest:test_RevertWhen_InvalidLockupPeriod() (gas: 815891)
+LockTest:test_RevertWhen_SenderIsNotVault() (gas: 10652)
+MigrateTest:testDeployment() (gas: 26193)
+MigrateTest:test_RevertWhen_NoPendingMigration() (gas: 677868)
 MigrateTest:test_RevertWhen_SenderIsNotVault() (gas: 10629)
 SetStakeManagerTest:testDeployment() (gas: 9774)
 SetStakeManagerTest:test_RevertWhen_InvalidStakeManagerAddress() (gas: 20481)
 SetStakeManagerTest:test_SetStakeManager() (gas: 19869)
-StakeManagerTest:testDeployment() (gas: 26172)
-StakeTest:testDeployment() (gas: 26172)
-StakeTest:test_RevertWhen_SenderIsNotVault() (gas: 10638)
+StakeManagerTest:testDeployment() (gas: 26193)
+StakeTest:testDeployment() (gas: 26421)
+StakeTest:test_RevertWhen_InvalidLockupPeriod() (gas: 827181)
+StakeTest:test_RevertWhen_SenderIsNotVault() (gas: 10672)
 StakedTokenTest:testStakeToken() (gas: 7638)
-UnstakeTest:testDeployment() (gas: 26355)
-UnstakeTest:test_RevertWhen_FundsLocked() (gas: 990991)
+UnstakeTest:testDeployment() (gas: 26376)
+UnstakeTest:test_RevertWhen_FundsLocked() (gas: 991901)
 UnstakeTest:test_RevertWhen_SenderIsNotVault() (gas: 10609)
 VaultFactoryTest:testDeployment() (gas: 9774)

contracts/StakeManager.sol

@@ -14,6 +14,7 @@ contract StakeManager is Ownable {
     error StakeManager__PendingMigration();
     error StakeManager__SenderIsNotPreviousStakeManager();
     error StakeManager__InvalidLimitEpoch();
+    error StakeManager__InvalidLockupPeriod();
 
     struct Account {
         uint256 lockUntil;
@@ -32,6 +33,8 @@ contract StakeManager is Ownable {
 
     uint256 public constant EPOCH_SIZE = 1 weeks;
     uint256 public constant YEAR = 365 days;
+    uint256 public constant MIN_LOCKUP_PERIOD = 12 weeks; // 3 months
+    uint256 public constant MAX_LOCKUP_PERIOD = 4 * YEAR; // 4 years
     uint256 public constant MP_APY = 1;
     uint256 public constant MAX_BOOST = 4;
 
@@ -64,8 +67,13 @@ contract StakeManager is Ownable {
      * Increases balance of msg.sender;
      * @param _amount Amount of balance to be decreased.
      * @param _time Seconds from block.timestamp to lock balance.
+     *
+     * @dev Reverts when `_time` is not in range of [MIN_LOCKUP_PERIOD, MAX_LOCKUP_PERIOD]
      */
     function stake(uint256 _amount, uint256 _time) external onlyVault {
+        if (_time > 0 && (_time < MIN_LOCKUP_PERIOD || _time > MAX_LOCKUP_PERIOD)) {
+            revert StakeManager__InvalidLockupPeriod();
+        }
         Account storage account = accounts[msg.sender];
         processAccount(account, currentEpoch);
         account.balance += _amount;
@@ -94,8 +102,14 @@ contract StakeManager is Ownable {
     /**
      * @notice Locks entire balance for more amount of time.
      * @param _time amount of time to lock from now.
+     *
+     * @dev Reverts when `_time` is bigger than `MAX_LOCKUP_PERIOD`
+     * @dev Reverts when `_time + block.timestamp` is smaller than current lock time.
      */
     function lock(uint256 _time) external onlyVault {
+        if (_time > MAX_LOCKUP_PERIOD) {
+            revert StakeManager__InvalidLockupPeriod();
+        }
         Account storage account = accounts[msg.sender];
         processAccount(account, currentEpoch);
         if (block.timestamp + _time < account.lockUntil) {

test/StakeManager.t.sol

@@ -54,6 +54,23 @@ contract StakeTest is StakeManagerTest {
         vm.expectRevert(StakeManager.StakeManager__SenderIsNotVault.selector);
         stakeManager.stake(100, 1);
     }
+
+    function test_RevertWhen_InvalidLockupPeriod() public {
+        // ensure user has funds
+        deal(stakeToken, testUser, 1000);
+        StakeVault userVault = _createTestVault(testUser);
+
+        vm.startPrank(testUser);
+        ERC20(stakeToken).approve(address(userVault), 100);
+
+        uint256 lockTime = stakeManager.MIN_LOCKUP_PERIOD() - 1;
+        vm.expectRevert(StakeManager.StakeManager__InvalidLockupPeriod.selector);
+        userVault.stake(100, lockTime);
+
+        lockTime = stakeManager.MAX_LOCKUP_PERIOD() + 1;
+        vm.expectRevert(StakeManager.StakeManager__InvalidLockupPeriod.selector);
+        userVault.stake(100, lockTime);
+    }
 }
 
 contract UnstakeTest is StakeManagerTest {
@@ -74,7 +91,7 @@ contract UnstakeTest is StakeManagerTest {
         vm.startPrank(testUser);
         ERC20(stakeToken).approve(address(userVault), 100);
 
-        uint256 lockTime = 1 days;
+        uint256 lockTime = stakeManager.MIN_LOCKUP_PERIOD();
         userVault.stake(100, lockTime);
 
         vm.expectRevert(StakeManager.StakeManager__FundsLocked.selector);
@@ -92,6 +109,20 @@ contract LockTest is StakeManagerTest {
         stakeManager.lock(100);
     }
 
+    function test_RevertWhen_InvalidLockupPeriod() public {
+        // ensure user has funds
+        deal(stakeToken, testUser, 1000);
+        StakeVault userVault = _createTestVault(testUser);
+
+        vm.startPrank(testUser);
+        // ensure user vault can spend user tokens
+        ERC20(stakeToken).approve(address(userVault), 100);
+
+        uint256 lockTime = stakeManager.MAX_LOCKUP_PERIOD() + 1;
+        vm.expectRevert(StakeManager.StakeManager__InvalidLockupPeriod.selector);
+        userVault.stake(100, lockTime);
+    }
+
     function test_RevertWhen_DecreasingLockTime() public {
         // ensure user has funds
         deal(stakeToken, testUser, 1000);
@@ -101,7 +132,7 @@ contract LockTest is StakeManagerTest {
         // ensure user vault can spend user tokens
         ERC20(stakeToken).approve(address(userVault), 100);
 
-        uint256 lockTime = 1 days;
+        uint256 lockTime = stakeManager.MIN_LOCKUP_PERIOD() + 1;
         userVault.stake(100, lockTime);
 
         vm.expectRevert(StakeManager.StakeManager__DecreasingLockTime.selector);
@@ -157,7 +188,7 @@ contract ExecuteAccountTest is StakeManagerTest {
         vm.startPrank(testUser);
         ERC20(stakeToken).approve(address(userVault), 100);
 
-        uint256 lockTime = 1 days;
+        uint256 lockTime = stakeManager.MIN_LOCKUP_PERIOD();
         userVault.stake(100, lockTime);
 
         uint256 currentEpoch = stakeManager.currentEpoch();