diff --git a/contracts/signature/IERC1271.sol b/contracts/signature/IERC1271.sol new file mode 100644 index 00000000..90e1927b --- /dev/null +++ b/contracts/signature/IERC1271.sol @@ -0,0 +1,19 @@ +// SPDX-License-Identifier: LGPL-3.0-only +/* solhint-disable one-contract-per-file */ +pragma solidity >=0.7.0 <0.9.0; + +interface IERC1271 { + /** + * @notice EIP1271 method to validate a signature. + * @param hash Hash of the data signed on the behalf of address(this). + * @param signature Signature byte array associated with _data. + * + * MUST return the bytes4 magic value 0x1626ba7e when function passes. + * MUST NOT modify state (using STATICCALL for solc < 0.5, view modifier for solc > 0.5) + * MUST allow external calls + */ + function isValidSignature( + bytes32 hash, + bytes memory signature + ) external view returns (bytes4); +} diff --git a/contracts/signature/SignatureChecker.sol b/contracts/signature/SignatureChecker.sol index f1d3d072..bff85941 100644 --- a/contracts/signature/SignatureChecker.sol +++ b/contracts/signature/SignatureChecker.sol @@ -1,6 +1,8 @@ // SPDX-License-Identifier: LGPL-3.0-only 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 supporting eip-712 and eip-1271 signatures abstract contract SignatureChecker { @@ -24,17 +26,29 @@ abstract contract SignatureChecker { * @dev When signature present in calldata, returns the address of the signer. */ function moduleTxSignedBy() internal view returns (address signer) { - if (msg.data.length >= 4 + 32 + 32 + 1) { + bytes calldata data = msg.data; + if (data.length >= 4 + 65) { ( - bytes calldata dataTrimmed, - uint8 v, bytes32 r, - bytes32 s - ) = _splitSignature(msg.data); + bytes32 s, + uint8 v, + bool isContractSignature, + uint256 start, + uint256 end + ) = _splitSignature(data); - bytes32 txHash = _moduleTxHash(dataTrimmed, nonce); + bytes32 hash = _moduleTxHash(data[:start], nonce); - signer = ecrecover(txHash, v, r, s); + if (isContractSignature) { + // When handling contract signatures the address + // of the contract is encoded into r + signer = address(uint160(uint256(r))); + if (!isValidContractSignature(signer, hash, data[start:end])) { + signer = address(0); + } + } else { + signer = ecrecover(hash, v, r, s); + } } } @@ -47,13 +61,22 @@ abstract contract SignatureChecker { ) private pure - returns (bytes calldata dataTrimmed, uint8 v, bytes32 r, bytes32 s) + returns ( + bytes32 r, + bytes32 s, + uint8 v, + bool isEIP1271, + uint256 start, + uint256 end + ) { uint256 length = data.length; - dataTrimmed = data[0:length - 65]; 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; + end = isEIP1271 ? length - 65 : length; } /** @@ -78,6 +101,30 @@ abstract contract SignatureChecker { return keccak256(moduleTxData); } + function isValidContractSignature( + address signer, + bytes32 hash, + bytes calldata signature + ) internal view returns (bool result) { + uint256 size; + assembly { + size := extcodesize(signer) + } + if (size == 0) { + return false; + } + + (bool success, bytes memory returnData) = signer.staticcall( + abi.encodeWithSelector( + IERC1271.isValidSignature.selector, + hash, + signature + ) + ); + + return success == true && bytes4(returnData) == EIP1271_MAGIC_VALUE; + } + // keccak256( // "EIP712Domain(uint256 chainId,address verifyingContract)" // ); @@ -89,4 +136,7 @@ abstract contract SignatureChecker { // ); bytes32 private constant MODULE_TX_TYPEHASH = 0xd6c6b5df57eef4e79cab990a377d29dc4c5bbb016a6293120d53f49c54144227; + + // bytes4(keccak256("isValidSignature(bytes32,bytes)") + bytes4 private constant EIP1271_MAGIC_VALUE = 0x1626ba7e; } diff --git a/contracts/test/TestSignature.sol b/contracts/test/TestSignature.sol index e1afb074..5a76edd9 100644 --- a/contracts/test/TestSignature.sol +++ b/contracts/test/TestSignature.sol @@ -3,6 +3,35 @@ pragma solidity >=0.8.0; import "../signature/SignatureChecker.sol"; +contract ContractSignerYes is IERC1271 { + function isValidSignature( + bytes32, + bytes memory + ) external pure override returns (bytes4) { + return 0x1626ba7e; + } +} + +contract ContractSignerNo is IERC1271 { + function isValidSignature( + bytes32, + bytes memory + ) external pure override returns (bytes4) { + return 0x33333333; + } +} + +contract ContractSignerFaulty {} + +contract ContractSignerReturnSize { + function isValidSignature( + bytes32, + bytes memory + ) external pure returns (bytes2) { + return 0x1626; + } +} + contract TestSignature is SignatureChecker { event Hello(address signer); diff --git a/test/06_Eip712Signature.spec.ts b/test/06_Eip712Signature.spec.ts deleted file mode 100644 index 03411de5..00000000 --- a/test/06_Eip712Signature.spec.ts +++ /dev/null @@ -1,85 +0,0 @@ -import hre from "hardhat"; -import { TestSignature__factory } from "../typechain-types"; -import { loadFixture } from "@nomicfoundation/hardhat-network-helpers"; -import { PopulatedTransaction } from "ethers"; -import { expect } from "chai"; -import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; - -import typedDataForTransaction from "./typesDataForTransaction"; - -describe("EIP712Signature", async () => { - async function setup() { - const [signer, relayer] = await hre.ethers.getSigners(); - const TestSignature = await hre.ethers.getContractFactory("TestSignature"); - const testSignature = await TestSignature.deploy(); - - return { - testSignature: TestSignature__factory.connect( - testSignature.address, - relayer - ), - signer, - relayer, - }; - } - - const AddressZero = "0x0000000000000000000000000000000000000000"; - - it("correctly detects an appended signature, for an entrypoint no arguments", async () => { - const { testSignature, signer, relayer } = await loadFixture(setup); - - const transaction = await testSignature.populateTransaction.hello(); - const signature = await sign(testSignature.address, transaction, signer); - const transactionWithSig = { - ...transaction, - data: `${transaction.data}${signature.slice(2)}`, - }; - - await expect(await relayer.sendTransaction(transaction)) - .to.emit(testSignature, "Hello") - .withArgs(AddressZero); - - await expect(await relayer.sendTransaction(transactionWithSig)) - .to.emit(testSignature, "Hello") - .withArgs(signer.address); - }); - - it("correctly detects an appended signature, entrypoint with arguments", async () => { - const { testSignature, signer, relayer } = await loadFixture(setup); - - const transaction = await testSignature.populateTransaction.goodbye( - 0, - "0xbadfed" - ); - const signature = await sign(testSignature.address, transaction, signer); - 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(signer.address); - }); - - it("it publicly exposes the eip712 nonce", async () => { - const { testSignature } = await loadFixture(setup); - expect(await testSignature.moduleTxNonce()).to.equal(0); - }); -}); - -async function sign( - contract: string, - transaction: PopulatedTransaction, - signer: SignerWithAddress -) { - const { domain, types, message } = typedDataForTransaction( - { contract, chainId: 31337, nonce: 0 }, - transaction.data || "0x" - ); - return await signer._signTypedData(domain, types, message); -} diff --git a/test/06_SignatureChecker.spec.ts b/test/06_SignatureChecker.spec.ts new file mode 100644 index 00000000..9ba46598 --- /dev/null +++ b/test/06_SignatureChecker.spec.ts @@ -0,0 +1,234 @@ +import hre from "hardhat"; +import { TestSignature__factory } from "../typechain-types"; +import { loadFixture } from "@nomicfoundation/hardhat-network-helpers"; +import { PopulatedTransaction } from "ethers"; +import { expect } from "chai"; +import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; + +import typedDataForTransaction from "./typesDataForTransaction"; +import { defaultAbiCoder, solidityPack } from "ethers/lib/utils"; + +describe("SignatureChecker", async () => { + async function setup() { + const [signer, relayer] = await hre.ethers.getSigners(); + const TestSignature = await hre.ethers.getContractFactory("TestSignature"); + const testSignature = await TestSignature.deploy(); + + return { + testSignature: TestSignature__factory.connect( + testSignature.address, + relayer + ), + signer, + relayer, + }; + } + + const AddressZero = "0x0000000000000000000000000000000000000000"; + + it("correctly detects an appended signature, for an entrypoint no arguments", async () => { + const { testSignature, signer, relayer } = await loadFixture(setup); + + const transaction = await testSignature.populateTransaction.hello(); + const signature = await sign(testSignature.address, transaction, signer); + const transactionWithSig = { + ...transaction, + data: `${transaction.data}${signature.slice(2)}`, + }; + + await expect(await relayer.sendTransaction(transaction)) + .to.emit(testSignature, "Hello") + .withArgs(AddressZero); + + await expect(await relayer.sendTransaction(transactionWithSig)) + .to.emit(testSignature, "Hello") + .withArgs(signer.address); + }); + + it("correctly detects an appended signature, entrypoint with arguments", async () => { + const { testSignature, signer, relayer } = await loadFixture(setup); + + const transaction = await testSignature.populateTransaction.goodbye( + 0, + "0xbadfed" + ); + const signature = await sign(testSignature.address, transaction, signer); + 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(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); + }); +}); + +async function sign( + contract: string, + transaction: PopulatedTransaction, + signer: SignerWithAddress +) { + const { domain, types, message } = typedDataForTransaction( + { contract, chainId: 31337, nonce: 0 }, + transaction.data || "0x" + ); + return await signer._signTypedData(domain, types, message); +} + +function makeContractSignature( + contract: string, + transaction: PopulatedTransaction, + signature: string +) { + const dataBytesLength = (transaction.data?.length as number) / 2 - 1; + + const r = defaultAbiCoder.encode(["address"], [contract]); + const s = defaultAbiCoder.encode(["uint256"], [dataBytesLength]); + const v = solidityPack(["uint8"], [0]); + + return `${signature}${r.slice(2)}${s.slice(2)}${v.slice(2)}`; +}