Extend error handling on ValidatorRegistry contract

stader-labs · Sep 26, 2024 · 029fa36 · 029fa36
1 parent 095f29b
commit 029fa36
Show file tree

Hide file tree

Showing 6 changed files with 385 additions and 180 deletions.
diff --git a/contracts/MaticX.sol b/contracts/MaticX.sol
@@ -430,11 +430,6 @@ contract MaticX is
 		);
 	}
 
-	/// @notice Toggles the paused status of this contract.
-	function togglePause() external override onlyRole(DEFAULT_ADMIN_ROLE) {
-		paused() ? _unpause() : _pause();
-	}
-
 	/// ------------------------------ Setters ---------------------------------
 
 	/// @notice Sets a fee percent.
@@ -498,6 +493,11 @@ contract MaticX is
 		emit SetVersion(_version);
 	}
 
+	/// @notice Toggles the paused status of this contract.
+	function togglePause() external override onlyRole(DEFAULT_ADMIN_ROLE) {
+		paused() ? _unpause() : _pause();
+	}
+
 	/// ------------------------------ Getters ---------------------------------
 
 	/// @notice Converts an amount of MaticX shares to POL tokens.

diff --git a/contracts/ValidatorRegistry.sol b/contracts/ValidatorRegistry.sol
@@ -4,6 +4,7 @@ pragma solidity 0.8.7;
 import { AccessControlUpgradeable } from "@openzeppelin/contracts-upgradeable/access/AccessControlUpgradeable.sol";
 import { PausableUpgradeable } from "@openzeppelin/contracts-upgradeable/security/PausableUpgradeable.sol";
 import { ReentrancyGuardUpgradeable } from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
+import { StringsUpgradeable } from "@openzeppelin/contracts-upgradeable/utils/StringsUpgradeable.sol";
 import { IStakeManager } from "./interfaces/IStakeManager.sol";
 import { IValidatorShare } from "./interfaces/IValidatorShare.sol";
 import { IValidatorRegistry } from "./interfaces/IValidatorRegistry.sol";
@@ -16,22 +17,28 @@ contract ValidatorRegistry is
 	AccessControlUpgradeable,
 	ReentrancyGuardUpgradeable
 {
+	using StringsUpgradeable for string;
+
 	bytes32 public constant BOT = keccak256("BOT");
 
-	address private stakeManager;
+	IStakeManager private stakeManager;
 	address private maticToken;
 	address private maticX;
-
 	string public override version;
 	uint256 public override preferredDepositValidatorId;
 	uint256 public override preferredWithdrawalValidatorId;
 	mapping(uint256 => bool) public override validatorIdExists;
-
 	uint256[] private validators;
 	address private polToken;
 
 	/// ------------------------------ Modifiers -------------------------------
 
+	/// @notice Checks if the given validator id is not zero.
+	modifier validatoIdIsZero(uint256 _validatorId) {
+		require(_validatorId != 0, "Zero validator id");
+		_;
+	}
+
 	/// @notice Checks if the given validator id exists in the registry.
 	/// @param _validatorId - Validator id
 	modifier whenValidatorIdExists(uint256 _validatorId) {
@@ -75,7 +82,7 @@ contract ValidatorRegistry is
 		PausableUpgradeable.__Pausable_init();
 
 		require(_stakeManager != address(0), "Zero stake manager address");
-		stakeManager = _stakeManager;
+		stakeManager = IStakeManager(_stakeManager);
 
 		require(_maticToken != address(0), "Zero matic token address");
 		maticToken = _maticToken;
@@ -111,19 +118,20 @@ contract ValidatorRegistry is
 		external
 		override
 		whenNotPaused
-		whenValidatorIdDoesNotExist(_validatorId)
 		onlyRole(DEFAULT_ADMIN_ROLE)
+		validatoIdIsZero(_validatorId)
+		whenValidatorIdDoesNotExist(_validatorId)
 	{
-		IStakeManager.Validator memory smValidator = IStakeManager(stakeManager)
-			.validators(_validatorId);
-
+		IStakeManager.Validator memory validator = stakeManager.validators(
+			_validatorId
+		);
 		require(
-			smValidator.contractAddress != address(0),
+			validator.contractAddress != address(0),
 			"Validator has no validator share"
 		);
 		require(
-			(smValidator.status == IStakeManager.Status.Active) &&
-				smValidator.deactivationEpoch == 0,
+			(validator.status == IStakeManager.Status.Active) &&
+				validator.deactivationEpoch == 0,
 			"Validator isn't active"
 		);
 
@@ -142,8 +150,9 @@ contract ValidatorRegistry is
 		external
 		override
 		whenNotPaused
-		whenValidatorIdExists(_validatorId)
 		onlyRole(DEFAULT_ADMIN_ROLE)
+		validatoIdIsZero(_validatorId)
+		whenValidatorIdExists(_validatorId)
 	{
 		require(
 			preferredDepositValidatorId != _validatorId,
@@ -154,8 +163,9 @@ contract ValidatorRegistry is
 			"Can't remove a preferred validator for withdrawals"
 		);
 
-		address validatorShare = IStakeManager(stakeManager)
-			.getValidatorContract(_validatorId);
+		address validatorShare = stakeManager.getValidatorContract(
+			_validatorId
+		);
 		(uint256 validatorBalance, ) = IValidatorShare(validatorShare)
 			.getTotalStake(maticX);
 		require(validatorBalance == 0, "Validator has some shares left");
@@ -166,6 +176,7 @@ contract ValidatorRegistry is
 				validators[i] = validators[iterationCount];
 				break;
 			}
+
 			unchecked {
 				++i;
 			}
@@ -187,8 +198,9 @@ contract ValidatorRegistry is
 		external
 		override
 		whenNotPaused
-		whenValidatorIdExists(_validatorId)
 		onlyRole(BOT)
+		validatoIdIsZero(_validatorId)
+		whenValidatorIdExists(_validatorId)
 	{
 		preferredDepositValidatorId = _validatorId;
 
@@ -203,8 +215,9 @@ contract ValidatorRegistry is
 		external
 		override
 		whenNotPaused
-		whenValidatorIdExists(_validatorId)
 		onlyRole(BOT)
+		validatoIdIsZero(_validatorId)
+		whenValidatorIdExists(_validatorId)
 	{
 		preferredWithdrawalValidatorId = _validatorId;
 
@@ -227,6 +240,7 @@ contract ValidatorRegistry is
 	function setVersion(
 		string memory _version
 	) external override onlyRole(DEFAULT_ADMIN_ROLE) {
+		require(!_version.equal(""), "Empty version");
 		version = _version;
 
 		emit SetVersion(_version);
@@ -249,7 +263,7 @@ contract ValidatorRegistry is
 		view
 		override
 		returns (
-			address _stakeManager,
+			IStakeManager _stakeManager,
 			address _maticToken,
 			address _maticX,
 			address _polToken

diff --git a/contracts/interfaces/IMaticX.sol b/contracts/interfaces/IMaticX.sol
@@ -148,9 +148,6 @@ interface IMaticX is IERC20Upgradeable {
 		uint256 _amount
 	) external;
 
-	/// @notice Toggles the paused status of this contract.
-	function togglePause() external;
-
 	/// @notice Sets a fee percent.
 	/// @param _feePercent - Fee percent (10 = 10%)
 	function setFeePercent(uint8 _feePercent) external;
@@ -171,6 +168,9 @@ interface IMaticX is IERC20Upgradeable {
 	/// @param _version - New version of this contract
 	function setVersion(string calldata _version) external;
 
+	/// @notice Toggles the paused status of this contract.
+	function togglePause() external;
+
 	/// @notice Converts an amount of MaticX shares to POL tokens.
 	/// @param _balance - Balance in MaticX shares
 	/// @return Balance in POL tokens

diff --git a/contracts/interfaces/IValidatorRegistry.sol b/contracts/interfaces/IValidatorRegistry.sol
@@ -1,6 +1,8 @@
 // SPDX-License-Identifier: GPL-3.0
 pragma solidity 0.8.7;
 
+import { IStakeManager } from "./IStakeManager.sol";
+
 /// @title IValidatorRegistry
 /// @notice Defines a public interface for the ValidatorRegistry contract.
 interface IValidatorRegistry {
@@ -80,7 +82,7 @@ interface IValidatorRegistry {
 		external
 		view
 		returns (
-			address _stakeManager,
+			IStakeManager _stakeManager,
 			address _maticToken,
 			address _maticX,
 			address _polToken

diff --git a/test/MaticX.forking.spec.ts b/test/MaticX.forking.spec.ts
@@ -23,6 +23,7 @@ const envVars = extractEnvironmentVariables();
 describe("MaticX (Forking)", function () {
 	const stakeAmount = ethers.utils.parseUnits("100", 18);
 	const tripleStakeAmount = stakeAmount.mul(3);
+	const version = "1";
 
 	async function deployFixture(fullMaticXInitialization = true) {
 		await reset(envVars.ROOT_CHAIN_RPC, envVars.FORKING_ROOT_BLOCK_NUMBER);
@@ -585,61 +586,6 @@ describe("MaticX (Forking)", function () {
 		});
 	});
 
-	describe("Toggle pause", function () {
-		describe("Negative", function () {
-			it("Should revert with the right error if called by a non admin", async function () {
-				const { maticX, executor, defaultAdminRole } =
-					await loadFixture(deployFixture);
-
-				const promise = maticX.connect(executor).togglePause();
-				await expect(promise).to.be.revertedWith(
-					`AccessControl: account ${executor.address.toLowerCase()} is missing role ${defaultAdminRole}`
-				);
-			});
-		});
-
-		describe("Positive", function () {
-			it("Should emit the Paused event if pausing", async function () {
-				const { maticX, manager } = await loadFixture(deployFixture);
-
-				const promise = maticX.connect(manager).togglePause();
-				await expect(promise)
-					.to.emit(maticX, "Paused")
-					.withArgs(manager.address);
-			});
-
-			it("Should emit the Unpaused event if pausing", async function () {
-				const { maticX, manager } = await loadFixture(deployFixture);
-
-				await maticX.connect(manager).togglePause();
-
-				const promise = maticX.connect(manager).togglePause();
-				await expect(promise)
-					.to.emit(maticX, "Unpaused")
-					.withArgs(manager.address);
-			});
-
-			it("Should return the right paused status if toggling once", async function () {
-				const { maticX, manager } = await loadFixture(deployFixture);
-
-				await maticX.connect(manager).togglePause();
-
-				const paused = await maticX.paused();
-				expect(paused).to.be.true;
-			});
-
-			it("Should return the right paused status if toggling twice", async function () {
-				const { maticX, manager } = await loadFixture(deployFixture);
-
-				await maticX.connect(manager).togglePause();
-				await maticX.connect(manager).togglePause();
-
-				const paused = await maticX.paused();
-				expect(paused).to.be.false;
-			});
-		});
-	});
-
 	describe("Initialize V2", function () {
 		describe("Negative", function () {
 			it("Should revert with the right error if reinitializing", async function () {
@@ -2283,16 +2229,14 @@ describe("MaticX (Forking)", function () {
 	});
 
 	describe("Set a version", function () {
-		const version = "1";
-
 		describe("Negative", function () {
 			it("Should revert with the right error if called by a non admin", async function () {
-				const { maticX, stakerA, defaultAdminRole } =
+				const { maticX, executor, defaultAdminRole } =
 					await loadFixture(deployFixture);
 
-				const promise = maticX.connect(stakerA).setVersion(version);
+				const promise = maticX.connect(executor).setVersion(version);
 				await expect(promise).to.be.revertedWith(
-					`AccessControl: account ${stakerA.address.toLowerCase()} is missing role ${defaultAdminRole}`
+					`AccessControl: account ${executor.address.toLowerCase()} is missing role ${defaultAdminRole}`
 				);
 			});
 
@@ -2315,4 +2259,59 @@ describe("MaticX (Forking)", function () {
 			});
 		});
 	});
+
+	describe("Toggle a pause", function () {
+		describe("Negative", function () {
+			it("Should revert with the right error if called by a non admin", async function () {
+				const { maticX, executor, defaultAdminRole } =
+					await loadFixture(deployFixture);
+
+				const promise = maticX.connect(executor).togglePause();
+				await expect(promise).to.be.revertedWith(
+					`AccessControl: account ${executor.address.toLowerCase()} is missing role ${defaultAdminRole}`
+				);
+			});
+		});
+
+		describe("Positive", function () {
+			it("Should emit the Paused event if pausing", async function () {
+				const { maticX, manager } = await loadFixture(deployFixture);
+
+				const promise = maticX.connect(manager).togglePause();
+				await expect(promise)
+					.to.emit(maticX, "Paused")
+					.withArgs(manager.address);
+			});
+
+			it("Should emit the Unpaused event if pausing", async function () {
+				const { maticX, manager } = await loadFixture(deployFixture);
+
+				await maticX.connect(manager).togglePause();
+
+				const promise = maticX.connect(manager).togglePause();
+				await expect(promise)
+					.to.emit(maticX, "Unpaused")
+					.withArgs(manager.address);
+			});
+
+			it("Should return the right paused status if toggling once", async function () {
+				const { maticX, manager } = await loadFixture(deployFixture);
+
+				await maticX.connect(manager).togglePause();
+
+				const paused = await maticX.paused();
+				expect(paused).to.be.true;
+			});
+
+			it("Should return the right paused status if toggling twice", async function () {
+				const { maticX, manager } = await loadFixture(deployFixture);
+
+				await maticX.connect(manager).togglePause();
+				await maticX.connect(manager).togglePause();
+
+				const paused = await maticX.paused();
+				expect(paused).to.be.false;
+			});
+		});
+	});
 });