From a0c291495e0e7631c63fd87e0b31a683e5ae2676 Mon Sep 17 00:00:00 2001 From: Marco Tabasco Date: Fri, 29 Sep 2023 11:02:53 +0200 Subject: [PATCH] Fix network withdrawals (#262) * fix sequential network withdrawals issue --- CHANGELOG.md | 13 +++++++++ contracts/modules/SSVDAO.sol | 1 + test/account/withdraw.ts | 46 +++++++++++++++++++++++++------- test/dao/network-fee-withdraw.ts | 16 ++++++++--- 4 files changed, 64 insertions(+), 12 deletions(-) create mode 100644 CHANGELOG.md diff --git a/CHANGELOG.md b/CHANGELOG.md new file mode 100644 index 00000000..166b5261 --- /dev/null +++ b/CHANGELOG.md @@ -0,0 +1,13 @@ +# Changelog + +All notable changes to SSV Network contracts will be documented in this file. + +This project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). + +## [Unreleased] + +## [v1.0.1.rc4] - 2023-09-19 + +### Fixed + +- [22d2859](https://github.com/bloxapp/ssv-network/pull/262/commits/22d2859d8fe6267b09c7a1c9c645df19bdaa03ff) Fix bug in network earnings withdrawals. diff --git a/contracts/modules/SSVDAO.sol b/contracts/modules/SSVDAO.sol index cec7a6cf..e3e0a700 100644 --- a/contracts/modules/SSVDAO.sol +++ b/contracts/modules/SSVDAO.sol @@ -34,6 +34,7 @@ contract SSVDAO is ISSVDAO { } sp.daoBalance = networkBalance - shrunkAmount; + sp.daoIndexBlockNumber = uint32(block.number); CoreLib.transferBalance(msg.sender, amount); diff --git a/test/account/withdraw.ts b/test/account/withdraw.ts index 0ab67d61..5c5c6068 100644 --- a/test/account/withdraw.ts +++ b/test/account/withdraw.ts @@ -5,13 +5,15 @@ import { expect } from 'chai'; import { trackGas, GasGroup } from '../helpers/gas-usage'; // Declare globals -let ssvNetworkContract: any, ssvToken: any, cluster1: any, minDepositAmount: any; +let ssvNetworkContract: any, ssvViews: any, ssvToken: any, cluster1: any, minDepositAmount: any; describe('Withdraw Tests', () => { beforeEach(async () => { // Initialize contract const metadata = (await helpers.initializeContract()); ssvNetworkContract = metadata.contract; + ssvNetworkContract = metadata.contract; + ssvViews = metadata.ssvViews; ssvToken = metadata.ssvToken; // Register operators @@ -48,19 +50,19 @@ describe('Withdraw Tests', () => { }); it('Withdraw from operator balance emits "OperatorWithdrawn"', async () => { - await expect(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawOperatorEarnings(uint64,uint256)'](1, helpers.CONFIG.minimalOperatorFee)).to.emit(ssvNetworkContract, 'OperatorWithdrawn'); + await expect(ssvNetworkContract.connect(helpers.DB.owners[0]).withdrawOperatorEarnings(1, helpers.CONFIG.minimalOperatorFee)).to.emit(ssvNetworkContract, 'OperatorWithdrawn'); }); it('Withdraw from operator balance gas limits', async () => { - await trackGas(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawOperatorEarnings(uint64,uint256)'](1, helpers.CONFIG.minimalOperatorFee), [GasGroup.WITHDRAW_OPERATOR_BALANCE]); + await trackGas(ssvNetworkContract.connect(helpers.DB.owners[0]).withdrawOperatorEarnings(1, helpers.CONFIG.minimalOperatorFee), [GasGroup.WITHDRAW_OPERATOR_BALANCE]); }); it('Withdraw the total operator balance emits "OperatorWithdrawn"', async () => { - await expect(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawAllOperatorEarnings(uint64)'](1)).to.emit(ssvNetworkContract, 'OperatorWithdrawn'); + await expect(ssvNetworkContract.connect(helpers.DB.owners[0]).withdrawAllOperatorEarnings(1)).to.emit(ssvNetworkContract, 'OperatorWithdrawn'); }); it('Withdraw the total operator balance gas limits', async () => { - await trackGas(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawAllOperatorEarnings(uint64)'](1), [GasGroup.WITHDRAW_OPERATOR_BALANCE]); + await trackGas(ssvNetworkContract.connect(helpers.DB.owners[0]).withdrawAllOperatorEarnings(1), [GasGroup.WITHDRAW_OPERATOR_BALANCE]); }); it('Withdraw from a cluster that has a removed operator emits "ClusterWithdrawn"', async () => { @@ -72,6 +74,21 @@ describe('Withdraw Tests', () => { await expect(ssvNetworkContract.connect(helpers.DB.owners[4]).withdraw(cluster1.operatorIds, minDepositAmount, cluster1.cluster)).to.be.revertedWithCustomError(ssvNetworkContract, 'InsufficientBalance'); }); + it('Sequentially withdraw more than the cluster balance reverts "InsufficientBalance"', async () => { + const burnPerBlock = helpers.CONFIG.minimalOperatorFee * 4; + + cluster1 = await helpers.deposit(1, cluster1.owner, cluster1.operatorIds, (minDepositAmount * 2).toString(), cluster1.cluster); + expect(await ssvViews.getBalance(helpers.DB.owners[4].address, cluster1.operatorIds, cluster1.cluster)).to.be.equals(minDepositAmount * 3 - (burnPerBlock * 2)); + + cluster1 = await helpers.withdraw(4, cluster1.operatorIds, minDepositAmount, cluster1.cluster); + expect(await ssvViews.getBalance(helpers.DB.owners[4].address, cluster1.operatorIds, cluster1.cluster)).to.be.equals(minDepositAmount * 2 - (burnPerBlock * 3)); + + cluster1 = await helpers.withdraw(4, cluster1.operatorIds, minDepositAmount, cluster1.cluster); + expect(await ssvViews.getBalance(helpers.DB.owners[4].address, cluster1.operatorIds, cluster1.cluster)).to.be.equals(minDepositAmount - (burnPerBlock * 4)); + + await expect(ssvNetworkContract.connect(helpers.DB.owners[4]).withdraw(cluster1.operatorIds, minDepositAmount, cluster1.cluster)).to.be.revertedWithCustomError(ssvNetworkContract, 'InsufficientBalance'); + }); + it('Withdraw from a liquidatable cluster reverts "InsufficientBalance" (liquidation threshold)', async () => { await utils.progressBlocks(20); await expect(ssvNetworkContract.connect(helpers.DB.owners[4]).withdraw(cluster1.operatorIds, 4000000000, cluster1.cluster)).to.be.revertedWithCustomError(ssvNetworkContract, 'InsufficientBalance'); @@ -88,20 +105,31 @@ describe('Withdraw Tests', () => { }); it('Withdraw balance from an operator I do not own reverts "CallerNotOwner"', async () => { - await expect(ssvNetworkContract.connect(helpers.DB.owners[2])['withdrawOperatorEarnings(uint64,uint256)'](1, minDepositAmount)).to.be.revertedWithCustomError(ssvNetworkContract, 'CallerNotOwner'); + await expect(ssvNetworkContract.connect(helpers.DB.owners[2]).withdrawOperatorEarnings(1, minDepositAmount)).to.be.revertedWithCustomError(ssvNetworkContract, 'CallerNotOwner'); }); it('Withdraw more than the operator balance reverts "InsufficientBalance"', async () => { - await expect(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawOperatorEarnings(uint64,uint256)'](1, minDepositAmount + await expect(ssvNetworkContract.connect(helpers.DB.owners[0]).withdrawOperatorEarnings(1, minDepositAmount + )).to.be.revertedWithCustomError(ssvNetworkContract, 'InsufficientBalance'); + }); + + it('Sequentially withdraw more than the operator balance reverts "InsufficientBalance"', async () => { + await ssvNetworkContract.connect(helpers.DB.owners[0]).withdrawOperatorEarnings(1, helpers.CONFIG.minimalOperatorFee * 3); + expect(await ssvViews.getOperatorEarnings(1)).to.be.equals(helpers.CONFIG.minimalOperatorFee * 4 - helpers.CONFIG.minimalOperatorFee * 3); + + await ssvNetworkContract.connect(helpers.DB.owners[0]).withdrawOperatorEarnings(1, helpers.CONFIG.minimalOperatorFee * 3); + expect(await ssvViews.getOperatorEarnings(1)).to.be.equals(helpers.CONFIG.minimalOperatorFee * 6 - helpers.CONFIG.minimalOperatorFee * 6); + + await expect(ssvNetworkContract.connect(helpers.DB.owners[0]).withdrawOperatorEarnings(1, helpers.CONFIG.minimalOperatorFee * 3 )).to.be.revertedWithCustomError(ssvNetworkContract, 'InsufficientBalance'); }); it('Withdraw the total balance from an operator I do not own reverts "CallerNotOwner"', async () => { - await expect(ssvNetworkContract.connect(helpers.DB.owners[2])['withdrawAllOperatorEarnings(uint64)'](12)).to.be.revertedWithCustomError(ssvNetworkContract, 'CallerNotOwner'); + await expect(ssvNetworkContract.connect(helpers.DB.owners[2]).withdrawAllOperatorEarnings(12)).to.be.revertedWithCustomError(ssvNetworkContract, 'CallerNotOwner'); }); it('Withdraw more than the operator total balance reverts "InsufficientBalance"', async () => { - await expect(ssvNetworkContract.connect(helpers.DB.owners[0])['withdrawAllOperatorEarnings(uint64)'](12)).to.be.revertedWithCustomError(ssvNetworkContract, 'InsufficientBalance'); + await expect(ssvNetworkContract.connect(helpers.DB.owners[0]).withdrawAllOperatorEarnings(12)).to.be.revertedWithCustomError(ssvNetworkContract, 'InsufficientBalance'); }); it('Withdraw from a cluster without validators', async () => { diff --git a/test/dao/network-fee-withdraw.ts b/test/dao/network-fee-withdraw.ts index 79721816..c09cdad1 100644 --- a/test/dao/network-fee-withdraw.ts +++ b/test/dao/network-fee-withdraw.ts @@ -33,9 +33,6 @@ describe('DAO Network Fee Withdraw Tests', () => { await helpers.registerValidators(4, 1, minDepositAmount, helpers.DataGenerator.cluster.new(), [GasGroup.REGISTER_VALIDATOR_NEW_STATE]); await utils.progressBlocks(10); - // Temporary till deposit logic not available - // Mint tokens - await helpers.DB.ssvToken.mint(ssvNetworkContract.address, minDepositAmount); }); it('Withdraw network earnings emits "NetworkEarningsWithdrawn"', async () => { @@ -68,4 +65,17 @@ describe('DAO Network Fee Withdraw Tests', () => { await expect(ssvNetworkContract.connect(helpers.DB.owners[3]).withdrawNetworkEarnings(amount )).to.be.revertedWith('Ownable: caller is not the owner'); }); + + it('Withdraw network earnings sequentially when not enough balance reverts "InsufficientBalance"', async () => { + const amount = await ssvViews.getNetworkEarnings() / 2; + + await ssvNetworkContract.withdrawNetworkEarnings(amount); + expect(await ssvViews.getNetworkEarnings()).to.be.equals(((networkFee * 13) + (networkFee * 11) - amount)); + + await ssvNetworkContract.withdrawNetworkEarnings(amount); + expect(await ssvViews.getNetworkEarnings()).to.be.equals(((networkFee * 14) + (networkFee * 12) - amount * 2)); + + await expect(ssvNetworkContract.withdrawNetworkEarnings(amount + )).to.be.revertedWithCustomError(ssvNetworkContract, 'InsufficientBalance'); + }); }); \ No newline at end of file