Invalid Parameter caller for HookModule verification

[jdubpark]

Problem

Each commercial policy has a commercializer checker that gets called for verification on minting and linking.

See below for the verification callbacks

protocol-core/contracts/modules/licensing/PILPolicyFrameworkManager.sol

Lines 104 to 110 in 03b8c2b

	if (policy.commercializerChecker != address(0)) {
	// No need to check if the commercializerChecker supports the IHookModule interface, as it was checked
	// when the policy was registered.
	if (!IHookModule(policy.commercializerChecker).verify(caller, policy.commercializerCheckerData)) {
	return false;
	}
	}

protocol-core/contracts/modules/licensing/PILPolicyFrameworkManager.sol

Lines 138 to 144 in 03b8c2b

	if (policy.commercializerChecker != address(0)) {
	// No need to check if the commercializerChecker supports the IHookModule interface, as it was checked
	// when the policy was registered.
	if (!IHookModule(policy.commercializerChecker).verify(caller, policy.commercializerCheckerData)) {
	return false;
	}
	}

The issue with the current implementation is that the caller parameter in not cessearily the IPAccount owner.

Here is an example call path that leads to the bug (verification failing):

1. IP owner calls registrationModule.registerDerivativeIP()
2. There, RegistrationModule calls licensingModule.linkIpToParents()
3. Now, in verifyLink (called via linkIpToParents), the value of caller is the RegistrationModule address.

In particular, if I use a token-gated hook module as the commercializer checker, then the hook module will check for NFT ownership of the RegistrationModule, since caller is the registration module.

This is NOT an intended behavior - we want to check ownership of the actual caller (EOA or IPAccount).

This bug will lead to reverts/fails (return "false" on hook module verification) on all contract-to-contract interactions that call linkIpToParents with a policy that has a commercializer checker (so, the caller is a contract, not EOA or IPAccount).

This breaks

• RegistrationModule’s registerDerivativeIp
• IPAssetRegistry’s register (for derivative IP)
• SPG’s or any external contract’s call to linkIpToParents

This logic was introduced in #85 but was uncaught in integration tests due to many moving pieces in contracts and logic after the PR. Particularly, this bug is only reproducible with a valid PolicyFrameworkManager, in our case PIL PFM, which was finalized yesterday / today.

Solution

We can pass in licensee address for the caller parameter.

protocol-core/contracts/modules/licensing/LicensingModule.sol

Line 301 in 03b8c2b

_linkIpToParent(i, licenseId, licenseData.policyId, pol, licenseData.licensorIpId, childIpId);

When the LicensingModule calls verifyLink or verifyLink, it knows the value of the licensee (owner of the licenses). So we can trust the module to pass in the correct value.

protocol-core/contracts/modules/licensing/LicensingModule.sol

Lines 291 to 293 in 03b8c2b

	if (!LICENSE_REGISTRY.isLicensee(licenseId, holder)) {
	revert Errors.LicensingModule__NotLicensee();
	}

[LeoHChen]

My thinking is to disable the commercializers for beta release. We may need some more time to rethink how is can be supported post beta.

[jdubpark]

Thoughts @kingster-will @Ramarti ?

The patch in #119 solves these issues, which are checked with additional tests in the integration test. I think commercializers are important for beta release, especially to observe the potential capability/creativity of hooks for minting and licensing.

[Ramarti]

I think minting is pretty well defined, either:

• Permissioned (only licensor IPA, owner or delegated by IPA + policy parameters)
• Permissionless (only policy parameters)

Policy parameters means the address should pass commercializers
For linking, we need the caller to be:

• Holding the license and
• Owning the child IPA, being the IPA or delegated by IPA

Up to the caller how to satisfy that

Commercializers is just a hook. I think the most confusing part is the name, which comes straight from the legal text.

I would be to make more explicit in the code that is a Hook, by renaming the variable. For the PIL to make sense, the metadata should still say "Commercializers" in the attribute, since the hook defines those.

[kingster-will]

The issue in question is not related to the HookModule, nor is it caused by the code snippet highlighted in the issue description. Instead, the core of the issue lies in determining who should possess the NFT during the verification of the commercializer.

To resolve this, we need to decide the rightful license holder during the IP Parent linking process. We have three potential options:

1. The Function Caller: This could be the delegator, IP account, or IP account owner. If we decide that the caller should hold the license, then the current code behavior aligns with this expectation.
2. The IP Account Owner: If we choose this option, we need to consider whether the IP account owner should always be the same entity as the license NFT holder.
3. The IP Account Itself: Conceptually, this option is clearer, as it involves the IP account holding the license NFT and linking itself to the parent IP. This approach could be more secure, as it requires transferring the license NFT directly to the IP account.

Each option has implications:

• If we select option 1, it's necessary for the NFT to be transferred to the caller before linking, if they are not already the holder.
• In option 2, the key question is the feasibility of enforcing that the IP account owner and the license NFT holder are always the same.
• For option 3, the straightforward requirement is that the license NFT must be transferred to the IP account. This seems more secure and conceptually sound.

[LeoHChen]

I felt the issue is why we allow RegistrationModule call linkIPtoParent?