Review fixes

stakewise · May 8, 2024 · e433931 · e433931
1 parent 7001d18
commit e433931
Show file tree

Hide file tree

Showing 4 changed files with 57 additions and 49 deletions.
diff --git a/contracts/interfaces/IEthValidatorsChecker.sol b/contracts/interfaces/IEthValidatorsChecker.sol
@@ -2,26 +2,27 @@
 
 pragma solidity =0.8.22;
 
-import {IKeeperValidators} from './IKeeperValidators.sol';
+import {IERC5267} from '@openzeppelin/contracts/interfaces/IERC5267.sol';
 
 /**
  * @title IEthValidatorsChecker
  * @author StakeWise
  * @notice Defines the interface for EthValidatorsChecker
  */
-interface IEthValidatorsChecker {
+interface IEthValidatorsChecker is IERC5267 {
   /**
    * @notice Function for checking validators manager signature
    * @param vault The address of the vault
    * @param validatorsRegistryRoot The validators registry root
    * @param publicKeys The concatenation of the validators' public keys
+   * @return Current block number
    */
   function checkValidatorsManagerSignature(
     address vault,
     bytes32 validatorsRegistryRoot,
     bytes calldata publicKeys,
     bytes calldata signature
-  ) external view returns (uint256 blockNumber);
+  ) external view returns (uint256);
 
   /**
    * @notice Function for checking deposit data root
@@ -31,6 +32,7 @@ interface IEthValidatorsChecker {
    * @param proof The proof used for the merkle tree verification
    * @param proofFlags The multi proof flags for the merkle tree verification
    * @param proofIndexes The indexes of the leaves for the merkle tree multi proof verification
+   * @return Current block number
    */
   function checkDepositDataRoot(
     address vault,
@@ -39,5 +41,5 @@ interface IEthValidatorsChecker {
     bytes32[] calldata proof,
     bool[] calldata proofFlags,
     uint256[] calldata proofIndexes
-  ) external view returns (uint256 blockNumber);
+  ) external view returns (uint256);
 }
diff --git a/contracts/validators/EthValidatorsChecker.sol b/contracts/validators/EthValidatorsChecker.sol
@@ -8,16 +8,16 @@ import {EIP712} from '@openzeppelin/contracts/utils/cryptography/EIP712.sol';
 import {MessageHashUtils} from '@openzeppelin/contracts/utils/cryptography/MessageHashUtils.sol';
 
 import {IValidatorsRegistry} from '../interfaces/IValidatorsRegistry.sol';
-import {Errors} from '../libraries/Errors.sol';
 import {IKeeper} from '../interfaces/IKeeper.sol';
 import {IEthValidatorsChecker} from '../interfaces/IEthValidatorsChecker.sol';
 import {IVaultState} from '../interfaces/IVaultState.sol';
 import {IVaultVersion} from '../interfaces/IVaultVersion.sol';
-import {IVaultVersion} from '../interfaces/IVaultVersion.sol';
 import {IDepositDataRegistry} from '../interfaces/IDepositDataRegistry.sol';
 import {IVaultsRegistry} from '../interfaces/IVaultsRegistry.sol';
 import {IVaultValidators} from '../interfaces/IVaultValidators.sol';
 
+import {Errors} from '../libraries/Errors.sol';
+
 interface IVaultValidatorsV1 {
   function validatorsRoot() external view returns (bytes32);
   function validatorIndex() external view returns (uint256);
@@ -38,7 +38,7 @@ contract EthValidatorsChecker is IEthValidatorsChecker, EIP712 {
 
   bytes32 private constant _validatorsManagerSignatureTypeHash =
     keccak256(
-      'EthValidatorsCheckerData(bytes32 validatorsRegistryRoot,address vault,bytes validators)'
+      'EthValidatorsChecker(bytes32 validatorsRegistryRoot,address vault,bytes validators)'
     );
 
   /**
@@ -66,9 +66,7 @@ contract EthValidatorsChecker is IEthValidatorsChecker, EIP712 {
     bytes32 validatorsRegistryRoot,
     bytes calldata publicKeys,
     bytes calldata signature
-  ) external view override returns (uint256 blockNumber) {
-    blockNumber = block.number;
-
+  ) external view override returns (uint256) {
     if (_validatorsRegistry.get_deposit_root() != validatorsRegistryRoot) {
       revert Errors.InvalidValidatorsRegistryRoot();
     }
@@ -77,8 +75,10 @@ contract EthValidatorsChecker is IEthValidatorsChecker, EIP712 {
     }
 
     // verify vault has enough assets
-    if (!_keeper.isCollateralized(vault)) {
-      if (IVaultState(vault).withdrawableAssets() < 32 ether) revert Errors.AccessDenied();
+    if (
+      !_keeper.isCollateralized(vault) && IVaultState(vault).withdrawableAssets() < depositAmount()
+    ) {
+      revert Errors.InsufficientAssets();
     }
 
     // compose signing message
@@ -96,6 +96,8 @@ contract EthValidatorsChecker is IEthValidatorsChecker, EIP712 {
 
     // verify validators manager ECDSA signature
     if (IVaultValidators(vault).validatorsManager() != signer) revert Errors.AccessDenied();
+
+    return block.number;
   }
 
   /// @inheritdoc IEthValidatorsChecker
@@ -106,17 +108,17 @@ contract EthValidatorsChecker is IEthValidatorsChecker, EIP712 {
     bytes32[] calldata proof,
     bool[] calldata proofFlags,
     uint256[] calldata proofIndexes
-  ) external view override returns (uint256 blockNumber) {
-    blockNumber = block.number;
-
+  ) external view override returns (uint256) {
     if (_validatorsRegistry.get_deposit_root() != validatorsRegistryRoot) {
       revert Errors.InvalidValidatorsRegistryRoot();
     }
     if (!_vaultsRegistry.vaults(vault)) revert Errors.InvalidVault();
 
     // verify vault has enough assets
-    if (!_keeper.isCollateralized(vault)) {
-      if (IVaultState(vault).withdrawableAssets() < 32 ether) revert Errors.AccessDenied();
+    if (
+      !_keeper.isCollateralized(vault) && IVaultState(vault).withdrawableAssets() < depositAmount()
+    ) {
+      revert Errors.InsufficientAssets();
     }
 
     uint8 vaultVersion = IVaultVersion(vault).version();
@@ -170,5 +172,11 @@ contract EthValidatorsChecker is IEthValidatorsChecker, EIP712 {
     if (!MerkleProof.multiProofVerifyCalldata(proof, proofFlags, depositDataRoot, leaves)) {
       revert Errors.InvalidProof();
     }
+
+    return block.number;
+  }
+
+  function depositAmount() public view virtual returns (uint256) {
+    return 32 ether;
   }
 }
diff --git a/test/EthValidatorsChecker.spec.ts b/test/EthValidatorsChecker.spec.ts
@@ -125,25 +125,15 @@ describe('EthValidatorsChecker', () => {
       await expect(
         ethValidatorsChecker
           .connect(admin)
-          .checkValidatorsManagerSignature(
-            await vault.getAddress(),
-            fakeRoot,
-            Buffer.from('', 'utf-8'),
-            Buffer.from('', 'utf-8')
-          )
+          .checkValidatorsManagerSignature(await vault.getAddress(), fakeRoot, '0x', '0x')
       ).to.be.revertedWithCustomError(ethValidatorsChecker, 'InvalidValidatorsRegistryRoot')
     })
 
     it('fails for non-vault', async () => {
       await expect(
         ethValidatorsChecker
           .connect(admin)
-          .checkValidatorsManagerSignature(
-            other.address,
-            validatorsRegistryRoot,
-            Buffer.from('', 'utf-8'),
-            Buffer.from('', 'utf-8')
-          )
+          .checkValidatorsManagerSignature(other.address, validatorsRegistryRoot, '0x', '0x')
       ).to.be.revertedWithCustomError(ethValidatorsChecker, 'InvalidVault')
     })
 
@@ -154,8 +144,8 @@ describe('EthValidatorsChecker', () => {
           .checkValidatorsManagerSignature(
             await vaultV1.getAddress(),
             validatorsRegistryRoot,
-            Buffer.from('', 'utf-8'),
-            Buffer.from('', 'utf-8')
+            '0x',
+            '0x'
           )
       ).to.be.revertedWithCustomError(ethValidatorsChecker, 'InvalidVault')
     })
@@ -167,10 +157,10 @@ describe('EthValidatorsChecker', () => {
           .checkValidatorsManagerSignature(
             await vaultNotDeposited.getAddress(),
             validatorsRegistryRoot,
-            Buffer.from('', 'utf-8'),
-            Buffer.from('', 'utf-8')
+            '0x',
+            '0x'
           )
-      ).to.be.revertedWithCustomError(ethValidatorsChecker, 'AccessDenied')
+      ).to.be.revertedWithCustomError(ethValidatorsChecker, 'InsufficientAssets')
     })
 
     it('fails for signer who is not validators manager', async () => {
@@ -213,17 +203,18 @@ describe('EthValidatorsChecker', () => {
         data: typedData,
         version: SignTypedDataVersion.V4,
       })
+      const blockNumber = await ethers.provider.getBlockNumber()
 
-      await expect(
-        ethValidatorsChecker
+      expect(
+        await ethValidatorsChecker
           .connect(admin)
           .checkValidatorsManagerSignature(
             vaultAddress,
             validatorsRegistryRoot,
             Buffer.concat(publicKeys),
             ethers.getBytes(signature)
           )
-      ).to.eventually.be.greaterThan(0)
+      ).to.eq(blockNumber)
     })
   })
 
@@ -298,12 +289,11 @@ describe('EthValidatorsChecker', () => {
             proofFlags,
             proofIndexes
           )
-      ).to.be.revertedWithCustomError(ethValidatorsChecker, 'AccessDenied')
+      ).to.be.revertedWithCustomError(ethValidatorsChecker, 'InsufficientAssets')
     })
 
     it('fails for validators manager not equal to deposit data registry', async () => {
-      const [validatorsManager] = await ethers.getSigners()
-      await vault.connect(admin).setValidatorsManager(validatorsManager.address)
+      await vault.connect(admin).setValidatorsManager(other.address)
       const { proof, proofFlags, proofIndexes } = getMultiProofArgs()
 
       await expect(
@@ -340,9 +330,10 @@ describe('EthValidatorsChecker', () => {
 
     it('succeeds for vault v1', async () => {
       const { proof, proofFlags, proofIndexes } = getMultiProofArgs()
+      const blockNumber = await ethers.provider.getBlockNumber()
 
-      await expect(
-        ethValidatorsChecker
+      expect(
+        await ethValidatorsChecker
           .connect(admin)
           .checkDepositDataRoot(
             await vaultV1.getAddress(),
@@ -352,14 +343,15 @@ describe('EthValidatorsChecker', () => {
             proofFlags,
             proofIndexes
           )
-      ).to.eventually.be.greaterThan(0)
+      ).to.eq(blockNumber)
     })
 
     it('succeeds for vault v2', async () => {
       const { proof, proofFlags, proofIndexes } = getMultiProofArgs()
+      const blockNumber = await ethers.provider.getBlockNumber()
 
-      await expect(
-        ethValidatorsChecker
+      expect(
+        await ethValidatorsChecker
           .connect(admin)
           .checkDepositDataRoot(
             await vault.getAddress(),
@@ -369,7 +361,7 @@ describe('EthValidatorsChecker', () => {
             proofFlags,
             proofIndexes
           )
-      ).to.eventually.be.greaterThan(0)
+      ).to.eq(blockNumber)
     })
   })
 })
diff --git a/test/shared/validators.ts b/test/shared/validators.ts
@@ -4,7 +4,13 @@ import { ethers, network } from 'hardhat'
 import { Buffer } from 'buffer'
 import { BytesLike, Contract, ContractTransactionResponse, Signer } from 'ethers'
 import bls from 'bls-eth-wasm'
-import { DepositDataRegistry, EthVault, Keeper, DepositDataRegistry, EthValidatorsChecker } from '../../typechain-types'
+import {
+  DepositDataRegistry,
+  EthVault,
+  Keeper,
+  DepositDataRegistry,
+  EthValidatorsChecker,
+} from '../../typechain-types'
 import {
   EIP712Domain,
   KeeperUpdateExitSignaturesSig,
@@ -302,8 +308,8 @@ export async function getEthValidatorsCheckerSigningData(
   validatorsRegistryRoot: BytesLike
 ) {
   return {
-    primaryType: 'EthValidatorsCheckerData',
-    types: { EIP712Domain, EthValidatorsCheckerData: EthValidatorsCheckerSig },
+    primaryType: 'EthValidatorsChecker',
+    types: { EIP712Domain, EthValidatorsChecker: EthValidatorsCheckerSig },
     domain: {
       name: 'EthValidatorsChecker',
       version: '1',