Skip to content

Commit

Permalink
[#53] Registry checks for hooks
Browse files Browse the repository at this point in the history
  • Loading branch information
@akshay-ap
akshay-ap committed Aug 16, 2023
1 parent 9400aef commit 402e7a0
Show file tree
Hide file tree
Showing 3 changed files with 33 additions and 33 deletions.
10 changes: 3 additions & 7 deletions contracts/base/HooksManager.sol
View file
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
// SPDX-License-Identifier: LGPL-3.0-only
pragma solidity ^0.8.18;
import {ISafeProtocolHooks} from "../interfaces/Integrations.sol";
import {RegistryManager} from "./RegistryManager.sol";

contract HooksManager {
abstract contract HooksManager is RegistryManager {
mapping(address => address) public enabledHooks;

/// @notice This variable should store the address of the hooks contract whenever
Expand All @@ -13,9 +14,6 @@ contract HooksManager {
// Events
event HooksChanged(address indexed safe, address indexed hooksAddress);

// Errors
error AddressDoesNotImplementHooksInterface(address hooksAddress);

/**
* @notice Returns the address of hooks for a Safe account provided as a fucntion parameter.
* Returns address(0) is no hooks are enabled.
Expand All @@ -31,9 +29,7 @@ contract HooksManager {
* @param hooks Address of the hooks to be enabled for msg.sender.
*/
function setHooks(address hooks) external {
if (hooks != address(0) && !ISafeProtocolHooks(hooks).supportsInterface(type(ISafeProtocolHooks).interfaceId)) {
revert AddressDoesNotImplementHooksInterface(hooks);
}
if (hooks != address(0)) checkPermittedIntegration(hooks);
enabledHooks[msg.sender] = hooks;
emit HooksChanged(msg.sender, hooks);
}
Expand Down
10 changes: 10 additions & 0 deletions test/SafeProtocolManager.spec.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -397,6 +397,7 @@ describe("SafeProtocolManager", async () => {
const { safeProtocolManager, safe, safeProtocolRegistry } = await loadFixture(deployContractsWithEnabledManagerFixture);
// Enable hooks on a safe
const hooks = await getHooksWithPassingChecks();
await safeProtocolRegistry.connect(owner).addIntegration(hooks.target, IntegrationType.Hooks);
const dataSetHooks = safeProtocolManager.interface.encodeFunctionData("setHooks", [await hooks.getAddress()]);
await safe.exec(await safeProtocolManager.getAddress(), 0, dataSetHooks);

Expand Down Expand Up @@ -431,6 +432,7 @@ describe("SafeProtocolManager", async () => {
const { safeProtocolManager, safe, safeProtocolRegistry } = await loadFixture(deployContractsWithEnabledManagerFixture);
// Enable hooks on a safe
const hooks = await getHooksWithFailingPrechecks();
await safeProtocolRegistry.connect(owner).addIntegration(hooks.target, IntegrationType.Hooks);

const dataSetHooks = safeProtocolManager.interface.encodeFunctionData("setHooks", [await hooks.getAddress()]);
await safe.exec(await safeProtocolManager.getAddress(), 0, dataSetHooks);
Expand All @@ -452,6 +454,7 @@ describe("SafeProtocolManager", async () => {
const safeProtocolManagerAddress = await safeProtocolManager.getAddress();
// Enable hooks on a safe
const hooks = await getHooksWithFailingPostCheck();
await safeProtocolRegistry.connect(owner).addIntegration(hooks.target, IntegrationType.Hooks);

const dataSetHooks = safeProtocolManager.interface.encodeFunctionData("setHooks", [await hooks.getAddress()]);
await safe.exec(safeProtocolManagerAddress, 0, dataSetHooks);
Expand Down Expand Up @@ -577,8 +580,11 @@ describe("SafeProtocolManager", async () => {
it("Should execute a transaction from root access enabled plugin with hooks enabled", async () => {
const { safeProtocolManager, safe, safeProtocolRegistry } = await loadFixture(deployContractsWithEnabledManagerFixture);
const safeAddress = await safe.getAddress();

// Enable hooks on a safe
const hooks = await getHooksWithPassingChecks();
await safeProtocolRegistry.connect(owner).addIntegration(hooks.target, IntegrationType.Hooks);

const dataSetHooks = safeProtocolManager.interface.encodeFunctionData("setHooks", [await hooks.getAddress()]);
await safe.exec(await safeProtocolManager.getAddress(), 0, dataSetHooks);

Expand Down Expand Up @@ -622,6 +628,7 @@ describe("SafeProtocolManager", async () => {
const { safeProtocolManager, safe, safeProtocolRegistry } = await loadFixture(deployContractsWithEnabledManagerFixture);
// Enable hooks on a safe
const hooks = await getHooksWithFailingPrechecks();
await safeProtocolRegistry.connect(owner).addIntegration(hooks.target, IntegrationType.Hooks);

const dataSetHooks = safeProtocolManager.interface.encodeFunctionData("setHooks", [await hooks.getAddress()]);
await safe.exec(await safeProtocolManager.getAddress(), 0, dataSetHooks);
Expand Down Expand Up @@ -655,6 +662,8 @@ describe("SafeProtocolManager", async () => {

// Enable hooks on a safe
const hooks = await getHooksWithFailingPostCheck();
await safeProtocolRegistry.connect(owner).addIntegration(hooks.target, IntegrationType.Hooks);

const dataSetHooks = safeProtocolManager.interface.encodeFunctionData("setHooks", [await hooks.getAddress()]);
await safe.exec(safeProtocolManagerAddress, 0, dataSetHooks);

Expand Down Expand Up @@ -832,6 +841,7 @@ describe("SafeProtocolManager", async () => {

await safeProtocolRegistry.connect(owner).addIntegration(hooks.target, IntegrationType.Hooks);
await safeProtocolRegistry.connect(owner).addIntegration(hooksWithFailingPreChecks.target, IntegrationType.Hooks);
await safeProtocolRegistry.connect(owner).addIntegration(hooksWithFailingPostCheck.target, IntegrationType.Hooks);

return { safe, safeProtocolManager, hooks, hooksWithFailingPreChecks, hooksWithFailingPostCheck };
};
Expand Down
46 changes: 20 additions & 26 deletions test/base/HooksManager.spec.ts
View file
Original file line number Diff line number Diff line change
@@ -1,47 +1,52 @@
import hre from "hardhat";
import { loadFixture } from "@nomicfoundation/hardhat-toolbox/network-helpers";
import hre, { deployments } from "hardhat";
import { expect } from "chai";
import { SignerWithAddress } from "@nomicfoundation/hardhat-ethers/signers";
import { getHooksWithPassingChecks } from "../utils/mockHooksBuilder";
import { IntegrationType } from "../utils/constants";

describe("HooksManager", async () => {
let deployer: SignerWithAddress, user1: SignerWithAddress;
let deployer: SignerWithAddress, owner: SignerWithAddress, user1: SignerWithAddress;

before(async () => {
[deployer, user1] = await hre.ethers.getSigners();
[deployer, owner, user1] = await hre.ethers.getSigners();
});

async function deployContractsFixture() {
[deployer, user1] = await hre.ethers.getSigners();
const setupTests = deployments.createFixture(async ({ deployments }) => {
await deployments.fixture();
[deployer, owner, user1] = await hre.ethers.getSigners();
const safeProtocolRegistry = await hre.ethers.deployContract("SafeProtocolRegistry", [owner.address]);

const hooksManager = await hre.ethers.deployContract("HooksManager", { signer: deployer });
// HooksManager is abstract so using SafeProtocolManager instance
const hooksManager = await hre.ethers.deployContract("SafeProtocolManager", [owner.address, safeProtocolRegistry.target], {
signer: deployer,
});
const hooks = await getHooksWithPassingChecks();

return { hooksManager, hooks };
}
await safeProtocolRegistry.connect(owner).addIntegration(hooks.target, IntegrationType.Hooks);
return { hooksManager, hooks, safeProtocolRegistry };
});

it("Should emit HooksChanged event when hooks are enabled", async () => {
const { hooksManager, hooks } = await loadFixture(deployContractsFixture);
const { hooksManager, hooks } = await setupTests();
const hooksAddress = await hooks.getAddress();
expect(await hooksManager.connect(user1).setHooks(hooksAddress))
.to.emit(hooksManager, "HooksChanged")
.withArgs(user1, hooksAddress);
});

it("Should return correct hooks address", async () => {
const { hooksManager, hooks } = await loadFixture(deployContractsFixture);
const { hooksManager, hooks } = await setupTests();
const hooksAddress = await hooks.getAddress();
await hooksManager.connect(user1).setHooks(hooksAddress);
expect(await hooksManager.getEnabledHooks(user1.address)).to.be.equal(hooksAddress);
});

it("Should return zero address if hooks are not enabled", async () => {
const { hooksManager } = await loadFixture(deployContractsFixture);
const { hooksManager } = await setupTests();
expect(await hooksManager.getEnabledHooks(user1.address)).to.be.equal(hre.ethers.ZeroAddress);
});

it("Should return zero address if hooks address is reset to zero address", async () => {
const { hooksManager, hooks } = await loadFixture(deployContractsFixture);
const { hooksManager, hooks } = await setupTests();

const hooksAddress = await hooks.getAddress();
expect(await hooksManager.connect(user1).setHooks(hooksAddress));
Expand All @@ -50,19 +55,8 @@ describe("HooksManager", async () => {
});

it("Should revert if user attempts to set random address as hooks", async () => {
const { hooksManager } = await loadFixture(deployContractsFixture);
const { hooksManager } = await setupTests();
const hooksAddress = hre.ethers.getAddress(hre.ethers.hexlify(hre.ethers.randomBytes(20)));
await expect(hooksManager.setHooks(hooksAddress)).to.be.reverted;
});

it("Should revert AddressDoesNotImplementHooksInterface if user attempts address does not implement Hooks interface", async () => {
const { hooksManager } = await loadFixture(deployContractsFixture);
const contractNotImplementingHooksInterface = await (await hre.ethers.getContractFactory("MockContract")).deploy();
await contractNotImplementingHooksInterface.givenMethodReturnBool("0x01ffc9a7", false);

await expect(hooksManager.setHooks(await contractNotImplementingHooksInterface.getAddress())).to.be.revertedWithCustomError(
hooksManager,
"AddressDoesNotImplementHooksInterface",
);
});
});

0 comments on commit 402e7a0

Please sign in to comment.