From e43393186a941f71f266aee61630f74cd624a463 Mon Sep 17 00:00:00 2001 From: Evgeny Gusarov Date: Wed, 8 May 2024 22:09:42 +0300 Subject: [PATCH] Review fixes --- .../interfaces/IEthValidatorsChecker.sol | 10 ++-- contracts/validators/EthValidatorsChecker.sol | 34 ++++++++----- test/EthValidatorsChecker.spec.ts | 50 ++++++++----------- test/shared/validators.ts | 12 +++-- 4 files changed, 57 insertions(+), 49 deletions(-) diff --git a/contracts/interfaces/IEthValidatorsChecker.sol b/contracts/interfaces/IEthValidatorsChecker.sol index 4775c248..ac5a19c3 100644 --- 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 index ed4b69ae..c920d4e5 100644 --- 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 index 8e49c627..91dc702a 100644 --- a/test/EthValidatorsChecker.spec.ts +++ b/test/EthValidatorsChecker.spec.ts @@ -125,12 +125,7 @@ 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') }) @@ -138,12 +133,7 @@ describe('EthValidatorsChecker', () => { 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,9 +203,10 @@ describe('EthValidatorsChecker', () => { data: typedData, version: SignTypedDataVersion.V4, }) + const blockNumber = await ethers.provider.getBlockNumber() - await expect( - ethValidatorsChecker + expect( + await ethValidatorsChecker .connect(admin) .checkValidatorsManagerSignature( vaultAddress, @@ -223,7 +214,7 @@ describe('EthValidatorsChecker', () => { 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 index f9f451c4..cf78cd74 100644 --- 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',