Default License Terms should remain immutable for old IPs

[jdubpark]

Issue

Currently, the protocol admin can change the "default" license terms of IPs at any time.

Old root and child IPs that were registered before the change must retain the old license terms. Only the IPs registered after the change should reflect the new license terms.

For example, (assume all licenses are from Template A)

1. Default License Term is set to Template A, ID 1
2. Root IP 1 is created, attached Template A, ID 2
3. Child IP is created
4. Default License Term is changed to Template A, ID 3
5. Root IP 2 is created

Expected Behavior

After 4, we expect

• Root IP 1 has terms: ID 1 (old default) and ID 2
• Root IP 2 has terms: ID 3 (new default)
• Child IP has terms: ID 1 (old default) and ID 2

Actual Behavior

After 4, we get

• Root IP 1 has terms: ID 3 (new default) and ID 2
• Root IP 2 has terms: ID 3 (new default)
• Child IP has terms: ID 3 (new default) and ID 2

Solution

With the current implementation, all IPs have a pointer to the default license terms, which can be updated anytime.

Instead, all IPs should store the value of default license terms at the time of registration. Alternatively, there should be a "version control" for default license terms, and pointers point to the version (index).

[jdubpark]

45 has a suggestion to make the default terms optional (ie. IP can refuse to attach the default terms)

[kingster-will]

I don't think we should remove the setDefaultLicenseTerms() . The setDefaultLicenseTerms() cannot be called by Procotol Admin. If the protocol admin does something bad, the complete protocol won't work. With setDefaultLicenseTerms(), it will allow us to adjust the default terms in case needed.

[jdubpark]

The flexibility of setDefaultLicenseTerms means there's a non-zero chance of the protocol freely changing the default license terms. I'm unsure what case requires us to change the default license terms at all.

Changing the default terms will have huge protocol-wide implications, such as "What does it mean for child IPs to be licensed using obsolete default terms, which are no longer 'attached' to parent IP"

[jdubpark]

+ I think changing default license terms is not legally allowed, e.g. suddenly all IPs are forced to permit to permissionless minting of new license terms, which was not a known requirement previously.

[LeoHChen]

Let's write a testcase to cover if default license term updated.

[jdubpark]

This test case fails (noted where they fail), showing that IP default term invariance is not satisfied.

function test_LicensingModule_Invariants() public {
    uint256 defaultTermsId = pilTemplate.registerLicenseTerms(PILFlavors.defaultValuesLicenseTerms());
    uint256 ncSocialRemixTermsId = pilTemplate.registerLicenseTerms(PILFlavors.nonCommercialSocialRemixing());
    uint256 comRemixTermsId = pilTemplate.registerLicenseTerms(
        PILFlavors.commercialRemix(0, 0, address(royaltyPolicyLAP), address(erc20))
    );

    // (1) Set default license terms
    vm.prank(u.admin);
    licenseRegistry.setDefaultLicenseTerms(address(pilTemplate), ncSocialRemixTermsId);

    // Register Root IP
    mockNft.mintId(ipOwner1, 10);
    address ipId = ipAssetRegistry.register(address(mockNft), 10);

    // Attach Commercial Remix terms to Root IP
    vm.prank(ipOwner1);
    licensingModule.attachLicenseTerms(ipId, address(pilTemplate), comRemixTermsId);

    // Invariant: we expect Root IP to have two terms 
    // 1. default terms from (1) - NC Social Remix
    // 2. commercial remix terms
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), ncSocialRemixTermsId));

    // Create child IP
    mockNft.mintId(ipOwner1, 11);
    address childIpId = ipAssetRegistry.register(address(mockNft), 11);
    address[] memory parentIpIds = new address[](1);
    parentIpIds[0] = ipId;
    uint256[] memory licenseTermsIds = new uint256[](1);
    licenseTermsIds[0] = comRemixTermsId;

    vm.prank(ipOwner1);
    licensingModule.registerDerivative(childIpId, parentIpIds, licenseTermsIds, address(pilTemplate), "");

    // Invariant: we expect Child IP to have two terms
    // 1. default terms from (1) - NC Social Remix
    // 2. commercial remix terms
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), ncSocialRemixTermsId));
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), comRemixTermsId));

    // (2) Set default license terms AGAIN
    vm.prank(u.admin);
    licenseRegistry.setDefaultLicenseTerms(address(pilTemplate), defaultTermsId);

    // Invariant: we expect Root IP to have two terms
    // 1. default terms from (1) - NC Social Remix
    // 2. commercial remix terms
    // !!THIS FAILS!!
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), ncSocialRemixTermsId));
    // (this succeeds)
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), comRemixTermsId));

    // Invariant: we expect Child IP to have two terms
    // 1. default terms from (1) - NC Social Remix 
    // 2. commercial remix terms

    // !!THIS FAILS!!
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), ncSocialRemixTermsId));
    // (this succeeds)
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), comRemixTermsId));
}

[jzhao23]

This is a huge security risk, thanks for flagging

Solutions in order of preference:

1. "all IPs should store the value of default license terms at the time of registration. "
2. Preventing Admin from changing default license ever. (not preferred)

[kingster-will]

To clarify, in the aforementioned test case, the child IP does not link to the parent IP via the default license (ncSocialRemixTermsId). Instead, the child IP links to the parent IP under a different license (comRemixTermsId) that is attached by the parent IP's owner. Therefore, changes to the default license are irrelevant to the relationships of derivative works in this scenario. I find the test case slightly misleading because it presents both the default license and derivative, but does not actually use the default license to register the derivative IP.

A more relevant test case would involve registering a child IP as a derivative of a parent IP through the default license (ncSocialRemixTermsId), then changing the default license to a different one (defaultTermsId). Such a test would help us confirm that the child IP remains linked to the original default license (ncSocialRemixTermsId), even after the default has been changed.

The actual current behavior of the default license is as follows:

1. Every IP has a default license (e.g., ID: 1).
2. A child IP can link to a parent IP using this default license (ID: 1).
3. The Protocol Admin can change the default license to a new one (ID: 2).
4. Consequently, the default license for all IPs updates to the new license (ID: 2).
5. Existing child IPs remain linked to the original default license (ID: 1) with NO CHANGES.
6. New child IPs can only establish links to a parent IP using the new default license (ID: 2).
7. New child IPs cannot link to a parent IP using the old default license (ID: 1).

In summary:

1. The Protocol Admin has the authority to modify the default license for all existing IPs.
2. Such changes do not affect existing child IPs.
3. Only new child IPs are impacted by changes to the default license.

We might consider introducing a feature that allows existing IPs to retain their old default license (ID:1). However, we must also support the Protocol Admin's ability to change the old default license of existing IPs, to ensure we can manage unforeseen circumstances. Importantly, changing the default license of existing IPs poses no risk, as it does not affect existing child IPs and only impacts new child IPs. This approach aligns with how we handle license expiration and IP owners disabling default license per IP (if supported).

[jdubpark]

The purpose of this PR is to disable

We must also support the Protocol Admin's ability to change the old default license of existing IPs, to ensure we can manage unforeseen circumstances

I'm certain that this is not legally allowed as we are suddenly asking old IPs to adhere to new default license terms. At the time of registration, IPs agreed on the old default license terms, not the new one. If these IPs don't agree with the new default license terms, there's nothing they can do (there's no "deregister").

This is what we should do

all IPs should store the value of default license terms at the time of registration.

[jdubpark]

I've updated the test case to the steps you've described, and it still fails, at step 5.

IPs that are linked using the default license don't actually "attach" the terms, but maintain the pointer to the default license terms. The value dereferenced from this pointer can change, and the invariance is not satisfied. We do not want this, and I believe it is not legally allowed to have the parent-child link change.

function test_LicensingModule_Invariants() public {
    uint256 ncSocialRemixTermsId = pilTemplate.registerLicenseTerms(PILFlavors.nonCommercialSocialRemixing());

    // (1) Set default license terms
    vm.prank(u.admin);
    licenseRegistry.setDefaultLicenseTerms(address(pilTemplate), ncSocialRemixTermsId);

    // Register Root IP
    mockNft.mintId(ipOwner1, 10);
    address ipId = ipAssetRegistry.register(address(mockNft), 10);

    // Invariant: we expect Root IP to have one term
    // 1. default terms from (1) - NC Social Remix
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), ncSocialRemixTermsId));

    // Create child IP
    mockNft.mintId(ipOwner1, 11);
    address childIpId = ipAssetRegistry.register(address(mockNft), 11);
    address[] memory parentIpIds = new address[](1);
    parentIpIds[0] = ipId;
    uint256[] memory licenseTermsIds = new uint256[](1);
    licenseTermsIds[0] = ncSocialRemixTermsId;

    vm.prank(ipOwner1);
    licensingModule.registerDerivative(childIpId, parentIpIds, licenseTermsIds, address(pilTemplate), "");

    // Invariant: we expect Child IP to have one term
    // 1. default terms parent - NC Social Remix
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), ncSocialRemixTermsId));

    // (2) Set default license terms AGAIN
    vm.prank(u.admin);
    licenseRegistry.setDefaultLicenseTerms(address(pilTemplate), defaultTermsId);

    // Invariant: we expect Root IP to have one term
    // 1. default terms from (1) - NC Social Remix
    // !!THIS FAILS!!
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), ncSocialRemixTermsId));

    // Invariant: we expect Child IP to have one term
    // 1. default terms parent - NC Social Remix
    // !!THIS FAILS!!
    assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(ipId, address(pilTemplate), ncSocialRemixTermsId));
}

[jdubpark]

This can easily be fixed by actually attaching "default license terms" as a license term in IPs.

Currently, it's treated as a special case in relevant checks and getters. This "special treatment" dynamically loads the values, resulting in mutability for existing IPs.

If we attach default terms, then the value (license token template & ID) is permanently stored in IPs. Thus, we treat it like any other license terms attached, making it immutable for existing IPs.

[kingster-will]

I'm certain that this is not legally allowed as we are suddenly asking old IPs to adhere to new default license terms. At the time of registration, IPs agreed on the old default license terms, not the new one. If these IPs don't agree with the new default license terms, there's nothing they can do (there's no "deregister").

IP registration is permissionless; anyone can register an IP, which means there is no assumption that the IP owner accepts the default license terms upon registration.

The Protocol Admin should have the capability to change the default license terms of existing IPs. However, this does not imply that we will change the default license terms frequently or without careful consideration.

While we might not really change the default license terms, maintaining the capability to do so is crucial for addressing unforeseen situations.

[kingster-will]

I've updated the test case to the steps you've described, and it still fails, at step 5. The link between child and parent NEVER change. the test case failure because your test code has a bug. you suppose to check default license of chaildIp per the comments, but actually check with paranteIp here.

The test case below will PASS.

    function test_LicensingModule_defaultLicenseChange() public {
        // Register License Terms of Non-Commercial Social Remixing (ID: 1)
        PILTerms memory ncSocialRemixingTerms = PILTerms({
            transferable: true,
            royaltyPolicy: address(0),
            mintingFee: 0,
            expiration: 0,
            commercialUse: false,
            commercialAttribution: false,
            commercializerChecker: address(0),
            commercializerCheckerData: "",
            commercialRevShare: 0,
            commercialRevCelling: 0,
            derivativesAllowed: true,
            derivativesAttribution: true,
            derivativesApproval: false,
            derivativesReciprocal: true,
            derivativeRevCelling: 0,
            currency: address(0)
        });
        uint256 defaultTermsId = piLicenseTemplate.registerLicenseTerms(ncSocialRemixingTerms);

        // Set default license terms to Non-Commercial Social Remixing (ID: 1)
        vm.prank(admin);
        licenseRegistry.setDefaultLicenseTerms(address(piLicenseTemplate), defaultTermsId);

        // Register Parent IP
        uint256 tokenId1 = mockNft.mint(alice);
        address parentIpId = ipAssetRegistry.register(address(mockNft), tokenId1);

        // 1. Expect Parent IP has default license terms (ID: 1)
        assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(parentIpId, address(piLicenseTemplate), defaultTermsId));

        // Create Child IP
        uint256 tokenId2 = mockNft.mint(bob);
        address childIpId = ipAssetRegistry.register(address(mockNft), tokenId2);

        // Link Child IP to Parent IP using default License Terms (ID: 1)
        address[] memory parentIpIds = new address[](1);
        parentIpIds[0] = parentIpId;
        uint256[] memory licenseTermsIds = new uint256[](1);
        licenseTermsIds[0] = defaultTermsId;  // default license terms (ID: 1)
        vm.prank(bob);
        licensingModule.registerDerivative(childIpId, parentIpIds, licenseTermsIds, address(piLicenseTemplate), "");

        // 2. Expect Child IP has default license terms (ID: 1)
        assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(childIpId, address(piLicenseTemplate), defaultTermsId));

        // Register new License Terms for Non-Commercial Social Remixing (ID: 2)
        ncSocialRemixingTerms.transferable = false; // adjust default terms to disable LT transfer
        uint256 newDefaultTermsId = piLicenseTemplate.registerLicenseTerms(ncSocialRemixingTerms);

        // Protocol Admin change default license terms to new one (ID: 2)
        vm.prank(admin);
        licenseRegistry.setDefaultLicenseTerms(address(piLicenseTemplate), newDefaultTermsId);

        // 3. Expect Parent IP has new default license terms (ID: 2)
        assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(parentIpId, address(piLicenseTemplate), newDefaultTermsId));
        // 4. Expect Parent IP has NOT old default license terms (ID: 1)
        assertFalse(licenseRegistry.hasIpAttachedLicenseTerms(parentIpId, address(piLicenseTemplate), defaultTermsId));

        // 5. Expect NO CHANGE: Child IP still link to OLD default license terms (ID: 1) which inherited from Parent IP
        assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(childIpId, address(piLicenseTemplate), defaultTermsId));
        // 6. Expect Child IP also has new default license terms (ID: 2)
        assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(childIpId, address(piLicenseTemplate), newDefaultTermsId));
    }

[jdubpark]

The test passes because you expect the parent to have the new license terms.

I see that since it's permissionless registration, IP owner doesn't have control over their time of registration.

I think the simplest approach is to have immutable, NC Social Remix as default for v1, and if we see the need to change, upgrade to reflect the new license terms for all IPs.

Having a setter that can change terms anytime vs. governance upgrade with interval, the latter is much preferred. I don't think we should ever update the default license terms, but who knows what will happen.

[LeoHChen]

I think the question is a governance question. Do we want the protocol admin update the default license or not without upgrade of the contract.

My opinion is this could be a governance option, just like a protocol fee can be enabled or not.

@jzhao23 @Ramarti

[jdubpark]

Here's the comment from @jzhao23

I strongly support the recommendation attached - this is a P0 and I don’t think we can have any further conversation on this. We CANNOT allow a change in the default license to change EXISTING licenses that were registered under the old default license.

There is a separate question, which is should the protocol admin be able to change the default license at all? I think that is very reasonable.

To summarize, two separate decisions:

1. If default license changes, does it change existing IPA terms which were registered under the OLD default license (ie, their license changes to the new default license automatically)? Certainly not, I cannot see a single case where this would be useful, and in every case, it is extremely hazardous
2. Can the protocol admin change default license ever? Imo, but feel less strongly, and this is less of an red alert decision

[LeoHChen]

For 1, I don't think any existing license token will be impacted. @kingster-will please provide more details or code snippet to elaborate.

[jdubpark]

• Existing IPs will use new default terms, per the test cases above (both Kingter and mine).
• Existing License Tokens are not impacted.

See for the first point:

// 3. Expect Parent IP has new default license terms (ID: 2)
assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(parentIpId, address(piLicenseTemplate), newDefaultTermsId));
// 4. Expect Parent IP has NOT old default license terms (ID: 1)
assertFalse(licenseRegistry.hasIpAttachedLicenseTerms(parentIpId, address(piLicenseTemplate), defaultTermsId));

[kingster-will]

For 1, I don't think any existing license token will be impacted. @kingster-will please provide more details or code snippet to elaborate.

Yes, for 1, this aligns with current system behavior. Licenses for child IPs that were registered under a parent IP's previous default license will remain unchanged, even after a new default license has been set.

  // 5. Expect NO CHANGE: Child IP still link to OLD default license terms (ID: 1) which inherited from Parent IP
  assertTrue(licenseRegistry.hasIpAttachedLicenseTerms(childIpId, address(piLicenseTemplate), defaultTermsId));

[jdubpark]

To summarize, two different behaviors.

Child will have

1. New default license terms
2. Old license terms attached from parents

I want to note that (2) is as expected/desired, but (1) is where the debate is on.

Two sides:

1. Changing default license terms should not affect the default terms of existing IPs.
2. Changing default license terms should affect the default terms of existing IPs.