Skip to content

Commit 0bb381d

Browse files
eigenmikemeigenmikem
authored
refactor: remove unnecessary signature validation and change param name (#1509)
**Motivation:** Unnecessary signature validation and confusing param name in ecdsa cert verifier. **Modifications:** Fixed **Result:** Contract cleaner and more efficient
1 parent 269674b commit 0bb381d

File tree

3 files changed

+43
-42
lines changed

3 files changed

+43
-42
lines changed

‎src/contracts/interfaces/IECDSACertificateVerifier.sol‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,11 +51,12 @@ interface IECDSACertificateVerifier is IECDSACertificateVerifierEvents, IBaseCer
5151
* @param cert a certificate
5252
* @return signedStakes amount of stake that signed the certificate for each stake
5353
* type. Each index corresponds to a stake type in the `weights` array in the `ECDSAOperatorInfo`
54+
* @return signers array of addresses that signed the certificate
5455
*/
5556
function verifyCertificate(
5657
OperatorSet calldata operatorSet,
5758
ECDSACertificate memory cert
58-
) external returns (uint256[] memory signedStakes);
59+
) external returns (uint256[] memory signedStakes, address[] memory signers);
5960

6061
/**
6162
* @notice verifies a certificate and makes sure that the signed stakes meet
@@ -65,12 +66,13 @@ interface IECDSACertificateVerifier is IECDSACertificateVerifierEvents, IBaseCer
6566
* the signed stake of the certificate should meet. Each index corresponds to
6667
* a stake type in the `weights` array in the `ECDSAOperatorInfo`
6768
* @return Whether or not the certificate is valid and meets thresholds
69+
* @return signers array of addresses that signed the certificate
6870
*/
6971
function verifyCertificateProportion(
7072
OperatorSet calldata operatorSet,
7173
ECDSACertificate memory cert,
7274
uint16[] memory totalStakeProportionThresholds
73-
) external returns (bool);
75+
) external returns (bool, address[] memory signers);
7476

7577
/**
7678
* @notice verifies a certificate and makes sure that the signed stakes meet
@@ -80,12 +82,13 @@ interface IECDSACertificateVerifier is IECDSACertificateVerifierEvents, IBaseCer
8082
* the signed stake of the certificate should meet. Each index corresponds to
8183
* a stake type in the `weights` array in the `ECDSAOperatorInfo`
8284
* @return Whether or not the certificate is valid and meets thresholds
85+
* @return signers array of addresses that signed the certificate
8386
*/
8487
function verifyCertificateNominal(
8588
OperatorSet calldata operatorSet,
8689
ECDSACertificate memory cert,
8790
uint256[] memory totalStakeNominalThresholds
88-
) external returns (bool);
91+
) external returns (bool, address[] memory signers);
8992

9093
/**
9194
* @notice Get operator infos for a timestamp

‎src/contracts/multichain/ECDSACertificateVerifier.sol‎

Lines changed: 17 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -74,42 +74,43 @@ contract ECDSACertificateVerifier is Initializable, ECDSACertificateVerifierStor
7474
function verifyCertificate(
7575
OperatorSet calldata operatorSet,
7676
ECDSACertificate calldata cert
77-
) external view returns (uint256[] memory) {
78-
return _verifyECDSACertificate(operatorSet, cert);
77+
) external view returns (uint256[] memory, address[] memory) {
78+
(uint256[] memory signedStakes, address[] memory signers) = _verifyECDSACertificate(operatorSet, cert);
79+
return (signedStakes, signers);
7980
}
8081

8182
///@inheritdoc IECDSACertificateVerifier
8283
function verifyCertificateProportion(
8384
OperatorSet calldata operatorSet,
8485
ECDSACertificate calldata cert,
8586
uint16[] calldata totalStakeProportionThresholds
86-
) external view returns (bool) {
87-
uint256[] memory signedStakes = _verifyECDSACertificate(operatorSet, cert);
87+
) external view returns (bool, address[] memory) {
88+
(uint256[] memory signedStakes, address[] memory signers) = _verifyECDSACertificate(operatorSet, cert);
8889
uint256[] memory totalStakes = getTotalStakes(operatorSet, cert.referenceTimestamp);
8990
require(signedStakes.length == totalStakeProportionThresholds.length, ArrayLengthMismatch());
9091
for (uint256 i = 0; i < signedStakes.length; i++) {
9192
uint256 threshold = (totalStakes[i] * totalStakeProportionThresholds[i]) / 10_000;
9293
if (signedStakes[i] < threshold) {
93-
return false;
94+
return (false, signers);
9495
}
9596
}
96-
return true;
97+
return (true, signers);
9798
}
9899

99100
///@inheritdoc IECDSACertificateVerifier
100101
function verifyCertificateNominal(
101102
OperatorSet calldata operatorSet,
102103
ECDSACertificate calldata cert,
103104
uint256[] memory totalStakeNominalThresholds
104-
) external view returns (bool) {
105-
uint256[] memory signedStakes = _verifyECDSACertificate(operatorSet, cert);
105+
) external view returns (bool, address[] memory) {
106+
(uint256[] memory signedStakes, address[] memory signers) = _verifyECDSACertificate(operatorSet, cert);
106107
require(signedStakes.length == totalStakeNominalThresholds.length, ArrayLengthMismatch());
107108
for (uint256 i = 0; i < signedStakes.length; i++) {
108109
if (signedStakes[i] < totalStakeNominalThresholds[i]) {
109-
return false;
110+
return (false, signers);
110111
}
111112
}
112-
return true;
113+
return (true, signers);
113114
}
114115

115116
/**
@@ -120,7 +121,7 @@ contract ECDSACertificateVerifier is Initializable, ECDSACertificateVerifierStor
120121
function _verifyECDSACertificate(
121122
OperatorSet calldata operatorSet,
122123
ECDSACertificate calldata cert
123-
) internal view returns (uint256[] memory) {
124+
) internal view returns (uint256[] memory, address[] memory) {
124125
bytes32 operatorSetKey = operatorSet.key();
125126

126127
// Assert that reference timestamp is not stale
@@ -171,7 +172,7 @@ contract ECDSACertificateVerifier is Initializable, ECDSACertificateVerifierStor
171172
}
172173
}
173174

174-
return signedStakes;
175+
return (signedStakes, signers);
175176
}
176177

177178
/**
@@ -182,17 +183,17 @@ contract ECDSACertificateVerifier is Initializable, ECDSACertificateVerifierStor
182183

183184
/**
184185
* @notice Parse signatures from the concatenated signature bytes
185-
* @param messageHash The message hash that was signed
186+
* @param signableDigest The signable digest that was signed
186187
* @param signatures The concatenated signatures
187188
* @return signers Array of addresses that signed the message
188189
* @return valid Whether all signatures are valid
189190
* @dev Signatures must be ordered by signer address (ascending)
190191
* @dev This does not support smart contract based signatures for multichain
191192
*/
192193
function _parseSignatures(
193-
bytes32 messageHash,
194+
bytes32 signableDigest,
194195
bytes memory signatures
195-
) internal view returns (address[] memory signers, bool valid) {
196+
) internal pure returns (address[] memory signers, bool valid) {
196197
// Each ECDSA signature is 65 bytes: r (32 bytes) + s (32 bytes) + v (1 byte)
197198
require(signatures.length > 0 && signatures.length % 65 == 0, InvalidSignatureLength());
198199

@@ -206,7 +207,7 @@ contract ECDSACertificateVerifier is Initializable, ECDSACertificateVerifierStor
206207
}
207208

208209
// Recover the signer
209-
(address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(messageHash, signature);
210+
(address recovered, ECDSA.RecoverError error) = ECDSA.tryRecover(signableDigest, signature);
210211
if (error != ECDSA.RecoverError.NoError || recovered == address(0)) {
211212
return (signers, false);
212213
}
@@ -216,9 +217,6 @@ contract ECDSACertificateVerifier is Initializable, ECDSACertificateVerifierStor
216217
return (signers, false);
217218
}
218219

219-
// Verify that the recovered address actually signed the message
220-
_checkIsValidSignatureNow(recovered, messageHash, signature, type(uint256).max);
221-
222220
signers[i] = recovered;
223221
}
224222

‎src/test/unit/ECDSACertificateVerifierUnit.t.sol‎

Lines changed: 20 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -441,7 +441,7 @@ contract ECDSACertificateVerifierUnitTests_verifyCertificate is ECDSACertificate
441441
IECDSACertificateVerifierTypes.ECDSACertificate memory cert =
442442
_createCertificate(referenceTimestamp, defaultMsgHash, nonSignerIndices, operators, signerPrivKeys);
443443

444-
uint[] memory signedStakes = verifier.verifyCertificate(defaultOperatorSet, cert);
444+
(uint[] memory signedStakes, address[] memory signers) = verifier.verifyCertificate(defaultOperatorSet, cert);
445445

446446
// Calculate total stakes
447447
uint[] memory totalStakes = new uint[](2);
@@ -470,7 +470,7 @@ contract ECDSACertificateVerifierUnitTests_verifyCertificate is ECDSACertificate
470470
IECDSACertificateVerifierTypes.ECDSACertificate memory cert =
471471
_createCertificate(referenceTimestamp, defaultMsgHash, nonSignerIndices, operators, signerPrivKeys);
472472

473-
uint[] memory signedStakes = verifier.verifyCertificate(defaultOperatorSet, cert);
473+
(uint[] memory signedStakes, address[] memory signers) = verifier.verifyCertificate(defaultOperatorSet, cert);
474474

475475
// Check that the signed stakes are correct
476476
assertEq(signedStakes.length, 2, "Wrong number of stake types");
@@ -525,7 +525,7 @@ contract ECDSACertificateVerifierUnitTests_verifyCertificate is ECDSACertificate
525525
IECDSACertificateVerifierTypes.ECDSACertificate memory cert =
526526
_createCertificate(secondTimestamp, defaultMsgHash, nonSignerIndices, operators, signerPrivKeys);
527527

528-
uint[] memory signedStakes = verifier.verifyCertificate(defaultOperatorSet, cert);
528+
(uint[] memory signedStakes, address[] memory signers) = verifier.verifyCertificate(defaultOperatorSet, cert);
529529

530530
// Verify all stakes are signed
531531
uint[] memory totalStakes = new uint[](2);
@@ -565,7 +565,7 @@ contract ECDSACertificateVerifierUnitTests_verifyCertificate is ECDSACertificate
565565
IECDSACertificateVerifierTypes.ECDSACertificate memory cert =
566566
_createCertificate(secondTimestamp, defaultMsgHash, nonSignerIndices, secondOperators, signerPrivKeys);
567567

568-
uint[] memory signedStakes = verifier.verifyCertificate(defaultOperatorSet, cert);
568+
(uint[] memory signedStakes, address[] memory signers) = verifier.verifyCertificate(defaultOperatorSet, cert);
569569

570570
// Verify only signers' stakes are counted
571571
uint[] memory expectedSignedStakes = new uint[](2);
@@ -800,7 +800,7 @@ contract ECDSACertificateVerifierUnitTests_verifyCertificateProportion is ECDSAC
800800
thresholds[0] = 6000; // 60%
801801
thresholds[1] = 6000; // 60%
802802

803-
bool meetsThresholds = verifier.verifyCertificateProportion(defaultOperatorSet, cert, thresholds);
803+
(bool meetsThresholds, address[] memory signers) = verifier.verifyCertificateProportion(defaultOperatorSet, cert, thresholds);
804804

805805
// Calculate expected result based on the number of signers
806806
uint signedPercentage = (numSigners * 10_000) / numOperators;
@@ -829,10 +829,10 @@ contract ECDSACertificateVerifierUnitTests_verifyCertificateProportion is ECDSAC
829829
thresholds[0] = 9000; // 90%
830830
thresholds[1] = 9000; // 90%
831831

832-
bool meetsThresholds = verifier.verifyCertificateProportion(defaultOperatorSet, cert, thresholds);
832+
(bool meetsThresholds, address[] memory signers) = verifier.verifyCertificateProportion(defaultOperatorSet, cert, thresholds);
833833

834834
// Calculate percentage of signed stakes to determine if it should meet threshold
835-
uint[] memory signedStakes = verifier.verifyCertificate(defaultOperatorSet, cert);
835+
(uint[] memory signedStakes, address[] memory certSigners) = verifier.verifyCertificate(defaultOperatorSet, cert);
836836
uint[] memory totalStakes = verifier.getTotalStakes(defaultOperatorSet, referenceTimestamp);
837837
uint signedPercentage0 = (signedStakes[0] * 10_000) / totalStakes[0];
838838
uint signedPercentage1 = (signedStakes[1] * 10_000) / totalStakes[1];
@@ -864,10 +864,10 @@ contract ECDSACertificateVerifierUnitTests_verifyCertificateProportion is ECDSAC
864864
thresholds[0] = threshold0;
865865
thresholds[1] = threshold1;
866866

867-
bool meetsThresholds = verifier.verifyCertificateProportion(defaultOperatorSet, cert, thresholds);
867+
(bool meetsThresholds, address[] memory signers) = verifier.verifyCertificateProportion(defaultOperatorSet, cert, thresholds);
868868

869869
// Calculate expected result
870-
uint[] memory signedStakes = verifier.verifyCertificate(defaultOperatorSet, cert);
870+
(uint[] memory signedStakes, address[] memory certSigners) = verifier.verifyCertificate(defaultOperatorSet, cert);
871871
uint[] memory totalStakes = verifier.getTotalStakes(defaultOperatorSet, referenceTimestamp);
872872

873873
bool expectedResult = true;
@@ -927,14 +927,14 @@ contract ECDSACertificateVerifierUnitTests_verifyCertificateNominal is ECDSACert
927927
_createCertificate(referenceTimestamp, defaultMsgHash, nonSignerIndices, operators, signerPrivKeys);
928928

929929
// Get the signed stakes for reference
930-
uint[] memory signedStakes = verifier.verifyCertificate(defaultOperatorSet, cert);
930+
(uint[] memory signedStakes, address[] memory certSigners) = verifier.verifyCertificate(defaultOperatorSet, cert);
931931

932932
// Test with thresholds lower than signed stakes (should pass)
933933
uint[] memory passThresholds = new uint[](2);
934934
passThresholds[0] = signedStakes[0] > 10 ? signedStakes[0] - 10 : 0;
935935
passThresholds[1] = signedStakes[1] > 10 ? signedStakes[1] - 10 : 0;
936936

937-
bool meetsThresholds = verifier.verifyCertificateNominal(defaultOperatorSet, cert, passThresholds);
937+
(bool meetsThresholds, address[] memory signers) = verifier.verifyCertificateNominal(defaultOperatorSet, cert, passThresholds);
938938
assertTrue(meetsThresholds, "Certificate should meet nominal thresholds");
939939
}
940940

@@ -954,14 +954,14 @@ contract ECDSACertificateVerifierUnitTests_verifyCertificateNominal is ECDSACert
954954
_createCertificate(referenceTimestamp, defaultMsgHash, nonSignerIndices, operators, signerPrivKeys);
955955

956956
// Get the signed stakes for reference
957-
uint[] memory signedStakes = verifier.verifyCertificate(defaultOperatorSet, cert);
957+
(uint[] memory signedStakes, address[] memory certSigners) = verifier.verifyCertificate(defaultOperatorSet, cert);
958958

959959
// Test with thresholds higher than signed stakes (should fail)
960960
uint[] memory failThresholds = new uint[](2);
961961
failThresholds[0] = signedStakes[0] + 10;
962962
failThresholds[1] = signedStakes[1] + 10;
963963

964-
bool meetsThresholds = verifier.verifyCertificateNominal(defaultOperatorSet, cert, failThresholds);
964+
(bool meetsThresholds, address[] memory signers) = verifier.verifyCertificateNominal(defaultOperatorSet, cert, failThresholds);
965965
assertFalse(meetsThresholds, "Certificate should not meet impossible nominal thresholds");
966966
}
967967

@@ -982,20 +982,20 @@ contract ECDSACertificateVerifierUnitTests_verifyCertificateNominal is ECDSACert
982982
_createCertificate(referenceTimestamp, defaultMsgHash, nonSignerIndices, operators, signerPrivKeys);
983983

984984
// Get the signed stakes for reference
985-
uint[] memory signedStakes = verifier.verifyCertificate(defaultOperatorSet, cert);
985+
(uint[] memory signedStakes,) = verifier.verifyCertificate(defaultOperatorSet, cert);
986986

987987
// Bound thresholds to reasonable values
988-
threshold0 = bound(threshold0, 0, signedStakes[0] * 2);
989-
threshold1 = bound(threshold1, 0, signedStakes[1] * 2);
988+
uint boundedThreshold0 = bound(threshold0, 0, signedStakes[0] * 2);
989+
uint boundedThreshold1 = bound(threshold1, 0, signedStakes[1] * 2);
990990

991991
uint[] memory thresholds = new uint[](2);
992-
thresholds[0] = threshold0;
993-
thresholds[1] = threshold1;
992+
thresholds[0] = boundedThreshold0;
993+
thresholds[1] = boundedThreshold1;
994994

995-
bool meetsThresholds = verifier.verifyCertificateNominal(defaultOperatorSet, cert, thresholds);
995+
(bool meetsThresholds,) = verifier.verifyCertificateNominal(defaultOperatorSet, cert, thresholds);
996996

997997
// Expected result
998-
bool expectedResult = (signedStakes[0] >= threshold0) && (signedStakes[1] >= threshold1);
998+
bool expectedResult = (signedStakes[0] >= boundedThreshold0) && (signedStakes[1] >= boundedThreshold1);
999999

10001000
assertEq(meetsThresholds, expectedResult, "Nominal threshold check mismatch");
10011001
}

0 commit comments

Comments
 (0)