From bb50fd7e453b9bd83b70e7e72dfa4349684ceefc Mon Sep 17 00:00:00 2001 From: Ivan Zhelyazkov Date: Thu, 19 Oct 2023 10:31:51 +0300 Subject: [PATCH 1/2] burn network fees --- contracts/network/BancorNetwork.sol | 76 +++++++----- .../network/interfaces/IBancorNetwork.sol | 8 +- deploy/tests/network.ts | 1 - test/network/BancorNetwork.ts | 112 ++++++++++++------ utils/Roles.ts | 3 +- 5 files changed, 124 insertions(+), 76 deletions(-) diff --git a/contracts/network/BancorNetwork.sol b/contracts/network/BancorNetwork.sol index c92b36589..ba3413d91 100644 --- a/contracts/network/BancorNetwork.sol +++ b/contracts/network/BancorNetwork.sol @@ -95,9 +95,6 @@ contract BancorNetwork is IBancorNetwork, Upgradeable, ReentrancyGuardUpgradeabl // the emergency manager role is required to pause/unpause the network bytes32 private constant ROLE_EMERGENCY_STOPPER = keccak256("ROLE_EMERGENCY_STOPPER"); - // the network fee manager role is required to pull the accumulated pending network fees - bytes32 private constant ROLE_NETWORK_FEE_MANAGER = keccak256("ROLE_NETWORK_FEE_MANAGER"); - // the address of the BNT token IERC20 private immutable _bnt; @@ -149,15 +146,18 @@ contract BancorNetwork is IBancorNetwork, Upgradeable, ReentrancyGuardUpgradeabl // a mapping between pools and their respective pool collections mapping(Token => IPoolCollection) private _collectionByPool; - // the pending network fee amount to be burned by the vortex + // the pending network fee amount to be burned uint256 internal _pendingNetworkFeeAmount; bool private _depositingEnabled = true; uint32 private _polRewardsPPM; + // min network fee amount that can be burned + uint256 private _minNetworkFeeBurn; + // upgrade forward-compatibility storage gap - uint256[MAX_GAP - 11] private __gap; + uint256[MAX_GAP - 12] private __gap; /** * @dev triggered when a new pool collection is added @@ -217,9 +217,9 @@ contract BancorNetwork is IBancorNetwork, Upgradeable, ReentrancyGuardUpgradeabl event FlashLoanCompleted(Token indexed token, address indexed borrower, uint256 amount, uint256 feeAmount); /** - * @dev triggered when network fees are withdrawn + * @dev triggered when network fees are burned */ - event NetworkFeesWithdrawn(address indexed caller, address indexed recipient, uint256 amount); + event NetworkFeesBurned(address indexed caller, uint256 amount); /** * @dev triggered when pool surplus tokens are withdrawn @@ -231,6 +231,11 @@ contract BancorNetwork is IBancorNetwork, Upgradeable, ReentrancyGuardUpgradeabl */ event POLRewardsPPMUpdated(uint32 oldRewardsPPM, uint32 newRewardsPPM); + /** + * @dev triggered when the min network fee burn is updated + */ + event MinNetworkFeeBurnUpdated(uint256 oldMinNetworkFeeBurn, uint256 newMinNetworkFeeBurn); + /** * @dev a "virtual" constructor that is only used to set immutable state variables */ @@ -315,11 +320,11 @@ contract BancorNetwork is IBancorNetwork, Upgradeable, ReentrancyGuardUpgradeabl // set up administrative roles _setRoleAdmin(ROLE_MIGRATION_MANAGER, ROLE_ADMIN); _setRoleAdmin(ROLE_EMERGENCY_STOPPER, ROLE_ADMIN); - _setRoleAdmin(ROLE_NETWORK_FEE_MANAGER, ROLE_ADMIN); _depositingEnabled = true; _setPOLRewardsPPM(2000); + _setMinNetworkFeeBurn(1_000_000e18); } // solhint-enable func-name-mixedcase @@ -360,14 +365,7 @@ contract BancorNetwork is IBancorNetwork, Upgradeable, ReentrancyGuardUpgradeabl } /** - * @dev returns the network fee manager role - */ - function roleNetworkFeeManager() external pure returns (bytes32) { - return ROLE_NETWORK_FEE_MANAGER; - } - - /** - * @dev returns the pending network fee amount to be burned by the vortex + * @dev returns the pending network fee amount to be burned */ function pendingNetworkFeeAmount() external view returns (uint256) { return _pendingNetworkFeeAmount; @@ -820,26 +818,17 @@ contract BancorNetwork is IBancorNetwork, Upgradeable, ReentrancyGuardUpgradeabl /** * @inheritdoc IBancorNetwork */ - function withdrawNetworkFees( - address recipient - ) - external - whenNotPaused - onlyRoleMember(ROLE_NETWORK_FEE_MANAGER) - validAddress(recipient) - nonReentrant - returns (uint256) - { + function burnNetworkFees() external whenNotPaused nonReentrant returns (uint256) { uint256 currentPendingNetworkFeeAmount = _pendingNetworkFeeAmount; - if (currentPendingNetworkFeeAmount == 0) { + if (currentPendingNetworkFeeAmount < _minNetworkFeeBurn) { return 0; } _pendingNetworkFeeAmount = 0; - _masterVault.withdrawFunds(Token(address(_bnt)), payable(recipient), currentPendingNetworkFeeAmount); + _masterVault.withdrawFunds(Token(address(_bnt)), payable(address(_bnt)), currentPendingNetworkFeeAmount); - emit NetworkFeesWithdrawn(msg.sender, recipient, currentPendingNetworkFeeAmount); + emit NetworkFeesBurned(msg.sender, currentPendingNetworkFeeAmount); return currentPendingNetworkFeeAmount; } @@ -915,6 +904,35 @@ contract BancorNetwork is IBancorNetwork, Upgradeable, ReentrancyGuardUpgradeabl emit POLRewardsPPMUpdated(oldRewardsPPM, newRewardsPPM); } + /** + * @dev returns the min network fee burn + */ + function minNetworkFeeBurn() external view returns (uint256) { + return _minNetworkFeeBurn; + } + + /** + * @dev set the min network fee burn + */ + function setMinNetworkFeeBurn( + uint256 newMinNetworkFeeBurn + ) external onlyAdmin greaterThanZero(newMinNetworkFeeBurn) { + _setMinNetworkFeeBurn(newMinNetworkFeeBurn); + } + + /** + * @dev set the min network fee burn + */ + function _setMinNetworkFeeBurn(uint256 newMinNetworkFeeBurn) private { + uint256 oldMinNetworkFeeBurn = _minNetworkFeeBurn; + if (oldMinNetworkFeeBurn == newMinNetworkFeeBurn) { + return; + } + + _minNetworkFeeBurn = newMinNetworkFeeBurn; + emit MinNetworkFeeBurnUpdated(oldMinNetworkFeeBurn, newMinNetworkFeeBurn); + } + /** * @dev generates context ID for a deposit request */ diff --git a/contracts/network/interfaces/IBancorNetwork.sol b/contracts/network/interfaces/IBancorNetwork.sol index 47a200a9c..0c6c3e6b0 100644 --- a/contracts/network/interfaces/IBancorNetwork.sol +++ b/contracts/network/interfaces/IBancorNetwork.sol @@ -218,13 +218,9 @@ interface IBancorNetwork is IUpgradeable { ) external payable; /** - * @dev withdraws pending network fees, and returns the amount of fees withdrawn - * - * requirements: - * - * - the caller must have the ROLE_NETWORK_FEE_MANAGER privilege + * @dev burns pending network fees, and returns the amount of fees burned */ - function withdrawNetworkFees(address recipient) external returns (uint256); + function burnNetworkFees() external returns (uint256); /** * @dev withdraws surplus tokens from a given pool to CarbonPOL contract, diff --git a/deploy/tests/network.ts b/deploy/tests/network.ts index 8243b3875..11af015e2 100644 --- a/deploy/tests/network.ts +++ b/deploy/tests/network.ts @@ -187,7 +187,6 @@ import { getNamedAccounts } from 'hardhat'; await expectRoleMembers(network, Roles.Upgradeable.ROLE_ADMIN, [daoMultisig.address]); await expectRoleMembers(network, Roles.BancorNetwork.ROLE_MIGRATION_MANAGER, [liquidityProtection.address]); await expectRoleMembers(network, Roles.BancorNetwork.ROLE_EMERGENCY_STOPPER); - await expectRoleMembers(network, Roles.BancorNetwork.ROLE_NETWORK_FEE_MANAGER, [daoMultisig.address]); await expectRoleMembers(standardRewards, Roles.Upgradeable.ROLE_ADMIN, [daoMultisig.address]); diff --git a/test/network/BancorNetwork.ts b/test/network/BancorNetwork.ts index 6c60040d4..6514d15b4 100644 --- a/test/network/BancorNetwork.ts +++ b/test/network/BancorNetwork.ts @@ -333,7 +333,6 @@ describe('BancorNetwork', () => { await expectRole(network, Roles.Upgradeable.ROLE_ADMIN, Roles.Upgradeable.ROLE_ADMIN, [deployer.address]); await expectRole(network, Roles.BancorNetwork.ROLE_MIGRATION_MANAGER, Roles.Upgradeable.ROLE_ADMIN); await expectRole(network, Roles.BancorNetwork.ROLE_EMERGENCY_STOPPER, Roles.Upgradeable.ROLE_ADMIN); - await expectRole(network, Roles.BancorNetwork.ROLE_NETWORK_FEE_MANAGER, Roles.Upgradeable.ROLE_ADMIN); expect(await network.paused()).to.be.false; expect(await network.poolCollections()).to.be.empty; @@ -3224,29 +3223,53 @@ describe('BancorNetwork', () => { await network .connect(deployer) .grantRole(Roles.BancorNetwork.ROLE_EMERGENCY_STOPPER, emergencyStopper.address); - await network - .connect(deployer) - .grantRole(Roles.BancorNetwork.ROLE_NETWORK_FEE_MANAGER, networkFeeManager.address); }); - it('should revert when a non-network fee manager is attempting to withdraw the fees', async () => { - await expect(network.connect(deployer).pause()).to.be.revertedWithError('AccessDenied'); + it('should revert when a non-admin is attempting to set the min burn amount', async () => { + await expect(network.connect(emergencyStopper).setMinNetworkFeeBurn(1)).to.be.revertedWithError( + 'AccessDenied' + ); + }); + + it('should revert when attempting to set min burn amount to an invalid amount', async () => { + await expect(network.setMinNetworkFeeBurn(0)).to.be.revertedWithError('ZeroValue'); + }); + + it('admin should be able to set min network fee burn', async () => { + const newMinNetworkFeeBurn = toWei(1000); + await network.connect(deployer).setMinNetworkFeeBurn(newMinNetworkFeeBurn); + const minNetworkFeeBurn = await network.minNetworkFeeBurn(); + expect(minNetworkFeeBurn).to.be.eq(newMinNetworkFeeBurn); + }); + + it('setting min network fee burn should emit an event', async () => { + const newMinNetworkFeeBurn = toWei(1000); + const oldNetworkFeeBurn = await network.minNetworkFeeBurn(); + await expect(network.connect(deployer).setMinNetworkFeeBurn(newMinNetworkFeeBurn)) + .to.emit(network, 'MinNetworkFeeBurnUpdated') + .withArgs(oldNetworkFeeBurn, newMinNetworkFeeBurn); + }); + + it('should ignore setting the min network fee burn to the same value', async () => { + const oldMinNetworkFeeBurn = await network.minNetworkFeeBurn(); + await expect(network.connect(deployer).setMinNetworkFeeBurn(oldMinNetworkFeeBurn)).not.to.emit( + network, + 'MinNetworkFeeBurnUpdated' + ); }); context('without any pending network fees', () => { - it('should not withdraw any pending network fees', async () => { - const prevBNTBalance = await bnt.balanceOf(networkFeeManager.address); + it('should not burn any pending network fees', async () => { + const burnedNetworkFees = await network.callStatic.burnNetworkFees(); + expect(burnedNetworkFees).to.equal(0); - const withdrawNetworkFees = await network - .connect(networkFeeManager) - .callStatic.withdrawNetworkFees(networkFeeManager.address); - expect(withdrawNetworkFees).to.equal(0); + const totalSupplyBefore = await bnt.totalSupply(); - const res = await network.connect(networkFeeManager).withdrawNetworkFees(networkFeeManager.address); + const res = await network.burnNetworkFees(); - await expect(res).to.not.emit(network, 'NetworkFeesWithdrawn'); + await expect(res).to.not.emit(network, 'NetworkFeesBurned'); - expect(await bnt.balanceOf(networkFeeManager.address)).to.equal(prevBNTBalance); + expect(await bnt.totalSupply()).to.equal(totalSupplyBefore); }); }); @@ -3255,37 +3278,52 @@ describe('BancorNetwork', () => { await tradeBySourceAmount(deployer, bnt, token, toWei(1000), 1, MAX_UINT256, deployer.address, network); expect(await network.pendingNetworkFeeAmount()).to.be.gt(0); + expect(await network.minNetworkFeeBurn()).to.be.eq(toWei(1_000_000)); }); - it('should revert when the withdrawal caller is not a network-fee manager', async () => { - await expect( - network.connect(deployer).withdrawNetworkFees(networkFeeManager.address) - ).to.be.revertedWithError('AccessDenied'); - }); + it('should not burn any pending network fees if the pending amount is below the min network fee burn amount', async () => { + const minNetworkFeeBurnAmount = await network.minNetworkFeeBurn(); + expect(await network.pendingNetworkFeeAmount()).to.be.lt(minNetworkFeeBurnAmount); - it('should revert when the withdrawal recipient is invalid', async () => { - await expect( - network.connect(networkFeeManager).withdrawNetworkFees(ZERO_ADDRESS) - ).to.be.revertedWithError('InvalidAddress'); + const burnedNetworkFees = await network.callStatic.burnNetworkFees(); + expect(burnedNetworkFees).to.equal(0); + + const totalSupplyBefore = await bnt.totalSupply(); + + const res = await network.burnNetworkFees(); + + await expect(res).to.not.emit(network, 'NetworkFeesBurned'); + + expect(await bnt.totalSupply()).to.equal(totalSupplyBefore); }); - it('should withdraw all the pending network fees', async () => { - const recipient = nonOwner.address; - const prevBNTBalance = await bnt.balanceOf(networkFeeManager.address); + it('should burn all the pending network fees if the amount is above the min network fee burn amount', async () => { + await tradeBySourceAmount( + deployer, + bnt, + token, + toWei(10000000), + 1, + MAX_UINT256, + deployer.address, + network + ); + const minNetworkFeeBurnAmount = await network.minNetworkFeeBurn(); const pendingNetworkFeeAmount = await network.pendingNetworkFeeAmount(); + expect(await network.pendingNetworkFeeAmount()).to.be.gt(minNetworkFeeBurnAmount); + + const burnedNetworkFees = await network.callStatic.burnNetworkFees(); + expect(burnedNetworkFees).to.equal(pendingNetworkFeeAmount); - const withdrawNetworkFees = await network - .connect(networkFeeManager) - .callStatic.withdrawNetworkFees(recipient); - expect(withdrawNetworkFees).to.equal(pendingNetworkFeeAmount); + const totalSupplyBefore = await bnt.totalSupply(); - const res = await network.connect(networkFeeManager).withdrawNetworkFees(recipient); + const res = await network.burnNetworkFees(); await expect(res) - .to.emit(network, 'NetworkFeesWithdrawn') - .withArgs(networkFeeManager.address, recipient, pendingNetworkFeeAmount); + .to.emit(network, 'NetworkFeesBurned') + .withArgs(deployer.address, pendingNetworkFeeAmount); - expect(await bnt.balanceOf(recipient)).to.equal(prevBNTBalance.add(pendingNetworkFeeAmount)); + expect(await bnt.totalSupply()).to.equal(totalSupplyBefore.sub(pendingNetworkFeeAmount)); expect(await network.pendingNetworkFeeAmount()).to.equal(0); }); @@ -3296,9 +3334,7 @@ describe('BancorNetwork', () => { }); it('should revert when attempting to withdraw the pending network fees', async () => { - await expect( - network.connect(networkFeeManager).withdrawNetworkFees(networkFeeManager.address) - ).to.be.revertedWithError('Pausable: paused'); + await expect(network.burnNetworkFees()).to.be.revertedWithError('Pausable: paused'); }); }); }); diff --git a/utils/Roles.ts b/utils/Roles.ts index 8371971db..696eb409d 100644 --- a/utils/Roles.ts +++ b/utils/Roles.ts @@ -9,8 +9,7 @@ export const Roles = { BancorNetwork: { ROLE_MIGRATION_MANAGER: id('ROLE_MIGRATION_MANAGER'), - ROLE_EMERGENCY_STOPPER: id('ROLE_EMERGENCY_STOPPER'), - ROLE_NETWORK_FEE_MANAGER: id('ROLE_NETWORK_FEE_MANAGER') + ROLE_EMERGENCY_STOPPER: id('ROLE_EMERGENCY_STOPPER') }, MasterVault: { From 094af713b5f4c9b438d0c2904649954e056d41b7 Mon Sep 17 00:00:00 2001 From: Ivan Zhelyazkov Date: Fri, 20 Oct 2023 17:09:06 +0300 Subject: [PATCH 2/2] add upgrade scripts --- contracts/network/BancorNetwork.sol | 3 +- deploy/scripts/000064-upgrade-network-v10.ts | 44 ++++++++++++++++++++ deploy/tests/000064-upgrade-network-v10.ts | 18 ++++++++ test/network/BancorNetwork.ts | 2 +- 4 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 deploy/scripts/000064-upgrade-network-v10.ts create mode 100644 deploy/tests/000064-upgrade-network-v10.ts diff --git a/contracts/network/BancorNetwork.sol b/contracts/network/BancorNetwork.sol index ba3413d91..86d97d2fe 100644 --- a/contracts/network/BancorNetwork.sol +++ b/contracts/network/BancorNetwork.sol @@ -347,7 +347,7 @@ contract BancorNetwork is IBancorNetwork, Upgradeable, ReentrancyGuardUpgradeabl * @inheritdoc Upgradeable */ function version() public pure override(IVersioned, Upgradeable) returns (uint16) { - return 9; + return 10; } /** @@ -826,6 +826,7 @@ contract BancorNetwork is IBancorNetwork, Upgradeable, ReentrancyGuardUpgradeabl _pendingNetworkFeeAmount = 0; + // transferring bnt to the token's address burns the tokens _masterVault.withdrawFunds(Token(address(_bnt)), payable(address(_bnt)), currentPendingNetworkFeeAmount); emit NetworkFeesBurned(msg.sender, currentPendingNetworkFeeAmount); diff --git a/deploy/scripts/000064-upgrade-network-v10.ts b/deploy/scripts/000064-upgrade-network-v10.ts new file mode 100644 index 000000000..5d8306e5b --- /dev/null +++ b/deploy/scripts/000064-upgrade-network-v10.ts @@ -0,0 +1,44 @@ +import { DeployedContracts, execute, InstanceName, setDeploymentMetadata, upgradeProxy } from '../../utils/Deploy'; +import { toWei } from '../../utils/Types'; +import { DeployFunction } from 'hardhat-deploy/types'; +import { HardhatRuntimeEnvironment } from 'hardhat/types'; + +const func: DeployFunction = async ({ getNamedAccounts }: HardhatRuntimeEnvironment) => { + const { deployer, bancorArbitrageAddress, carbonPOLAddress } = await getNamedAccounts(); + + // get the deployed contracts + const externalProtectionVault = await DeployedContracts.ExternalProtectionVault.deployed(); + const masterVault = await DeployedContracts.MasterVault.deployed(); + const networkSettings = await DeployedContracts.NetworkSettings.deployed(); + const bntGovernance = await DeployedContracts.BNTGovernance.deployed(); + const vbntGovernance = await DeployedContracts.VBNTGovernance.deployed(); + const bnBNT = await DeployedContracts.bnBNT.deployed(); + + // upgrade the BancorNetwork contract + await upgradeProxy({ + name: InstanceName.BancorNetwork, + args: [ + bntGovernance.address, + vbntGovernance.address, + networkSettings.address, + masterVault.address, + externalProtectionVault.address, + bnBNT.address, + bancorArbitrageAddress, + carbonPOLAddress + ], + from: deployer + }); + + // set the min network fee burn to 1M + await execute({ + name: InstanceName.BancorNetwork, + methodName: 'setMinNetworkFeeBurn', + args: [toWei(1_000_000)], + from: deployer + }); + + return true; +}; + +export default setDeploymentMetadata(__filename, func); diff --git a/deploy/tests/000064-upgrade-network-v10.ts b/deploy/tests/000064-upgrade-network-v10.ts new file mode 100644 index 000000000..77e1046d5 --- /dev/null +++ b/deploy/tests/000064-upgrade-network-v10.ts @@ -0,0 +1,18 @@ +import { BancorNetwork } from '../../components/Contracts'; +import { describeDeployment } from '../../test/helpers/Deploy'; +import { DeployedContracts } from '../../utils/Deploy'; +import { toWei } from '../../utils/Types'; +import { expect } from 'chai'; + +describeDeployment(__filename, () => { + let network: BancorNetwork; + + beforeEach(async () => { + network = await DeployedContracts.BancorNetwork.deployed(); + }); + + it('should upgrade the network contract properly', async () => { + expect(await network.version()).to.equal(10); + expect(await network.minNetworkFeeBurn()).to.equal(toWei(1_000_000)); + }); +}); diff --git a/test/network/BancorNetwork.ts b/test/network/BancorNetwork.ts index 6514d15b4..a58b13d37 100644 --- a/test/network/BancorNetwork.ts +++ b/test/network/BancorNetwork.ts @@ -326,7 +326,7 @@ describe('BancorNetwork', () => { }); it('should be properly initialized', async () => { - expect(await network.version()).to.equal(9); + expect(await network.version()).to.equal(10); await expectRoles(network, Roles.BancorNetwork);