Skip to content

Commit

Permalink
refactor: custom errors KMSVerifier.sol
Browse files Browse the repository at this point in the history
  • Loading branch information
PacificYield committed Dec 18, 2024
1 parent 6be5c69 commit 4658e31
Show file tree
Hide file tree
Showing 2 changed files with 53 additions and 12 deletions.
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");

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

0 comments on commit 4658e31

Please sign in to comment.