fix: tweaks to thawing pool management (#1048)

* fix: wip tweaks to thawing pool management

Signed-off-by: Tomás Migone <[email protected]>

* fix: handle edge cases for thawing pools

Signed-off-by: Tomás Migone <[email protected]>

* fix: simplify some conditions and checks

Signed-off-by: Tomás Migone <[email protected]>

* chore: fix test comment

Signed-off-by: Tomás Migone <[email protected]>

* fix: typos and variable rename

Signed-off-by: Tomás Migone <[email protected]>

* fix: merge conflicts

Signed-off-by: Tomás Migone <[email protected]>

* fix: improve thawing pool reset condition

Signed-off-by: Tomás Migone <[email protected]>

---------

Signed-off-by: Tomás Migone <[email protected]>

Author: tmigone

packages/horizon/contracts/interfaces/internal/IHorizonStakingMain.sol

@@ -254,8 +254,15 @@ interface IHorizonStakingMain {
      * @param tokens The amount of tokens being released
      * @param shares The amount of shares being released
      * @param thawingUntil The timestamp until the stake has thawed
+     * @param valid Whether the thaw request was valid at the time of fulfillment
      */
-    event ThawRequestFulfilled(bytes32 indexed thawRequestId, uint256 tokens, uint256 shares, uint64 thawingUntil);
+    event ThawRequestFulfilled(
+        bytes32 indexed thawRequestId,
+        uint256 tokens,
+        uint256 shares,
+        uint64 thawingUntil,
+        bool valid
+    );
 
     /**
      * @notice Emitted when a series of thaw requests are fulfilled.
@@ -742,6 +749,8 @@ interface IHorizonStakingMain {
      * Tokens can be automatically re-delegated to another provision by setting `newServiceProvider`.
      * @dev The parameter `nThawRequests` can be set to a non zero value to fulfill a specific number of thaw
      * requests in the event that fulfilling all of them results in a gas limit error.
+     * @dev If the delegation pool was completely slashed before withdrawing, calling this function will fulfill
+     * the thaw requests with an amount equal to zero.
      *
      * Requirements:
      * - Must have previously initiated a thaw request using {undelegate}.

packages/horizon/contracts/interfaces/internal/IHorizonStakingTypes.sol

@@ -32,6 +32,8 @@ interface IHorizonStakingTypes {
         uint32 maxVerifierCutPending;
         // Pending value for `thawingPeriod`. Verifier needs to accept it before it becomes active.
         uint64 thawingPeriodPending;
+        // Value of the current thawing nonce. Thaw requests with older nonces are invalid.
+        uint256 thawingNonce;
     }
 
     /**
@@ -75,6 +77,8 @@ interface IHorizonStakingTypes {
         uint256 tokensThawing;
         // Shares representing the thawing tokens
         uint256 sharesThawing;
+        // Value of the current thawing nonce. Thaw requests with older nonces are invalid.
+        uint256 thawingNonce;
     }
 
     /**
@@ -101,6 +105,8 @@ interface IHorizonStakingTypes {
         uint256 tokensThawing;
         // Shares representing the thawing tokens
         uint256 sharesThawing;
+        // Value of the current thawing nonce. Thaw requests with older nonces are invalid.
+        uint256 thawingNonce;
     }
 
     /**
@@ -137,5 +143,7 @@ interface IHorizonStakingTypes {
         uint64 thawingUntil;
         // Id of the next thaw request in the linked list
         bytes32 next;
+        // Used to invalidate unfulfilled thaw requests
+        uint256 thawingNonce;
     }
 }

packages/horizon/contracts/staking/HorizonStaking.sol

@@ -411,6 +411,14 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
                 (FIXED_POINT_PRECISION);
             prov.tokens = prov.tokens - providerTokensSlashed;
 
+            // If the slashing leaves the thawing shares with no thawing tokens, cancel pending thawings by:
+            // - deleting all thawing shares
+            // - incrementing the nonce to invalidate pending thaw requests
+            if (prov.sharesThawing != 0 && prov.tokensThawing == 0) {
+                prov.sharesThawing = 0;
+                prov.thawingNonce++;
+            }
+
             // Service provider accounting
             _serviceProviders[serviceProvider].tokensProvisioned =
                 _serviceProviders[serviceProvider].tokensProvisioned -
@@ -438,6 +446,16 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
                     (pool.tokensThawing * (FIXED_POINT_PRECISION - delegationFractionSlashed)) /
                     FIXED_POINT_PRECISION;
 
+                // If the slashing leaves the thawing shares with no thawing tokens, cancel pending thawings by:
+                // - deleting all thawing shares
+                // - incrementing the nonce to invalidate pending thaw requests
+                // Note that thawing shares are completely lost, delegators won't get back the corresponding
+                // delegation pool shares.
+                if (pool.sharesThawing != 0 && pool.tokensThawing == 0) {
+                    pool.sharesThawing = 0;
+                    pool.thawingNonce++;
+                }
+
                 emit DelegationSlashed(serviceProvider, verifier, tokensToSlash);
             } else {
                 emit DelegationSlashingSkipped(serviceProvider, verifier, tokensToSlash);
@@ -644,7 +662,8 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
             thawingPeriod: _thawingPeriod,
             createdAt: uint64(block.timestamp),
             maxVerifierCutPending: _maxVerifierCut,
-            thawingPeriodPending: _thawingPeriod
+            thawingPeriodPending: _thawingPeriod,
+            thawingNonce: 0
         });
 
         ServiceProviderInternal storage sp = _serviceProviders[_serviceProvider];
@@ -672,25 +691,34 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
 
     /**
      * @notice See {IHorizonStakingMain-thaw}.
+     * @dev We use a thawing pool to keep track of tokens thawing for multiple thaw requests.
+     * If due to slashing the thawing pool loses all of its tokens, the pool is reset and all pending thaw
+     * requests are invalidated.
      */
     function _thaw(address _serviceProvider, address _verifier, uint256 _tokens) private returns (bytes32) {
         require(_tokens != 0, HorizonStakingInvalidZeroTokens());
         uint256 tokensAvailable = _getProviderTokensAvailable(_serviceProvider, _verifier);
         require(tokensAvailable >= _tokens, HorizonStakingInsufficientTokens(tokensAvailable, _tokens));
 
         Provision storage prov = _provisions[_serviceProvider][_verifier];
-        uint256 thawingShares = prov.sharesThawing == 0 ? _tokens : (prov.sharesThawing * _tokens) / prov.tokensThawing;
+
+        // Calculate shares to issue
+        // Thawing pool is reset/initialized when the pool is empty: prov.tokensThawing == 0
+        uint256 thawingShares = prov.tokensThawing == 0
+            ? _tokens
+            : ((prov.sharesThawing * _tokens) / prov.tokensThawing);
         uint64 thawingUntil = uint64(block.timestamp + uint256(prov.thawingPeriod));
 
-        prov.tokensThawing = prov.tokensThawing + _tokens;
         prov.sharesThawing = prov.sharesThawing + thawingShares;
+        prov.tokensThawing = prov.tokensThawing + _tokens;
 
         bytes32 thawRequestId = _createThawRequest(
             _serviceProvider,
             _verifier,
             _serviceProvider,
             thawingShares,
-            thawingUntil
+            thawingUntil,
+            prov.thawingNonce
         );
         emit ProvisionThawed(_serviceProvider, _verifier, _tokens);
         return thawRequestId;
@@ -715,7 +743,8 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
             _serviceProvider,
             tokensThawing,
             sharesThawing,
-            _nThawRequests
+            _nThawRequests,
+            prov.thawingNonce
         );
 
         prov.tokens = prov.tokens - tokensThawed_;
@@ -741,15 +770,19 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
         DelegationPoolInternal storage pool = _getDelegationPool(_serviceProvider, _verifier);
         DelegationInternal storage delegation = pool.delegators[msg.sender];
 
+        // An invalid delegation pool has shares but no tokens
         require(
-            pool.tokens != 0 || (pool.shares == 0 && pool.sharesThawing == 0),
+            pool.tokens != 0 || pool.shares == 0,
             HorizonStakingInvalidDelegationPoolState(_serviceProvider, _verifier)
         );
 
         // Calculate shares to issue
-        uint256 shares = (pool.tokens == 0 || pool.tokens == pool.tokensThawing)
-            ? _tokens
-            : ((_tokens * pool.shares) / (pool.tokens - pool.tokensThawing));
+        // Delegation pool is reset/initialized in any of the following cases:
+        // - pool.tokens == 0 and pool.shares == 0, pool is completely empty. Note that we don't test shares == 0 because
+        //   the invalid delegation pool check already ensures shares are 0 if tokens are 0
+        // - pool.tokens == pool.tokensThawing, the entire pool is thawing
+        bool initializePool = pool.tokens == 0 || pool.tokens == pool.tokensThawing;
+        uint256 shares = initializePool ? _tokens : ((_tokens * pool.shares) / (pool.tokens - pool.tokensThawing));
         require(shares != 0 && shares >= _minSharesOut, HorizonStakingSlippageProtection(shares, _minSharesOut));
 
         pool.tokens = pool.tokens + _tokens;
@@ -764,6 +797,12 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
      * @notice See {IHorizonStakingMain-undelegate}.
      * @dev To allow delegation to be slashable even while thawing without breaking accounting
      * the delegation pool shares are burned and replaced with thawing pool shares.
+     * @dev Note that due to slashing the delegation pool can enter an invalid state if all it's tokens are slashed.
+     * An invalid pool can only be recovered by adding back tokens into the pool with {IHorizonStakingMain-addToDelegationPool}.
+     * Any time the delegation pool is invalidated, the thawing pool is also reset and any pending undelegate requests get
+     * invalidated.
+     * Note that delegation that is caught thawing when the pool is invalidated will be completely lost! However delegation shares
+     * that were not thawing will be preserved.
      */
     function _undelegate(
         address _serviceProvider,
@@ -775,23 +814,30 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
         DelegationPoolInternal storage pool = _getDelegationPool(_serviceProvider, _verifier);
         DelegationInternal storage delegation = pool.delegators[msg.sender];
         require(delegation.shares >= _shares, HorizonStakingInsufficientShares(delegation.shares, _shares));
+
+        // An invalid delegation pool has shares but no tokens (previous require check ensures shares > 0)
         require(pool.tokens != 0, HorizonStakingInvalidDelegationPoolState(_serviceProvider, _verifier));
 
+        // Calculate thawing shares to issue - convert delegation pool shares to thawing pool shares
+        // delegation pool shares -> delegation pool tokens -> thawing pool shares
+        // Thawing pool is reset/initialized when the pool is empty: prov.tokensThawing == 0
         uint256 tokens = (_shares * (pool.tokens - pool.tokensThawing)) / pool.shares;
         uint256 thawingShares = pool.tokensThawing == 0 ? tokens : ((tokens * pool.sharesThawing) / pool.tokensThawing);
         uint64 thawingUntil = uint64(block.timestamp + uint256(_provisions[_serviceProvider][_verifier].thawingPeriod));
-        pool.shares = pool.shares - _shares;
+
         pool.tokensThawing = pool.tokensThawing + tokens;
         pool.sharesThawing = pool.sharesThawing + thawingShares;
 
+        pool.shares = pool.shares - _shares;
         delegation.shares = delegation.shares - _shares;
 
         bytes32 thawRequestId = _createThawRequest(
             _serviceProvider,
             _verifier,
             beneficiary,
             thawingShares,
-            thawingUntil
+            thawingUntil,
+            pool.thawingNonce
         );
 
         emit TokensUndelegated(_serviceProvider, _verifier, msg.sender, tokens);
@@ -809,7 +855,12 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
         uint256 _nThawRequests
     ) private {
         DelegationPoolInternal storage pool = _getDelegationPool(_serviceProvider, _verifier);
-        require(pool.tokens != 0, HorizonStakingInvalidDelegationPoolState(_serviceProvider, _verifier));
+
+        // An invalid delegation pool has shares but no tokens
+        require(
+            pool.tokens != 0 || pool.shares == 0,
+            HorizonStakingInvalidDelegationPoolState(_serviceProvider, _verifier)
+        );
 
         uint256 tokensThawed = 0;
         uint256 sharesThawing = pool.sharesThawing;
@@ -820,9 +871,12 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
             msg.sender,
             tokensThawing,
             sharesThawing,
-            _nThawRequests
+            _nThawRequests,
+            pool.thawingNonce
         );
 
+        // The next subtraction should never revert becase: pool.tokens >= pool.tokensThawing and pool.tokensThawing >= tokensThawed
+        // In the event the pool gets completely slashed tokensThawed will fulfil to 0.
         pool.tokens = pool.tokens - tokensThawed;
         pool.sharesThawing = sharesThawing;
         pool.tokensThawing = tokensThawing;
@@ -849,20 +903,27 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
      * @param _owner The address of the owner of the thaw request
      * @param _shares The number of shares to thaw
      * @param _thawingUntil The timestamp until which the shares are thawing
+     * @param _thawingNonce Owner's validity nonce for the thaw request
      * @return The ID of the thaw request
      */
     function _createThawRequest(
         address _serviceProvider,
         address _verifier,
         address _owner,
         uint256 _shares,
-        uint64 _thawingUntil
+        uint64 _thawingUntil,
+        uint256 _thawingNonce
     ) private returns (bytes32) {
         LinkedList.List storage thawRequestList = _thawRequestLists[_serviceProvider][_verifier][_owner];
         require(thawRequestList.count < MAX_THAW_REQUESTS, HorizonStakingTooManyThawRequests());
 
         bytes32 thawRequestId = keccak256(abi.encodePacked(_serviceProvider, _verifier, _owner, thawRequestList.nonce));
-        _thawRequests[thawRequestId] = ThawRequest({ shares: _shares, thawingUntil: _thawingUntil, next: bytes32(0) });
+        _thawRequests[thawRequestId] = ThawRequest({
+            shares: _shares,
+            thawingUntil: _thawingUntil,
+            next: bytes32(0),
+            thawingNonce: _thawingNonce
+        });
 
         if (thawRequestList.count != 0) _thawRequests[thawRequestList.tail].next = thawRequestId;
         thawRequestList.addTail(thawRequestId);
@@ -880,6 +941,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
      * @param _tokensThawing The current amount of tokens already thawing
      * @param _sharesThawing The current amount of shares already thawing
      * @param _nThawRequests The number of thaw requests to fulfill. If set to 0, all thaw requests are fulfilled.
+     * @param _thawingNonce The current valid thawing nonce. Any thaw request with a different nonce is invalid and should be ignored.
      * @return The amount of thawed tokens
      * @return The amount of tokens still thawing
      * @return The amount of shares still thawing
@@ -890,7 +952,8 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
         address _owner,
         uint256 _tokensThawing,
         uint256 _sharesThawing,
-        uint256 _nThawRequests
+        uint256 _nThawRequests,
+        uint256 _thawingNonce
     ) private returns (uint256, uint256, uint256) {
         LinkedList.List storage thawRequestList = _thawRequestLists[_serviceProvider][_verifier][_owner];
         require(thawRequestList.count > 0, HorizonStakingNothingThawing());
@@ -900,7 +963,7 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
             _getNextThawRequest,
             _fulfillThawRequest,
             _deleteThawRequest,
-            abi.encode(tokensThawed, _tokensThawing, _sharesThawing),
+            abi.encode(tokensThawed, _tokensThawing, _sharesThawing, _thawingNonce),
             _nThawRequests
         );
 
@@ -929,20 +992,30 @@ contract HorizonStaking is HorizonStakingBase, IHorizonStakingMain {
         }
 
         // decode
-        (uint256 tokensThawed, uint256 tokensThawing, uint256 sharesThawing) = abi.decode(
+        (uint256 tokensThawed, uint256 tokensThawing, uint256 sharesThawing, uint256 thawingNonce) = abi.decode(
             _acc,
-            (uint256, uint256, uint256)
+            (uint256, uint256, uint256, uint256)
         );
 
-        // process
-        uint256 tokens = (thawRequest.shares * tokensThawing) / sharesThawing;
-        tokensThawing = tokensThawing - tokens;
-        sharesThawing = sharesThawing - thawRequest.shares;
-        tokensThawed = tokensThawed + tokens;
-        emit ThawRequestFulfilled(_thawRequestId, tokens, thawRequest.shares, thawRequest.thawingUntil);
+        // process - only fulfill thaw requests for the current valid nonce
+        uint256 tokens = 0;
+        bool validThawRequest = thawRequest.thawingNonce == thawingNonce;
+        if (validThawRequest) {
+            tokens = (thawRequest.shares * tokensThawing) / sharesThawing;
+            tokensThawing = tokensThawing - tokens;
+            sharesThawing = sharesThawing - thawRequest.shares;
+            tokensThawed = tokensThawed + tokens;
+        }
+        emit ThawRequestFulfilled(
+            _thawRequestId,
+            tokens,
+            thawRequest.shares,
+            thawRequest.thawingUntil,
+            validThawRequest
+        );
 
         // encode
-        _acc = abi.encode(tokensThawed, tokensThawing, sharesThawing);
+        _acc = abi.encode(tokensThawed, tokensThawing, sharesThawing, thawingNonce);
         return (false, _acc);
     }
 

packages/horizon/contracts/staking/HorizonStakingBase.sol

@@ -96,6 +96,7 @@ abstract contract HorizonStakingBase is
         pool.shares = poolInternal.shares;
         pool.tokensThawing = poolInternal.tokensThawing;
         pool.sharesThawing = poolInternal.sharesThawing;
+        pool.thawingNonce = poolInternal.thawingNonce;
         return pool;
     }