Extract badSignature checks to their own test cases, instead of mixed…

… with success case tests
gnosisguild · Sep 28, 2023 · 9274399 · 9274399
1 parent 0167e0b
commit 9274399
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 33 deletions.
diff --git a/contracts/signature/Eip712Signature.sol b/contracts/signature/Eip712Signature.sol
@@ -76,11 +76,7 @@ abstract contract EIP712Signature {
                     bytes1(0x01),
                     domainSeparator,
                     keccak256(
-                        abi.encode(
-                            TRANSACTION_TYPEHASH,
-                            keccak256(data),
-                            _nonce
-                        )
+                        abi.encode(MODULE_TX_TYPEHASH, keccak256(data), _nonce)
                     )
                 )
             );
@@ -95,6 +91,6 @@ abstract contract EIP712Signature {
     // keccak256(
     //     "ModuleTx(bytes data,uint256 nonce)"
     // );
-    bytes32 private constant TRANSACTION_TYPEHASH =
+    bytes32 private constant MODULE_TX_TYPEHASH =
         0xd6c6b5df57eef4e79cab990a377d29dc4c5bbb016a6293120d53f49c54144227;
 }
diff --git a/test/03_Modifier.spec.ts b/test/03_Modifier.spec.ts
@@ -313,6 +313,37 @@ describe("Modifier", async () => {
       ).to.emit(modifier, "executed");
     });
     it("execute a transaction with signature.", async () => {
+      const { modifier, tx } = await loadFixture(setupTests);
+      const [user1, 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.execTransactionFromModule(
+          tx.to,
+          tx.value,
+          tx.data,
+          tx.operation
+        );
+
+      const signature = await sign(modifier.address, transaction, user1);
+
+      const transactionWithSig = {
+        ...transaction,
+        data: `${transaction.data}${signature.slice(2)}`,
+      };
+
+      await expect(
+        relayer.sendTransaction(transaction)
+      ).to.be.revertedWithCustomError(modifier, "NotAuthorized");
+
+      await expect(relayer.sendTransaction(transactionWithSig)).to.emit(
+        modifier,
+        "executed"
+      );
+    });
+    it("reverts if signature not valid.", async () => {
       const { modifier, tx } = await loadFixture(setupTests);
       const [user1, user2, relayer] = await hre.ethers.getSigners();
       await expect(await modifier.enableModule(user1.address))
@@ -330,29 +361,24 @@ describe("Modifier", async () => {
       const signatureOk = await sign(modifier.address, transaction, user1);
       const signatureBad = await sign(modifier.address, transaction, user2);
 
-      const transactionWithOkSig = {
+      const transactionWithBadSig = {
         ...transaction,
-        data: `${transaction.data}${signatureOk.slice(2)}`,
+        data: `${transaction.data}${signatureBad.slice(2)}`,
       };
 
-      const transactionWithBadSig = {
+      const transactionWithOkSig = {
         ...transaction,
-        data: `${transaction.data}${signatureBad.slice(2)}`,
+        data: `${transaction.data}${signatureOk.slice(2)}`,
       };
 
-      await expect(user1.sendTransaction(transaction)).to.emit(
-        modifier,
-        "executed"
-      );
+      await expect(
+        relayer.sendTransaction(transactionWithBadSig)
+      ).to.be.revertedWithCustomError(modifier, "NotAuthorized");
 
       await expect(relayer.sendTransaction(transactionWithOkSig)).to.emit(
         modifier,
         "executed"
       );
-
-      await expect(
-        relayer.sendTransaction(transactionWithBadSig)
-      ).to.be.revertedWithCustomError(modifier, "NotAuthorized");
     });
     it("execute a transaction via sender nonce stays same.", async () => {
       const { modifier, tx } = await loadFixture(setupTests);
@@ -427,7 +453,6 @@ describe("Modifier", async () => {
         .to.be.revertedWithCustomError(modifier, "NotAuthorized")
         .withArgs("0xf39Fd6e51aad88F6F4ce6aB8827279cffFb92266");
     });
-
     it("execute a transaction.", async () => {
       const { modifier, tx } = await loadFixture(setupTests);
       const [user1] = await hre.ethers.getSigners();
@@ -444,10 +469,9 @@ describe("Modifier", async () => {
         )
       ).to.emit(modifier, "executedAndReturnedData");
     });
-
     it("execute a transaction with signature.", async () => {
       const { modifier, tx } = await loadFixture(setupTests);
-      const [user1, user2, relayer] = await hre.ethers.getSigners();
+      const [user1, relayer] = await hre.ethers.getSigners();
       await expect(await modifier.enableModule(user1.address))
         .to.emit(modifier, "EnabledModule")
         .withArgs(user1.address);
@@ -460,32 +484,58 @@ describe("Modifier", async () => {
           tx.operation
         );
 
-      const signatureOk = await sign(modifier.address, transaction, user1);
-      const signatureBad = await sign(modifier.address, transaction, user2);
+      const signature = await sign(modifier.address, transaction, user1);
 
-      const transactionWithOkSig = {
+      const transactionWithSig = {
         ...transaction,
-        data: `${transaction.data}${signatureOk.slice(2)}`,
+        data: `${transaction.data}${signature.slice(2)}`,
       };
 
+      await expect(
+        relayer.sendTransaction(transaction)
+      ).to.be.revertedWithCustomError(modifier, "NotAuthorized");
+
+      await expect(relayer.sendTransaction(transactionWithSig)).to.emit(
+        modifier,
+        "executedAndReturnedData"
+      );
+    });
+    it("reverts if signature not valid.", 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 signatureBad = await sign(modifier.address, transaction, user2);
+      const signatureOk = await sign(modifier.address, transaction, user1);
+
       const transactionWithBadSig = {
         ...transaction,
         data: `${transaction.data}${signatureBad.slice(2)}`,
       };
 
-      await expect(user1.sendTransaction(transaction)).to.emit(
-        modifier,
-        "executedAndReturnedData"
-      );
+      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"
       );
-
-      await expect(
-        relayer.sendTransaction(transactionWithBadSig)
-      ).to.be.revertedWithCustomError(modifier, "NotAuthorized");
     });
   });
 });