From 260ce31a7663482d531c9d2f84e1d1979527dde0 Mon Sep 17 00:00:00 2001 From: cristovaoth Date: Thu, 12 Oct 2023 16:34:29 +0200 Subject: [PATCH] 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"); });