👷 KSX - Remove escrow on inflationary rewards and remove staking cooldown

contracts/EscrowMigrator.sol

@@ -33,6 +33,7 @@ import {IStakingRewardsIntegrator} from "./interfaces/IStakingRewardsIntegrator.
 /// @title KWENTA Escrow Migrator
 /// Used to migrate escrow entries from RewardEscrowV1 to RewardEscrowV2
 /// @author tommyrharper ([email protected])
+/// @author Flocqst ([email protected])
 contract EscrowMigrator is
     IEscrowMigrator,
     Ownable2StepUpgradeable,
@@ -410,7 +411,6 @@ contract EscrowMigrator is
         _payForMigration(_account);
 
         uint256 migratedEscrow;
-        uint256 cooldown = stakingRewardsV2.cooldownPeriod();
         mapping(uint256 => VestingEntry) storage userEntries = registeredVestingSchedules[_account];
 
         for (uint256 i = 0; i < _entryIDs.length; i++) {
@@ -432,15 +432,6 @@ contract EscrowMigrator is
             registeredEntry.migrated = true;
             migratedEscrow += originalEscrowAmount;
 
-            /// @dev it essential for security that the duration is not less than the cooldown period,
-            /// otherwise the user could do a governance attack by bypassing the unstaking cooldown lock
-            /// by migrating their escrow then staking, voting, and vesting immediately
-            if (duration < cooldown) {
-                uint256 timeCreated = endTime - duration;
-                duration = cooldown;
-                endTime = timeCreated + cooldown;
-            }
-
             IRewardEscrowV2.VestingEntry memory entry = IRewardEscrowV2.VestingEntry({
                 escrowAmount: originalEscrowAmount,
                 duration: duration,

contracts/RewardEscrowV2.sol

@@ -19,7 +19,7 @@ import {IEscrowMigrator} from "./interfaces/IEscrowMigrator.sol";
 /// @title KWENTA Reward Escrow V2
 /// @author Originally inspired by SYNTHETIX RewardEscrow
 /// @author Kwenta's RewardEscrow V1 by JaredBorders ([email protected]), JChiaramonte7 ([email protected])
-/// @author RewardEscrowV2 by tommyrharper ([email protected])
+/// @author RewardEscrowV2 by tommyrharper ([email protected]), Flocqst ([email protected])
 /// @notice Updated version of Synthetix's RewardEscrow with new features specific to Kwenta
 contract RewardEscrowV2 is
     IRewardEscrowV2,
@@ -379,7 +379,7 @@ contract RewardEscrowV2 is
                 unchecked {
                     amountToUnstake = totalWithFee - unstakedEscrow;
                 }
-                stakingRewards.unstakeEscrowSkipCooldown(msg.sender, amountToUnstake);
+                stakingRewards.unstakeEscrowAdmin(msg.sender, amountToUnstake);
             }
 
             // update balances
@@ -432,8 +432,7 @@ contract RewardEscrowV2 is
         if (_earlyVestingFee > MAXIMUM_EARLY_VESTING_FEE) revert EarlyVestingFeeTooHigh();
         if (_earlyVestingFee < MINIMUM_EARLY_VESTING_FEE) revert EarlyVestingFeeTooLow();
         if (_deposit == 0) revert ZeroAmount();
-        uint256 minimumDuration = stakingRewards.cooldownPeriod();
-        if (_duration < minimumDuration || _duration > MAX_DURATION) revert InvalidDuration();
+        if (_duration == 0 || _duration > MAX_DURATION) revert InvalidDuration();
 
         /// @dev this will revert if the kwenta token transfer fails
         kwenta.transferFrom(msg.sender, address(this), _deposit);

contracts/StakingRewardsV2.sol

@@ -15,7 +15,7 @@ import {IRewardEscrowV2} from "./interfaces/IRewardEscrowV2.sol";
 /// @title KWENTA Staking Rewards V2
 /// @author Originally inspired by SYNTHETIX StakingRewards
 /// @author Kwenta's StakingRewards V1 by JaredBorders ([email protected]), JChiaramonte7 ([email protected])
-/// @author StakingRewardsV2 (this) by tommyrharper ([email protected])
+/// @author StakingRewardsV2 (this) by tommyrharper ([email protected]), Flocqst ([email protected])
 /// @notice Updated version of Synthetix's StakingRewards with new features specific to Kwenta
 contract StakingRewardsV2 is
     IStakingRewardsV2,
@@ -27,12 +27,6 @@ contract StakingRewardsV2 is
                         CONSTANTS/IMMUTABLES
     ///////////////////////////////////////////////////////////////*/
 
-    /// @notice minimum time length of the unstaking cooldown period
-    uint256 public constant MIN_COOLDOWN_PERIOD = 1 weeks;
-
-    /// @notice maximum time length of the unstaking cooldown period
-    uint256 public constant MAX_COOLDOWN_PERIOD = 52 weeks;
-
     /// @notice Contract for KWENTA ERC20 token - used for BOTH staking and rewards
     /// @custom:oz-upgrades-unsafe-allow state-variable-immutable
     IKwenta public immutable kwenta;
@@ -81,9 +75,6 @@ contract StakingRewardsV2 is
     /// @notice summation of rewardRate divided by total staked tokens
     uint256 public rewardPerTokenStored;
 
-    /// @inheritdoc IStakingRewardsV2
-    uint256 public cooldownPeriod;
-
     /// @notice represents the rewardPerToken
     /// value the last time the staker calculated earned() rewards
     mapping(address => uint256) public userRewardPerTokenPaid;
@@ -136,17 +127,6 @@ contract StakingRewardsV2 is
         if (msg.sender != address(rewardsNotifier)) revert OnlyRewardsNotifier();
     }
 
-    /// @notice only allow execution after the unstaking cooldown period has elapsed
-    modifier afterCooldown(address _account) {
-        _afterCooldown(_account);
-        _;
-    }
-
-    function _afterCooldown(address _account) internal view {
-        uint256 canUnstakeAt = userLastStakeTime[_account] + cooldownPeriod;
-        if (canUnstakeAt > block.timestamp) revert MustWaitForUnlock(canUnstakeAt);
-    }
-
     /*///////////////////////////////////////////////////////////////
                         CONSTRUCTOR / INITIALIZER
     ///////////////////////////////////////////////////////////////*/
@@ -191,7 +171,6 @@ contract StakingRewardsV2 is
 
         // define values
         rewardsDuration = 1 weeks;
-        cooldownPeriod = 2 weeks;
     }
 
     /*///////////////////////////////////////////////////////////////
@@ -239,28 +218,30 @@ contract StakingRewardsV2 is
     ///////////////////////////////////////////////////////////////*/
 
     /// @inheritdoc IStakingRewardsV2
-    function stake(uint256 _amount) external whenNotPaused updateReward(msg.sender) {
+    function stake(uint256 _amount) external {
+        _stake(msg.sender, _amount);
+
+        // transfer token to this contract from the caller
+        kwenta.transferFrom(msg.sender, address(this), _amount);
+    }
+
+    function _stake(address _account, uint256 _amount)
+        internal
+        whenNotPaused
+        updateReward(_account)
+    {
         if (_amount == 0) revert AmountZero();
 
         // update state
-        userLastStakeTime[msg.sender] = block.timestamp;
         _addTotalSupplyCheckpoint(totalSupply() + _amount);
-        _addBalancesCheckpoint(msg.sender, balanceOf(msg.sender) + _amount);
-
-        // emit staking event and index msg.sender
-        emit Staked(msg.sender, _amount);
+        _addBalancesCheckpoint(_account, balanceOf(_account) + _amount);
 
-        // transfer token to this contract from the caller
-        kwenta.transferFrom(msg.sender, address(this), _amount);
+        // emit staking event and index _account
+        emit Staked(_account, _amount);
     }
 
     /// @inheritdoc IStakingRewardsV2
-    function unstake(uint256 _amount)
-        public
-        whenNotPaused
-        updateReward(msg.sender)
-        afterCooldown(msg.sender)
-    {
+    function unstake(uint256 _amount) public whenNotPaused updateReward(msg.sender) {
         if (_amount == 0) revert AmountZero();
         uint256 nonEscrowedBalance = nonEscrowedBalanceOf(msg.sender);
         if (_amount > nonEscrowedBalance) revert InsufficientBalance(nonEscrowedBalance);
@@ -291,7 +272,6 @@ contract StakingRewardsV2 is
         if (_amount > unstakedEscrow) revert InsufficientUnstakedEscrow(unstakedEscrow);
 
         // update state
-        userLastStakeTime[_account] = block.timestamp;
         _addBalancesCheckpoint(_account, balanceOf(_account) + _amount);
         _addEscrowedBalancesCheckpoint(_account, escrowedBalanceOf(_account) + _amount);
 
@@ -304,15 +284,12 @@ contract StakingRewardsV2 is
     }
 
     /// @inheritdoc IStakingRewardsV2
-    function unstakeEscrow(uint256 _amount) external afterCooldown(msg.sender) {
+    function unstakeEscrow(uint256 _amount) external {
         _unstakeEscrow(msg.sender, _amount);
     }
 
     /// @inheritdoc IStakingRewardsV2
-    function unstakeEscrowSkipCooldown(address _account, uint256 _amount)
-        external
-        onlyRewardEscrow
-    {
+    function unstakeEscrowAdmin(address _account, uint256 _amount) external onlyRewardEscrow {
         _unstakeEscrow(_account, _amount);
     }
 
@@ -361,31 +338,44 @@ contract StakingRewardsV2 is
         whenNotPaused
         updateReward(_account)
     {
-        uint256 reward = rewards[_account];
-        if (reward > 0) {
+        _processReward(_account, _to, true);
+    }
+
+    /// @notice Process KWENTA and USDC rewards
+    /// @dev transferKwenta is set to false when compounding KWENTA rewards
+    /// @param _account The address of the account to process rewards for
+    /// @param _to The address to transfer rewards to
+    /// @param transferKwenta Boolean flag to determine if Kwenta should be transferred
+    /// @return kwentaReward The amount of Kwenta reward processed
+    function _processReward(address _account, address _to, bool transferKwenta)
+        internal
+        updateReward(_account)
+        returns (uint256 kwentaReward)
+    {
+        // Process Kwenta reward
+        kwentaReward = rewards[_account];
+        if (kwentaReward > 0) {
             // update state (first)
             rewards[_account] = 0;
 
             // emit reward claimed event and index account
-            emit RewardPaid(_account, reward);
+            emit RewardPaid(_account, kwentaReward);
 
-            // transfer token from this contract to the rewardEscrow
-            // and create a vesting entry at the _to address
-            kwenta.transfer(address(rewardEscrow), reward);
-            rewardEscrow.appendVestingEntry(_to, reward);
+            if (transferKwenta) {
+                kwenta.transfer(_to, kwentaReward);
+            }
         }
 
-        uint256 rewardUSDC = rewardsUSDC[_account] / PRECISION;
-        if (rewardUSDC > 0) {
+        // Process USDC reward
+        uint256 usdcReward = rewardsUSDC[_account] / PRECISION;
+        if (usdcReward > 0) {
             // update state (first)
             rewardsUSDC[_account] = 0;
 
             // emit reward claimed event and index account
-            emit RewardPaidUSDC(_account, rewardUSDC);
+            emit RewardPaidUSDC(_account, usdcReward);
 
-            // transfer token from this contract to the account
-            // as newly issued rewards from inflation are now issued as non-escrowed
-            usdc.transfer(_to, rewardUSDC);
+            usdc.transfer(_to, usdcReward);
         }
     }
 
@@ -397,8 +387,8 @@ contract StakingRewardsV2 is
     /// @dev internal helper to compound for a given account
     /// @param _account the account to compound for
     function _compound(address _account) internal {
-        _getReward(_account);
-        _stakeEscrow(_account, unstakedEscrowedBalanceOf(_account));
+        uint256 reward = _processReward(_account, _account, false);
+        _stake(_account, reward);
     }
 
     /*///////////////////////////////////////////////////////////////
@@ -688,17 +678,6 @@ contract StakingRewardsV2 is
         emit RewardsDurationUpdated(rewardsDuration);
     }
 
-    /// @inheritdoc IStakingRewardsV2
-    function setCooldownPeriod(uint256 _cooldownPeriod) external onlyOwner {
-        if (_cooldownPeriod < MIN_COOLDOWN_PERIOD) revert CooldownPeriodTooLow(MIN_COOLDOWN_PERIOD);
-        if (_cooldownPeriod > MAX_COOLDOWN_PERIOD) {
-            revert CooldownPeriodTooHigh(MAX_COOLDOWN_PERIOD);
-        }
-
-        cooldownPeriod = _cooldownPeriod;
-        emit CooldownPeriodUpdated(cooldownPeriod);
-    }
-
     /*///////////////////////////////////////////////////////////////
                                 PAUSABLE
     ///////////////////////////////////////////////////////////////*/

contracts/interfaces/IStakingRewardsV2.sol

@@ -59,10 +59,7 @@ interface IStakingRewardsV2 {
     /// @param _account: address to check
     /// @return amount of tokens escrowed but not staked
     function unstakedEscrowedBalanceOf(address _account) external view returns (uint256);
-
-    /// @notice the period of time a user has to wait after staking to unstake
-    function cooldownPeriod() external view returns (uint256);
-
+
     // rewards
 
     /// @notice calculate the total rewards for one duration based on the current rate
@@ -158,11 +155,11 @@ interface IStakingRewardsV2 {
     /// @dev updateReward() called prior to function logic
     function unstakeEscrow(uint256 _amount) external;
 
-    /// @notice unstake escrowed token skipping the cooldown wait period
+    /// @notice unstake escrowed token on behalf of another account
     /// @param _account: address of account to unstake from
     /// @param _amount: amount to unstake
     /// @dev this function is used to allow tokens to be vested at any time by RewardEscrowV2
-    function unstakeEscrowSkipCooldown(address _account, uint256 _amount) external;
+    function unstakeEscrowAdmin(address _account, uint256 _amount) external;
 
     /// @notice unstake all available staked non-escrowed tokens and
     /// claim any rewards
@@ -211,10 +208,6 @@ interface IStakingRewardsV2 {
     /// @param _rewardsDuration: denoted in seconds
     function setRewardsDuration(uint256 _rewardsDuration) external;
 
-    /// @notice set unstaking cooldown period
-    /// @param _cooldownPeriod: denoted in seconds
-    function setCooldownPeriod(uint256 _cooldownPeriod) external;
-
     // pausable
 
     /// @dev Triggers stopped state
@@ -279,10 +272,6 @@ interface IStakingRewardsV2 {
     /// @param amount: amount of token recovered
     event Recovered(address token, uint256 amount);
 
-    /// @notice emitted when the unstaking cooldown period is updated
-    /// @param cooldownPeriod: the new unstaking cooldown period
-    event CooldownPeriodUpdated(uint256 cooldownPeriod);
-
     /// @notice emitted when an operator is approved
     /// @param owner: owner of tokens
     /// @param operator: address of operator
@@ -322,21 +311,9 @@ interface IStakingRewardsV2 {
     /// @notice recovering the usdc reward token is not allowed
     error CannotRecoverRewardToken();
 
-    /// @notice error when user tries unstake during the cooldown period
-    /// @param canUnstakeAt timestamp when user can unstake
-    error MustWaitForUnlock(uint256 canUnstakeAt);
-
     /// @notice error when trying to set a rewards duration that is too short
     error RewardsDurationCannotBeZero();
 
-    /// @notice error when trying to set a cooldown period below the minimum
-    /// @param minCooldownPeriod minimum cooldown period
-    error CooldownPeriodTooLow(uint256 minCooldownPeriod);
-
-    /// @notice error when trying to set a cooldown period above the maximum
-    /// @param maxCooldownPeriod maximum cooldown period
-    error CooldownPeriodTooHigh(uint256 maxCooldownPeriod);
-
     /// @notice the caller is not approved to take this action
     error NotApproved();
 

test/foundry/integration/escrow.migrator.fork.t.sol

@@ -794,26 +794,7 @@ contract StakingV2MigrationForkTests is EscrowMigratorTestHelpers {
         // check final state - user2 didn't manage to migrate any entries
         checkStateAfterStepThree(user1, 0, 0);
     }
-
-    function test_Cannot_Bypass_Unstaking_Cooldown_Lock() public {
-        // this is the malicious entry - the duration is set to 1
-        createRewardEscrowEntryV1(user1, 50 ether, 1);
-
-        (uint256[] memory _entryIDs, uint256 numVestingEntries,) = claimAndFullyMigrate(user1);
-        checkStateAfterStepThree(user1, _entryIDs);
-
-        // specifically
-        uint256[] memory migratedEntryIDs =
-            rewardEscrowV2.getAccountVestingEntryIDs(user1, numVestingEntries - 2, 1);
-        uint256 maliciousEntryID = migratedEntryIDs[0];
-        (uint256 endTime, uint256 escrowAmount, uint256 duration, uint256 earlyVestingFee) =
-            rewardEscrowV2.getVestingEntry(maliciousEntryID);
-        assertEq(endTime, block.timestamp + stakingRewardsV2.cooldownPeriod());
-        assertEq(escrowAmount, 50 ether);
-        assertEq(duration, stakingRewardsV2.cooldownPeriod());
-        assertEq(earlyVestingFee, 90);
-    }
-
+
     function test_Cannot_Migrate_In_Non_Initiated_State() public {
         (uint256[] memory _entryIDs,) = claimAndCheckInitialState(user1);
 

test/foundry/integration/stakingV2.migration.fork.t.sol

@@ -103,8 +103,11 @@ contract StakingV2MigrationForkTests is StakingTestHelpers {
         // get rewards
         getStakingRewardsV2(user1);
 
+        // assert v2 rewards have been earned
+        assertGt(kwenta.balanceOf(user1), 0);
+
         // stake the rewards
-        stakeAllUnstakedEscrowV2(user1);
+        stakeFundsV2(user1, kwenta.balanceOf(user1));
 
         // check StakingRewardsV1 balance unchanged
         assertEq(stakingRewardsV1.nonEscrowedBalanceOf(user1), 0);
@@ -117,8 +120,6 @@ contract StakingV2MigrationForkTests is StakingTestHelpers {
         uint256 user1EscrowStakedV2 = stakingRewardsV2.escrowedBalanceOf(user1);
         uint256 user1NonEscrowedStakeV2 = stakingRewardsV2.nonEscrowedBalanceOf(user1);
 
-        // assert v2 rewards have been earned
-        assertGt(rewardEscrowV2.escrowedBalanceOf(user1), 0);
         // v2 staked balance is equal to escrowed + non-escrowed balance
         assertEq(stakingRewardsV2.balanceOf(user1), user1EscrowStakedV2 + user1NonEscrowedStakeV2);
         // v2 reward escrow balance is equal to escrow staked balance