Skip to content

Commit 6f7f60f

Browse files
gonzaotcgonzaotc
committed
chore: improved signature validation algorithmic complexity
1 parent 0aedfa2 commit 6f7f60f

File tree

3 files changed

+84
-49
lines changed

3 files changed

+84
-49
lines changed

‎contracts/account/extensions/ERC7579Modules/SocialRecoveryExecutor.sol

Lines changed: 6 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -82,8 +82,8 @@ contract SocialRecoveryExecutor is IERC7579Module, EIP712, Nonces {
8282
error TooManyGuardians();
8383
error AlreadyGuardian();
8484

85+
error DuplicatedOrUnsortedGuardianSignatures();
8586
error ExecutionDiffersFromPending();
86-
error DuplicateGuardianSignature();
8787
error TooManyGuardianSignatures();
8888
error InvalidGuardianSignature();
8989
error ThresholdNotMet();
@@ -329,9 +329,9 @@ contract SocialRecoveryExecutor is IERC7579Module, EIP712, Nonces {
329329
//////////////////////////////////////////////////////////////////////////*/
330330

331331
/// @notice Verifies multiple guardian signatures for a given digest
332-
/// @dev Ensures each signer is an unique guardian, and threshold is met
332+
/// @dev Ensures signatures are unique, and threshold is met
333333
/// @param account The account the signatures are for
334-
/// @param guardianSignatures Array of guardian signatures to verify
334+
/// @param guardianSignatures Array of guardian signatures sorted by signer address in ascending order
335335
/// @param digest The digest to verify the signatures against
336336
function _validateGuardianSignatures(
337337
address account,
@@ -357,12 +357,9 @@ contract SocialRecoveryExecutor is IERC7579Module, EIP712, Nonces {
357357
) {
358358
revert InvalidGuardianSignature();
359359
}
360-
// @TBD optimize O(n^2): check for signature duplication
361-
address currentSigner = guardianSignatures[i].signer;
362-
for (uint256 j = 0; j < i; j++) {
363-
if (guardianSignatures[j].signer == currentSigner) {
364-
revert DuplicateGuardianSignature();
365-
}
360+
// ensures signers are unique in O(n), requires sorted signatures by signer address in ascending order
361+
if (i > 0 && uint160(guardianSignatures[i].signer) <= uint160(guardianSignatures[i - 1].signer)) {
362+
revert DuplicatedOrUnsortedGuardianSignatures();
366363
}
367364
}
368365
}

‎test/account/extensions/ERC7579Modules/SocialRecoveryExecutorModule.test.js

Lines changed: 64 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,9 @@ async function fixture() {
2424
const newSigner = ethers.Wallet.createRandom();
2525

2626
// ERC-4337 account
27-
const helper = new ERC4337Helper();
28-
const env = await helper.wait();
29-
const accountMock = await helper.newAccount('$AccountERC7579Mock', [
27+
const erc4337Helper = new ERC4337Helper();
28+
const env = await erc4337Helper.wait();
29+
const accountMock = await erc4337Helper.newAccount('$AccountERC7579Mock', [
3030
'AccountERC7579',
3131
'1',
3232
validatorMock.target,
@@ -55,16 +55,16 @@ async function fixture() {
5555
// impersonate ERC-4337 Canonical Entrypoint
5656
const accountMockFromEntrypoint = accountMock.connect(await impersonate(entrypoint.target));
5757

58+
// ERC-7579 Social Recovery Executor Module
59+
const mock = await ethers.deployContract('$SocialRecoveryExecutor', ['SocialRecoveryExecutor', '0.0.1']);
60+
5861
// ERC-7579 Social Recovery Executor Module Initial Config
5962
const recoveryConfig = {
6063
guardians: new Array(3).fill(null).map(() => ethers.Wallet.createRandom()),
6164
threshold: 2,
6265
timelock: time.duration.days(1),
6366
};
6467

65-
// ERC-7579 Social Recovery Executor Module
66-
const mock = await ethers.deployContract('$SocialRecoveryExecutor', ['SocialRecoveryExecutor', '0.0.1']);
67-
6868
return {
6969
...env,
7070
validatorMock,
@@ -149,14 +149,33 @@ describe('SocialRecoveryExecutorModule', function () {
149149
const message = 'Hello Social Recovery';
150150
const digest = ethers.hashMessage(message);
151151

152-
const guardianSignatures = invalidGuardians.map(g => ({
152+
const guardianSignatures = SocialRecoveryExecutorHelper.sortGuardianSignatures(
153+
invalidGuardians.map(g => ({
154+
signer: g.address,
155+
signature: g.signMessage(message),
156+
})),
157+
);
158+
159+
await expect(
160+
this.mock.validateGuardianSignatures(this.accountMock.target, guardianSignatures, digest),
161+
).to.be.revertedWithCustomError(this.mock, 'InvalidGuardianSignature');
162+
});
163+
164+
it('should invalidate unsorted guardian signatures', async function () {
165+
const guardians = this.recoveryConfig.guardians.slice(0, 2);
166+
const reversedGuardians = guardians.sort().reverse();
167+
168+
const message = 'Hello Social Recovery';
169+
const digest = ethers.hashMessage(message);
170+
171+
const guardianSignatures = reversedGuardians.map(g => ({
153172
signer: g.address,
154173
signature: g.signMessage(message),
155174
}));
156175

157176
await expect(
158177
this.mock.validateGuardianSignatures(this.accountMock.target, guardianSignatures, digest),
159-
).to.be.revertedWithCustomError(this.mock, 'InvalidGuardianSignature');
178+
).to.be.revertedWithCustomError(this.mock, 'DuplicatedOrUnsortedGuardianSignatures');
160179
});
161180

162181
it('should invalidate duplicate guardian signatures', async function () {
@@ -166,25 +185,29 @@ describe('SocialRecoveryExecutorModule', function () {
166185
const message = 'Hello Social Recovery';
167186
const digest = ethers.hashMessage(message);
168187

169-
const guardianSignatures = identicalGuardians.map(g => ({
170-
signer: g.address,
171-
signature: g.signMessage(message),
172-
}));
188+
const guardianSignatures = SocialRecoveryExecutorHelper.sortGuardianSignatures(
189+
identicalGuardians.map(g => ({
190+
signer: g.address,
191+
signature: g.signMessage(message),
192+
})),
193+
);
173194

174195
await expect(
175196
this.mock.validateGuardianSignatures(this.accountMock.target, guardianSignatures, digest),
176-
).to.be.revertedWithCustomError(this.mock, 'DuplicateGuardianSignature');
197+
).to.be.revertedWithCustomError(this.mock, 'DuplicatedOrUnsortedGuardianSignatures');
177198
});
178199

179200
it('should fail if threshold is not met', async function () {
180201
const insufficientGuardians = this.recoveryConfig.guardians.slice(0, this.recoveryConfig.threshold - 1);
181202
const message = 'Hello Social Recovery';
182203
const digest = ethers.hashMessage(message);
183204

184-
const guardianSignatures = insufficientGuardians.map(g => ({
185-
signer: g.address,
186-
signature: g.signMessage(message),
187-
}));
205+
const guardianSignatures = SocialRecoveryExecutorHelper.sortGuardianSignatures(
206+
insufficientGuardians.map(g => ({
207+
signer: g.address,
208+
signature: g.signMessage(message),
209+
})),
210+
);
188211

189212
await expect(
190213
this.mock.validateGuardianSignatures(this.accountMock.target, guardianSignatures, digest),
@@ -196,10 +219,12 @@ describe('SocialRecoveryExecutorModule', function () {
196219
const message = 'Hello Social Recovery';
197220
const digest = ethers.hashMessage(message);
198221

199-
const guardianSignatures = guardians.map(g => ({
200-
signer: g.address,
201-
signature: g.signMessage(message),
202-
}));
222+
const guardianSignatures = SocialRecoveryExecutorHelper.sortGuardianSignatures(
223+
guardians.map(g => ({
224+
signer: g.address,
225+
signature: g.signMessage(message),
226+
})),
227+
);
203228

204229
await expect(this.mock.validateGuardianSignatures(this.accountMock.target, guardianSignatures, digest)).to.not
205230
.be.reverted;
@@ -232,10 +257,12 @@ describe('SocialRecoveryExecutorModule', function () {
232257
executionCalldata: executionCalldata,
233258
};
234259

235-
const guardianSignatures = guardians.map(g => ({
236-
signer: g.address,
237-
signature: g.signTypedData(domain, SocialRecoveryExecutorHelper.START_RECOVERY_TYPEHASH, message),
238-
}));
260+
const guardianSignatures = SocialRecoveryExecutorHelper.sortGuardianSignatures(
261+
guardians.map(g => ({
262+
signer: g.address,
263+
signature: g.signTypedData(domain, SocialRecoveryExecutorHelper.START_RECOVERY_TYPEHASH, message),
264+
})),
265+
);
239266

240267
await expect(this.mock.startRecovery(this.accountMock.target, guardianSignatures, executionCalldata))
241268
.to.emit(this.mock, 'RecoveryStarted')
@@ -353,10 +380,12 @@ describe('SocialRecoveryExecutorModule', function () {
353380
nonce: await this.mock.nonces(this.accountMock.target),
354381
};
355382

356-
const guardianSignatures = guardians.map(g => ({
357-
signer: g.address,
358-
signature: g.signTypedData(domain, SocialRecoveryExecutorHelper.CANCEL_RECOVERY_TYPEHASH, message),
359-
}));
383+
const guardianSignatures = SocialRecoveryExecutorHelper.sortGuardianSignatures(
384+
guardians.map(g => ({
385+
signer: g.address,
386+
signature: g.signTypedData(domain, SocialRecoveryExecutorHelper.CANCEL_RECOVERY_TYPEHASH, message),
387+
})),
388+
);
360389

361390
await expect(this.mock.cancelRecovery(this.accountMock.target, guardianSignatures))
362391
.to.emit(this.mock, 'RecoveryCancelled')
@@ -418,10 +447,12 @@ describe('SocialRecoveryExecutorModule', function () {
418447
executionCalldata: executionCalldata,
419448
};
420449

421-
const guardianSignatures = guardians.map(g => ({
422-
signer: g.address,
423-
signature: g.signTypedData(domain, SocialRecoveryExecutorHelper.START_RECOVERY_TYPEHASH, message),
424-
}));
450+
const guardianSignatures = SocialRecoveryExecutorHelper.sortGuardianSignatures(
451+
guardians.map(g => ({
452+
signer: g.address,
453+
signature: g.signTypedData(domain, SocialRecoveryExecutorHelper.START_RECOVERY_TYPEHASH, message),
454+
})),
455+
);
425456

426457
await expect(
427458
this.mock.startRecovery(this.accountMock.target, guardianSignatures, executionCalldata),

‎test/helpers/erc7579-modules.js

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,22 @@
11
const { Enum } = require('@openzeppelin/contracts/test/helpers/enums');
22
const { formatType } = require('@openzeppelin/contracts/test/helpers/eip712');
33

4-
const SocialRecoveryExecutorHelper = {
5-
RecoveryStatus: Enum('NotStarted', 'Started', 'Ready'),
6-
START_RECOVERY_TYPEHASH: {
4+
class SocialRecoveryExecutorHelper {
5+
static RecoveryStatus = Enum('NotStarted', 'Started', 'Ready');
6+
static START_RECOVERY_TYPEHASH = {
77
StartRecovery: formatType({ account: 'address', executionCalldata: 'bytes', nonce: 'uint256' }),
8-
},
9-
CANCEL_RECOVERY_TYPEHASH: {
8+
};
9+
static CANCEL_RECOVERY_TYPEHASH = {
1010
CancelRecovery: formatType({ account: 'address', nonce: 'uint256' }),
11-
},
12-
};
11+
};
12+
static sortGuardianSignatures(guardianSignatures) {
13+
return guardianSignatures.sort((a, b) => {
14+
if (a.signer < b.signer) return -1;
15+
if (a.signer > b.signer) return 1;
16+
return 0;
17+
});
18+
}
19+
}
1320

1421
module.exports = {
1522
SocialRecoveryExecutorHelper,

0 commit comments

Comments
 (0)