Skip to content

Commit

Permalink
Refactor Access Control in HookRegistry Contract (#175)
Browse files Browse the repository at this point in the history
* Refactor hook registry access control by IPOrg owner
* Add hooksRegistry() function to modules
  • Loading branch information
kingster-will authored Nov 16, 2023
1 parent cfb2a85 commit dde3a04
Show file tree
Hide file tree
Showing 9 changed files with 174 additions and 169 deletions.
4 changes: 2 additions & 2 deletions contracts/lib/Errors.sol
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,8 @@ library Errors {
error HookRegistry_RegisteringDuplicatedHook();
/// @notice This error is thrown when trying to register a hook with the address 0.
error HookRegistry_RegisteringZeroAddressHook();
/// @notice This error is thrown when the caller is not an admin of the HookRegistry.
error HookRegistry_CallerNotAdmin();
/// @notice This error is thrown when the caller is not IP Org owner.
error HookRegistry_CallerNotIPOrgOwner();
/// @notice This error is thrown when trying to register more than the maximum allowed number of hooks.
error HookRegistry_MaxHooksExceeded();
/// @notice This error is thrown when the length of the hooks configuration array does not match the length of the hooks array.
Expand Down
1 change: 0 additions & 1 deletion contracts/modules/base/BaseModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -125,7 +125,6 @@ abstract contract BaseModule is IModule, HookRegistry {
return result == HookResult.Completed;
}

function _hookRegistryAdmin() virtual override internal view returns (address);
function _configure(IIPOrg ipOrg_, address caller_, bytes calldata params_) virtual internal returns (bytes memory);
function _verifyExecution(IIPOrg ipOrg_, address caller_, bytes calldata params_) virtual internal {}
function _performAction(IIPOrg ipOrg_, address caller_, bytes calldata params_) virtual internal returns (bytes memory result) {}
Expand Down
32 changes: 17 additions & 15 deletions contracts/modules/base/HookRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pragma solidity ^0.8.13;

import { Errors } from "contracts/lib/Errors.sol";
import { IHook } from "contracts/interfaces/hooks/base/IHook.sol";
import { IIPOrg } from "contracts/interfaces/ip-org/IIPOrg.sol";

/// @title HookRegistry
/// @notice This contract is an abstract contract that manages the registration of hooks.
Expand All @@ -29,29 +30,34 @@ abstract contract HookRegistry {
event HooksRegistered(HookType indexed hType, bytes32 indexed registryKey, address[] indexed hook);
event HooksCleared(HookType indexed hType, bytes32 indexed registryKey);

/// @dev Modifier to check if the caller is the hook registry admin.
/// Reverts if the caller is not the admin.
modifier onlyHookRegistryAdmin() {
if (msg.sender != _hookRegistryAdmin())
revert Errors.HookRegistry_CallerNotAdmin();
/// @dev Modifier to check if the caller is the IPOrg owner.
/// Reverts if the caller is not the IP Org owner.
modifier onlyIpOrgOwner(IIPOrg ipOrg_) {
if (address(ipOrg_) == address(0)) {
revert Errors.ZeroAddress();
}

if (msg.sender != ipOrg_.owner())
revert Errors.HookRegistry_CallerNotIPOrgOwner();
_;
}

/// @dev Registers hooks for a specific type and registry key.
/// Clears any existing hooks for the same type and registry key.
/// Emits a HooksRegistered event.
/// Can only be called by the hook registry admin.
/// Can only be called by the IP Org owner.
/// @param hookType_ The type of the hooks to register.
/// @param registryKey_ The registry key for the hooks.
/// @param hooks_ The addresses of the hooks to register.
/// @param hooksConfig_ The configurations for the hooks.
function registerHooks(
HookType hookType_,
IIPOrg ipOrg_,
bytes32 registryKey_,
address[] calldata hooks_,
bytes[] calldata hooksConfig_
) public onlyHookRegistryAdmin {
clearHooks(hookType_, registryKey_);
) public onlyIpOrgOwner(ipOrg_) {
clearHooks(hookType_, ipOrg_, registryKey_);
_registerHooks(
_hooksForType(hookType_, registryKey_),
_hooksConfigForType(hookType_, registryKey_),
Expand Down Expand Up @@ -134,13 +140,14 @@ abstract contract HookRegistry {

/// @dev Clears all hooks for a specific type and registry key.
/// Emits a HooksCleared event.
/// Can only be called by the hook registry admin.
/// Can only be called by the IP Org owner.
/// @param hookType_ The type of the hooks to clear.
/// @param registryKey_ The registry key for the hooks.
function clearHooks(
HookType hookType_,
IIPOrg ipOrg_,
bytes32 registryKey_
) public onlyHookRegistryAdmin {
) public onlyIpOrgOwner(ipOrg_) {
if (hookType_ == HookType.PreAction && _preActionHooks[registryKey_].length > 0) {
delete _preActionHooks[registryKey_];
delete _preActionHooksConfig[registryKey_];
Expand All @@ -164,11 +171,6 @@ abstract contract HookRegistry {
return _hookIndex(_hooksForType(hookType_, registryKey_), hook_);
}

/// @dev Returns the address of the hook registry admin.
/// This function should be overridden in derived contracts to provide the actual admin address.
/// @return The address of the hook registry admin.
function _hookRegistryAdmin() internal view virtual returns (address);

/// @dev Returns the hooks for a specific type and registry key.
/// @param hookType_ The type of the hooks.
/// @param registryKey_ The registry key for the hooks.
Expand Down
10 changes: 0 additions & 10 deletions contracts/modules/licensing/LicenseCreatorModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -122,16 +122,6 @@ contract LicenseCreatorModule is BaseModule, TermsRepository {
}
}

function _hookRegistryAdmin()
internal
view
virtual
override
returns (address)
{
return address(0);
}

////////////////////////////////////////////////////////////////////////////
// Create License //
////////////////////////////////////////////////////////////////////////////
Expand Down
36 changes: 22 additions & 14 deletions contracts/modules/registration/RegistrationModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,23 @@ contract RegistrationModule is BaseModule, IRegistrationModule, AccessControlled
address accessControl_
) BaseModule(params_) AccessControlled(accessControl_) {}


/// @notice Registers hooks for a specific type and IP Org.
/// @dev This function can only be called by the IP Org owner.
/// @param hType_ The type of the hooks to register.
/// @param ipOrg_ The IP Org for which the hooks are being registered.
/// @param hooks_ The addresses of the hooks to register.
/// @param hooksConfig_ The configurations for the hooks.
function registerHooks(
HookType hType_,
IIPOrg ipOrg_,
address[] calldata hooks_,
bytes[] calldata hooksConfig_
) external onlyIpOrgOwner(ipOrg_) {
bytes32 registryKey = _generateRegistryKey(ipOrg_);
registerHooks(hType_, ipOrg_, registryKey, hooks_, hooksConfig_);
}

/// @notice Gets the contract URI for an IP Org.
/// @param ipOrg_ The address of the IP Org.
function contractURI(address ipOrg_) public view returns (string memory) {
Expand Down Expand Up @@ -278,24 +295,15 @@ contract RegistrationModule is BaseModule, IRegistrationModule, AccessControlled
}
}

/// @dev Returns the administrator for the registration module hooks.
/// TODO(kingter) Define the administrator for this call.
function _hookRegistryAdmin()
internal
view
virtual
override
returns (address)
{
return address(0);
}

function _hookRegistryKey(
IIPOrg ipOrg_,
address,
bytes calldata params_
bytes calldata
) internal view virtual override returns(bytes32) {
return keccak256(abi.encode(address(ipOrg_), "REGISTRATION"));
return _generateRegistryKey(ipOrg_);
}

function _generateRegistryKey(IIPOrg ipOrg_) private pure returns(bytes32) {
return keccak256(abi.encode(address(ipOrg_), "REGISTRATION"));
}
}
36 changes: 24 additions & 12 deletions contracts/modules/relationships/RelationshipModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,25 @@ contract RelationshipModule is BaseModule, IRelationshipModule, AccessControlled
address accessControl_
) BaseModule(params_) AccessControlled(accessControl_) {}



/// @notice Registers hooks for a specific hook type, based on IP Org and relationship type.
/// @dev This function can only be called by the IP Org owner.
/// @param hType_ The type of the hooks to register.
/// @param ipOrg_ The IP Org for which the hooks are being registered.
/// @param relType_ The relationship type for which the hooks are being registered.
/// @param hooks_ The addresses of the hooks to register.
/// @param hooksConfig_ The configurations for the hooks.
function registerHooks(
HookType hType_,
IIPOrg ipOrg_,
string calldata relType_,
address[] calldata hooks_,
bytes[] calldata hooksConfig_
) external onlyIpOrgOwner(ipOrg_) {
bytes32 registryKey = _generateRegistryKey(ipOrg_, relType_);
registerHooks(hType_, ipOrg_, registryKey, hooks_, hooksConfig_);
}

/// Gets relationship type definition for a given relationship type name
/// Will revert if no relationship type is found
/// @param ipOrg_ IP Org address or zero address for protocol level relationships
Expand Down Expand Up @@ -76,16 +94,6 @@ contract RelationshipModule is BaseModule, IRelationshipModule, AccessControlled
return _relHashes[keccak256(abi.encode(rel_))] != 0;
}

function _hookRegistryAdmin()
internal
view
virtual
override
returns (address)
{
return address(0); // TODO
}

/// Relationship module supports configuration to add or remove relationship types
function _configure(
IIPOrg ipOrg_,
Expand Down Expand Up @@ -252,6 +260,10 @@ contract RelationshipModule is BaseModule, IRelationshipModule, AccessControlled
bytes calldata params_
) internal view virtual override returns(bytes32) {
LibRelationship.CreateRelationshipParams memory createParams = abi.decode(params_, (LibRelationship.CreateRelationshipParams));
return keccak256(abi.encode(address(ipOrg_), createParams.relType));
return _generateRegistryKey(ipOrg_, createParams.relType);
}

function _generateRegistryKey(IIPOrg ipOrg_, string memory relType_) private pure returns(bytes32) {
return keccak256(abi.encode(address(ipOrg_), relType_));
}
}
14 changes: 2 additions & 12 deletions test/foundry/mocks/MockBaseModule.sol
Original file line number Diff line number Diff line change
Expand Up @@ -36,16 +36,6 @@ contract MockBaseModule is BaseModule {
return _callStack[index_];
}

function _hookRegistryAdmin()
internal
view
virtual
override
returns (address)
{
return _admin;
}

function _configure(
IIPOrg ipOrg_,
address caller_,
Expand Down Expand Up @@ -77,9 +67,9 @@ contract MockBaseModule is BaseModule {
string memory hookRegistrationInfo_,
address[] calldata hooks_,
bytes[] calldata hooksConfig_
) external onlyHookRegistryAdmin {
) external onlyIpOrgOwner(ipOrg_) {
bytes32 registryKey = _generateRegistryKey(address(ipOrg_), hookRegistrationInfo_);
registerHooks(hType_, registryKey, hooks_, hooksConfig_);
registerHooks(hType_, ipOrg_, registryKey, hooks_, hooksConfig_);
}

function hookRegistryKey(
Expand Down
16 changes: 0 additions & 16 deletions test/foundry/mocks/MockHookRegistry.sol
Original file line number Diff line number Diff line change
Expand Up @@ -7,22 +7,6 @@ import { IIPOrg } from "contracts/interfaces/ip-org/IIPOrg.sol";
/// @title Mock Hook Registry
/// @notice This mock contract is used for testing the base hook registry.
contract MockHookRegistry is HookRegistry {
address public immutable ADMIN;


constructor() {
ADMIN = msg.sender;
}

function _hookRegistryAdmin()
internal
view
virtual
override
returns (address)
{
return ADMIN;
}

function hookRegistryKey(
address ipOrg_,
Expand Down
Loading

0 comments on commit dde3a04

Please sign in to comment.