fix(protocol-contracts): skip zero-amount stake/unstake operations (N-08)

Author: melanciani

protocol-contracts/staking/contracts/OperatorStaking.sol

@@ -75,6 +75,9 @@ contract OperatorStaking is ERC1363Upgradeable, ReentrancyGuardTransient, UUPSUp
     /// @dev Thrown when the controller address is not valid (e.g., zero address).
     error InvalidController();
 
+    /// @dev Thrown when trying to redeem with no assets to withdraw from ProtocolStaking.
+    error NoAssetsToWithdraw();
+
     modifier onlyOwner() {
         require(msg.sender == owner(), CallerNotProtocolStakingOwner(msg.sender));
         _;
@@ -190,8 +193,11 @@ contract OperatorStaking is ERC1363Upgradeable, ReentrancyGuardTransient, UUPSUp
                 IERC20(asset()).balanceOf(address(this)) + protocolStaking_.awaitingRelease(address(this))
             );
 
+        // Revert if no assets to withdraw to prevent zero-amount unstake from advancing release time.
+        require(assetsToWithdraw > 0, NoAssetsToWithdraw());
+
         (, uint48 lastReleaseTime, uint208 controllerSharesRedeemed) = $._redeemRequests[controller].latestCheckpoint();
-        uint48 releaseTime = protocolStaking_.unstake(SafeCast.toUint256(SignedMath.max(assetsToWithdraw, 0)));
+        uint48 releaseTime = protocolStaking_.unstake(SafeCast.toUint256(assetsToWithdraw));
         assert(releaseTime >= lastReleaseTime); // should never happen
         $._redeemRequests[controller].push(releaseTime, controllerSharesRedeemed + shares);
 

protocol-contracts/staking/contracts/ProtocolStaking.sol

@@ -75,6 +75,8 @@ contract ProtocolStaking is AccessControlDefaultAdminRulesUpgradeable, ERC20Vote
     error TransferDisabled();
     /// @dev The unstake cooldown period is invalid.
     error InvalidUnstakeCooldownPeriod();
+    /// @dev The unstake amount is zero.
+    error ZeroUnstakeAmount();
 
     /// @custom:oz-upgrades-unsafe-allow constructor
     constructor() {
@@ -107,6 +109,8 @@ contract ProtocolStaking is AccessControlDefaultAdminRulesUpgradeable, ERC20Vote
      * @param amount The amount of tokens to stake.
      */
     function stake(uint256 amount) public {
+        if (amount == 0) return;
+
         _mint(msg.sender, amount);
         IERC20(stakingToken()).safeTransferFrom(msg.sender, address(this), amount);
 
@@ -120,11 +124,14 @@ contract ProtocolStaking is AccessControlDefaultAdminRulesUpgradeable, ERC20Vote
      * WARNING: Unstake release times are strictly increasing per account even if the cooldown period
      * is reduced. For a given account to fully realize the reduction in cooldown period, they may need
      * to wait up to `OLD_COOLDOWN_PERIOD - NEW_COOLDOWN_PERIOD` seconds after the cooldown period is updated.
+     * NOTE: Unstake amount must be greater than zero to prevent zero-amount unstake from advancing release time.
      *
      * @param amount The amount of tokens to unstake.
      * @return releaseTime The timestamp when the unstaked tokens can be released.
      */
     function unstake(uint256 amount) public returns (uint48) {
+        require(amount > 0, ZeroUnstakeAmount());
+
         _burn(msg.sender, amount);
 
         ProtocolStakingStorage storage $ = _getProtocolStakingStorage();

protocol-contracts/staking/test/OperatorStaking.test.ts

@@ -479,20 +479,15 @@ describe('OperatorStaking', function () {
       await this.mock.connect(this.delegator1).requestRedeem(ethers.parseEther('1'), this.delegator1, this.delegator1);
       await this.protocolStaking.slash(this.mock, ethers.parseEther('1'));
 
-      await timeIncreaseNoMine(30);
-
+      // Should revert when there are no assets to withdraw from ProtocolStaking.
       await expect(
         this.mock.connect(this.delegator2).requestRedeem(ethers.parseEther('1'), this.delegator2, this.delegator2),
-      )
-        .to.emit(this.protocolStaking, 'TokensUnstaked')
-        .withArgs(this.mock, 0, anyValue);
+      ).to.be.revertedWithCustomError(this.mock, 'NoAssetsToWithdraw');
 
-      await time.increase(30);
+      // Cooldown period is completed, only delegator1 can redeem now.
+      await time.increase(60);
       await expect(this.mock.maxRedeem(this.delegator2)).to.eventually.eq(0);
       await expect(this.mock.maxRedeem(this.delegator1)).to.eventually.eq(ethers.parseEther('1'));
-
-      await time.increase(30);
-      await expect(this.mock.maxRedeem(this.delegator2)).to.eventually.eq(ethers.parseEther('1'));
     });
 
     it('symmetrically passes on losses from withdrawal balance', async function () {

protocol-contracts/staking/test/ProtocolStaking.test.ts

@@ -117,6 +117,11 @@ describe('Protocol Staking', function () {
       await expect(this.mock.balanceOf(this.staker1)).to.eventually.equal(ethers.parseEther('100'));
     });
 
+    it('zero stake should return early without state changes', async function () {
+      await expect(this.mock.connect(this.staker1).stake(0)).to.not.emit(this.mock, 'TokensStaked');
+      await expect(this.mock.balanceOf(this.staker1)).to.eventually.equal(0);
+    });
+
     it("should not reward accounts that aren't eligible", async function () {
       await this.mock.connect(this.staker1).stake(ethers.parseEther('100'));
 
@@ -213,6 +218,13 @@ describe('Protocol Staking', function () {
         .to.not.emit(this.token, 'Transfer');
     });
 
+    it('zero unstake should revert', async function () {
+      await expect(this.mock.connect(this.staker1).unstake(0)).to.be.revertedWithCustomError(
+        this.mock,
+        'ZeroUnstakeAmount',
+      );
+    });
+
     describe('Release', function () {
       it('should transfer after cooldown complete', async function () {
         await this.mock.connect(this.manager).setUnstakeCooldownPeriod(60); // 1 minute