From f719d7db3d09cfae6ac6f73950273d17a10515ef Mon Sep 17 00:00:00 2001 From: Spablob Date: Wed, 27 Nov 2024 16:43:03 +0000 Subject: [PATCH 1/2] add ERC165 check for external policy registration --- .../royalty/policies/IExternalRoyaltyPolicy.sol | 11 ++++------- .../policies/IExternalRoyaltyPolicyBase.sol | 11 +++++++++++ .../modules/royalty/policies/IRoyaltyPolicy.sol | 4 ++-- contracts/lib/Errors.sol | 3 +++ contracts/modules/royalty/RoyaltyModule.sol | 17 +++++++++++------ .../mocks/policy/MockExternalRoyaltyPolicy1.sol | 9 ++++++++- .../mocks/policy/MockExternalRoyaltyPolicy2.sol | 9 ++++++++- .../foundry/modules/royalty/RoyaltyModule.t.sol | 5 +++++ 8 files changed, 52 insertions(+), 17 deletions(-) create mode 100644 contracts/interfaces/modules/royalty/policies/IExternalRoyaltyPolicyBase.sol diff --git a/contracts/interfaces/modules/royalty/policies/IExternalRoyaltyPolicy.sol b/contracts/interfaces/modules/royalty/policies/IExternalRoyaltyPolicy.sol index 9038a1ca5..5ef4dfaaf 100644 --- a/contracts/interfaces/modules/royalty/policies/IExternalRoyaltyPolicy.sol +++ b/contracts/interfaces/modules/royalty/policies/IExternalRoyaltyPolicy.sol @@ -1,11 +1,8 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity 0.8.26; +import { IERC165 } from "@openzeppelin/contracts/utils/introspection/IERC165.sol"; +import { IExternalRoyaltyPolicyBase } from "./IExternalRoyaltyPolicyBase.sol"; + /// @title IExternalRoyaltyPolicy interface -interface IExternalRoyaltyPolicy { - /// @notice Returns the amount of royalty tokens required to link a child to a given IP asset - /// @param ipId The ipId of the IP asset - /// @param licensePercent The percentage of the license - /// @return The amount of royalty tokens required to link a child to a given IP asset - function getPolicyRtsRequiredToLink(address ipId, uint32 licensePercent) external view returns (uint32); -} +interface IExternalRoyaltyPolicy is IExternalRoyaltyPolicyBase, IERC165 {} diff --git a/contracts/interfaces/modules/royalty/policies/IExternalRoyaltyPolicyBase.sol b/contracts/interfaces/modules/royalty/policies/IExternalRoyaltyPolicyBase.sol new file mode 100644 index 000000000..5e44ab37b --- /dev/null +++ b/contracts/interfaces/modules/royalty/policies/IExternalRoyaltyPolicyBase.sol @@ -0,0 +1,11 @@ +// SPDX-License-Identifier: BUSL-1.1 +pragma solidity 0.8.26; + +/// @title IExternalRoyaltyPolicyBase interface +interface IExternalRoyaltyPolicyBase { + /// @notice Returns the amount of royalty tokens required to link a child to a given IP asset + /// @param ipId The ipId of the IP asset + /// @param licensePercent The percentage of the license + /// @return The amount of royalty tokens required to link a child to a given IP asset + function getPolicyRtsRequiredToLink(address ipId, uint32 licensePercent) external view returns (uint32); +} diff --git a/contracts/interfaces/modules/royalty/policies/IRoyaltyPolicy.sol b/contracts/interfaces/modules/royalty/policies/IRoyaltyPolicy.sol index 991151381..f30f76c0f 100644 --- a/contracts/interfaces/modules/royalty/policies/IRoyaltyPolicy.sol +++ b/contracts/interfaces/modules/royalty/policies/IRoyaltyPolicy.sol @@ -1,10 +1,10 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity 0.8.26; -import { IExternalRoyaltyPolicy } from "./IExternalRoyaltyPolicy.sol"; +import { IExternalRoyaltyPolicyBase } from "./IExternalRoyaltyPolicyBase.sol"; /// @title RoyaltyPolicy interface -interface IRoyaltyPolicy is IExternalRoyaltyPolicy { +interface IRoyaltyPolicy is IExternalRoyaltyPolicyBase { /// @notice Executes royalty related logic on minting a license /// @dev Enforced to be only callable by RoyaltyModule /// @param ipId The ipId whose license is being minted (licensor) diff --git a/contracts/lib/Errors.sol b/contracts/lib/Errors.sol index a87b89c43..05b1e9f9e 100644 --- a/contracts/lib/Errors.sol +++ b/contracts/lib/Errors.sol @@ -616,6 +616,9 @@ library Errors { /// @notice IP is expired. error RoyaltyModule__IpExpired(); + /// @notice Invalid external royalty policy. + error RoyaltyModule__InvalidExternalRoyaltyPolicy(); + //////////////////////////////////////////////////////////////////////////// // Royalty Policy LAP // //////////////////////////////////////////////////////////////////////////// diff --git a/contracts/modules/royalty/RoyaltyModule.sol b/contracts/modules/royalty/RoyaltyModule.sol index 1d5b26190..2956d1ba5 100644 --- a/contracts/modules/royalty/RoyaltyModule.sol +++ b/contracts/modules/royalty/RoyaltyModule.sol @@ -14,7 +14,7 @@ import { BaseModule } from "../BaseModule.sol"; import { VaultController } from "./policies/VaultController.sol"; import { IRoyaltyModule } from "../../interfaces/modules/royalty/IRoyaltyModule.sol"; import { IRoyaltyPolicy } from "../../interfaces/modules/royalty/policies/IRoyaltyPolicy.sol"; -import { IExternalRoyaltyPolicy } from "../../interfaces/modules/royalty/policies/IExternalRoyaltyPolicy.sol"; +import { IExternalRoyaltyPolicyBase } from "../../interfaces/modules/royalty/policies/IExternalRoyaltyPolicyBase.sol"; import { IGroupIPAssetRegistry } from "../../interfaces/registries/IGroupIPAssetRegistry.sol"; import { IIpRoyaltyVault } from "../../interfaces/modules/royalty/policies/IIpRoyaltyVault.sol"; import { IDisputeModule } from "../../interfaces/modules/dispute/IDisputeModule.sol"; @@ -223,12 +223,17 @@ contract RoyaltyModule is IRoyaltyModule, VaultController, ReentrancyGuardUpgrad $.isRegisteredExternalRoyaltyPolicy[externalRoyaltyPolicy] ) revert Errors.RoyaltyModule__PolicyAlreadyWhitelistedOrRegistered(); - // checks if the IExternalRoyaltyPolicy call does not revert // external royalty policies contracts should inherit IExternalRoyaltyPolicy interface - if (IExternalRoyaltyPolicy(externalRoyaltyPolicy).getPolicyRtsRequiredToLink(address(0), 0) >= uint32(0)) { - $.isRegisteredExternalRoyaltyPolicy[externalRoyaltyPolicy] = true; - emit ExternalRoyaltyPolicyRegistered(externalRoyaltyPolicy); - } + // and implement the getPolicyRtsRequiredToLink() and ERC165 supportsInterface() functions + if ( + !ERC165Checker.supportsInterface( + externalRoyaltyPolicy, + IExternalRoyaltyPolicyBase.getPolicyRtsRequiredToLink.selector + ) + ) revert Errors.RoyaltyModule__InvalidExternalRoyaltyPolicy(); + + $.isRegisteredExternalRoyaltyPolicy[externalRoyaltyPolicy] = true; + emit ExternalRoyaltyPolicyRegistered(externalRoyaltyPolicy); } /// @notice Executes royalty related logic on license minting diff --git a/test/foundry/mocks/policy/MockExternalRoyaltyPolicy1.sol b/test/foundry/mocks/policy/MockExternalRoyaltyPolicy1.sol index f1548c283..91bcbfd66 100644 --- a/test/foundry/mocks/policy/MockExternalRoyaltyPolicy1.sol +++ b/test/foundry/mocks/policy/MockExternalRoyaltyPolicy1.sol @@ -1,11 +1,18 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity 0.8.26; +import { IERC165, ERC165 } from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; + // solhint-disable-next-line max-line-length import { IExternalRoyaltyPolicy } from "../../../../contracts/interfaces/modules/royalty/policies/IExternalRoyaltyPolicy.sol"; -contract MockExternalRoyaltyPolicy1 is IExternalRoyaltyPolicy { +contract MockExternalRoyaltyPolicy1 is ERC165, IExternalRoyaltyPolicy { function getPolicyRtsRequiredToLink(address ipId, uint32 licensePercent) external view returns (uint32) { return licensePercent * 2; } + + /// @notice IERC165 interface support + function supportsInterface(bytes4 interfaceId) public override(ERC165, IERC165) view returns (bool) { + return interfaceId == this.getPolicyRtsRequiredToLink.selector || super.supportsInterface(interfaceId); + } } diff --git a/test/foundry/mocks/policy/MockExternalRoyaltyPolicy2.sol b/test/foundry/mocks/policy/MockExternalRoyaltyPolicy2.sol index bfeb3ef20..cc8280f3c 100644 --- a/test/foundry/mocks/policy/MockExternalRoyaltyPolicy2.sol +++ b/test/foundry/mocks/policy/MockExternalRoyaltyPolicy2.sol @@ -1,11 +1,18 @@ // SPDX-License-Identifier: BUSL-1.1 pragma solidity 0.8.26; +import { IERC165, ERC165 } from "@openzeppelin/contracts/utils/introspection/ERC165.sol"; + // solhint-disable-next-line max-line-length import { IExternalRoyaltyPolicy } from "../../../../contracts/interfaces/modules/royalty/policies/IExternalRoyaltyPolicy.sol"; -contract MockExternalRoyaltyPolicy2 is IExternalRoyaltyPolicy { +contract MockExternalRoyaltyPolicy2 is ERC165, IExternalRoyaltyPolicy { function getPolicyRtsRequiredToLink(address ipId, uint32 licensePercent) external view returns (uint32) { return 10 * 10 ** 6; } + + /// @notice IERC165 interface support + function supportsInterface(bytes4 interfaceId) public override(ERC165, IERC165) view returns (bool) { + return interfaceId == this.getPolicyRtsRequiredToLink.selector || super.supportsInterface(interfaceId); + } } diff --git a/test/foundry/modules/royalty/RoyaltyModule.t.sol b/test/foundry/modules/royalty/RoyaltyModule.t.sol index 4a9efbfff..2e7497d6e 100644 --- a/test/foundry/modules/royalty/RoyaltyModule.t.sol +++ b/test/foundry/modules/royalty/RoyaltyModule.t.sol @@ -380,6 +380,11 @@ contract TestRoyaltyModule is BaseTest { royaltyModule.registerExternalRoyaltyPolicy(address(royaltyPolicyLAP)); } + function test_RoyaltyModule_registerExternalRoyaltyPolicy_revert_InvalidExternalRoyaltyPolicy() public { + vm.expectRevert(Errors.RoyaltyModule__InvalidExternalRoyaltyPolicy.selector); + royaltyModule.registerExternalRoyaltyPolicy(address(1)); + } + function test_RoyaltyModule_registerExternalRoyaltyPolicy() public { address externalRoyaltyPolicy = address(new MockExternalRoyaltyPolicy1()); assertEq(royaltyModule.isRegisteredExternalRoyaltyPolicy(externalRoyaltyPolicy), false); From 3730db2313f9298e80f756becd663acac9e60e34 Mon Sep 17 00:00:00 2001 From: Spablob Date: Wed, 27 Nov 2024 16:44:01 +0000 Subject: [PATCH 2/2] fix lint --- test/foundry/mocks/policy/MockExternalRoyaltyPolicy1.sol | 2 +- test/foundry/mocks/policy/MockExternalRoyaltyPolicy2.sol | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/test/foundry/mocks/policy/MockExternalRoyaltyPolicy1.sol b/test/foundry/mocks/policy/MockExternalRoyaltyPolicy1.sol index 91bcbfd66..2a8ade403 100644 --- a/test/foundry/mocks/policy/MockExternalRoyaltyPolicy1.sol +++ b/test/foundry/mocks/policy/MockExternalRoyaltyPolicy1.sol @@ -12,7 +12,7 @@ contract MockExternalRoyaltyPolicy1 is ERC165, IExternalRoyaltyPolicy { } /// @notice IERC165 interface support - function supportsInterface(bytes4 interfaceId) public override(ERC165, IERC165) view returns (bool) { + function supportsInterface(bytes4 interfaceId) public view override(ERC165, IERC165) returns (bool) { return interfaceId == this.getPolicyRtsRequiredToLink.selector || super.supportsInterface(interfaceId); } } diff --git a/test/foundry/mocks/policy/MockExternalRoyaltyPolicy2.sol b/test/foundry/mocks/policy/MockExternalRoyaltyPolicy2.sol index cc8280f3c..deb2f113a 100644 --- a/test/foundry/mocks/policy/MockExternalRoyaltyPolicy2.sol +++ b/test/foundry/mocks/policy/MockExternalRoyaltyPolicy2.sol @@ -12,7 +12,7 @@ contract MockExternalRoyaltyPolicy2 is ERC165, IExternalRoyaltyPolicy { } /// @notice IERC165 interface support - function supportsInterface(bytes4 interfaceId) public override(ERC165, IERC165) view returns (bool) { + function supportsInterface(bytes4 interfaceId) public view override(ERC165, IERC165) returns (bool) { return interfaceId == this.getPolicyRtsRequiredToLink.selector || super.supportsInterface(interfaceId); } }