From adfcf6ce055096298eb392821af37db90323050e Mon Sep 17 00:00:00 2001 From: Mike-CZ Date: Mon, 16 Dec 2024 15:59:05 +0100 Subject: [PATCH 1/2] [OZ][N-09] Non-Upgradable ConstantsManager Inherits Upgradeable Contracts --- contracts/sfc/ConstantsManager.sol | 8 +++----- package-lock.json | 4 ++-- package.json | 1 + 3 files changed, 6 insertions(+), 7 deletions(-) diff --git a/contracts/sfc/ConstantsManager.sol b/contracts/sfc/ConstantsManager.sol index 1c41030..0e23abc 100644 --- a/contracts/sfc/ConstantsManager.sol +++ b/contracts/sfc/ConstantsManager.sol @@ -1,13 +1,13 @@ // SPDX-License-Identifier: UNLICENSED pragma solidity 0.8.27; -import {OwnableUpgradeable} from "@openzeppelin/contracts-upgradeable/access/OwnableUpgradeable.sol"; +import {Ownable} from "@openzeppelin/contracts/access/Ownable.sol"; import {Decimal} from "../common/Decimal.sol"; /** * @custom:security-contact security@fantom.foundation */ -contract ConstantsManager is OwnableUpgradeable { +contract ConstantsManager is Ownable { // Minimum amount of stake for a validator, i.e., 500000 FTM uint256 public minSelfStake; // Maximum ratio of delegations a validator can have, say, 15 times of self-stake @@ -47,9 +47,7 @@ contract ConstantsManager is OwnableUpgradeable { */ error ValueTooLarge(); - constructor(address owner) initializer { - __Ownable_init(owner); - } + constructor(address owner) Ownable(owner) {} function updateMinSelfStake(uint256 v) external virtual onlyOwner { if (v < 100000 * 1e18) { diff --git a/package-lock.json b/package-lock.json index 28bd9ae..3811c6a 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9,6 +9,7 @@ "version": "3.0.5-rc.1", "license": "MIT", "dependencies": { + "@openzeppelin/contracts": "^5.1.0", "@openzeppelin/contracts-upgradeable": "^5.1.0", "dotenv": "^16.0.3" }, @@ -1618,8 +1619,7 @@ "version": "5.1.0", "resolved": "https://registry.npmjs.org/@openzeppelin/contracts/-/contracts-5.1.0.tgz", "integrity": "sha512-p1ULhl7BXzjjbha5aqst+QMLY+4/LCWADXOCsmLHRM77AqiPjnd9vvUN9sosUfhL9JGKpZ0TjEGxgvnizmWGSA==", - "license": "MIT", - "peer": true + "license": "MIT" }, "node_modules/@openzeppelin/contracts-upgradeable": { "version": "5.1.0", diff --git a/package.json b/package.json index 975e686..16df1ff 100644 --- a/package.json +++ b/package.json @@ -45,6 +45,7 @@ "typescript-eslint": "^8.8.0" }, "dependencies": { + "@openzeppelin/contracts": "^5.1.0", "@openzeppelin/contracts-upgradeable": "^5.1.0", "dotenv": "^16.0.3" } From 70b03aa35d263d29e5d7ff94aa0db95da87461e1 Mon Sep 17 00:00:00 2001 From: Mike-CZ Date: Mon, 16 Dec 2024 16:34:18 +0100 Subject: [PATCH 2/2] [OZ][L-04] Treasury Fees Can Be Locked in the SFC (#102) --- contracts/sfc/SFC.sol | 32 +++++++++++ contracts/test/FailingReceiver.sol | 9 +++ test/SFC.ts | 92 ++++++++++++++++++++++++------ test/helpers/BlockchainNode.ts | 12 ++-- 4 files changed, 122 insertions(+), 23 deletions(-) create mode 100644 contracts/test/FailingReceiver.sol diff --git a/contracts/sfc/SFC.sol b/contracts/sfc/SFC.sol index 5b97ec3..b5d6a07 100644 --- a/contracts/sfc/SFC.sol +++ b/contracts/sfc/SFC.sol @@ -50,6 +50,9 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version { // total stake of active (OK_STATUS) validators (total weight) uint256 public totalActiveStake; + // unresolved fees that failed to be send to the treasury + uint256 public unresolvedTreasuryFees; + // delegator => validator ID => stashed rewards (to be claimed/restaked) mapping(address delegator => mapping(uint256 validatorID => uint256 stashedRewards)) internal _rewardsStash; @@ -190,6 +193,10 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version { error ValidatorNotSlashed(); error RefundRatioTooHigh(); + // treasury + error TreasuryNotSet(); + error NoUnresolvedTreasuryFees(); + event DeactivatedValidator(uint256 indexed validatorID, uint256 deactivatedEpoch, uint256 deactivatedTime); event ChangedValidatorStatus(uint256 indexed validatorID, uint256 status); event CreatedValidator( @@ -207,6 +214,7 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version { event UpdatedSlashingRefundRatio(uint256 indexed validatorID, uint256 refundRatio); event RefundedSlashedLegacyDelegation(address indexed delegator, uint256 indexed validatorID, uint256 amount); event AnnouncedRedirection(address indexed from, address indexed to); + event TreasuryFeesResolved(uint256 amount); modifier onlyDriver() { if (!isNode(msg.sender)) { @@ -419,6 +427,27 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version { } } + /// Resolve failed treasury transfers and send the unresolved fees to the treasury address. + function resolveTreasuryFees() external { + if (treasuryAddress == address(0)) { + revert TreasuryNotSet(); + } + if (unresolvedTreasuryFees == 0) { + revert NoUnresolvedTreasuryFees(); + } + + // zero the fees before sending to prevent re-entrancy + uint256 fees = unresolvedTreasuryFees; + unresolvedTreasuryFees = 0; + + (bool success, ) = treasuryAddress.call{value: fees, gas: 1000000}(""); + if (!success) { + revert TransferFailed(); + } + + emit TreasuryFeesResolved(fees); + } + /// burnFTM allows SFC to burn an arbitrary amount of FTM tokens. function burnFTM(uint256 amount) external onlyOwner { _burnFTM(amount); @@ -909,6 +938,9 @@ contract SFC is OwnableUpgradeable, UUPSUpgradeable, Version { if (!success) { // ignore treasury transfer failure // the treasury failure must not endanger the epoch sealing + + // store the unresolved treasury fees to be resolved later + unresolvedTreasuryFees += feeShare; } } } diff --git a/contracts/test/FailingReceiver.sol b/contracts/test/FailingReceiver.sol new file mode 100644 index 0000000..632069a --- /dev/null +++ b/contracts/test/FailingReceiver.sol @@ -0,0 +1,9 @@ +// SPDX-License-Identifier: UNLICENSED +pragma solidity 0.8.27; + +contract FailingReceiver { + // Fallback function to reject any received Ether + receive() external payable { + revert("Forced transfer failure"); + } +} diff --git a/test/SFC.ts b/test/SFC.ts index c3351c3..7c72c77 100644 --- a/test/SFC.ts +++ b/test/SFC.ts @@ -644,10 +644,10 @@ describe('SFC', () => { }); it('Should succeed and seal epochs', async function () { - const validatorsMetrics: Map = new Map(); + const validatorsMetrics: Map = new Map(); const validatorIDs = await this.sfc.lastValidatorID(); - for (let i = 0; i < validatorIDs; i++) { + for (let i = 1n; i <= validatorIDs; i++) { validatorsMetrics.set(i, { offlineTime: 0, offlineBlocks: 0, @@ -661,8 +661,8 @@ describe('SFC', () => { const offlineBlocks = []; const uptimes = []; const originatedTxsFees = []; - for (let i = 0; i < validatorIDs; i++) { - allValidators.push(i + 1); + for (let i = 1n; i <= validatorIDs; i++) { + allValidators.push(i); offlineTimes.push(validatorsMetrics.get(i)!.offlineTime); offlineBlocks.push(validatorsMetrics.get(i)!.offlineBlocks); uptimes.push(validatorsMetrics.get(i)!.uptime); @@ -674,11 +674,69 @@ describe('SFC', () => { await this.sfc.sealEpochValidators(allValidators); }); + describe('Treasury', () => { + it('Should revert when treasury is not set', async function () { + await expect(this.sfc.resolveTreasuryFees()).to.be.revertedWithCustomError(this.sfc, 'TreasuryNotSet'); + }); + + it('Should revert when no unresolved treasury fees are available', async function () { + const treasury = ethers.Wallet.createRandom(); + await this.sfc.connect(this.owner).updateTreasuryAddress(treasury); + await expect(this.sfc.resolveTreasuryFees()).to.be.revertedWithCustomError( + this.sfc, + 'NoUnresolvedTreasuryFees', + ); + }); + + it('Should succeed and resolve treasury fees', async function () { + // set treasury as failing receiver to trigger treasury fee accumulation + const failingReceiver = await ethers.deployContract('FailingReceiver'); + await this.sfc.connect(this.owner).updateTreasuryAddress(failingReceiver); + + // set validators metrics and their fees + const validatorsMetrics: Map = new Map(); + const validatorIDs = await this.sfc.lastValidatorID(); + for (let i = 1n; i <= validatorIDs; i++) { + validatorsMetrics.set(i, { + offlineTime: 0, + offlineBlocks: 0, + uptime: 24 * 60 * 60, + originatedTxsFee: ethers.parseEther('100'), + }); + } + + // seal epoch to trigger fees calculation and distribution + await this.blockchainNode.sealEpoch(24 * 60 * 60, validatorsMetrics); + + const fees = + (validatorIDs * ethers.parseEther('100') * (await this.constants.treasuryFeeShare())) / BigInt(1e18); + expect(await this.sfc.unresolvedTreasuryFees()).to.equal(fees); + + // update treasury to a valid receiver + const treasury = ethers.Wallet.createRandom(); + await this.sfc.connect(this.owner).updateTreasuryAddress(treasury); + + // set sfc some balance to cover treasury fees + // the funds cannot be sent directly as it rejects any incoming transfers + await ethers.provider.send('hardhat_setBalance', [ + await this.sfc.getAddress(), + ethers.toBeHex(ethers.parseEther('1000')), + ]); + + // resolve treasury fees + const tx = await this.sfc.resolveTreasuryFees(); + await expect(tx).to.emit(this.sfc, 'TreasuryFeesResolved').withArgs(fees); + await expect(tx).to.changeEtherBalance(treasury, fees); + await expect(tx).to.changeEtherBalance(this.sfc, -fees); + expect(await this.sfc.unresolvedTreasuryFees()).to.equal(0); + }); + }); + it('Should succeed and seal epoch on Validators', async function () { - const validatorsMetrics: Map = new Map(); + const validatorsMetrics: Map = new Map(); const validatorIDs = await this.sfc.lastValidatorID(); - for (let i = 0; i < validatorIDs; i++) { + for (let i = 1n; i <= validatorIDs; i++) { validatorsMetrics.set(i, { offlineTime: 0, offlineBlocks: 0, @@ -692,8 +750,8 @@ describe('SFC', () => { const offlineBlocks = []; const uptimes = []; const originatedTxsFees = []; - for (let i = 0; i < validatorIDs; i++) { - allValidators.push(i + 1); + for (let i = 1n; i <= validatorIDs; i++) { + allValidators.push(i); offlineTimes.push(validatorsMetrics.get(i)!.offlineTime); offlineBlocks.push(validatorsMetrics.get(i)!.offlineBlocks); uptimes.push(validatorsMetrics.get(i)!.uptime); @@ -746,10 +804,10 @@ describe('SFC', () => { }); it('Should revert when calling sealEpoch if not NodeDriver', async function () { - const validatorsMetrics: Map = new Map(); + const validatorsMetrics: Map = new Map(); const validatorIDs = await this.sfc.lastValidatorID(); - for (let i = 0; i < validatorIDs; i++) { + for (let i = 1n; i <= validatorIDs; i++) { validatorsMetrics.set(i, { offlineTime: 0, offlineBlocks: 0, @@ -763,8 +821,8 @@ describe('SFC', () => { const offlineBlocks = []; const uptimes = []; const originatedTxsFees = []; - for (let i = 0; i < validatorIDs; i++) { - allValidators.push(i + 1); + for (let i = 1n; i <= validatorIDs; i++) { + allValidators.push(i); offlineTimes.push(validatorsMetrics.get(i)!.offlineTime); offlineBlocks.push(validatorsMetrics.get(i)!.offlineBlocks); uptimes.push(validatorsMetrics.get(i)!.uptime); @@ -982,7 +1040,7 @@ describe('SFC', () => { // validator online 100% of time in the first epoch => average 100% await this.blockchainNode.sealEpoch( 100, - new Map([[this.validatorId as number, new ValidatorMetrics(0, 0, 100, 0n)]]), + new Map([[this.validatorId, new ValidatorMetrics(0, 0, 100, 0n)]]), ); expect(await this.sfc.getEpochAverageUptime(await this.sfc.currentSealedEpoch(), this.validatorId)).to.equal( 1000000000000000000n, @@ -991,7 +1049,7 @@ describe('SFC', () => { // validator online 20% of time in the second epoch => average 60% await this.blockchainNode.sealEpoch( 100, - new Map([[this.validatorId as number, new ValidatorMetrics(0, 0, 20, 0n)]]), + new Map([[this.validatorId, new ValidatorMetrics(0, 0, 20, 0n)]]), ); expect(await this.sfc.getEpochAverageUptime(await this.sfc.currentSealedEpoch(), this.validatorId)).to.equal( 600000000000000000n, @@ -1000,7 +1058,7 @@ describe('SFC', () => { // validator online 30% of time in the third epoch => average 50% await this.blockchainNode.sealEpoch( 100, - new Map([[this.validatorId as number, new ValidatorMetrics(0, 0, 30, 0n)]]), + new Map([[this.validatorId, new ValidatorMetrics(0, 0, 30, 0n)]]), ); expect(await this.sfc.getEpochAverageUptime(await this.sfc.currentSealedEpoch(), this.validatorId)).to.equal( 500000000000000000n, @@ -1010,7 +1068,7 @@ describe('SFC', () => { for (let i = 0; i < 10; i++) { await this.blockchainNode.sealEpoch( 100, - new Map([[this.validatorId as number, new ValidatorMetrics(0, 0, 50, 0n)]]), + new Map([[this.validatorId, new ValidatorMetrics(0, 0, 50, 0n)]]), ); expect(await this.sfc.getEpochAverageUptime(await this.sfc.currentSealedEpoch(), this.validatorId)).to.equal( 500000000000000000n, @@ -1020,7 +1078,7 @@ describe('SFC', () => { // (50 * 10 + 28) / 11 = 48 await this.blockchainNode.sealEpoch( 100, - new Map([[this.validatorId as number, new ValidatorMetrics(0, 0, 28, 0n)]]), + new Map([[this.validatorId, new ValidatorMetrics(0, 0, 28, 0n)]]), ); expect(await this.sfc.getEpochAverageUptime(await this.sfc.currentSealedEpoch(), this.validatorId)).to.equal( 480000000000000000n, diff --git a/test/helpers/BlockchainNode.ts b/test/helpers/BlockchainNode.ts index 2e9e669..91682fd 100644 --- a/test/helpers/BlockchainNode.ts +++ b/test/helpers/BlockchainNode.ts @@ -1,4 +1,4 @@ -import { SFCUnitTestI } from '../../typechain-types'; +import { UnitTestSFC } from '../../typechain-types'; import { TransactionResponse } from 'ethers'; import { ethers } from 'hardhat'; @@ -17,11 +17,11 @@ class ValidatorMetrics { } class BlockchainNode { - public readonly sfc: SFCUnitTestI; - public validatorWeights: Map; - public nextValidatorWeights: Map; + public readonly sfc: UnitTestSFC; + public validatorWeights: Map; + public nextValidatorWeights: Map; - constructor(sfc: SFCUnitTestI) { + constructor(sfc: UnitTestSFC) { this.sfc = sfc; this.validatorWeights = new Map(); this.nextValidatorWeights = new Map(); @@ -44,7 +44,7 @@ class BlockchainNode { } } - async sealEpoch(duration: number, validatorMetrics?: Map) { + async sealEpoch(duration: number, validatorMetrics?: Map) { const validatorIds = Array.from(this.validatorWeights.keys()); const nextValidatorIds = Array.from(this.nextValidatorWeights.keys());