Skip to content

Commit 53122d1

Browse files
rmeissnerrmeissner
authored
Add checkNSignatures method (#298)
1 parent f5983b5 commit 53122d1

File tree

3 files changed

+172
-6
lines changed

3 files changed

+172
-6
lines changed

‎contracts/GnosisSafe.sol‎

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -227,16 +227,32 @@ contract GnosisSafe is
227227
uint256 _threshold = threshold;
228228
// Check that a threshold is set
229229
require(_threshold > 0, "GS001");
230+
checkNSignatures(dataHash, data, signatures, _threshold);
231+
}
232+
233+
/**
234+
* @dev Checks whether the signature provided is valid for the provided data, hash. Will revert otherwise.
235+
* @param dataHash Hash of the data (could be either a message hash or transaction hash)
236+
* @param data That should be signed (this is passed to an external validator contract)
237+
* @param signatures Signature data that should be verified. Can be ECDSA signature, contract signature (EIP-1271) or approved hash.
238+
* @param requiredSignatures Amount of required valid signatures.
239+
*/
240+
function checkNSignatures(
241+
bytes32 dataHash,
242+
bytes memory data,
243+
bytes memory signatures,
244+
uint256 requiredSignatures
245+
) public view {
230246
// Check that the provided signature data is not too short
231-
require(signatures.length >= _threshold.mul(65), "GS020");
247+
require(signatures.length >= requiredSignatures.mul(65), "GS020");
232248
// There cannot be an owner with address 0.
233249
address lastOwner = address(0);
234250
address currentOwner;
235251
uint8 v;
236252
bytes32 r;
237253
bytes32 s;
238254
uint256 i;
239-
for (i = 0; i < _threshold; i++) {
255+
for (i = 0; i < requiredSignatures; i++) {
240256
(v, r, s) = signatureSplit(signatures, i);
241257
if (v == 0) {
242258
// If v is 0 then it is a contract signature
@@ -246,7 +262,7 @@ contract GnosisSafe is
246262
// Check that signature data pointer (s) is not pointing inside the static part of the signatures bytes
247263
// This check is not completely accurate, since it is possible that more signatures than the threshold are send.
248264
// Here we only check that the pointer is not pointing inside the part that is being processed
249-
require(uint256(s) >= _threshold.mul(65), "GS021");
265+
require(uint256(s) >= requiredSignatures.mul(65), "GS021");
250266

251267
// Check that signature data pointer (s) is in bounds (points to the length of data -> 32 bytes)
252268
require(uint256(s).add(32) <= signatures.length, "GS022");

‎contracts/libraries/Migrate_1_3_0_to_1_2_0.sol‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ contract Migration {
2525
bytes32 private _slot3;
2626
bytes32 private _slot4;
2727
bytes32 private _slot5;
28-
// For the migration we need to set the old domain seperator in storage
28+
// For the migration we need to set the old domain separator in storage
2929
bytes32 private domainSeparator;
3030

3131
// Define guard last to avoid conflict with Safe storage layout

‎test/core/GnosisSafe.Signatures.spec.ts‎

Lines changed: 152 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -230,7 +230,7 @@ describe("GnosisSafe", async () => {
230230
).to.be.revertedWith("GS021")
231231
})
232232

233-
it('should fail if sigantures data is not present', async () => {
233+
it('should fail if signatures data is not present', async () => {
234234
const { safe } = await setupTests()
235235
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
236236
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
@@ -243,7 +243,7 @@ describe("GnosisSafe", async () => {
243243
).to.be.revertedWith("GS022")
244244
})
245245

246-
it('should fail if sigantures data is too short', async () => {
246+
it('should fail if signatures data is too short', async () => {
247247
const { safe } = await setupTests()
248248
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
249249
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
@@ -335,4 +335,154 @@ describe("GnosisSafe", async () => {
335335
await safe.checkSignatures(txHash, txHashData, signatures)
336336
})
337337
})
338+
339+
describe("checkSignatures", async () => {
340+
it('should fail if signature points into static part', async () => {
341+
const { safe } = await setupTests()
342+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
343+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
344+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
345+
const signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000020" + "00" + // r, s, v
346+
"0000000000000000000000000000000000000000000000000000000000000000" // Some data to read
347+
await expect(
348+
safe.checkNSignatures(txHash, txHashData, signatures, 1)
349+
).to.be.revertedWith("GS021")
350+
})
351+
352+
it('should fail if signatures data is not present', async () => {
353+
const { safe } = await setupTests()
354+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
355+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
356+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
357+
358+
const signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000041" + "00" // r, s, v
359+
360+
await expect(
361+
safe.checkNSignatures(txHash, txHashData, signatures, 1)
362+
).to.be.revertedWith("GS022")
363+
})
364+
365+
it('should fail if signatures data is too short', async () => {
366+
const { safe } = await setupTests()
367+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
368+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
369+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
370+
371+
const signatures = "0x" + "000000000000000000000000" + user1.address.slice(2) + "0000000000000000000000000000000000000000000000000000000000000041" + "00" + // r, s, v
372+
"0000000000000000000000000000000000000000000000000000000000000020" // length
373+
374+
await expect(
375+
safe.checkNSignatures(txHash, txHashData, signatures, 1)
376+
).to.be.revertedWith("GS023")
377+
})
378+
379+
it('should not be able to use different chainId for signing', async () => {
380+
await setupTests()
381+
const safe = await getSafeWithOwners([user1.address])
382+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
383+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
384+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
385+
const signatures = buildSignatureBytes([await safeSignTypedData(user1, safe, tx, 1)])
386+
await expect(
387+
safe.checkNSignatures(txHash, txHashData, signatures, 1)
388+
).to.be.revertedWith("GS026")
389+
})
390+
391+
it('if not msg.sender on-chain approval is required', async () => {
392+
const { safe } = await setupTests()
393+
const user2Safe = safe.connect(user2)
394+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
395+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
396+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
397+
const signatures = buildSignatureBytes([await safeApproveHash(user1, safe, tx, true)])
398+
await expect(
399+
user2Safe.checkNSignatures(txHash, txHashData, signatures, 1)
400+
).to.be.revertedWith("GS025")
401+
})
402+
403+
it('should revert if not the required amount of signature data is provided', async () => {
404+
await setupTests()
405+
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address])
406+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
407+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
408+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
409+
await expect(
410+
safe.checkNSignatures(txHash, txHashData, "0x", 1)
411+
).to.be.revertedWith("GS020")
412+
})
413+
414+
it('should not be able to use different signature type of same owner', async () => {
415+
await setupTests()
416+
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address])
417+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
418+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
419+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
420+
const signatures = buildSignatureBytes([
421+
await safeApproveHash(user1, safe, tx),
422+
await safeSignTypedData(user1, safe, tx),
423+
await safeSignTypedData(user3, safe, tx)
424+
])
425+
await expect(
426+
safe.checkNSignatures(txHash, txHashData, signatures, 3)
427+
).to.be.revertedWith("GS026")
428+
})
429+
430+
it('should be able to mix all signature types', async () => {
431+
await setupTests()
432+
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address])
433+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
434+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
435+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
436+
const signatures = buildSignatureBytes([
437+
await safeApproveHash(user1, safe, tx, true),
438+
await safeApproveHash(user4, safe, tx),
439+
await safeSignTypedData(user2, safe, tx),
440+
await safeSignTypedData(user3, safe, tx)
441+
])
442+
443+
await safe.checkNSignatures(txHash, txHashData, signatures, 3)
444+
})
445+
446+
it('should be able to require no signatures', async () => {
447+
await setupTests()
448+
const safe = await getSafeTemplate()
449+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
450+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
451+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
452+
453+
await safe.checkNSignatures(txHash, txHashData, "0x", 0)
454+
})
455+
456+
it('should be able to require less signatures than the threshold', async () => {
457+
await setupTests()
458+
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address])
459+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
460+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
461+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
462+
const signatures = buildSignatureBytes([
463+
await safeSignTypedData(user3, safe, tx)
464+
])
465+
466+
await safe.checkNSignatures(txHash, txHashData, signatures, 1)
467+
})
468+
469+
it('should be able to require more signatures than the threshold', async () => {
470+
await setupTests()
471+
const safe = await getSafeWithOwners([user1.address, user2.address, user3.address, user4.address], 2)
472+
const tx = buildSafeTransaction({ to: safe.address, nonce: await safe.nonce() })
473+
const txHashData = preimageSafeTransactionHash(safe, tx, await chainId())
474+
const txHash = calculateSafeTransactionHash(safe, tx, await chainId())
475+
const signatures = buildSignatureBytes([
476+
await safeApproveHash(user1, safe, tx, true),
477+
await safeApproveHash(user4, safe, tx),
478+
await safeSignTypedData(user2, safe, tx)
479+
])
480+
// Should fail as only 3 signaures are provided
481+
await expect(
482+
safe.checkNSignatures(txHash, txHashData, signatures, 4)
483+
).to.be.revertedWith("GS020")
484+
485+
await safe.checkNSignatures(txHash, txHashData, signatures, 3)
486+
})
487+
})
338488
})

0 commit comments

Comments
 (0)