Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: custom errors KMSVerifier.sol #194

Merged
merged 1 commit into from
Dec 19, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 44 additions & 6 deletions contracts/contracts/KMSVerifier.sol
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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");
PacificYield marked this conversation as resolved.
Show resolved Hide resolved

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++;
Expand Down
15 changes: 9 additions & 6 deletions contracts/test/kmsVerifier/kmsVerifier.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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);

Expand Down Expand Up @@ -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
Expand Down
Loading