From deef062485b64f26678aa9621f02cf3293f0e242 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Sun, 1 Oct 2023 10:54:53 +0200 Subject: [PATCH] Update comments. Use v,s,r order. Add test cases --- contracts/signature/SignatureChecker.sol | 68 ++-- test/06_SignatureChecker.spec.ts | 412 +++++++++++++++-------- 2 files changed, 312 insertions(+), 168 deletions(-) diff --git a/contracts/signature/SignatureChecker.sol b/contracts/signature/SignatureChecker.sol index 91886fb3..65163cfc 100644 --- a/contracts/signature/SignatureChecker.sol +++ b/contracts/signature/SignatureChecker.sol @@ -3,21 +3,17 @@ pragma solidity >=0.8.0 <0.9.0; import "./IERC1271.sol"; -/// @title SignatureChecker - A contract that extracts and inspects signatures appended to calldata. -/// @notice currently supports eip-712 and eip-1271 +/// @title SignatureChecker - A contract that validates signatures appended to calldata. +/// @dev currently supports eip-712 and eip-1271 abstract contract SignatureChecker { uint256 private nonce; - /** - * @notice Returns the current nonce value. - */ + /// @notice Returns the current nonce value. function moduleTxNonce() public view returns (uint256) { return nonce; } - /** - * @notice Increments nonce. - */ + /// @notice Increments nonce. function moduleTxNonceBump() internal { nonce = nonce + 1; } @@ -31,25 +27,25 @@ abstract contract SignatureChecker { bytes calldata data = msg.data; /* - * The overarching idea is to provide transparent checking to - * modules using `onlyModule` + * The idea is to extend `onlyModule` and provide signature checking + * to modules, without code changes. * * Since it's a generic mechanism we don't have a definitive way to * know if the trailling bytes are a signature. We just slice those - * bytes and recover a signature + * out and recover signer. * * Hence, we require the calldata to be at least as long as a function - * selector plus a signature - 4 + 65. Anything than that is guaranteed - * to not contain a signature + * selector plus a signature - 4 + 65. Anything less than that is + * guaranteed to not contain a signature */ if (data.length < 4 + 65) { return address(0); } ( + uint8 v, bytes32 r, bytes32 s, - uint8 v, bool isContractSignature, uint256 start ) = _splitSignature(data); @@ -57,8 +53,6 @@ abstract contract SignatureChecker { bytes32 txHash = moduleTxHash(data[:start], nonce); if (isContractSignature) { - // When handling contract signatures the address - // of the contract is encoded into r address signer = address(uint160(uint256(r))); return _isValidContractSignature( @@ -74,7 +68,8 @@ abstract contract SignatureChecker { } /** - * @dev Hashes the EIP-712 data structure that will be signed. + * @notice Hashes the EIP-712 data structure. + * @dev The produced hash is intended to be signed. * @param data The current transaction's calldata. * @param _nonce The nonce value. * @return The 32-byte hash that is to be signed. @@ -98,13 +93,24 @@ abstract contract SignatureChecker { /** * @dev Extracts signature from calldata, and divides it into `uint8 v, bytes32 r, bytes32 s`. * @param data The current transaction's calldata. + * @return v The ECDSA v value + * @return r The ECDSA r value + * @return s The ECDSA s value + * @return isEIP1271Signature Indicates whether it is a contract signature + * @return start offset in calldata where signature starts */ function _splitSignature( bytes calldata data ) private pure - returns (bytes32 r, bytes32 s, uint8 v, bool isEIP1271, uint256 start) + returns ( + uint8 v, + bytes32 r, + bytes32 s, + bool isEIP1271Signature, + uint256 start + ) { /* * When handling contract signatures: @@ -113,22 +119,26 @@ abstract contract SignatureChecker { * s - contains the offset within calldata where the signer specific * signature is located * - * We detect contract signatures by the following three conditions: - * 1- v is zero - * 2- s points within te buffer - * 3- the position pointed by S has non zero length + * We detect contract signatures by the following two conditions: + * 1- `v` is zero + * 2- `s` points within the buffer, is after selector and before sig + * + * The data pointed by s should be non zero, as EIP-1271 describes a + * a non-zero length signer specific signature */ - uint256 length = data.length; + v = uint8(bytes1(data[length - 1:])); r = bytes32(data[length - 65:]); s = bytes32(data[length - 33:]); - v = uint8(bytes1(data[length - 1:])); - isEIP1271 = v == 0 && (uint256(s) < (length - 65)); - start = isEIP1271 ? uint256(s) : length - 65; + isEIP1271Signature = + v == 0 && + uint256(s) > 3 && + uint256(s) < (length - 65); + start = isEIP1271Signature ? uint256(s) : length - 65; } /** - * @dev Calls the signer contract, and validates the provided signature. + * @dev Calls the signer contract, and validates signature. * @param signer The address of the signer. * @param txHash The hash of the message that was signed. * @param signature The signature to validate. @@ -147,7 +157,7 @@ abstract contract SignatureChecker { return false; } - (bool success, bytes memory returnData) = signer.staticcall( + (, bytes memory returnData) = signer.staticcall( abi.encodeWithSelector( IERC1271.isValidSignature.selector, txHash, @@ -155,7 +165,7 @@ abstract contract SignatureChecker { ) ); - return success == true && bytes4(returnData) == EIP1271_MAGIC_VALUE; + return bytes4(returnData) == EIP1271_MAGIC_VALUE; } // keccak256( diff --git a/test/06_SignatureChecker.spec.ts b/test/06_SignatureChecker.spec.ts index 9ba46598..5d7c0de3 100644 --- a/test/06_SignatureChecker.spec.ts +++ b/test/06_SignatureChecker.spec.ts @@ -67,144 +67,277 @@ describe("SignatureChecker", async () => { .withArgs(signer.address); }); - it("contract signature, signer returns isValid yes", async () => { - const { testSignature, relayer } = await loadFixture(setup); - - const ContractSigner = await hre.ethers.getContractFactory( - "ContractSignerYes" - ); - const contractSigner = await ContractSigner.deploy(); - - const transaction = await testSignature.populateTransaction.goodbye( - 0, - "0xbadfed" - ); - - const signature = makeContractSignature( - contractSigner.address, - transaction, - "0xaabbccddeeff" - ); - - const transactionWithSig = { - ...transaction, - data: `${transaction.data}${signature.slice(2)}`, - }; - - await expect(await relayer.sendTransaction(transaction)) - .to.emit(testSignature, "Goodbye") - .withArgs(AddressZero); - - await expect(await relayer.sendTransaction(transactionWithSig)) - .to.emit(testSignature, "Goodbye") - .withArgs(contractSigner.address); - }); - - it("contract signature, signer returns isValid no", async () => { - const { testSignature, relayer } = await loadFixture(setup); - - const Signer = await hre.ethers.getContractFactory("ContractSignerNo"); - const signer = await Signer.deploy(); - - const transaction = await testSignature.populateTransaction.hello(); - - const signature = makeContractSignature( - signer.address, - transaction, - "0xaabbccddeeff" - ); - - const transactionWithSig = { - ...transaction, - data: `${transaction.data}${signature.slice(2)}`, - }; - - await expect(await relayer.sendTransaction(transactionWithSig)) - .to.emit(testSignature, "Hello") - .withArgs(AddressZero); - }); - - it("contract signature, bad return size", async () => { - const { testSignature, relayer } = await loadFixture(setup); - - const Signer = await hre.ethers.getContractFactory( - "ContractSignerReturnSize" - ); - const signer = await Signer.deploy(); - - const transaction = await testSignature.populateTransaction.hello(); - - const signature = makeContractSignature( - signer.address, - transaction, - "0xaabbccddeeff" - ); - - const transactionWithSig = { - ...transaction, - data: `${transaction.data}${signature.slice(2)}`, - }; - - await expect(await relayer.sendTransaction(transactionWithSig)) - .to.emit(testSignature, "Hello") - .withArgs(AddressZero); - }); - - it("contract signature, signer with faulty entrypoint", async () => { - const { testSignature, relayer } = await loadFixture(setup); - - const Signer = await hre.ethers.getContractFactory("ContractSignerFaulty"); - const signer = await Signer.deploy(); - - const transaction = await testSignature.populateTransaction.hello(); - - const signature = makeContractSignature( - signer.address, - transaction, - "0xaabbccddeeff" - ); - - const transactionWithSig = { - ...transaction, - data: `${transaction.data}${signature.slice(2)}`, - }; - - await expect(await relayer.sendTransaction(transactionWithSig)) - .to.emit(testSignature, "Hello") - .withArgs(AddressZero); - }); - - it("contract signature, signer with no code deployed", async () => { - const { testSignature, relayer } = await loadFixture(setup); - - const signerAddress = "0x1234567890000000000000000000000123456789"; - - await expect(await hre.ethers.provider.getCode(signerAddress)).to.equal( - "0x" - ); - - const transaction = await testSignature.populateTransaction.hello(); - - const signature = makeContractSignature( - signerAddress, - transaction, - "0xaabbccddeeff" - ); - - const transactionWithSig = { - ...transaction, - data: `${transaction.data}${signature.slice(2)}`, - }; - - await expect(await relayer.sendTransaction(transactionWithSig)) - .to.emit(testSignature, "Hello") - .withArgs(AddressZero); - }); - it("it publicly exposes the eip712 nonce", async () => { const { testSignature } = await loadFixture(setup); expect(await testSignature.moduleTxNonce()).to.equal(0); }); + + describe("contract signature", () => { + it("s pointing out of bounds fails", async () => { + const { testSignature, relayer } = await loadFixture(setup); + + const ContractSigner = await hre.ethers.getContractFactory( + "ContractSignerYes" + ); + const signer = (await ContractSigner.deploy()).address; + + const transaction = await testSignature.populateTransaction.hello(); + + // 4 bytes of selector plus 3 bytes of custom signature + // an s of 4, 5 or 6 should be okay. 7 and higher should fail + let signature = makeContractSignature( + signer, + transaction, + "0xdddddd", + defaultAbiCoder.encode(["uint256"], [1000]) + ); + + await expect( + await relayer.sendTransaction({ + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }) + ) + .to.emit(testSignature, "Hello") + .withArgs(AddressZero); + + signature = makeContractSignature( + signer, + transaction, + "0xdddddd", + defaultAbiCoder.encode(["uint256"], [6]) + ); + + await expect( + await relayer.sendTransaction({ + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }) + ) + .to.emit(testSignature, "Hello") + .withArgs(signer); + }); + it("s pointing to selector fails", async () => { + const { testSignature, relayer } = await loadFixture(setup); + + const ContractSigner = await hre.ethers.getContractFactory( + "ContractSignerYes" + ); + const signer = (await ContractSigner.deploy()).address; + + const transaction = await testSignature.populateTransaction.hello(); + + let signature = makeContractSignature( + signer, + transaction, + "0xdddddd", + defaultAbiCoder.encode(["uint256"], [3]) + ); + + await expect( + await relayer.sendTransaction({ + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }) + ) + .to.emit(testSignature, "Hello") + .withArgs(AddressZero); + + signature = makeContractSignature( + signer, + transaction, + "0xdddddd", + defaultAbiCoder.encode(["uint256"], [4]) + ); + + await expect( + await relayer.sendTransaction({ + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }) + ) + .to.emit(testSignature, "Hello") + .withArgs(signer); + }); + it("s pointing to signature fails", async () => { + const { testSignature, relayer } = await loadFixture(setup); + + const ContractSigner = await hre.ethers.getContractFactory( + "ContractSignerYes" + ); + const signer = (await ContractSigner.deploy()).address; + + const transaction = await testSignature.populateTransaction.hello(); + + let signature = makeContractSignature( + signer, + transaction, + "0xdddddd", + defaultAbiCoder.encode(["uint256"], [60]) + ); + + await expect( + await relayer.sendTransaction({ + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }) + ) + .to.emit(testSignature, "Hello") + .withArgs(AddressZero); + + signature = makeContractSignature( + signer, + transaction, + "0xdddddd", + defaultAbiCoder.encode(["uint256"], [6]) + ); + + await expect( + await relayer.sendTransaction({ + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }) + ) + .to.emit(testSignature, "Hello") + .withArgs(signer); + }); + + it("signer returns isValid yes", async () => { + const { testSignature, relayer } = await loadFixture(setup); + + const ContractSigner = await hre.ethers.getContractFactory( + "ContractSignerYes" + ); + const contractSigner = await ContractSigner.deploy(); + + const transaction = await testSignature.populateTransaction.goodbye( + 0, + "0xbadfed" + ); + + const signature = makeContractSignature( + contractSigner.address, + transaction, + "0xaabbccddeeff" + ); + + const transactionWithSig = { + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }; + + await expect(await relayer.sendTransaction(transaction)) + .to.emit(testSignature, "Goodbye") + .withArgs(AddressZero); + + await expect(await relayer.sendTransaction(transactionWithSig)) + .to.emit(testSignature, "Goodbye") + .withArgs(contractSigner.address); + }); + + it("signer returns isValid no", async () => { + const { testSignature, relayer } = await loadFixture(setup); + + const Signer = await hre.ethers.getContractFactory("ContractSignerNo"); + const signer = await Signer.deploy(); + + const transaction = await testSignature.populateTransaction.hello(); + + const signature = makeContractSignature( + signer.address, + transaction, + "0xaabbccddeeff" + ); + + const transactionWithSig = { + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }; + + await expect(await relayer.sendTransaction(transactionWithSig)) + .to.emit(testSignature, "Hello") + .withArgs(AddressZero); + }); + + it("signer bad return size", async () => { + const { testSignature, relayer } = await loadFixture(setup); + + const Signer = await hre.ethers.getContractFactory( + "ContractSignerReturnSize" + ); + const signer = await Signer.deploy(); + + const transaction = await testSignature.populateTransaction.hello(); + + const signature = makeContractSignature( + signer.address, + transaction, + "0xaabbccddeeff" + ); + + const transactionWithSig = { + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }; + + await expect(await relayer.sendTransaction(transactionWithSig)) + .to.emit(testSignature, "Hello") + .withArgs(AddressZero); + }); + + it("signer with faulty entrypoint", async () => { + const { testSignature, relayer } = await loadFixture(setup); + + const Signer = await hre.ethers.getContractFactory( + "ContractSignerFaulty" + ); + const signer = await Signer.deploy(); + + const transaction = await testSignature.populateTransaction.hello(); + + const signature = makeContractSignature( + signer.address, + transaction, + "0xaabbccddeeff" + ); + + const transactionWithSig = { + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }; + + await expect(await relayer.sendTransaction(transactionWithSig)) + .to.emit(testSignature, "Hello") + .withArgs(AddressZero); + }); + + it("signer with no code deployed", async () => { + const { testSignature, relayer } = await loadFixture(setup); + + const signerAddress = "0x1234567890000000000000000000000123456789"; + + await expect(await hre.ethers.provider.getCode(signerAddress)).to.equal( + "0x" + ); + + const transaction = await testSignature.populateTransaction.hello(); + + const signature = makeContractSignature( + signerAddress, + transaction, + "0xaabbccddeeff" + ); + + const transactionWithSig = { + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }; + + await expect(await relayer.sendTransaction(transactionWithSig)) + .to.emit(testSignature, "Hello") + .withArgs(AddressZero); + }); + }); }); async function sign( @@ -220,15 +353,16 @@ async function sign( } function makeContractSignature( - contract: string, + signer: string, transaction: PopulatedTransaction, - signature: string + signerSpecificSignature: string, + s?: string ) { const dataBytesLength = (transaction.data?.length as number) / 2 - 1; - const r = defaultAbiCoder.encode(["address"], [contract]); - const s = defaultAbiCoder.encode(["uint256"], [dataBytesLength]); + const r = defaultAbiCoder.encode(["address"], [signer]); + s = s || defaultAbiCoder.encode(["uint256"], [dataBytesLength]); const v = solidityPack(["uint8"], [0]); - return `${signature}${r.slice(2)}${s.slice(2)}${v.slice(2)}`; + return `${signerSpecificSignature}${r.slice(2)}${s.slice(2)}${v.slice(2)}`; }