diff --git a/protocol-contracts/staking/contracts/OperatorStaking.sol b/protocol-contracts/staking/contracts/OperatorStaking.sol index 2f047261d3..d3e969aa99 100644 --- a/protocol-contracts/staking/contracts/OperatorStaking.sol +++ b/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); diff --git a/protocol-contracts/staking/contracts/ProtocolStaking.sol b/protocol-contracts/staking/contracts/ProtocolStaking.sol index 5e51d77866..2974c5d807 100644 --- a/protocol-contracts/staking/contracts/ProtocolStaking.sol +++ b/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(); diff --git a/protocol-contracts/staking/test/OperatorStaking.test.ts b/protocol-contracts/staking/test/OperatorStaking.test.ts index 086f48f0d7..8df10104a1 100644 --- a/protocol-contracts/staking/test/OperatorStaking.test.ts +++ b/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 () { diff --git a/protocol-contracts/staking/test/ProtocolStaking.test.ts b/protocol-contracts/staking/test/ProtocolStaking.test.ts index 8a4aff78b1..b574218cb7 100644 --- a/protocol-contracts/staking/test/ProtocolStaking.test.ts +++ b/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