From 50b338c270bd87a3a1e59f94a0bc8cb539a7cb9f Mon Sep 17 00:00:00 2001 From: Zero Ekkusu <94782988+ZeroEkkusu@users.noreply.github.com> Date: Fri, 14 Jul 2023 11:53:49 +0200 Subject: [PATCH] fix: gov, add initializers --- contracts/child/ForkParams.sol | 7 +++--- contracts/child/NetworkParams.sol | 22 +++++++++++++++++-- contracts/child/validator/RewardPool.sol | 2 +- contracts/child/validator/ValidatorSet.sol | 21 +++++++++--------- .../child/validator/IValidatorSet.sol | 5 ----- test/forge/child/validator/RewardPool.t.sol | 10 +++------ test/forge/child/validator/ValidatorSet.t.sol | 14 +++++++++--- 7 files changed, 49 insertions(+), 32 deletions(-) diff --git a/contracts/child/ForkParams.sol b/contracts/child/ForkParams.sol index 8768e9bd..b1c297dd 100644 --- a/contracts/child/ForkParams.sol +++ b/contracts/child/ForkParams.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.19; import "@openzeppelin/contracts/access/Ownable.sol"; +import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; /** @title ForkParams @@ -9,17 +10,17 @@ import "@openzeppelin/contracts/access/Ownable.sol"; @notice Configurable softfork features that are read by the client on each epoch @dev The contract allows for configurable softfork parameters without genesis updation */ -contract ForkParams is Ownable { +contract ForkParams is Ownable, Initializable { mapping(bytes32 => uint256) public featureToBlockNumber; // keccak256("FEATURE_NAME") -> blockNumber event NewFeature(bytes32 indexed feature, uint256 indexed block); event UpdatedFeature(bytes32 indexed feature, uint256 indexed block); /** - * @notice constructor function to set the owner + * @notice initialize function to set the owner * @param newOwner address to transfer the ownership to */ - constructor(address newOwner) { + function initialize(address newOwner) public initializer { _transferOwnership(newOwner); } diff --git a/contracts/child/NetworkParams.sol b/contracts/child/NetworkParams.sol index cd0907cc..2e45e75a 100644 --- a/contracts/child/NetworkParams.sol +++ b/contracts/child/NetworkParams.sol @@ -2,6 +2,7 @@ pragma solidity 0.8.19; import "@openzeppelin/contracts/access/Ownable2Step.sol"; +import "@openzeppelin/contracts-upgradeable/proxy/utils/Initializable.sol"; /** @title NetworkParams @@ -9,11 +10,12 @@ import "@openzeppelin/contracts/access/Ownable2Step.sol"; @notice Configurable network parameters that are read by the client on each epoch @dev The contract allows for configurable network parameters without the need for a hardfork */ -contract NetworkParams is Ownable2Step { +contract NetworkParams is Ownable2Step, Initializable { struct InitParams { address newOwner; uint256 newBlockGasLimit; uint256 newCheckpointBlockInterval; // in blocks + uint256 newEpochSize; // in blocks uint256 newEpochReward; // in wei uint256 newMinValidatorSetSize; uint256 newMaxValidatorSetSize; @@ -28,6 +30,7 @@ contract NetworkParams is Ownable2Step { uint256 public blockGasLimit; uint256 public checkpointBlockInterval; // in blocks + uint256 public epochSize; // in blocks uint256 public epochReward; // in wei uint256 public minValidatorSetSize; uint256 public maxValidatorSetSize; @@ -41,6 +44,7 @@ contract NetworkParams is Ownable2Step { event NewBlockGasLimit(uint256 indexed gasLimit); event NewCheckpointBlockInterval(uint256 indexed checkpointInterval); + event NewEpochSize(uint256 indexed size); event NewEpochReward(uint256 indexed reward); event NewMinValidatorSetSize(uint256 indexed minValidatorSet); event NewMaxValdidatorSetSize(uint256 indexed maxValidatorSet); @@ -57,11 +61,12 @@ contract NetworkParams is Ownable2Step { * @dev disallows setting of zero values for sanity check purposes * @param initParams initial set of values for the network */ - constructor(InitParams memory initParams) { + function initialize(InitParams memory initParams) public initializer { require( initParams.newOwner != address(0) && initParams.newBlockGasLimit != 0 && initParams.newCheckpointBlockInterval != 0 && + initParams.newEpochSize != 0 && initParams.newEpochReward != 0 && initParams.newMinValidatorSetSize != 0 && initParams.newMaxValidatorSetSize != 0 && @@ -76,6 +81,7 @@ contract NetworkParams is Ownable2Step { ); blockGasLimit = initParams.newBlockGasLimit; checkpointBlockInterval = initParams.newCheckpointBlockInterval; + epochSize = initParams.newEpochSize; epochReward = initParams.newEpochReward; minValidatorSetSize = initParams.newMinValidatorSetSize; maxValidatorSetSize = initParams.newMaxValidatorSetSize; @@ -113,6 +119,18 @@ contract NetworkParams is Ownable2Step { emit NewCheckpointBlockInterval(newCheckpointBlockInterval); } + /** + * @notice function to set new epoch size + * @dev disallows setting of a zero value for sanity check purposes + * @param newEpochSize new epoch reward + */ + function setNewEpochSize(uint256 newEpochSize) external onlyOwner { + require(newEpochSize != 0, "NetworkParams: INVALID_EPOCH_SIZE"); + epochSize = newEpochSize; + + emit NewEpochSize(newEpochSize); + } + /** * @notice function to set new epoch reward * @dev disallows setting of a zero value for sanity check purposes diff --git a/contracts/child/validator/RewardPool.sol b/contracts/child/validator/RewardPool.sol index cc210b6f..3b0be195 100644 --- a/contracts/child/validator/RewardPool.sol +++ b/contracts/child/validator/RewardPool.sol @@ -46,7 +46,7 @@ contract RewardPool is IRewardPool, System, Initializable { require(paidRewardPerEpoch[epochId] == 0, "REWARD_ALREADY_DISTRIBUTED"); uint256 totalBlocks = validatorSet.totalBlocks(epochId); require(totalBlocks != 0, "EPOCH_NOT_COMMITTED"); - uint256 epochSize = validatorSet.EPOCH_SIZE(); + uint256 epochSize = networkParams.epochSize(); // slither-disable-next-line divide-before-multiply uint256 reward = (networkParams.epochReward() * totalBlocks) / epochSize; // TODO disincentivize long epoch times diff --git a/contracts/child/validator/ValidatorSet.sol b/contracts/child/validator/ValidatorSet.sol index 6d08d0b3..3803475a 100644 --- a/contracts/child/validator/ValidatorSet.sol +++ b/contracts/child/validator/ValidatorSet.sol @@ -5,6 +5,7 @@ import "@openzeppelin/contracts-upgradeable/token/ERC20/extensions/ERC20VotesUpg import "../../lib/WithdrawalQueue.sol"; import "../../interfaces/child/validator/IValidatorSet.sol"; import "../../interfaces/IStateSender.sol"; +import "../../child/NetworkParams.sol"; import "../System.sol"; contract ValidatorSet is IValidatorSet, ERC20VotesUpgradeable, System { @@ -13,13 +14,11 @@ contract ValidatorSet is IValidatorSet, ERC20VotesUpgradeable, System { bytes32 private constant STAKE_SIG = keccak256("STAKE"); bytes32 private constant UNSTAKE_SIG = keccak256("UNSTAKE"); bytes32 private constant SLASH_SIG = keccak256("SLASH"); - uint256 public constant WITHDRAWAL_WAIT_PERIOD = 1; IStateSender private stateSender; address private stateReceiver; address private rootChainManager; - // slither-disable-next-line naming-convention - uint256 public EPOCH_SIZE; + NetworkParams private networkParams; uint256 public currentEpochId; @@ -32,21 +31,18 @@ contract ValidatorSet is IValidatorSet, ERC20VotesUpgradeable, System { address newStateSender, address newStateReceiver, address newRootChainManager, - uint256 newEpochSize, + address newNetworkParams, ValidatorInit[] memory initialValidators ) public initializer { require( - newStateSender != address(0) && - newStateReceiver != address(0) && - newRootChainManager != address(0) && - newEpochSize != 0, + newStateSender != address(0) && newStateReceiver != address(0) && newRootChainManager != address(0), "INVALID_INPUT" ); __ERC20_init("ValidatorSet", "VSET"); stateSender = IStateSender(newStateSender); stateReceiver = newStateReceiver; rootChainManager = newRootChainManager; - EPOCH_SIZE = newEpochSize; + networkParams = NetworkParams(newNetworkParams); for (uint256 i = 0; i < initialValidators.length; ) { _stake(initialValidators[i].addr, initialValidators[i].stake); unchecked { @@ -64,7 +60,10 @@ contract ValidatorSet is IValidatorSet, ERC20VotesUpgradeable, System { uint256 newEpochId = currentEpochId++; require(id == newEpochId, "UNEXPECTED_EPOCH_ID"); require(epoch.endBlock > epoch.startBlock, "NO_BLOCKS_COMMITTED"); - require((epoch.endBlock - epoch.startBlock + 1) % EPOCH_SIZE == 0, "EPOCH_MUST_BE_DIVISIBLE_BY_EPOCH_SIZE"); + require( + (epoch.endBlock - epoch.startBlock + 1) % networkParams.epochSize() == 0, + "EPOCH_MUST_BE_DIVISIBLE_BY_EPOCH_SIZE" + ); require(epochs[newEpochId - 1].endBlock + 1 == epoch.startBlock, "INVALID_START_BLOCK"); epochs[newEpochId] = epoch; _commitBlockNumbers[newEpochId] = block.number; @@ -130,7 +129,7 @@ contract ValidatorSet is IValidatorSet, ERC20VotesUpgradeable, System { } function _registerWithdrawal(address account, uint256 amount) internal { - withdrawals[account].append(amount, currentEpochId + WITHDRAWAL_WAIT_PERIOD); + withdrawals[account].append(amount, currentEpochId + networkParams.withdrawalWaitPeriod()); emit WithdrawalRegistered(account, amount); } diff --git a/contracts/interfaces/child/validator/IValidatorSet.sol b/contracts/interfaces/child/validator/IValidatorSet.sol index 58619b46..ad30828b 100644 --- a/contracts/interfaces/child/validator/IValidatorSet.sol +++ b/contracts/interfaces/child/validator/IValidatorSet.sol @@ -38,11 +38,6 @@ interface IValidatorSet is IStateReceiver { /// @dev calls the bridge to release the funds on root function withdraw() external; - /// @notice amount of blocks in an epoch - /// @dev when an epoch is committed a multiple of this number of blocks must be committed - // slither-disable-next-line naming-convention - function EPOCH_SIZE() external view returns (uint256); - /// @notice total amount of blocks in a given epoch function totalBlocks(uint256 epochId) external view returns (uint256 length); diff --git a/test/forge/child/validator/RewardPool.t.sol b/test/forge/child/validator/RewardPool.t.sol index b1cbd3e2..f91e3773 100644 --- a/test/forge/child/validator/RewardPool.t.sol +++ b/test/forge/child/validator/RewardPool.t.sol @@ -21,19 +21,15 @@ abstract contract Uninitialized is Test { NetworkParams networkParams; function setUp() public virtual { - networkParams = NetworkParams(makeAddr("networkParams")); - vm.mockCall( - address(networkParams), - abi.encodeCall(networkParams.epochReward, ()), - abi.encode(uint256(1 ether)) - ); + networkParams = new NetworkParams(); + networkParams.initialize(NetworkParams.InitParams(address(1), 1, 1, 64, 1 ether, 1, 1, 1, 1, 1, 1, 1, 1, 1)); token = new MockERC20(); validatorSet = new ValidatorSet(); ValidatorInit[] memory init = new ValidatorInit[](2); init[0] = ValidatorInit({addr: address(this), stake: 300}); init[1] = ValidatorInit({addr: alice, stake: 100}); - validatorSet.initialize(address(1), address(1), address(1), 64, init); + validatorSet.initialize(address(1), address(1), address(1), address(networkParams), init); Epoch memory epoch = Epoch({startBlock: 1, endBlock: 64, epochRoot: bytes32(0)}); vm.prank(SYSTEM); validatorSet.commitEpoch(1, epoch); diff --git a/test/forge/child/validator/ValidatorSet.t.sol b/test/forge/child/validator/ValidatorSet.t.sol index c827de05..df0a8f30 100644 --- a/test/forge/child/validator/ValidatorSet.t.sol +++ b/test/forge/child/validator/ValidatorSet.t.sol @@ -6,6 +6,8 @@ import {ValidatorSet, ValidatorInit, Epoch} from "contracts/child/validator/Vali import {L2StateSender} from "contracts/child/L2StateSender.sol"; import "contracts/interfaces/Errors.sol"; +import {NetworkParams} from "contracts/child/NetworkParams.sol"; + abstract contract Uninitialized is Test { address internal constant SYSTEM = 0xffffFFFfFFffffffffffffffFfFFFfffFFFfFFfE; bytes32 internal constant STAKE_SIG = keccak256("STAKE"); @@ -18,7 +20,14 @@ abstract contract Uninitialized is Test { address bob = makeAddr("bob"); uint256 epochSize = 64; + NetworkParams networkParams; + function setUp() public virtual { + networkParams = new NetworkParams(); + networkParams.initialize( + NetworkParams.InitParams(address(1), 1, 1, epochSize, 1 ether, 1, 1, 1, 1, 1, 1, 1, 1, 1) + ); + stateSender = new L2StateSender(); validatorSet = new ValidatorSet(); } @@ -30,7 +39,7 @@ abstract contract Initialized is Uninitialized { ValidatorInit[] memory init = new ValidatorInit[](2); init[0] = ValidatorInit({addr: address(this), stake: 300}); init[1] = ValidatorInit({addr: alice, stake: 100}); - validatorSet.initialize(address(stateSender), stateReceiver, rootChainManager, epochSize, init); + validatorSet.initialize(address(stateSender), stateReceiver, rootChainManager, address(networkParams), init); } } @@ -55,8 +64,7 @@ contract ValidatorSet_Initialize is Uninitialized { ValidatorInit[] memory init = new ValidatorInit[](2); init[0] = ValidatorInit({addr: address(this), stake: 300}); init[1] = ValidatorInit({addr: alice, stake: 100}); - validatorSet.initialize(address(stateSender), stateReceiver, rootChainManager, epochSize, init); - assertEq(validatorSet.EPOCH_SIZE(), epochSize); + validatorSet.initialize(address(stateSender), stateReceiver, rootChainManager, address(networkParams), init); assertEq(validatorSet.balanceOf(address(this)), 300); assertEq(validatorSet.balanceOf(alice), 100); assertEq(validatorSet.totalSupply(), 400);