From c74a97c5d085be95d2dc065ca2e764deb19e8b07 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Thu, 12 Oct 2023 11:11:59 +0200 Subject: [PATCH 01/15] Break down Modifier authorization logic in reusable parts --- contracts/core/Modifier.sol | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/contracts/core/Modifier.sol b/contracts/core/Modifier.sol index b050b36c..2b5ce002 100644 --- a/contracts/core/Modifier.sol +++ b/contracts/core/Modifier.sol @@ -69,16 +69,31 @@ abstract contract Modifier is Module, SignatureChecker, IAvatar { */ modifier moduleOnly() { - if (modules[msg.sender] == address(0)) { - if (modules[moduleTxSignedBy()] == address(0)) { - revert NotAuthorized(msg.sender); - } + (bool isAuthorized, address module) = isModuleTxAuthorized(); + if (!isAuthorized) { + revert NotAuthorized(msg.sender); + } + + if (module != msg.sender) { moduleTxNonceBump(); } _; } + function isModuleTxAuthorized() internal returns (bool, address) { + if (modules[msg.sender] != address(0)) { + return (true, msg.sender); + } + + address signer = moduleTxSignedBy(); + if (modules[signer] != address(0)) { + return (true, signer); + } + + return (false, address(0)); + } + /// @dev Disables a module on the modifier. /// @notice This can only be called by the owner. /// @param prevModule Module that pointed to the module to be removed in the linked list. From 6e65220e268d601c7f2e4beb6653f89c5997d9b2 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Thu, 12 Oct 2023 12:05:40 +0200 Subject: [PATCH 02/15] Add failing tests --- contracts/test/TestGuard.sol | 6 +- test/02_Module.spec.ts | 8 +-- test/04_Guard.spec.ts | 4 +- test/07_GuardableModifier.spec.ts | 100 +++++++++++++++++++++++++++--- 4 files changed, 98 insertions(+), 20 deletions(-) diff --git a/contracts/test/TestGuard.sol b/contracts/test/TestGuard.sol index 8dc75d70..c1323045 100644 --- a/contracts/test/TestGuard.sol +++ b/contracts/test/TestGuard.sol @@ -4,7 +4,7 @@ pragma solidity >=0.7.0 <0.9.0; import "../core/GuardableModule.sol"; contract TestGuard is FactoryFriendly, BaseGuard { - event PreChecked(bool checked); + event PreChecked(address sender); event PostChecked(bool checked); address public module; @@ -29,13 +29,13 @@ contract TestGuard is FactoryFriendly, BaseGuard { address, address payable, bytes memory, - address + address sender ) public override { require(to != address(0), "Cannot send to zero address"); require(value != 1337, "Cannot send 1337"); require(bytes3(data) != bytes3(0xbaddad), "Cannot call 0xbaddad"); require(operation != Enum.Operation(1), "No delegate calls"); - emit PreChecked(true); + emit PreChecked(sender); } function checkAfterExecution(bytes32, bool) public override { diff --git a/test/02_Module.spec.ts b/test/02_Module.spec.ts index 61578c48..f2d03b33 100644 --- a/test/02_Module.spec.ts +++ b/test/02_Module.spec.ts @@ -98,9 +98,7 @@ describe("Module", async () => { await module.setGuard(guard.address); await expect( module.executeTransaction(tx.to, tx.value, tx.data, tx.operation) - ) - .to.emit(guard, "PreChecked") - .withArgs(true); + ).to.emit(guard, "PreChecked"); }); it("executes a transaction", async () => { @@ -151,9 +149,7 @@ describe("Module", async () => { tx.data, tx.operation ) - ) - .to.emit(guard, "PreChecked") - .withArgs(true); + ).to.emit(guard, "PreChecked"); }); it("executes a transaction", async () => { diff --git a/test/04_Guard.spec.ts b/test/04_Guard.spec.ts index 68a78a40..af802bff 100644 --- a/test/04_Guard.spec.ts +++ b/test/04_Guard.spec.ts @@ -120,9 +120,7 @@ describe("BaseGuard", async () => { tx.signatures, user1.address ) - ) - .to.emit(guard, "PreChecked") - .withArgs(true); + ).to.emit(guard, "PreChecked"); }); }); diff --git a/test/07_GuardableModifier.spec.ts b/test/07_GuardableModifier.spec.ts index 723b64d3..e3f71cfc 100644 --- a/test/07_GuardableModifier.spec.ts +++ b/test/07_GuardableModifier.spec.ts @@ -7,24 +7,28 @@ import { TestGuard__factory, TestGuardableModifier__factory, } from "../typechain-types"; +import typedDataForTransaction from "./typesDataForTransaction"; +import { PopulatedTransaction } from "ethers"; +import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; describe("GuardableModifier", async () => { async function setupTests() { - const [signer, someone, executor] = await hre.ethers.getSigners(); + const [deployer, executor, signer, someone, relayer] = + await hre.ethers.getSigners(); const Avatar = await hre.ethers.getContractFactory("TestAvatar"); const avatar = TestAvatar__factory.connect( (await Avatar.deploy()).address, - signer + deployer ); const Modifier = await hre.ethers.getContractFactory( "TestGuardableModifier" ); const modifier = TestGuardableModifier__factory.connect( - (await Modifier.connect(signer).deploy(avatar.address, avatar.address)) + (await Modifier.connect(deployer).deploy(avatar.address, avatar.address)) .address, - signer + deployer ); const Guard = await hre.ethers.getContractFactory("TestGuard"); const guard = TestGuard__factory.connect( @@ -36,9 +40,11 @@ describe("GuardableModifier", async () => { await modifier.enableModule(executor.address); return { - avatar, - someone, executor, + signer, + someone, + relayer, + avatar, guard, modifier, }; @@ -67,7 +73,40 @@ describe("GuardableModifier", async () => { .execTransactionFromModule(avatar.address, 0, "0x", 0) ) .to.emit(guard, "PreChecked") - .withArgs(true); + .withArgs(executor.address); + }); + + it("pre-check gets called with signer when transaction is relayed", async () => { + const { signer, modifier, relayer, avatar, guard } = await loadFixture( + setupTests + ); + + await modifier.enableModule(signer.address); + await modifier.setGuard(guard.address); + + const inner = await avatar.populateTransaction.enableModule( + "0xff00000000000000000000000000000000ff3456" + ); + + const { from, ...transaction } = + await modifier.populateTransaction.execTransactionFromModule( + avatar.address, + 0, + inner.data as string, + 0 + ); + + const signature = await sign(modifier.address, transaction, signer); + const transactionWithSig = { + ...transaction, + to: modifier.address, + data: `${transaction.data}${signature.slice(2)}`, + value: 0, + }; + + await expect(await relayer.sendTransaction(transactionWithSig)) + .to.emit(guard, "PreChecked") + .withArgs(signer.address); }); it("pre-checks and reverts transaction if guard is set", async () => { @@ -134,7 +173,40 @@ describe("GuardableModifier", async () => { .execTransactionFromModuleReturnData(avatar.address, 0, "0x", 0) ) .to.emit(guard, "PreChecked") - .withArgs(true); + .withArgs(executor.address); + }); + + it("pre-check gets called with signer when transaction is relayed", async () => { + const { signer, modifier, relayer, avatar, guard } = await loadFixture( + setupTests + ); + + await modifier.enableModule(signer.address); + await modifier.setGuard(guard.address); + + const inner = await avatar.populateTransaction.enableModule( + "0xff00000000000000000000000000000000ff3456" + ); + + const { from, ...transaction } = + await modifier.populateTransaction.execTransactionFromModuleReturnData( + avatar.address, + 0, + inner.data as string, + 0 + ); + + const signature = await sign(modifier.address, transaction, signer); + const transactionWithSig = { + ...transaction, + to: modifier.address, + data: `${transaction.data}${signature.slice(2)}`, + value: 0, + }; + + await expect(await relayer.sendTransaction(transactionWithSig)) + .to.emit(guard, "PreChecked") + .withArgs(signer.address); }); it("pre-checks and reverts transaction if guard is set", async () => { @@ -178,3 +250,15 @@ describe("GuardableModifier", async () => { }); }); }); + +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); +} From c08684876ae755bac5d94b367fc7faf4b445237e Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Thu, 12 Oct 2023 13:18:51 +0200 Subject: [PATCH 03/15] Changing the base SignatureChecker to work with user proposed salt, instead of self managed nonce --- contracts/core/GuardableModifier.sol | 4 +- contracts/core/Modifier.sol | 19 ++-- contracts/signature/SignatureChecker.sol | 49 ++++------ contracts/test/TestSignature.sol | 6 +- test/03_Modifier.spec.ts | 2 +- test/06_SignatureChecker.spec.ts | 97 ++++++++++++------- test/07_GuardableModifier.spec.ts | 2 +- ...nsaction.ts => typedDataForTransaction.ts} | 8 +- 8 files changed, 100 insertions(+), 87 deletions(-) rename test/{typesDataForTransaction.ts => typedDataForTransaction.ts} (82%) diff --git a/contracts/core/GuardableModifier.sol b/contracts/core/GuardableModifier.sol index 60df8762..95ae2959 100644 --- a/contracts/core/GuardableModifier.sol +++ b/contracts/core/GuardableModifier.sol @@ -32,7 +32,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier { address(0), payable(0), "", - msg.sender + authorizer() ); } success = IAvatar(target).execTransactionFromModule( @@ -78,7 +78,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier { address(0), payable(0), "", - msg.sender + authorizer() ); } diff --git a/contracts/core/Modifier.sol b/contracts/core/Modifier.sol index 2b5ce002..a0b070cd 100644 --- a/contracts/core/Modifier.sol +++ b/contracts/core/Modifier.sol @@ -69,29 +69,26 @@ abstract contract Modifier is Module, SignatureChecker, IAvatar { */ modifier moduleOnly() { - (bool isAuthorized, address module) = isModuleTxAuthorized(); - if (!isAuthorized) { - revert NotAuthorized(msg.sender); - } + address module = authorizer(); - if (module != msg.sender) { - moduleTxNonceBump(); + if (module == address(0)) { + revert NotAuthorized(msg.sender); } _; } - function isModuleTxAuthorized() internal returns (bool, address) { + function authorizer() internal returns (address) { if (modules[msg.sender] != address(0)) { - return (true, msg.sender); + return msg.sender; } - address signer = moduleTxSignedBy(); + (, address signer) = moduleTxSignedBy(); if (modules[signer] != address(0)) { - return (true, signer); + return signer; } - return (false, address(0)); + return address(0); } /// @dev Disables a module on the modifier. diff --git a/contracts/signature/SignatureChecker.sol b/contracts/signature/SignatureChecker.sol index 292c244e..5d9c2b5d 100644 --- a/contracts/signature/SignatureChecker.sol +++ b/contracts/signature/SignatureChecker.sol @@ -6,24 +6,12 @@ import "./IERC1271.sol"; /// @title SignatureChecker - A contract that retrieves and validates signatures appended to transaction calldata. /// @dev currently supports eip-712 and eip-1271 signatures abstract contract SignatureChecker { - uint256 private nonce; - - /// @notice Returns the current nonce. - function moduleTxNonce() public view returns (uint256) { - return nonce; - } - - /// @notice Increments nonce. - function moduleTxNonceBump() internal { - nonce = nonce + 1; - } - /** * @notice Searches for a signature, validates it, and returns the signer's address. * @dev When signature not found or invalid, zero address is returned * @return The address of the signer. */ - function moduleTxSignedBy() internal view returns (address) { + function moduleTxSignedBy() internal view returns (bytes32, address) { bytes calldata data = msg.data; /* @@ -35,11 +23,11 @@ abstract contract SignatureChecker { * and recover signer. * * As a result, we impose a minimum calldata length equal to a function - * selector plus a signature (i.e., 4 + 65 bytes), any shorter and - * calldata it guaranteed to not contain a signature. + * selector plus salt, plus a signature (i.e., 4 + 32 + 65 bytes), any + * shorter and calldata it guaranteed to not contain a signature. */ - if (data.length < 4 + 65) { - return address(0); + if (data.length < 4 + 32 + 65) { + return (bytes32(0), address(0)); } ( @@ -50,7 +38,10 @@ abstract contract SignatureChecker { uint256 start ) = _splitSignature(data); - bytes32 hash = moduleTxHash(data[:start], nonce); + bytes32 hash = moduleTxHash( + data[:start], // slice the appended signature out + bytes32(data[data.length - 65 - 32:]) // get the salt + ); if (isContractSignature) { address signer = address(uint160(uint256(r))); @@ -60,10 +51,10 @@ abstract contract SignatureChecker { hash, data[start:data.length - 65] ) - ? signer - : address(0); + ? (hash, signer) + : (bytes32(0), address(0)); } else { - return ecrecover(hash, v, r, s); + return (hash, ecrecover(hash, v, r, s)); } } @@ -71,12 +62,12 @@ abstract contract SignatureChecker { * @notice Hashes the transaction 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. + * @param salt The salt value. * @return The 32-byte hash that is to be signed. */ function moduleTxHash( bytes calldata data, - uint256 _nonce + bytes32 salt ) public view returns (bytes32) { bytes32 domainSeparator = keccak256( abi.encode(DOMAIN_SEPARATOR_TYPEHASH, block.chainid, this) @@ -85,7 +76,7 @@ abstract contract SignatureChecker { bytes1(0x19), bytes1(0x01), domainSeparator, - keccak256(abi.encode(MODULE_TX_TYPEHASH, keccak256(data), _nonce)) + keccak256(abi.encode(MODULE_TX_TYPEHASH, keccak256(data), salt)) ); return keccak256(moduleTxData); } @@ -122,7 +113,7 @@ abstract contract SignatureChecker { * We detect contract signatures by checking: * 1- `v` is zero * 2- `s` points within the buffer, is after selector, is before - * signature and delimits a non-zero length buffer + * salt and delimits a non-zero length buffer */ uint256 length = data.length; v = uint8(bytes1(data[length - 1:])); @@ -131,8 +122,8 @@ abstract contract SignatureChecker { isEIP1271Signature = v == 0 && uint256(s) > 3 && - uint256(s) < (length - 65); - start = isEIP1271Signature ? uint256(s) : length - 65; + uint256(s) < (length - 65 - 32); + start = isEIP1271Signature ? uint256(s) : length - 65 - 32; } /** @@ -173,10 +164,10 @@ abstract contract SignatureChecker { 0x47e79534a245952e8b16893a336b85a3d9ea9fa8c573f3d803afb92a79469218; // keccak256( - // "ModuleTx(bytes data,uint256 nonce)" + // "ModuleTx(bytes data,bytes32 salt)" // ); bytes32 private constant MODULE_TX_TYPEHASH = - 0xd6c6b5df57eef4e79cab990a377d29dc4c5bbb016a6293120d53f49c54144227; + 0x2939aeeda3ca260200c9f7b436b19e13207547ccc65cfedc857751c5ea6d91d4; // bytes4(keccak256( // "isValidSignature(bytes32,bytes)" diff --git a/contracts/test/TestSignature.sol b/contracts/test/TestSignature.sol index 5a76edd9..7242e6d4 100644 --- a/contracts/test/TestSignature.sol +++ b/contracts/test/TestSignature.sol @@ -38,10 +38,12 @@ contract TestSignature is SignatureChecker { event Goodbye(address signer); function hello() public { - emit Hello(moduleTxSignedBy()); + (, address signer) = moduleTxSignedBy(); + emit Hello(signer); } function goodbye(uint256, bytes memory) public { - emit Goodbye(moduleTxSignedBy()); + (, address signer) = moduleTxSignedBy(); + emit Goodbye(signer); } } diff --git a/test/03_Modifier.spec.ts b/test/03_Modifier.spec.ts index 6eb0021e..ce1bf614 100644 --- a/test/03_Modifier.spec.ts +++ b/test/03_Modifier.spec.ts @@ -4,7 +4,7 @@ import { loadFixture } from "@nomicfoundation/hardhat-network-helpers"; import { expect } from "chai"; import { PopulatedTransaction } from "ethers"; import hre from "hardhat"; -import typedDataForTransaction from "./typesDataForTransaction"; +import typedDataForTransaction from "./typedDataForTransaction"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; import { TestAvatar__factory, TestModifier__factory } from "../typechain-types"; diff --git a/test/06_SignatureChecker.spec.ts b/test/06_SignatureChecker.spec.ts index 5d7c0de3..3acc4869 100644 --- a/test/06_SignatureChecker.spec.ts +++ b/test/06_SignatureChecker.spec.ts @@ -5,10 +5,15 @@ 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 () => { +import typedDataForTransaction from "./typedDataForTransaction"; +import { + defaultAbiCoder, + keccak256, + solidityPack, + toUtf8Bytes, +} from "ethers/lib/utils"; + +describe.only("SignatureChecker", async () => { async function setup() { const [signer, relayer] = await hre.ethers.getSigners(); const TestSignature = await hre.ethers.getContractFactory("TestSignature"); @@ -30,7 +35,12 @@ describe("SignatureChecker", async () => { const { testSignature, signer, relayer } = await loadFixture(setup); const transaction = await testSignature.populateTransaction.hello(); - const signature = await sign(testSignature.address, transaction, signer); + const signature = await sign( + testSignature.address, + transaction, + keccak256(toUtf8Bytes("Hello this is a salt")), + signer + ); const transactionWithSig = { ...transaction, data: `${transaction.data}${signature.slice(2)}`, @@ -52,7 +62,12 @@ describe("SignatureChecker", async () => { 0, "0xbadfed" ); - const signature = await sign(testSignature.address, transaction, signer); + const signature = await sign( + testSignature.address, + transaction, + keccak256(toUtf8Bytes("salt")), + signer + ); const transactionWithSig = { ...transaction, data: `${transaction.data}${signature.slice(2)}`, @@ -67,11 +82,6 @@ describe("SignatureChecker", async () => { .withArgs(signer.address); }); - 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); @@ -86,9 +96,10 @@ describe("SignatureChecker", async () => { // 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", + keccak256(toUtf8Bytes("salt")), + signer, defaultAbiCoder.encode(["uint256"], [1000]) ); @@ -102,9 +113,10 @@ describe("SignatureChecker", async () => { .withArgs(AddressZero); signature = makeContractSignature( - signer, transaction, "0xdddddd", + keccak256(toUtf8Bytes("salt")), + signer, defaultAbiCoder.encode(["uint256"], [6]) ); @@ -128,9 +140,10 @@ describe("SignatureChecker", async () => { const transaction = await testSignature.populateTransaction.hello(); let signature = makeContractSignature( - signer, transaction, "0xdddddd", + keccak256(toUtf8Bytes("salt")), + signer, defaultAbiCoder.encode(["uint256"], [3]) ); @@ -144,9 +157,10 @@ describe("SignatureChecker", async () => { .withArgs(AddressZero); signature = makeContractSignature( - signer, transaction, "0xdddddd", + keccak256(toUtf8Bytes("salt")), + signer, defaultAbiCoder.encode(["uint256"], [4]) ); @@ -170,9 +184,10 @@ describe("SignatureChecker", async () => { const transaction = await testSignature.populateTransaction.hello(); let signature = makeContractSignature( - signer, transaction, "0xdddddd", + keccak256(toUtf8Bytes("salt")), + signer, defaultAbiCoder.encode(["uint256"], [60]) ); @@ -186,9 +201,10 @@ describe("SignatureChecker", async () => { .withArgs(AddressZero); signature = makeContractSignature( - signer, transaction, "0xdddddd", + keccak256(toUtf8Bytes("salt")), + signer, defaultAbiCoder.encode(["uint256"], [6]) ); @@ -201,7 +217,6 @@ describe("SignatureChecker", async () => { .to.emit(testSignature, "Hello") .withArgs(signer); }); - it("signer returns isValid yes", async () => { const { testSignature, relayer } = await loadFixture(setup); @@ -216,9 +231,10 @@ describe("SignatureChecker", async () => { ); const signature = makeContractSignature( - contractSigner.address, transaction, - "0xaabbccddeeff" + "0xaabbccddeeff", + keccak256(toUtf8Bytes("salt")), + contractSigner.address ); const transactionWithSig = { @@ -234,7 +250,6 @@ describe("SignatureChecker", async () => { .to.emit(testSignature, "Goodbye") .withArgs(contractSigner.address); }); - it("signer returns isValid no", async () => { const { testSignature, relayer } = await loadFixture(setup); @@ -244,9 +259,10 @@ describe("SignatureChecker", async () => { const transaction = await testSignature.populateTransaction.hello(); const signature = makeContractSignature( - signer.address, transaction, - "0xaabbccddeeff" + "0xaabbccddeeff", + keccak256(toUtf8Bytes("salt")), + signer.address ); const transactionWithSig = { @@ -258,7 +274,6 @@ describe("SignatureChecker", async () => { .to.emit(testSignature, "Hello") .withArgs(AddressZero); }); - it("signer bad return size", async () => { const { testSignature, relayer } = await loadFixture(setup); @@ -270,9 +285,10 @@ describe("SignatureChecker", async () => { const transaction = await testSignature.populateTransaction.hello(); const signature = makeContractSignature( - signer.address, transaction, - "0xaabbccddeeff" + "0xaabbccddeeff", + keccak256(toUtf8Bytes("salt")), + signer.address ); const transactionWithSig = { @@ -284,7 +300,6 @@ describe("SignatureChecker", async () => { .to.emit(testSignature, "Hello") .withArgs(AddressZero); }); - it("signer with faulty entrypoint", async () => { const { testSignature, relayer } = await loadFixture(setup); @@ -296,9 +311,10 @@ describe("SignatureChecker", async () => { const transaction = await testSignature.populateTransaction.hello(); const signature = makeContractSignature( - signer.address, transaction, - "0xaabbccddeeff" + "0xaabbccddeeff", + keccak256(toUtf8Bytes("salt")), + signer.address ); const transactionWithSig = { @@ -310,7 +326,6 @@ describe("SignatureChecker", async () => { .to.emit(testSignature, "Hello") .withArgs(AddressZero); }); - it("signer with no code deployed", async () => { const { testSignature, relayer } = await loadFixture(setup); @@ -323,9 +338,10 @@ describe("SignatureChecker", async () => { const transaction = await testSignature.populateTransaction.hello(); const signature = makeContractSignature( - signerAddress, transaction, - "0xaabbccddeeff" + "0xaabbccddeeff", + keccak256(toUtf8Bytes("salt")), + signerAddress ); const transactionWithSig = { @@ -343,26 +359,33 @@ describe("SignatureChecker", async () => { async function sign( contract: string, transaction: PopulatedTransaction, + salt: string, signer: SignerWithAddress ) { const { domain, types, message } = typedDataForTransaction( - { contract, chainId: 31337, nonce: 0 }, + { contract, chainId: 31337, salt }, transaction.data || "0x" ); - return await signer._signTypedData(domain, types, message); + + const signature = await signer._signTypedData(domain, types, message); + + return `${salt}${signature.slice(2)}`; } function makeContractSignature( - signer: string, transaction: PopulatedTransaction, signerSpecificSignature: string, + salt: string, + r: string, s?: string ) { const dataBytesLength = (transaction.data?.length as number) / 2 - 1; - const r = defaultAbiCoder.encode(["address"], [signer]); + r = defaultAbiCoder.encode(["address"], [r]); s = s || defaultAbiCoder.encode(["uint256"], [dataBytesLength]); const v = solidityPack(["uint8"], [0]); - return `${signerSpecificSignature}${r.slice(2)}${s.slice(2)}${v.slice(2)}`; + return `${signerSpecificSignature}${salt.slice(2)}${r.slice(2)}${s.slice( + 2 + )}${v.slice(2)}`; } diff --git a/test/07_GuardableModifier.spec.ts b/test/07_GuardableModifier.spec.ts index e3f71cfc..6dbe3aeb 100644 --- a/test/07_GuardableModifier.spec.ts +++ b/test/07_GuardableModifier.spec.ts @@ -7,7 +7,7 @@ import { TestGuard__factory, TestGuardableModifier__factory, } from "../typechain-types"; -import typedDataForTransaction from "./typesDataForTransaction"; +import typedDataForTransaction from "./typedDataForTransaction"; import { PopulatedTransaction } from "ethers"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; diff --git a/test/typesDataForTransaction.ts b/test/typedDataForTransaction.ts similarity index 82% rename from test/typesDataForTransaction.ts rename to test/typedDataForTransaction.ts index 351bd952..81f4d175 100644 --- a/test/typesDataForTransaction.ts +++ b/test/typedDataForTransaction.ts @@ -4,11 +4,11 @@ export default function typedDataForTransaction( { contract, chainId, - nonce, + salt, }: { contract: string; chainId: BigNumberish; - nonce: BigNumberish; + salt: string; }, data: string ) { @@ -16,12 +16,12 @@ export default function typedDataForTransaction( const types = { ModuleTx: [ { type: "bytes", name: "data" }, - { type: "uint256", name: "nonce" }, + { type: "bytes32", name: "salt" }, ], }; const message = { data, - nonce, + salt, }; return { domain, types, message }; From 13b826e221a11318b34d583565c3e656d048b7b5 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Thu, 12 Oct 2023 14:09:22 +0200 Subject: [PATCH 04/15] Introduce execution tracking into Modifier.sol based on executed and invalidated hashes --- contracts/core/GuardableModifier.sol | 13 +- contracts/core/Modifier.sol | 41 ++-- contracts/signature/ExecutionTracker.sol | 12 ++ contracts/test/TestGuardableModifier.sol | 8 +- contracts/test/TestModifier.sol | 8 +- test/03_Modifier.spec.ts | 252 ++++++++++++++++++----- test/06_SignatureChecker.spec.ts | 2 +- test/07_GuardableModifier.spec.ts | 23 ++- 8 files changed, 283 insertions(+), 76 deletions(-) create mode 100644 contracts/signature/ExecutionTracker.sol diff --git a/contracts/core/GuardableModifier.sol b/contracts/core/GuardableModifier.sol index 95ae2959..1203ac75 100644 --- a/contracts/core/GuardableModifier.sol +++ b/contracts/core/GuardableModifier.sol @@ -32,7 +32,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier { address(0), payable(0), "", - authorizer() + _authorizer() ); } success = IAvatar(target).execTransactionFromModule( @@ -78,7 +78,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier { address(0), payable(0), "", - authorizer() + _authorizer() ); } @@ -89,4 +89,13 @@ abstract contract GuardableModifier is Module, Guardable, Modifier { IGuard(currentGuard).checkAfterExecution(bytes32(0), success); } } + + function _authorizer() private returns (address) { + if (modules[msg.sender] != address(0)) { + return msg.sender; + } + + (, address signer) = moduleTxSignedBy(); + return signer; + } } diff --git a/contracts/core/Modifier.sol b/contracts/core/Modifier.sol index a0b070cd..af96b549 100644 --- a/contracts/core/Modifier.sol +++ b/contracts/core/Modifier.sol @@ -2,11 +2,17 @@ pragma solidity >=0.7.0 <0.9.0; import "../interfaces/IAvatar.sol"; +import "../signature/ExecutionTracker.sol"; import "../signature/SignatureChecker.sol"; import "./Module.sol"; /// @title Modifier Interface - A contract that sits between a Module and an Avatar and enforce some additional logic. -abstract contract Modifier is Module, SignatureChecker, IAvatar { +abstract contract Modifier is + Module, + ExecutionTracker, + SignatureChecker, + IAvatar +{ address internal constant SENTINEL_MODULES = address(0x1); /// Mapping of modules. mapping(address => address) internal modules; @@ -15,6 +21,12 @@ abstract contract Modifier is Module, SignatureChecker, IAvatar { /// @param sender The address of the sender. error NotAuthorized(address sender); + /// @param hash already executed. + error HashAlreadyExecuted(bytes32 hash); + + /// @param hash already executed. + error HashInvalidated(bytes32 hash); + /// `module` is invalid. error InvalidModule(address module); @@ -69,26 +81,25 @@ abstract contract Modifier is Module, SignatureChecker, IAvatar { */ modifier moduleOnly() { - address module = authorizer(); + if (modules[msg.sender] == address(0)) { + (bytes32 hash, address signer) = moduleTxSignedBy(); - if (module == address(0)) { - revert NotAuthorized(msg.sender); - } + if (modules[signer] == address(0)) { + revert NotAuthorized(msg.sender); + } - _; - } + if (executed[signer][hash]) { + revert HashAlreadyExecuted(hash); + } - function authorizer() internal returns (address) { - if (modules[msg.sender] != address(0)) { - return msg.sender; - } + if (invalidated[signer][hash]) { + revert HashInvalidated(hash); + } - (, address signer) = moduleTxSignedBy(); - if (modules[signer] != address(0)) { - return signer; + executed[signer][hash] = true; } - return address(0); + _; } /// @dev Disables a module on the modifier. diff --git a/contracts/signature/ExecutionTracker.sol b/contracts/signature/ExecutionTracker.sol new file mode 100644 index 00000000..b9f21a0c --- /dev/null +++ b/contracts/signature/ExecutionTracker.sol @@ -0,0 +1,12 @@ +// SPDX-License-Identifier: LGPL-3.0-only +pragma solidity >=0.8.0 <0.9.0; + +/// @title ExecutionTracker - A contract that keeps track of executed and invalidated hashes +contract ExecutionTracker { + mapping(address => mapping(bytes32 => bool)) public executed; + mapping(address => mapping(bytes32 => bool)) public invalidated; + + function invalidate(bytes32 hash) external { + invalidated[msg.sender][hash] = true; + } +} diff --git a/contracts/test/TestGuardableModifier.sol b/contracts/test/TestGuardableModifier.sol index 6e38457c..4b2e02c2 100644 --- a/contracts/test/TestGuardableModifier.sol +++ b/contracts/test/TestGuardableModifier.sol @@ -4,7 +4,7 @@ pragma solidity >=0.7.0 <0.9.0; import "../core/GuardableModifier.sol"; contract TestGuardableModifier is GuardableModifier { - event executed( + event Executed( address to, uint256 value, bytes data, @@ -12,7 +12,7 @@ contract TestGuardableModifier is GuardableModifier { bool success ); - event executedAndReturnedData( + event ExecutedAndReturnedData( address to, uint256 value, bytes data, @@ -39,7 +39,7 @@ contract TestGuardableModifier is GuardableModifier { Enum.Operation operation ) public override moduleOnly returns (bool success) { success = exec(to, value, data, operation); - emit executed(to, value, data, operation, success); + emit Executed(to, value, data, operation, success); } /// @dev Passes a transaction to the modifier, expects return data. @@ -60,7 +60,7 @@ contract TestGuardableModifier is GuardableModifier { returns (bool success, bytes memory returnData) { (success, returnData) = execAndReturnData(to, value, data, operation); - emit executedAndReturnedData( + emit ExecutedAndReturnedData( to, value, data, diff --git a/contracts/test/TestModifier.sol b/contracts/test/TestModifier.sol index 8629d8a5..8746e78e 100644 --- a/contracts/test/TestModifier.sol +++ b/contracts/test/TestModifier.sol @@ -6,7 +6,7 @@ pragma solidity >=0.7.0 <0.9.0; import "../core/Modifier.sol"; contract TestModifier is Modifier { - event executed( + event Executed( address to, uint256 value, bytes data, @@ -14,7 +14,7 @@ contract TestModifier is Modifier { bool success ); - event executedAndReturnedData( + event ExecutedAndReturnedData( address to, uint256 value, bytes data, @@ -41,7 +41,7 @@ contract TestModifier is Modifier { Enum.Operation operation ) public override moduleOnly returns (bool success) { success = exec(to, value, data, operation); - emit executed(to, value, data, operation, success); + emit Executed(to, value, data, operation, success); } /// @dev Passes a transaction to the modifier, expects return data. @@ -62,7 +62,7 @@ contract TestModifier is Modifier { returns (bool success, bytes memory returnData) { (success, returnData) = execAndReturnData(to, value, data, operation); - emit executedAndReturnedData( + emit ExecutedAndReturnedData( to, value, data, diff --git a/test/03_Modifier.spec.ts b/test/03_Modifier.spec.ts index ce1bf614..c5e31e4f 100644 --- a/test/03_Modifier.spec.ts +++ b/test/03_Modifier.spec.ts @@ -1,12 +1,15 @@ +import hre from "hardhat"; +import { expect } from "chai"; +import { PopulatedTransaction } from "ethers"; + import { AddressZero } from "@ethersproject/constants"; import { AddressOne } from "@gnosis.pm/safe-contracts"; import { loadFixture } from "@nomicfoundation/hardhat-network-helpers"; -import { expect } from "chai"; -import { PopulatedTransaction } from "ethers"; -import hre from "hardhat"; -import typedDataForTransaction from "./typedDataForTransaction"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; + +import typedDataForTransaction from "./typedDataForTransaction"; import { TestAvatar__factory, TestModifier__factory } from "../typechain-types"; +import { keccak256, toUtf8Bytes } from "ethers/lib/utils"; describe("Modifier", async () => { const SENTINEL_MODULES = "0x0000000000000000000000000000000000000001"; @@ -316,7 +319,7 @@ describe("Modifier", async () => { tx.data, tx.operation ) - ).to.emit(modifier, "executed"); + ).to.emit(modifier, "Executed"); }); it("execute a transaction with signature.", async () => { const { modifier, tx } = await loadFixture(setupTests); @@ -333,7 +336,12 @@ describe("Modifier", async () => { tx.operation ); - const signature = await sign(modifier.address, transaction, user1); + const signature = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + user1 + ); const transactionWithSig = { ...transaction, @@ -346,7 +354,7 @@ describe("Modifier", async () => { await expect(relayer.sendTransaction(transactionWithSig)).to.emit( modifier, - "executed" + "Executed" ); }); it("reverts if signature not valid.", async () => { @@ -364,8 +372,18 @@ describe("Modifier", async () => { tx.operation ); - const signatureOk = await sign(modifier.address, transaction, user1); - const signatureBad = await sign(modifier.address, transaction, user2); + const signatureOk = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + user1 + ); + const signatureBad = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + user2 + ); const transactionWithBadSig = { ...transaction, @@ -383,38 +401,65 @@ describe("Modifier", async () => { await expect(relayer.sendTransaction(transactionWithOkSig)).to.emit( modifier, - "executed" + "Executed" ); }); - it("execute a transaction via sender nonce stays same.", async () => { + it("reverts if signature previously used for execution.", async () => { const { modifier, tx } = await loadFixture(setupTests); - const [user1] = await hre.ethers.getSigners(); + const [user1, user2, relayer] = await hre.ethers.getSigners(); await expect(await modifier.enableModule(user1.address)) .to.emit(modifier, "EnabledModule") .withArgs(user1.address); - await expect( - modifier.execTransactionFromModule( + const { from, ...transaction } = + await modifier.populateTransaction.execTransactionFromModule( tx.to, tx.value, tx.data, tx.operation - ) - ).to.emit(modifier, "executed"); + ); - expect( - await TestModifier__factory.connect( - modifier.address, - hre.ethers.provider - ).moduleTxNonce() - ).to.equal(0); + const signatureOk = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + user1 + ); + const signatureBad = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + user2 + ); + + const transactionWithBadSig = { + ...transaction, + data: `${transaction.data}${signatureBad.slice(2)}`, + }; + + const transactionWithOkSig = { + ...transaction, + data: `${transaction.data}${signatureOk.slice(2)}`, + }; + + await expect( + relayer.sendTransaction(transactionWithBadSig) + ).to.be.revertedWithCustomError(modifier, "NotAuthorized"); + + await expect(relayer.sendTransaction(transactionWithOkSig)).to.emit( + modifier, + "Executed" + ); + + await expect( + relayer.sendTransaction(transactionWithOkSig) + ).to.be.revertedWithCustomError(modifier, "HashAlreadyExecuted"); }); - it("execute a transaction with signature nonce is incremented.", async () => { + it("reverts if signature invalidated.", async () => { const { modifier, tx } = await loadFixture(setupTests); - const [user1, user2, relayer] = await hre.ethers.getSigners(); - await expect(await modifier.enableModule(user1.address)) - .to.emit(modifier, "EnabledModule") - .withArgs(user1.address); + const [user1, relayer] = await hre.ethers.getSigners(); + + await modifier.enableModule(user1.address); const { from, ...transaction } = await modifier.populateTransaction.execTransactionFromModule( @@ -424,24 +469,30 @@ describe("Modifier", async () => { tx.operation ); - const signature = await sign(modifier.address, transaction, user1); + const salt = keccak256(toUtf8Bytes("salt")); + + const signatureOk = await sign( + modifier.address, + transaction, + salt, + user1 + ); const transactionWithSig = { ...transaction, - data: `${transaction.data}${signature.slice(2)}`, + data: `${transaction.data}${signatureOk.slice(2)}`, }; - await expect(relayer.sendTransaction(transactionWithSig)).to.emit( - modifier, - "executed" + const hash = await modifier.moduleTxHash( + transaction.data as string, + salt ); - expect( - await TestModifier__factory.connect( - modifier.address, - hre.ethers.provider - ).moduleTxNonce() - ).to.equal(1); + await modifier.invalidate(hash); + + await expect( + relayer.sendTransaction(transactionWithSig) + ).to.be.revertedWithCustomError(modifier, "HashInvalidated"); }); }); @@ -473,7 +524,7 @@ describe("Modifier", async () => { tx.data, tx.operation ) - ).to.emit(modifier, "executedAndReturnedData"); + ).to.emit(modifier, "ExecutedAndReturnedData"); }); it("execute a transaction with signature.", async () => { const { modifier, tx } = await loadFixture(setupTests); @@ -490,7 +541,12 @@ describe("Modifier", async () => { tx.operation ); - const signature = await sign(modifier.address, transaction, user1); + const signature = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + user1 + ); const transactionWithSig = { ...transaction, @@ -503,7 +559,7 @@ describe("Modifier", async () => { await expect(relayer.sendTransaction(transactionWithSig)).to.emit( modifier, - "executedAndReturnedData" + "ExecutedAndReturnedData" ); }); it("reverts if signature not valid.", async () => { @@ -521,8 +577,65 @@ describe("Modifier", async () => { tx.operation ); - const signatureBad = await sign(modifier.address, transaction, user2); - const signatureOk = await sign(modifier.address, transaction, user1); + const signatureBad = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + user2 + ); + const signatureOk = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + user1 + ); + + const transactionWithBadSig = { + ...transaction, + data: `${transaction.data}${signatureBad.slice(2)}`, + }; + + const transactionWithOkSig = { + ...transaction, + data: `${transaction.data}${signatureOk.slice(2)}`, + }; + + await expect( + relayer.sendTransaction(transactionWithBadSig) + ).to.be.revertedWithCustomError(modifier, "NotAuthorized"); + + await expect(relayer.sendTransaction(transactionWithOkSig)).to.emit( + modifier, + "ExecutedAndReturnedData" + ); + }); + it("reverts if signature previously used for execution.", async () => { + const { modifier, tx } = await loadFixture(setupTests); + const [user1, user2, relayer] = await hre.ethers.getSigners(); + await expect(await modifier.enableModule(user1.address)) + .to.emit(modifier, "EnabledModule") + .withArgs(user1.address); + + const { from, ...transaction } = + await modifier.populateTransaction.execTransactionFromModuleReturnData( + tx.to, + tx.value, + tx.data, + tx.operation + ); + + const signatureOk = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + user1 + ); + const signatureBad = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + user2 + ); const transactionWithBadSig = { ...transaction, @@ -540,8 +653,51 @@ describe("Modifier", async () => { await expect(relayer.sendTransaction(transactionWithOkSig)).to.emit( modifier, - "executedAndReturnedData" + "ExecutedAndReturnedData" + ); + + await expect( + relayer.sendTransaction(transactionWithOkSig) + ).to.be.revertedWithCustomError(modifier, "HashAlreadyExecuted"); + }); + it("reverts if signature invalidated.", async () => { + const { modifier, tx } = await loadFixture(setupTests); + const [user1, relayer] = await hre.ethers.getSigners(); + + await modifier.enableModule(user1.address); + + const { from, ...transaction } = + await modifier.populateTransaction.execTransactionFromModuleReturnData( + tx.to, + tx.value, + tx.data, + tx.operation + ); + + const salt = keccak256(toUtf8Bytes("salt")); + + const signatureOk = await sign( + modifier.address, + transaction, + salt, + user1 + ); + + const transactionWithSig = { + ...transaction, + data: `${transaction.data}${signatureOk.slice(2)}`, + }; + + const hash = await modifier.moduleTxHash( + transaction.data as string, + salt ); + + await modifier.invalidate(hash); + + await expect( + relayer.sendTransaction(transactionWithSig) + ).to.be.revertedWithCustomError(modifier, "HashInvalidated"); }); }); }); @@ -549,11 +705,15 @@ describe("Modifier", async () => { async function sign( contract: string, transaction: PopulatedTransaction, + salt: string, signer: SignerWithAddress ) { const { domain, types, message } = typedDataForTransaction( - { contract, chainId: 31337, nonce: 0 }, + { contract, chainId: 31337, salt }, transaction.data || "0x" ); - return signer._signTypedData(domain, types, message); + + const signature = await signer._signTypedData(domain, types, message); + + return `${salt}${signature.slice(2)}`; } diff --git a/test/06_SignatureChecker.spec.ts b/test/06_SignatureChecker.spec.ts index 3acc4869..3343e199 100644 --- a/test/06_SignatureChecker.spec.ts +++ b/test/06_SignatureChecker.spec.ts @@ -13,7 +13,7 @@ import { toUtf8Bytes, } from "ethers/lib/utils"; -describe.only("SignatureChecker", async () => { +describe("SignatureChecker", async () => { async function setup() { const [signer, relayer] = await hre.ethers.getSigners(); const TestSignature = await hre.ethers.getContractFactory("TestSignature"); diff --git a/test/07_GuardableModifier.spec.ts b/test/07_GuardableModifier.spec.ts index 6dbe3aeb..40ff355a 100644 --- a/test/07_GuardableModifier.spec.ts +++ b/test/07_GuardableModifier.spec.ts @@ -10,6 +10,7 @@ import { import typedDataForTransaction from "./typedDataForTransaction"; import { PopulatedTransaction } from "ethers"; import { SignerWithAddress } from "@nomiclabs/hardhat-ethers/signers"; +import { keccak256, toUtf8Bytes } from "ethers/lib/utils"; describe("GuardableModifier", async () => { async function setupTests() { @@ -96,7 +97,12 @@ describe("GuardableModifier", async () => { 0 ); - const signature = await sign(modifier.address, transaction, signer); + const signature = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + signer + ); const transactionWithSig = { ...transaction, to: modifier.address, @@ -196,7 +202,12 @@ describe("GuardableModifier", async () => { 0 ); - const signature = await sign(modifier.address, transaction, signer); + const signature = await sign( + modifier.address, + transaction, + keccak256(toUtf8Bytes("salt")), + signer + ); const transactionWithSig = { ...transaction, to: modifier.address, @@ -254,11 +265,15 @@ describe("GuardableModifier", async () => { async function sign( contract: string, transaction: PopulatedTransaction, + salt: string, signer: SignerWithAddress ) { const { domain, types, message } = typedDataForTransaction( - { contract, chainId: 31337, nonce: 0 }, + { contract, chainId: 31337, salt }, transaction.data || "0x" ); - return await signer._signTypedData(domain, types, message); + + const signature = await signer._signTypedData(domain, types, message); + + return `${salt}${signature.slice(2)}`; } From e5a977f405b92054caa8eb4b77246949235d267f Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Thu, 12 Oct 2023 15:42:24 +0200 Subject: [PATCH 05/15] Rename auxiliary function --- contracts/core/GuardableModifier.sol | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/contracts/core/GuardableModifier.sol b/contracts/core/GuardableModifier.sol index 1203ac75..3ba9dec5 100644 --- a/contracts/core/GuardableModifier.sol +++ b/contracts/core/GuardableModifier.sol @@ -32,7 +32,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier { address(0), payable(0), "", - _authorizer() + sentOrSignedBy() ); } success = IAvatar(target).execTransactionFromModule( @@ -78,7 +78,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier { address(0), payable(0), "", - _authorizer() + sentOrSignedBy() ); } @@ -90,7 +90,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier { } } - function _authorizer() private returns (address) { + function sentOrSignedBy() private returns (address) { if (modules[msg.sender] != address(0)) { return msg.sender; } From b80933d53c73f96e6ed53b28efdae0d37274d8d7 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Thu, 12 Oct 2023 15:46:35 +0200 Subject: [PATCH 06/15] Moving the newly introducedd errors into ExecutionTracker --- contracts/core/GuardableModifier.sol | 2 +- contracts/core/Modifier.sol | 6 ------ contracts/signature/ExecutionTracker.sol | 3 +++ 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/contracts/core/GuardableModifier.sol b/contracts/core/GuardableModifier.sol index 3ba9dec5..6d8c0298 100644 --- a/contracts/core/GuardableModifier.sol +++ b/contracts/core/GuardableModifier.sol @@ -90,7 +90,7 @@ abstract contract GuardableModifier is Module, Guardable, Modifier { } } - function sentOrSignedBy() private returns (address) { + function sentOrSignedBy() private view returns (address) { if (modules[msg.sender] != address(0)) { return msg.sender; } diff --git a/contracts/core/Modifier.sol b/contracts/core/Modifier.sol index af96b549..3c8c5ae6 100644 --- a/contracts/core/Modifier.sol +++ b/contracts/core/Modifier.sol @@ -21,12 +21,6 @@ abstract contract Modifier is /// @param sender The address of the sender. error NotAuthorized(address sender); - /// @param hash already executed. - error HashAlreadyExecuted(bytes32 hash); - - /// @param hash already executed. - error HashInvalidated(bytes32 hash); - /// `module` is invalid. error InvalidModule(address module); diff --git a/contracts/signature/ExecutionTracker.sol b/contracts/signature/ExecutionTracker.sol index b9f21a0c..a8e6e07f 100644 --- a/contracts/signature/ExecutionTracker.sol +++ b/contracts/signature/ExecutionTracker.sol @@ -3,6 +3,9 @@ pragma solidity >=0.8.0 <0.9.0; /// @title ExecutionTracker - A contract that keeps track of executed and invalidated hashes contract ExecutionTracker { + error HashAlreadyExecuted(bytes32); + error HashInvalidated(bytes32); + mapping(address => mapping(bytes32 => bool)) public executed; mapping(address => mapping(bytes32 => bool)) public invalidated; From 260ce31a7663482d531c9d2f84e1d1979527dde0 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Thu, 12 Oct 2023 16:34:29 +0200 Subject: [PATCH 07/15] Fully cover guard with tests --- contracts/core/Modifier.sol | 3 + contracts/test/TestGuard.sol | 22 ++++++ contracts/test/TestModifier.sol | 4 +- test/04_Guard.spec.ts | 135 +++++++++++++++++++------------- 4 files changed, 108 insertions(+), 56 deletions(-) diff --git a/contracts/core/Modifier.sol b/contracts/core/Modifier.sol index 3c8c5ae6..ebf659fb 100644 --- a/contracts/core/Modifier.sol +++ b/contracts/core/Modifier.sol @@ -78,14 +78,17 @@ abstract contract Modifier is if (modules[msg.sender] == address(0)) { (bytes32 hash, address signer) = moduleTxSignedBy(); + // is the signer a module? if (modules[signer] == address(0)) { revert NotAuthorized(msg.sender); } + // is the provided signature fresh? if (executed[signer][hash]) { revert HashAlreadyExecuted(hash); } + // was the presented signature invalidated? if (invalidated[signer][hash]) { revert HashInvalidated(hash); } diff --git a/contracts/test/TestGuard.sol b/contracts/test/TestGuard.sol index c1323045..72bb06b1 100644 --- a/contracts/test/TestGuard.sol +++ b/contracts/test/TestGuard.sol @@ -51,3 +51,25 @@ contract TestGuard is FactoryFriendly, BaseGuard { module = _module; } } + +contract TestNonCompliantGuard is IERC165 { + function supportsInterface(bytes4) external pure returns (bool) { + return false; + } + + function checkTransaction( + address, + uint256, + bytes memory, + Enum.Operation, + uint256, + uint256, + uint256, + address, + address, + bytes memory, + address + ) public {} + + function checkAfterExecution(bytes32, bool) public {} +} diff --git a/contracts/test/TestModifier.sol b/contracts/test/TestModifier.sol index 8746e78e..e2bcc468 100644 --- a/contracts/test/TestModifier.sol +++ b/contracts/test/TestModifier.sol @@ -73,12 +73,12 @@ contract TestModifier is Modifier { } function setUp(bytes memory initializeParams) public override initializer { - setupModules(); - __Ownable_init(msg.sender); (address _avatar, address _target) = abi.decode( initializeParams, (address, address) ); + setupModules(); + _transferOwnership(msg.sender); avatar = _avatar; target = _target; } diff --git a/test/04_Guard.spec.ts b/test/04_Guard.spec.ts index af802bff..2914ddb2 100644 --- a/test/04_Guard.spec.ts +++ b/test/04_Guard.spec.ts @@ -1,32 +1,66 @@ -import { AddressZero } from "@ethersproject/constants"; -import { loadFixture } from "@nomicfoundation/hardhat-network-helpers"; import { expect } from "chai"; import hre from "hardhat"; -describe("Guardable", async () => { - async function setupTests() { - const Avatar = await hre.ethers.getContractFactory("TestAvatar"); - const avatar = await Avatar.deploy(); - const iAvatar = await hre.ethers.getContractAt("IAvatar", avatar.address); - const Module = await hre.ethers.getContractFactory("TestModule"); - const module = await Module.deploy(iAvatar.address, iAvatar.address); - await avatar.enableModule(module.address); - const Guard = await hre.ethers.getContractFactory("TestGuard"); - const guard = await Guard.deploy(module.address); - return { - iAvatar, - guard, - module, - }; - } +import { AddressZero } from "@ethersproject/constants"; +import { loadFixture } from "@nomicfoundation/hardhat-network-helpers"; +import { TestGuard__factory, TestModule__factory } from "../typechain-types"; + +async function setupTests() { + const [owner, other, relayer] = await hre.ethers.getSigners(); + const Avatar = await hre.ethers.getContractFactory("TestAvatar"); + const avatar = await Avatar.deploy(); + const Module = await hre.ethers.getContractFactory("TestModule"); + const module = TestModule__factory.connect( + (await Module.connect(owner).deploy(avatar.address, avatar.address)) + .address, + owner + ); + await avatar.enableModule(module.address); + const Guard = await hre.ethers.getContractFactory("TestGuard"); + const guard = TestGuard__factory.connect( + (await Guard.deploy(module.address)).address, + relayer + ); + + const GuardNonCompliant = await hre.ethers.getContractFactory( + "TestNonCompliantGuard" + ); + const guardNonCompliant = TestGuard__factory.connect( + (await GuardNonCompliant.deploy()).address, + hre.ethers.provider + ); + + const tx = { + to: avatar.address, + value: 0, + data: "0x", + operation: 0, + avatarTxGas: 0, + baseGas: 0, + gasPrice: 0, + gasToken: AddressZero, + refundReceiver: AddressZero, + signatures: "0x", + }; + + return { + owner, + other, + module, + guard, + guardNonCompliant, + tx, + }; +} + +describe("Guardable", async () => { describe("setGuard", async () => { it("reverts if reverts if caller is not the owner", async () => { - const { module } = await loadFixture(setupTests); - const [, user1] = await hre.ethers.getSigners(); - await expect(module.connect(user1).setGuard(user1.address)) + const { other, guard, module } = await loadFixture(setupTests); + await expect(module.connect(other).setGuard(guard.address)) .to.be.revertedWithCustomError(module, "OwnableUnauthorizedAccount") - .withArgs(user1.address); + .withArgs(other.address); }); it("reverts if guard does not implement ERC165", async () => { @@ -34,12 +68,30 @@ describe("Guardable", async () => { await expect(module.setGuard(module.address)).to.be.reverted; }); + it("reverts if guard implements ERC165 and returns false", async () => { + const { module, guardNonCompliant } = await loadFixture(setupTests); + await expect(module.setGuard(guardNonCompliant.address)) + .to.be.revertedWithCustomError(module, "NotIERC165Compliant") + .withArgs(guardNonCompliant.address); + }); + it("sets module and emits event", async () => { const { module, guard } = await loadFixture(setupTests); await expect(module.setGuard(guard.address)) .to.emit(module, "ChangedGuard") .withArgs(guard.address); }); + + it("sets guard back to zero", async () => { + const { module, guard } = await loadFixture(setupTests); + await expect(module.setGuard(guard.address)) + .to.emit(module, "ChangedGuard") + .withArgs(guard.address); + + await expect(module.setGuard(AddressZero)) + .to.emit(module, "ChangedGuard") + .withArgs(AddressZero); + }); }); describe("getGuard", async () => { @@ -54,39 +106,15 @@ describe("BaseGuard", async () => { const txHash = "0x0000000000000000000000000000000000000000000000000000000000000001"; - async function setupTests() { - const Avatar = await hre.ethers.getContractFactory("TestAvatar"); - const avatar = await Avatar.deploy(); - const iAvatar = await hre.ethers.getContractAt("IAvatar", avatar.address); - const Module = await hre.ethers.getContractFactory("TestModule"); - const module = await Module.deploy(iAvatar.address, iAvatar.address); - await avatar.enableModule(module.address); - const Guard = await hre.ethers.getContractFactory("TestGuard"); - const guard = await Guard.deploy(module.address); - const tx = { - to: avatar.address, - value: 0, - data: "0x", - operation: 0, - avatarTxGas: 0, - baseGas: 0, - gasPrice: 0, - gasToken: AddressZero, - refundReceiver: AddressZero, - signatures: "0x", - }; - return { - iAvatar, - guard, - module, - tx, - }; - } + it("supports interface", async () => { + const { guard } = await loadFixture(setupTests); + expect(await guard.supportsInterface("0xe6d7a83a")).to.be.true; + expect(await guard.supportsInterface("0x01ffc9a7")).to.be.true; + }); describe("checkTransaction", async () => { it("reverts if test fails", async () => { const { guard, tx } = await loadFixture(setupTests); - const [user1] = await hre.ethers.getSigners(); await expect( guard.checkTransaction( tx.to, @@ -99,13 +127,12 @@ describe("BaseGuard", async () => { tx.gasToken, tx.refundReceiver, tx.signatures, - user1.address + AddressZero ) ).to.be.revertedWith("Cannot send 1337"); }); it("checks transaction", async () => { const { guard, tx } = await loadFixture(setupTests); - const [user1] = await hre.ethers.getSigners(); await expect( guard.checkTransaction( tx.to, @@ -118,7 +145,7 @@ describe("BaseGuard", async () => { tx.gasToken, tx.refundReceiver, tx.signatures, - user1.address + AddressZero ) ).to.emit(guard, "PreChecked"); }); From c6775a592a56da2d8e0d3421a47db3e38eac63a4 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Fri, 13 Oct 2023 18:49:18 +0200 Subject: [PATCH 08/15] Make sentOrSigned by an internal helper function for Modifiers --- contracts/core/GuardableModifier.sol | 9 --------- contracts/core/Modifier.sol | 13 +++++++++++++ 2 files changed, 13 insertions(+), 9 deletions(-) diff --git a/contracts/core/GuardableModifier.sol b/contracts/core/GuardableModifier.sol index 6d8c0298..90c29b1a 100644 --- a/contracts/core/GuardableModifier.sol +++ b/contracts/core/GuardableModifier.sol @@ -89,13 +89,4 @@ abstract contract GuardableModifier is Module, Guardable, Modifier { IGuard(currentGuard).checkAfterExecution(bytes32(0), success); } } - - function sentOrSignedBy() private view returns (address) { - if (modules[msg.sender] != address(0)) { - return msg.sender; - } - - (, address signer) = moduleTxSignedBy(); - return signer; - } } diff --git a/contracts/core/Modifier.sol b/contracts/core/Modifier.sol index ebf659fb..3a39a3dc 100644 --- a/contracts/core/Modifier.sol +++ b/contracts/core/Modifier.sol @@ -99,6 +99,19 @@ abstract contract Modifier is _; } + function sentOrSignedBy() internal view returns (address) { + if (modules[msg.sender] != address(0)) { + return msg.sender; + } + + (, address signer) = moduleTxSignedBy(); + if (modules[signer] != address(0)) { + return signer; + } + + return address(0); + } + /// @dev Disables a module on the modifier. /// @notice This can only be called by the owner. /// @param prevModule Module that pointed to the module to be removed in the linked list. From b7e0ee3d12369c544101b53f645f2ac9642e5453 Mon Sep 17 00:00:00 2001 From: Cristovao Honorato Date: Wed, 18 Oct 2023 09:35:10 +0200 Subject: [PATCH 09/15] Make moduleTxSignedBy return always the hash, even when no signer found recovered --- contracts/signature/SignatureChecker.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/signature/SignatureChecker.sol b/contracts/signature/SignatureChecker.sol index 5d9c2b5d..416dddcc 100644 --- a/contracts/signature/SignatureChecker.sol +++ b/contracts/signature/SignatureChecker.sol @@ -52,7 +52,7 @@ abstract contract SignatureChecker { data[start:data.length - 65] ) ? (hash, signer) - : (bytes32(0), address(0)); + : (hash, address(0)); } else { return (hash, ecrecover(hash, v, r, s)); } From b2f80892a70d95fb817dcd9e6b4cd1edd51a8ca1 Mon Sep 17 00:00:00 2001 From: Cristovao Honorato Date: Wed, 18 Oct 2023 09:46:09 +0200 Subject: [PATCH 10/15] Consolidate hash tracking into one mapping --- contracts/core/Modifier.sol | 12 ++++-------- contracts/signature/ExecutionTracker.sol | 12 +++++++----- test/03_Modifier.spec.ts | 8 ++++---- 3 files changed, 15 insertions(+), 17 deletions(-) diff --git a/contracts/core/Modifier.sol b/contracts/core/Modifier.sol index 3a39a3dc..f2ce79c3 100644 --- a/contracts/core/Modifier.sol +++ b/contracts/core/Modifier.sol @@ -84,16 +84,12 @@ abstract contract Modifier is } // is the provided signature fresh? - if (executed[signer][hash]) { - revert HashAlreadyExecuted(hash); + if (consumed[signer][hash]) { + revert HashAlreadyConsumed(hash); } - // was the presented signature invalidated? - if (invalidated[signer][hash]) { - revert HashInvalidated(hash); - } - - executed[signer][hash] = true; + consumed[signer][hash] = true; + emit HashExecuted(hash); } _; diff --git a/contracts/signature/ExecutionTracker.sol b/contracts/signature/ExecutionTracker.sol index a8e6e07f..ad88b1d4 100644 --- a/contracts/signature/ExecutionTracker.sol +++ b/contracts/signature/ExecutionTracker.sol @@ -3,13 +3,15 @@ pragma solidity >=0.8.0 <0.9.0; /// @title ExecutionTracker - A contract that keeps track of executed and invalidated hashes contract ExecutionTracker { - error HashAlreadyExecuted(bytes32); - error HashInvalidated(bytes32); + error HashAlreadyConsumed(bytes32); - mapping(address => mapping(bytes32 => bool)) public executed; - mapping(address => mapping(bytes32 => bool)) public invalidated; + event HashExecuted(bytes32); + event HashInvalidated(bytes32); + + mapping(address => mapping(bytes32 => bool)) public consumed; function invalidate(bytes32 hash) external { - invalidated[msg.sender][hash] = true; + consumed[msg.sender][hash] = true; + emit HashInvalidated(hash); } } diff --git a/test/03_Modifier.spec.ts b/test/03_Modifier.spec.ts index c5e31e4f..30563533 100644 --- a/test/03_Modifier.spec.ts +++ b/test/03_Modifier.spec.ts @@ -453,7 +453,7 @@ describe("Modifier", async () => { await expect( relayer.sendTransaction(transactionWithOkSig) - ).to.be.revertedWithCustomError(modifier, "HashAlreadyExecuted"); + ).to.be.revertedWithCustomError(modifier, "HashAlreadyConsumed"); }); it("reverts if signature invalidated.", async () => { const { modifier, tx } = await loadFixture(setupTests); @@ -492,7 +492,7 @@ describe("Modifier", async () => { await expect( relayer.sendTransaction(transactionWithSig) - ).to.be.revertedWithCustomError(modifier, "HashInvalidated"); + ).to.be.revertedWithCustomError(modifier, "HashAlreadyConsumed"); }); }); @@ -658,7 +658,7 @@ describe("Modifier", async () => { await expect( relayer.sendTransaction(transactionWithOkSig) - ).to.be.revertedWithCustomError(modifier, "HashAlreadyExecuted"); + ).to.be.revertedWithCustomError(modifier, "HashAlreadyConsumed"); }); it("reverts if signature invalidated.", async () => { const { modifier, tx } = await loadFixture(setupTests); @@ -697,7 +697,7 @@ describe("Modifier", async () => { await expect( relayer.sendTransaction(transactionWithSig) - ).to.be.revertedWithCustomError(modifier, "HashInvalidated"); + ).to.be.revertedWithCustomError(modifier, "HashAlreadyConsumed"); }); }); }); From e71c183b0fe114c05b1b9abe6ff6f52a12db2d36 Mon Sep 17 00:00:00 2001 From: Cristovao Honorato Date: Thu, 19 Oct 2023 11:15:57 +0200 Subject: [PATCH 11/15] Ensure contract specific signature is sliced correctly --- contracts/signature/SignatureChecker.sol | 8 ++-- contracts/test/TestSignature.sol | 12 ++++++ test/06_SignatureChecker.spec.ts | 48 ++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 4 deletions(-) diff --git a/contracts/signature/SignatureChecker.sol b/contracts/signature/SignatureChecker.sol index 416dddcc..fd8c1af2 100644 --- a/contracts/signature/SignatureChecker.sol +++ b/contracts/signature/SignatureChecker.sol @@ -40,7 +40,7 @@ abstract contract SignatureChecker { bytes32 hash = moduleTxHash( data[:start], // slice the appended signature out - bytes32(data[data.length - 65 - 32:]) // get the salt + bytes32(data[data.length - (32 + 65):]) // get the salt ); if (isContractSignature) { @@ -49,7 +49,7 @@ abstract contract SignatureChecker { _isValidContractSignature( signer, hash, - data[start:data.length - 65] + data[start:data.length - (32 + 65)] ) ? (hash, signer) : (hash, address(0)); @@ -122,8 +122,8 @@ abstract contract SignatureChecker { isEIP1271Signature = v == 0 && uint256(s) > 3 && - uint256(s) < (length - 65 - 32); - start = isEIP1271Signature ? uint256(s) : length - 65 - 32; + uint256(s) < (length - (32 + 65)); + start = isEIP1271Signature ? uint256(s) : length - (32 + 65); } /** diff --git a/contracts/test/TestSignature.sol b/contracts/test/TestSignature.sol index 7242e6d4..70113528 100644 --- a/contracts/test/TestSignature.sol +++ b/contracts/test/TestSignature.sol @@ -21,6 +21,18 @@ contract ContractSignerNo is IERC1271 { } } +contract ContractSignerMaybe is IERC1271 { + function isValidSignature( + bytes32, + bytes memory contractSpecificSignature + ) external pure override returns (bytes4) { + bool isValid = contractSpecificSignature.length == 6 && + bytes6(contractSpecificSignature) == 0x001122334455; + + return isValid ? bytes4(0x1626ba7e) : bytes4(0x33333333); + } +} + contract ContractSignerFaulty {} contract ContractSignerReturnSize { diff --git a/test/06_SignatureChecker.spec.ts b/test/06_SignatureChecker.spec.ts index 3343e199..91724a22 100644 --- a/test/06_SignatureChecker.spec.ts +++ b/test/06_SignatureChecker.spec.ts @@ -217,6 +217,54 @@ describe("SignatureChecker", async () => { .to.emit(testSignature, "Hello") .withArgs(signer); }); + it("signer returns isValid maybe", async () => { + const { testSignature, relayer } = await loadFixture(setup); + + const ContractSigner = await hre.ethers.getContractFactory( + "ContractSignerMaybe" + ); + const contractSigner = await ContractSigner.deploy(); + + const transaction = await testSignature.populateTransaction.goodbye( + 0, + "0xbadfed" + ); + + const signatureGood = makeContractSignature( + transaction, + "0x001122334455", + keccak256(toUtf8Bytes("some irrelevant salt")), + contractSigner.address + ); + + const signatureBad = makeContractSignature( + transaction, + "0x00112233445566", + keccak256(toUtf8Bytes("some irrelevant salt")), + contractSigner.address + ); + + const transactionWithGoodSig = { + ...transaction, + data: `${transaction.data}${signatureGood.slice(2)}`, + }; + const transactionWithBadSig = { + ...transaction, + data: `${transaction.data}${signatureBad.slice(2)}`, + }; + + await expect(await relayer.sendTransaction(transaction)) + .to.emit(testSignature, "Goodbye") + .withArgs(AddressZero); + + await expect(await relayer.sendTransaction(transactionWithGoodSig)) + .to.emit(testSignature, "Goodbye") + .withArgs(contractSigner.address); + + await expect(await relayer.sendTransaction(transactionWithBadSig)) + .to.emit(testSignature, "Goodbye") + .withArgs(AddressZero); + }); it("signer returns isValid yes", async () => { const { testSignature, relayer } = await loadFixture(setup); From 8095be180447a8b936626d8e73d1c6e74cbd2fee Mon Sep 17 00:00:00 2001 From: Cristovao Honorato Date: Wed, 25 Oct 2023 19:42:39 +0200 Subject: [PATCH 12/15] Simplify splitSignature, and pull up logic that assembles the isValidContractSignature call --- contracts/signature/SignatureChecker.sol | 72 ++++++++---------------- 1 file changed, 25 insertions(+), 47 deletions(-) diff --git a/contracts/signature/SignatureChecker.sol b/contracts/signature/SignatureChecker.sol index fd8c1af2..69eadcbd 100644 --- a/contracts/signature/SignatureChecker.sol +++ b/contracts/signature/SignatureChecker.sol @@ -30,30 +30,37 @@ abstract contract SignatureChecker { return (bytes32(0), address(0)); } - ( - uint8 v, - bytes32 r, - bytes32 s, - bool isContractSignature, - uint256 start - ) = _splitSignature(data); + (uint8 v, bytes32 r, bytes32 s) = _splitSignature(data); - bytes32 hash = moduleTxHash( - data[:start], // slice the appended signature out - bytes32(data[data.length - (32 + 65):]) // get the salt - ); + uint256 end = data.length - (32 + 65); + bytes32 salt = bytes32(data[end:]); - if (isContractSignature) { + /* + * When handling contract signatures: + * v - is zero + * r - contains the signer + * s - contains the offset within calldata where the signer specific + * signature is located + * + * We detect contract signatures by checking: + * 1- `v` is zero + * 2- `s` points within the buffer, is after selector, is before + * salt and delimits a non-zero length buffer + */ + if (v == 0) { + uint256 start = uint256(s); + if (start < 4 || start > end) { + return (bytes32(0), address(0)); + } address signer = address(uint160(uint256(r))); + + bytes32 hash = moduleTxHash(data[:start], salt); return - _isValidContractSignature( - signer, - hash, - data[start:data.length - (32 + 65)] - ) + _isValidContractSignature(signer, hash, data[start:end]) ? (hash, signer) : (hash, address(0)); } else { + bytes32 hash = moduleTxHash(data[:end], salt); return (hash, ecrecover(hash, v, r, s)); } } @@ -87,43 +94,14 @@ abstract contract SignatureChecker { * @return v The ECDSA v value * @return r The ECDSA r value * @return s The ECDSA s value - * @return isEIP1271Signature Indicates whether the split signature is a contract signature - * @return start offset in calldata where signature starts */ function _splitSignature( bytes calldata data - ) - private - pure - returns ( - uint8 v, - bytes32 r, - bytes32 s, - bool isEIP1271Signature, - uint256 start - ) - { - /* - * When handling contract signatures: - * v - is zero - * r - contains the signer - * s - contains the offset within calldata where the signer specific - * signature is located - * - * We detect contract signatures by checking: - * 1- `v` is zero - * 2- `s` points within the buffer, is after selector, is before - * salt and delimits a non-zero length buffer - */ + ) private pure returns (uint8 v, bytes32 r, bytes32 s) { uint256 length = data.length; v = uint8(bytes1(data[length - 1:])); r = bytes32(data[length - 65:]); s = bytes32(data[length - 33:]); - isEIP1271Signature = - v == 0 && - uint256(s) > 3 && - uint256(s) < (length - (32 + 65)); - start = isEIP1271Signature ? uint256(s) : length - (32 + 65); } /** From 01a84848b53858630839a6b6fb02081f9a07794d Mon Sep 17 00:00:00 2001 From: Cristovao Honorato Date: Wed, 25 Oct 2023 19:57:13 +0200 Subject: [PATCH 13/15] Add test for contract specific signature empty --- contracts/test/TestSignature.sol | 10 +++++++ test/06_SignatureChecker.spec.ts | 51 +++++++++++++++++++++++++++++++- 2 files changed, 60 insertions(+), 1 deletion(-) diff --git a/contracts/test/TestSignature.sol b/contracts/test/TestSignature.sol index 70113528..1614cc26 100644 --- a/contracts/test/TestSignature.sol +++ b/contracts/test/TestSignature.sol @@ -33,6 +33,16 @@ contract ContractSignerMaybe is IERC1271 { } } +contract ContractSignerOnlyEmpty is IERC1271 { + function isValidSignature( + bytes32, + bytes memory contractSpecificSignature + ) external pure override returns (bytes4) { + bool isValid = contractSpecificSignature.length == 0; + return isValid ? bytes4(0x1626ba7e) : bytes4(0x33333333); + } +} + contract ContractSignerFaulty {} contract ContractSignerReturnSize { diff --git a/test/06_SignatureChecker.spec.ts b/test/06_SignatureChecker.spec.ts index 91724a22..6886b27a 100644 --- a/test/06_SignatureChecker.spec.ts +++ b/test/06_SignatureChecker.spec.ts @@ -322,6 +322,55 @@ describe("SignatureChecker", async () => { .to.emit(testSignature, "Hello") .withArgs(AddressZero); }); + + it("signer returns isValid for empty specific signature only", async () => { + const { testSignature, relayer } = await loadFixture(setup); + + const ContractSigner = await hre.ethers.getContractFactory( + "ContractSignerOnlyEmpty" + ); + const contractSigner = await ContractSigner.deploy(); + + const transaction = await testSignature.populateTransaction.goodbye( + 0, + "0xbadfed" + ); + + const signatureGood = makeContractSignature( + transaction, + "0x", + keccak256(toUtf8Bytes("some irrelevant salt")), + contractSigner.address + ); + + const signatureBad = makeContractSignature( + transaction, + "0xffff", + keccak256(toUtf8Bytes("some irrelevant salt")), + contractSigner.address + ); + + const transactionWithGoodSig = { + ...transaction, + data: `${transaction.data}${signatureGood.slice(2)}`, + }; + const transactionWithBadSig = { + ...transaction, + data: `${transaction.data}${signatureBad.slice(2)}`, + }; + + await expect(await relayer.sendTransaction(transaction)) + .to.emit(testSignature, "Goodbye") + .withArgs(AddressZero); + + await expect(await relayer.sendTransaction(transactionWithGoodSig)) + .to.emit(testSignature, "Goodbye") + .withArgs(contractSigner.address); + + await expect(await relayer.sendTransaction(transactionWithBadSig)) + .to.emit(testSignature, "Goodbye") + .withArgs(AddressZero); + }); it("signer bad return size", async () => { const { testSignature, relayer } = await loadFixture(setup); @@ -427,7 +476,7 @@ function makeContractSignature( r: string, s?: string ) { - const dataBytesLength = (transaction.data?.length as number) / 2 - 1; + const dataBytesLength = ((transaction.data?.length as number) - 2) / 2; r = defaultAbiCoder.encode(["address"], [r]); s = s || defaultAbiCoder.encode(["uint256"], [dataBytesLength]); From c9d69047cfbe59104e06697c9d8c245f73d9aa78 Mon Sep 17 00:00:00 2001 From: Cristovao Honorato Date: Wed, 25 Oct 2023 23:06:42 +0200 Subject: [PATCH 14/15] Remove aux variable --- contracts/signature/SignatureChecker.sol | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/contracts/signature/SignatureChecker.sol b/contracts/signature/SignatureChecker.sol index 69eadcbd..6d55a4f3 100644 --- a/contracts/signature/SignatureChecker.sol +++ b/contracts/signature/SignatureChecker.sol @@ -98,10 +98,9 @@ abstract contract SignatureChecker { function _splitSignature( bytes calldata data ) private pure returns (uint8 v, bytes32 r, bytes32 s) { - uint256 length = data.length; - v = uint8(bytes1(data[length - 1:])); - r = bytes32(data[length - 65:]); - s = bytes32(data[length - 33:]); + v = uint8(bytes1(data[data.length - 1:])); + r = bytes32(data[data.length - 65:]); + s = bytes32(data[data.length - 33:]); } /** From 9257ba7f3ab3c3db3bd7d71cdcbf7550577fa798 Mon Sep 17 00:00:00 2001 From: Cristovao Honorato Date: Thu, 26 Oct 2023 10:23:44 +0200 Subject: [PATCH 15/15] Consistently return hash from moduleTxSignedBy() --- contracts/signature/SignatureChecker.sol | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/contracts/signature/SignatureChecker.sol b/contracts/signature/SignatureChecker.sol index 6d55a4f3..21d1d145 100644 --- a/contracts/signature/SignatureChecker.sol +++ b/contracts/signature/SignatureChecker.sol @@ -58,7 +58,7 @@ abstract contract SignatureChecker { return _isValidContractSignature(signer, hash, data[start:end]) ? (hash, signer) - : (hash, address(0)); + : (bytes32(0), address(0)); } else { bytes32 hash = moduleTxHash(data[:end], salt); return (hash, ecrecover(hash, v, r, s));