Merge pull request #606 from ava-labs/verify-weights-removable

Check weights
ava-labs · Oct 18, 2024 · c1cf054 · c1cf054
2 parents 8162aea + fed48f4
commit c1cf054
Show file tree

Hide file tree

Showing 8 changed files with 178 additions and 62 deletions.
diff --git a/abi-bindings/go/validator-manager/ERC20TokenStakingManager/ERC20TokenStakingManager.go b/abi-bindings/go/validator-manager/ERC20TokenStakingManager/ERC20TokenStakingManager.go
diff --git a/abi-bindings/go/validator-manager/NativeTokenStakingManager/NativeTokenStakingManager.go b/abi-bindings/go/validator-manager/NativeTokenStakingManager/NativeTokenStakingManager.go
diff --git a/abi-bindings/go/validator-manager/PoAValidatorManager/PoAValidatorManager.go b/abi-bindings/go/validator-manager/PoAValidatorManager/PoAValidatorManager.go
diff --git a/contracts/validator-manager/ValidatorManager.sol b/contracts/validator-manager/ValidatorManager.sol
@@ -77,6 +77,7 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida
     error InvalidSubnetConversionID(
         bytes32 encodedSubnetConversionID, bytes32 expectedSubnetConversionID
     );
+    error InvalidTotalWeight(uint256 weight);
     error InvalidValidationID(bytes32 validationID);
     error InvalidValidatorStatus(ValidatorStatus status);
     error InvalidWarpMessage();
@@ -196,6 +197,12 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida
         }
         $._churnTracker.totalWeight = totalWeight;
 
+        // Rearranged equation for totalWeight < (100 / $._maximumChurnPercentage)
+        // Total weight must be above this value in order to not trigger churn limits with an added/removed weight of 1.
+        if (totalWeight * $._maximumChurnPercentage < 100) {
+            revert InvalidTotalWeight(totalWeight);
+        }
+
         // Verify that the sha256 hash of the Subnet conversion data matches with the Warp message's subnetConversionID.
         bytes32 subnetConversionID = ValidatorMessages.unpackSubnetConversionMessage(
             _getPChainWarpMessage(messageIndex).payload
@@ -573,6 +580,12 @@ abstract contract ValidatorManager is Initializable, ContextUpgradeable, IValida
         churnTracker.totalWeight += newValidatorWeight;
         churnTracker.totalWeight -= oldValidatorWeight;
 
+        // Rearranged equation for totalWeight < (100 / $._maximumChurnPercentage)
+        // Total weight must be above this value in order to not trigger churn limits with an added/removed weight of 1.
+        if (churnTracker.totalWeight * $._maximumChurnPercentage < 100) {
+            revert InvalidTotalWeight(churnTracker.totalWeight);
+        }
+
         $._churnTracker = churnTracker;
     }
 }
diff --git a/contracts/validator-manager/tests/ERC20TokenStakingManagerTests.t.sol b/contracts/validator-manager/tests/ERC20TokenStakingManagerTests.t.sol
@@ -11,7 +11,8 @@ import {PoSValidatorManager} from "../PoSValidatorManager.sol";
 import {ExampleRewardCalculator} from "../ExampleRewardCalculator.sol";
 import {
     ValidatorManagerSettings,
-    ValidatorRegistrationInput
+    ValidatorRegistrationInput,
+    IValidatorManager
 } from "../interfaces/IValidatorManager.sol";
 import {PoSValidatorManagerSettings} from "../interfaces/IPoSValidatorManager.sol";
 import {IRewardCalculator} from "../interfaces/IRewardCalculator.sol";
@@ -31,28 +32,7 @@ contract ERC20TokenStakingManagerTest is PoSValidatorManagerTest {
     function setUp() public override {
         ValidatorManagerTest.setUp();
 
-        // Construct the object under test
-        app = new ERC20TokenStakingManager(ICMInitializable.Allowed);
-        token = new ExampleERC20();
-        rewardCalculator = new ExampleRewardCalculator(DEFAULT_REWARD_RATE);
-        app.initialize(
-            PoSValidatorManagerSettings({
-                baseSettings: ValidatorManagerSettings({
-                    subnetID: DEFAULT_SUBNET_ID,
-                    churnPeriodSeconds: DEFAULT_CHURN_PERIOD,
-                    maximumChurnPercentage: DEFAULT_MAXIMUM_CHURN_PERCENTAGE
-                }),
-                minimumStakeAmount: DEFAULT_MINIMUM_STAKE_AMOUNT,
-                maximumStakeAmount: DEFAULT_MAXIMUM_STAKE_AMOUNT,
-                minimumStakeDuration: DEFAULT_MINIMUM_STAKE_DURATION,
-                minimumDelegationFeeBips: DEFAULT_MINIMUM_DELEGATION_FEE_BIPS,
-                maximumStakeMultiplier: DEFAULT_MAXIMUM_STAKE_MULTIPLIER,
-                rewardCalculator: rewardCalculator
-            }),
-            token
-        );
-        validatorManager = app;
-        posValidatorManager = app;
+        _setUp();
         _mockGetBlockchainID();
         _mockInitializeValidatorSet();
         app.initializeValidatorSet(_defaultSubnetConversionData(), 0);
@@ -261,6 +241,33 @@ contract ERC20TokenStakingManagerTest is PoSValidatorManagerTest {
         vm.expectCall(address(token), abi.encodeCall(IERC20Mintable.mint, (account, amount)));
     }
 
+    function _setUp() internal override returns (IValidatorManager) {
+        // Construct the object under test
+        app = new ERC20TokenStakingManager(ICMInitializable.Allowed);
+        token = new ExampleERC20();
+        rewardCalculator = new ExampleRewardCalculator(DEFAULT_REWARD_RATE);
+        app.initialize(
+            PoSValidatorManagerSettings({
+                baseSettings: ValidatorManagerSettings({
+                    subnetID: DEFAULT_SUBNET_ID,
+                    churnPeriodSeconds: DEFAULT_CHURN_PERIOD,
+                    maximumChurnPercentage: DEFAULT_MAXIMUM_CHURN_PERCENTAGE
+                }),
+                minimumStakeAmount: DEFAULT_MINIMUM_STAKE_AMOUNT,
+                maximumStakeAmount: DEFAULT_MAXIMUM_STAKE_AMOUNT,
+                minimumStakeDuration: DEFAULT_MINIMUM_STAKE_DURATION,
+                minimumDelegationFeeBips: DEFAULT_MINIMUM_DELEGATION_FEE_BIPS,
+                maximumStakeMultiplier: DEFAULT_MAXIMUM_STAKE_MULTIPLIER,
+                rewardCalculator: rewardCalculator
+            }),
+            token
+        );
+        validatorManager = app;
+        posValidatorManager = app;
+
+        return app;
+    }
+
     function _getStakeAssetBalance(address account) internal view override returns (uint256) {
         return token.balanceOf(account);
     }

diff --git a/contracts/validator-manager/tests/NativeTokenStakingManagerTests.t.sol b/contracts/validator-manager/tests/NativeTokenStakingManagerTests.t.sol
@@ -10,7 +10,8 @@ import {NativeTokenStakingManager} from "../NativeTokenStakingManager.sol";
 import {PoSValidatorManager} from "../PoSValidatorManager.sol";
 import {
     ValidatorManagerSettings,
-    ValidatorRegistrationInput
+    ValidatorRegistrationInput,
+    IValidatorManager
 } from "../interfaces/IValidatorManager.sol";
 import {PoSValidatorManagerSettings} from "../interfaces/IPoSValidatorManager.sol";
 import {IRewardCalculator} from "../interfaces/IRewardCalculator.sol";
@@ -26,26 +27,7 @@ contract NativeTokenStakingManagerTest is PoSValidatorManagerTest {
     function setUp() public override {
         ValidatorManagerTest.setUp();
 
-        // Construct the object under test
-        app = new NativeTokenStakingManager(ICMInitializable.Allowed);
-        rewardCalculator = new ExampleRewardCalculator(DEFAULT_REWARD_RATE);
-        app.initialize(
-            PoSValidatorManagerSettings({
-                baseSettings: ValidatorManagerSettings({
-                    subnetID: DEFAULT_SUBNET_ID,
-                    churnPeriodSeconds: DEFAULT_CHURN_PERIOD,
-                    maximumChurnPercentage: DEFAULT_MAXIMUM_CHURN_PERCENTAGE
-                }),
-                minimumStakeAmount: DEFAULT_MINIMUM_STAKE_AMOUNT,
-                maximumStakeAmount: DEFAULT_MAXIMUM_STAKE_AMOUNT,
-                minimumStakeDuration: DEFAULT_MINIMUM_STAKE_DURATION,
-                minimumDelegationFeeBips: DEFAULT_MINIMUM_DELEGATION_FEE_BIPS,
-                maximumStakeMultiplier: DEFAULT_MAXIMUM_STAKE_MULTIPLIER,
-                rewardCalculator: rewardCalculator
-            })
-        );
-        validatorManager = app;
-        posValidatorManager = app;
+        _setUp();
         _mockGetBlockchainID();
         _mockInitializeValidatorSet();
         app.initializeValidatorSet(_defaultSubnetConversionData(), 0);
@@ -229,6 +211,30 @@ contract NativeTokenStakingManagerTest is PoSValidatorManagerTest {
         vm.deal(account, account.balance + amount);
     }
 
+    function _setUp() internal override returns (IValidatorManager) {
+        // Construct the object under test
+        app = new NativeTokenStakingManager(ICMInitializable.Allowed);
+        rewardCalculator = new ExampleRewardCalculator(DEFAULT_REWARD_RATE);
+        app.initialize(
+            PoSValidatorManagerSettings({
+                baseSettings: ValidatorManagerSettings({
+                    subnetID: DEFAULT_SUBNET_ID,
+                    churnPeriodSeconds: DEFAULT_CHURN_PERIOD,
+                    maximumChurnPercentage: DEFAULT_MAXIMUM_CHURN_PERCENTAGE
+                }),
+                minimumStakeAmount: DEFAULT_MINIMUM_STAKE_AMOUNT,
+                maximumStakeAmount: DEFAULT_MAXIMUM_STAKE_AMOUNT,
+                minimumStakeDuration: DEFAULT_MINIMUM_STAKE_DURATION,
+                minimumDelegationFeeBips: DEFAULT_MINIMUM_DELEGATION_FEE_BIPS,
+                maximumStakeMultiplier: DEFAULT_MAXIMUM_STAKE_MULTIPLIER,
+                rewardCalculator: rewardCalculator
+            })
+        );
+        validatorManager = app;
+        posValidatorManager = app;
+        return app;
+    }
+
     function _getStakeAssetBalance(address account) internal view override returns (uint256) {
         return account.balance;
     }

diff --git a/contracts/validator-manager/tests/PoAValidatorManagerTests.t.sol b/contracts/validator-manager/tests/PoAValidatorManagerTests.t.sol
@@ -10,7 +10,8 @@ import {PoAValidatorManager} from "../PoAValidatorManager.sol";
 import {ICMInitializable} from "@utilities/ICMInitializable.sol";
 import {
     ValidatorManagerSettings,
-    ValidatorRegistrationInput
+    ValidatorRegistrationInput,
+    IValidatorManager
 } from "../interfaces/IValidatorManager.sol";
 import {OwnableUpgradeable} from
     "@openzeppelin/[email protected]/access/OwnableUpgradeable.sol";
@@ -24,16 +25,7 @@ contract PoAValidatorManagerTest is ValidatorManagerTest {
     function setUp() public override {
         ValidatorManagerTest.setUp();
 
-        app = new PoAValidatorManager(ICMInitializable.Allowed);
-        app.initialize(
-            ValidatorManagerSettings({
-                subnetID: DEFAULT_SUBNET_ID,
-                churnPeriodSeconds: DEFAULT_CHURN_PERIOD,
-                maximumChurnPercentage: DEFAULT_MAXIMUM_CHURN_PERCENTAGE
-            }),
-            address(this)
-        );
-        validatorManager = app;
+        _setUp();
         _mockGetBlockchainID();
         _mockInitializeValidatorSet();
         app.initializeValidatorSet(_defaultSubnetConversionData(), 0);
@@ -73,6 +65,21 @@ contract PoAValidatorManagerTest is ValidatorManagerTest {
         return app.initializeEndValidation(validationID);
     }
 
+    function _setUp() internal override returns (IValidatorManager) {
+        app = new PoAValidatorManager(ICMInitializable.Allowed);
+        app.initialize(
+            ValidatorManagerSettings({
+                subnetID: DEFAULT_SUBNET_ID,
+                churnPeriodSeconds: DEFAULT_CHURN_PERIOD,
+                maximumChurnPercentage: DEFAULT_MAXIMUM_CHURN_PERCENTAGE
+            }),
+            address(this)
+        );
+        validatorManager = app;
+
+        return app;
+    }
+
     // solhint-disable-next-line no-empty-blocks
     function _beforeSend(uint256 amount, address spender) internal virtual override {}
 }
diff --git a/contracts/validator-manager/tests/ValidatorManagerTests.t.sol b/contracts/validator-manager/tests/ValidatorManagerTests.t.sol
@@ -11,7 +11,8 @@ import {ValidatorMessages} from "../ValidatorMessages.sol";
 import {
     ValidatorStatus,
     ValidatorRegistrationInput,
-    PChainOwner
+    PChainOwner,
+    IValidatorManager
 } from "../interfaces/IValidatorManager.sol";
 import {
     WarpMessage,
@@ -280,6 +281,34 @@ abstract contract ValidatorManagerTest is Test {
         validatorManager.completeEndValidation(0);
     }
 
+    function testInitialWeightsTooLow() public {
+        vm.prank(address(123));
+        IValidatorManager manager = _setUp();
+
+        _mockGetBlockchainID();
+        vm.expectRevert(abi.encodeWithSelector(ValidatorManager.InvalidTotalWeight.selector, 4));
+        manager.initializeValidatorSet(_defaultSubnetConversionDataWeightsTooLow(), 0);
+    }
+
+    function testRemoveValidatorTotalWeight5() public {
+        // Use prank here, because otherwise each test will end up with a different contract address, leading to a different subnet conversion hash.
+        vm.prank(address(123));
+        IValidatorManager manager = _setUp();
+
+        _mockGetBlockchainID();
+        _mockGetPChainWarpMessage(
+            ValidatorMessages.packSubnetConversionMessage(
+                bytes32(hex"1d72565851401e05d6351ebf5443d9bdc04953f3233da1345af126e7e4be7464")
+            ),
+            true
+        );
+        manager.initializeValidatorSet(_defaultSubnetConversionDataTotalWeight5(), 0);
+
+        bytes32 validationID = sha256(abi.encodePacked(DEFAULT_SUBNET_ID, uint32(0)));
+        vm.expectRevert(abi.encodeWithSelector(ValidatorManager.InvalidTotalWeight.selector, 4));
+        _forceInitializeEndValidation(validationID, false);
+    }
+
     function testCumulativeChurnRegistration() public {
         uint64 churnThreshold =
             uint64(DEFAULT_STARTING_TOTAL_WEIGHT) * DEFAULT_MAXIMUM_CHURN_PERCENTAGE / 100;
@@ -556,6 +585,8 @@ abstract contract ValidatorManagerTest is Test {
         bool includeUptime
     ) internal virtual;
 
+    function _setUp() internal virtual returns (IValidatorManager);
+
     function _beforeSend(uint256 amount, address spender) internal virtual;
 
     function _defaultSubnetConversionData() internal view returns (SubnetConversionData memory) {
@@ -581,6 +612,58 @@ abstract contract ValidatorManagerTest is Test {
         });
     }
 
+    function _defaultSubnetConversionDataWeightsTooLow()
+        internal
+        view
+        returns (SubnetConversionData memory)
+    {
+        InitialValidator[] memory initialValidators = new InitialValidator[](2);
+
+        initialValidators[0] = InitialValidator({
+            nodeID: DEFAULT_INITIAL_VALIDATOR_NODE_ID_1,
+            weight: 1,
+            blsPublicKey: DEFAULT_BLS_PUBLIC_KEY
+        });
+        initialValidators[1] = InitialValidator({
+            nodeID: DEFAULT_INITIAL_VALIDATOR_NODE_ID_2,
+            weight: 3,
+            blsPublicKey: DEFAULT_BLS_PUBLIC_KEY
+        });
+
+        return SubnetConversionData({
+            subnetID: DEFAULT_SUBNET_ID,
+            validatorManagerBlockchainID: DEFAULT_SOURCE_BLOCKCHAIN_ID,
+            validatorManagerAddress: address(validatorManager),
+            initialValidators: initialValidators
+        });
+    }
+
+    function _defaultSubnetConversionDataTotalWeight5()
+        internal
+        view
+        returns (SubnetConversionData memory)
+    {
+        InitialValidator[] memory initialValidators = new InitialValidator[](2);
+
+        initialValidators[0] = InitialValidator({
+            nodeID: DEFAULT_INITIAL_VALIDATOR_NODE_ID_1,
+            weight: 1,
+            blsPublicKey: DEFAULT_BLS_PUBLIC_KEY
+        });
+        initialValidators[1] = InitialValidator({
+            nodeID: DEFAULT_INITIAL_VALIDATOR_NODE_ID_2,
+            weight: 4,
+            blsPublicKey: DEFAULT_BLS_PUBLIC_KEY
+        });
+
+        return SubnetConversionData({
+            subnetID: DEFAULT_SUBNET_ID,
+            validatorManagerBlockchainID: DEFAULT_SOURCE_BLOCKCHAIN_ID,
+            validatorManagerAddress: address(validatorManager),
+            initialValidators: initialValidators
+        });
+    }
+
     // This needs to be kept in line with the contract conversions, but we can't make external calls
     // to the contract and use vm.expectRevert at the same time.
     // These are okay to use for PoA as well, because they're just used for conversions inside the tests.