diff --git a/contracts/contracts/KMSVerifier.sol b/contracts/contracts/KMSVerifier.sol index 6ea689e8..9a2f695a 100644 --- a/contracts/contracts/KMSVerifier.sol +++ b/contracts/contracts/KMSVerifier.sol @@ -13,6 +13,26 @@ import "@openzeppelin/contracts/utils/Strings.sol"; /// @notice This contract allows for the management of signers and provides methods to verify signatures /// @dev The contract uses OpenZeppelin's EIP712Upgradeable for cryptographic operations contract KMSVerifier is UUPSUpgradeable, Ownable2StepUpgradeable, EIP712Upgradeable { + /// @notice Returned if the KMS signer to add is already a signer. + error KMSAlreadySigner(); + + /// @notice Returned if the recovered KMS signer is not a valid KMS signer. + /// @param invalidSigner Address of the invalid signer. + error KMSInvalidSigner(address invalidSigner); + + /// @notice Returned if the KMS signer to remove is not a signer. + error KMSNotASigner(); + + /// @notice Returned if the KMS signer to add is the null address. + error KMSSignerNull(); + + /// @notice Returned if the number of signatures is inferior to the threshold. + /// @param numSignatures Number of signatures. + error KMSSignatureThresholdNotReached(uint256 numSignatures); + + /// @notice Returned if the number of signatures is equal to 0. + error KMSZeroSignature(); + struct DecryptionResult { address aclAddress; uint256[] handlesList; @@ -113,9 +133,15 @@ contract KMSVerifier is UUPSUpgradeable, Ownable2StepUpgradeable, EIP712Upgradea /// @dev Only the owner can add a signer /// @param signer The address to be added as a signer function addSigner(address signer) public virtual onlyOwner { - require(signer != address(0), "KMSVerifier: Address is null"); + if (signer == address(0)) { + revert KMSSignerNull(); + } + KMSVerifierStorage storage $ = _getKMSVerifierStorage(); - require(!$.isSigner[signer], "KMSVerifier: Address is already a signer"); + if ($.isSigner[signer]) { + revert KMSAlreadySigner(); + } + $.isSigner[signer] = true; $.signers.push(signer); applyThreshold(); @@ -158,7 +184,9 @@ contract KMSVerifier is UUPSUpgradeable, Ownable2StepUpgradeable, EIP712Upgradea /// @param signer The address to be removed from signers function removeSigner(address signer) public virtual onlyOwner { KMSVerifierStorage storage $ = _getKMSVerifierStorage(); - require($.isSigner[signer], "KMSVerifier: Address is not a signer"); + if (!$.isSigner[signer]) { + revert KMSNotASigner(); + } // Remove signer from the mapping $.isSigner[signer] = false; @@ -225,14 +253,24 @@ contract KMSVerifier is UUPSUpgradeable, Ownable2StepUpgradeable, EIP712Upgradea /// @return true if enough provided signatures are valid, false otherwise function verifySignaturesDigest(bytes32 digest, bytes[] memory signatures) internal virtual returns (bool) { uint256 numSignatures = signatures.length; - require(numSignatures > 0, "KmsVerifier: no signatures provided"); + + if (numSignatures == 0) { + revert KMSZeroSignature(); + } + uint256 threshold = getThreshold(); - require(numSignatures >= threshold, "KmsVerifier: at least threshold number of signatures required"); + + if (numSignatures < threshold) { + revert KMSSignatureThresholdNotReached(numSignatures); + } + address[] memory recoveredSigners = new address[](numSignatures); uint256 uniqueValidCount; for (uint256 i = 0; i < numSignatures; i++) { address signerRecovered = recoverSigner(digest, signatures[i]); - require(isSigner(signerRecovered), "KmsVerifier: Invalid KMS signer"); + if (!isSigner(signerRecovered)) { + revert KMSInvalidSigner(signerRecovered); + } if (!tload(signerRecovered)) { recoveredSigners[uniqueValidCount] = signerRecovered; uniqueValidCount++; diff --git a/contracts/test/kmsVerifier/kmsVerifier.ts b/contracts/test/kmsVerifier/kmsVerifier.ts index 59bf260e..07870e00 100644 --- a/contracts/test/kmsVerifier/kmsVerifier.ts +++ b/contracts/test/kmsVerifier/kmsVerifier.ts @@ -42,8 +42,9 @@ describe('KMSVerifier', function () { expect(y).to.equal(true); // in this case, one signature still suffices to pass the decrypt (threshold is still 1) const kmsSignerDup = new ethers.Wallet(privKeySigner).connect(ethers.provider); - await expect(kmsVerifier.connect(deployer).addSigner(kmsSignerDup.address)).to.revertedWith( - 'KMSVerifier: Address is already a signer', + await expect(kmsVerifier.connect(deployer).addSigner(kmsSignerDup.address)).to.revertedWithCustomError( + kmsVerifier, + 'KMSAlreadySigner', ); // cannot add duplicated signer expect((await kmsVerifier.getSigners()).length).to.equal(2); @@ -59,9 +60,9 @@ describe('KMSVerifier', function () { const tx5 = await contract.requestUint4({ gasLimit: 5_000_000 }); await tx5.wait(); - await expect(awaitAllDecryptionResults()).to.revertedWith( - 'KmsVerifier: at least threshold number of signatures required', - ); // should revert because now we are below the threshold! (we receive only 1 signature but threshold is 2) + await expect(awaitAllDecryptionResults()) + .to.revertedWithCustomError(kmsVerifier, 'KMSSignatureThresholdNotReached') + .withArgs(1n); // should revert because now we are below the threshold! (we receive only 1 signature but threshold is 2) const y2 = await contract.yUint4(); expect(y2).to.equal(0); @@ -90,7 +91,9 @@ describe('KMSVerifier', function () { contract2.requestMixedBytes256Trustless(encryptedAmount2.handles[0], encryptedAmount2.inputProof, { gasLimit: 5_000_000, }), - ).to.revertedWith('KmsVerifier: at least threshold number of signatures required'); // this should fail because in this case the InputVerifier received only one KMS signature (instead of at least 2); + ) + .to.revertedWithCustomError(kmsVerifier, 'KMSSignatureThresholdNotReached') + .withArgs(1n); // should revert because now we are below the threshold! (we receive only 1 signature but threshold is 2) if (process.env.IS_COPROCESSOR === 'true') { // different format of inputProof for native