Proof of Creativity Protocol - Story

The RoyaltyModule is designed to manage the flow of revenue between parent and child IP Assets. Revenue can flow through the system when licenses are minted or when tips are sent directly to an IPA. In such cases, the revenue is intended to be distributed fairly among the IPAs within a group, ensuring that each member receives their rightful share.

However, a vulnerability exists in the reward distribution logic that allows a malicious IP owner to manipulate the system and redirect all rewards to a single IP account within a group.

When a user calls the claimReward function defined in the GroupingModule contract, they provide an array of ipIds representing the IP Assets that should receive rewards.

function claimReward(address groupId, address token, address[] calldata ipIds) external whenNotPaused {
    IGroupRewardPool pool = IGroupRewardPool(GROUP_IP_ASSET_REGISTRY.getGroupRewardPool(groupId));
    // Trigger group pool to distribute rewards to group members' vaults
    uint256[] memory rewards = pool.distributeRewards(groupId, token, ipIds);
    emit ClaimedReward(groupId, token, ipIds, rewards);
}

The distributeRewards function, from EvenSplitGroupPool contract, then calculates the available rewards for each IP Asset by calling the internal _getAvailableReward.

function distributeRewards(
    address groupId,
    address token,
    address[] calldata ipIds
) external whenNotPaused onlyGroupingModule returns (uint256[] memory rewards) {
    rewards = _getAvailableReward(groupId, token, ipIds);
    uint256 totalRewards = 0;
    for (uint256 i = 0; i < ipIds.length; i++) {
        totalRewards += rewards[i];
    }
    if (totalRewards == 0) return rewards;
    IERC20(token).approve(address(ROYALTY_MODULE), totalRewards);
    EvenSplitGroupPoolStorage storage $ = _getEvenSplitGroupPoolStorage();
    for (uint256 i = 0; i < ipIds.length; i++) {
        if (rewards[i] == 0) continue;
        // Calculate pending reward for each IP
        $.ipRewardDebt[groupId][token][ipIds[i]] += rewards[i];
        // Call royalty module to transfer reward to IP's vault as royalty
        ROYALTY_MODULE.payRoyaltyOnBehalf(ipIds[i], groupId, token, rewards[i]);
    }
}

In the _getAvailableReward function, the total accumulated pool balance is divided equally among all IP Assets in the group (totalAccumulatedPoolBalance / totalIps) to determine the rewardPerIP. The function then calculates the individual reward for each IP Asset by subtracting any previously recorded reward debt. However, the function does not enforce uniqueness within the ipIds array, allowing the same IP Asset to appear multiple times, before the reward debt is updated.

function _getAvailableReward(
    address groupId,
    address token,
    address[] memory ipIds
) internal view returns (uint256[] memory) {
    EvenSplitGroupPoolStorage storage $ = _getEvenSplitGroupPoolStorage();
    uint256 totalIps = $.totalMemberIps[groupId];
    if (totalIps == 0) return new uint256[](ipIds.length);

    uint256 totalAccumulatedPoolBalance = $.poolBalance[groupId][token];
    uint256[] memory rewards = new uint256[](ipIds.length);
    for (uint256 i = 0; i < ipIds.length; i++) {
        // Ignore if IP is not added to pool
        if (!_isIpAdded(groupId, ipIds[i])) {
            rewards[i] = 0;
            continue;
        }
        uint256 rewardPerIP = totalAccumulatedPoolBalance / totalIps;
        rewards[i] = rewardPerIP - $.ipRewardDebt[groupId][token][ipIds[i]];
    }
    return rewards;
}

Because of this oversight, a malicious user can pass the same ipId multiple times in the ipIds array when calling claimReward. Each occurrence of the ipId in the array results in the rewardPerIP being added to the IP Asset's reward debt multiple times. Consequently, the IP Asset accumulates a disproportionately large share of the total rewards, effectively taking the entire reward pool designated to be shared among other users.

The following test demonstrates how a malicious user can exploit this vulnerability:

function test_GroupingModule_duplicatedIPs() public {
    vm.warp(100);

    //Alice registers a new group IP
    vm.prank(alice);
    address groupId = groupingModule.registerGroup(address(rewardPool));

    uint256 termsId = pilTemplate.registerLicenseTerms(
        PILFlavors.commercialRemix({
            mintingFee: 0,
            commercialRevShare: 10_000_000,
            currencyToken: address(erc20),
            royaltyPolicy: address(royaltyPolicyLAP)
        })
    );

    //IP Owner 1 attaches license terms and mints license tokens
    vm.prank(ipOwner1);
    licensingModule.attachLicenseTerms(ipId1, address(pilTemplate), termsId);
    licensingModule.mintLicenseTokens(ipId1, address(pilTemplate), termsId, 1, address(this), "", 0);

    //IP Owner 2 attaches license terms and mints license tokens
    vm.prank(ipOwner2);
    licensingModule.attachLicenseTerms(ipId2, address(pilTemplate), termsId);
    licensingModule.mintLicenseTokens(ipId2, address(pilTemplate), termsId, 1, address(this), "", 0);

    //IP Owner 3 attaches license terms and mints license tokens
    vm.prank(ipOwner3);
    licensingModule.attachLicenseTerms(ipId3, address(pilTemplate), termsId);
    licensingModule.mintLicenseTokens(ipId3, address(pilTemplate), termsId, 1, address(this), "", 0);

    //Alice attaches license terms to the group IP
    vm.startPrank(alice);
    licensingModule.attachLicenseTerms(groupId, address(pilTemplate), termsId);
    vm.stopPrank();

    //Alice adds IPs to the group
    vm.startPrank(alice);
        address[] memory ipIds = new address[](3);
        ipIds[0] = ipId1;
        ipIds[1] = ipId2;
        ipIds[2] = ipId3;
        groupingModule.addIp(groupId, ipIds);
        assertEq(ipAssetRegistry.totalMembers(groupId), 3);
        assertEq(rewardPool.getTotalIps(groupId), 3);
    vm.stopPrank();

    //Register a derivative IP asset
    address[] memory parentIpIds = new address[](1);
    parentIpIds[0] = groupId;
    uint256[] memory licenseTermsIds = new uint256[](1);
    licenseTermsIds[0] = termsId;

    vm.prank(ipOwner4);
    licensingModule.registerDerivative(ipId4, parentIpIds, licenseTermsIds, address(pilTemplate), "", 0);

    //IP Owner 4 pays royalties that should be distributed amongs IP owners
    erc20.mint(ipOwner4, 150 ether);
    vm.startPrank(ipOwner4);
        erc20.approve(address(royaltyModule), 150 ether);
        royaltyModule.payRoyaltyOnBehalf(ipId4, ipOwner4, address(erc20), 150 ether);
    vm.stopPrank();

    //Transfer a portion of the royalties to the group's vault
    royaltyPolicyLAP.transferToVault(ipId4, groupId, address(erc20), 15 ether);
    vm.warp(vm.getBlockTimestamp() + 7 days);

    //Take a snapshot of the group's royalty vault
    uint256 snapshotId = IIpRoyaltyVault(royaltyModule.ipRoyaltyVaults(groupId)).snapshot();
    uint256[] memory snapshotIds = new uint256[](1);
    snapshotIds[0] = snapshotId;

    //Collect royalties for the group
    groupingModule.collectRoyalties(groupId, address(erc20), snapshotIds);

    //The same ipId1 is included three times
    address[] memory claimIpIds = new address[](3);
    claimIpIds[0] = ipId1;
    claimIpIds[1] = ipId1;
    claimIpIds[2] = ipId1;

    //Owner of IP1 claims rewards using the manipulated ipIds array
    vm.prank(ipOwner1);
        groupingModule.claimReward(groupId, address(erc20), claimIpIds);

    //Assert that ipId1 received the entire reward pool
    assertApproxEqAbs(erc20.balanceOf(royaltyModule.ipRoyaltyVaults(ipId1)), 15 ether, 100);

The result of the test is the following:

It is recommended to enforce uniqueness within the ipIds array provided to the claimReward function. This can be achieved by implementing a check that ensures each IP Asset appears only once in the array before proceeding with the reward distribution.

SOLVED: The Story team solved the issue in the specified commit id. The vulnerability was fixed by updating each IP Asset's reward debt immediately after calculating its reward within the loop, ensuring that if the same ipId appears multiple times in the ipIds array, subsequent calculations account for the updated debt and prevent extra rewards from being allocated to duplicates.

The registerGroup function in the GroupingModule allows users to register a new Group IP account. It internally calls the registerGroup function on the GroupIPAssetRegistry contract for the actual accounting process:

function registerGroup(address groupPool) external nonReentrant whenNotPaused returns (address groupId) {
        // mint Group NFT
        uint256 groupNftId = GROUP_NFT.mintGroupNft(msg.sender, msg.sender);
        // register Group NFT
        groupId = GROUP_IP_ASSET_REGISTRY.registerGroup(address(GROUP_NFT), groupNftId, groupPool);
        emit IPGroupRegistered(groupId, groupPool);
}

After registering the Group IP, the group owner is intended to attache license terms to it using the attachLicenseTerms function in the LicensingModule:

function attachLicenseTerms(
    address ipId,
    address licenseTemplate,
    uint256 licenseTermsId
) external verifyPermission(ipId) {
    _verifyIpNotDisputed(ipId);
    LICENSE_REGISTRY.attachLicenseTermsToIp(ipId, licenseTemplate, licenseTermsId);
    emit LicenseTermsAttached(msg.sender, ipId, licenseTemplate, licenseTermsId);
}

The owner then can attempt to add individual IPs to their group using the addIp function in the GroupingModule. Before adding IPs, the _checkIfGroupMembersLocked function checks if the group is locked due to previous actions, such as having derivative IPs or minted license tokens. The getTotalTokensByLicensor function in the LICENSE_TOKEN contract returns the total number of license tokens minted under the specified IP:

function _checkIfGroupMembersLocked(address groupIpId) internal view {
    if (LICENSE_REGISTRY.hasDerivativeIps(groupIpId)) {
        revert Errors.GroupingModule__GroupFrozenDueToHasDerivativeIps(groupIpId);
    }
    if (LICENSE_TOKEN.getTotalTokensByLicensor(groupIpId) > 0) {
        revert Errors.GroupingModule__GroupFrozenDueToAlreadyMintLicenseTokens(groupIpId);
    }
}

Because there is no permission check to verify that the caller is authorized to mint license tokens for licensorIpId, any user can front-run the owner transaction, invoking this function. An attacker can exploit the lack of access control in the mintLicenseTokens function to mint license tokens for Alice's Group IP without authorization, as presented in the Proof of Concept.

An attacker can prevent the legitimate owner from managing their Group IP by unauthorized minting of license tokens, causing a denial-of-service condition. This disrupts the core functionality of the Group IP management system and can lead to operational and financial losses for the rightful owner.

Note that the similar scenario applies also to removing IP from the Group IP by using the removeIp function, which also uses the internal _checkIfGroupMembersLocked.

When Alice calls addIp function, the _checkIfGroupMembersLocked function checks if any license tokens have been minted under groupId. Since the attacker has front-run Alice's transaction and minted a license token, getTotalTokensByLicensor(groupId) returns a value greater than zero, causing the function to revert and preventing Alice from adding IPs to her group:

    function test_GroupingModule_revertOnFrontRunning() public {
        vm.warp(100);

        //Alice registers a new group IP
        vm.prank(alice);
        address groupId = groupingModule.registerGroup(address(rewardPool));

        uint256 termsId = pilTemplate.registerLicenseTerms(
            PILFlavors.commercialRemix({
                mintingFee: 0,
                commercialRevShare: 10_000_000,
                currencyToken: address(erc20),
                royaltyPolicy: address(royaltyPolicyLAP)
            })
        );

        vm.prank(ipOwner1);
            licensingModule.attachLicenseTerms(ipId1, address(pilTemplate), termsId);
            licensingModule.mintLicenseTokens(ipId1, address(pilTemplate), termsId, 1, address(this), "", 0);
        vm.prank(ipOwner2);
            licensingModule.attachLicenseTerms(ipId2, address(pilTemplate), termsId);
            licensingModule.mintLicenseTokens(ipId2, address(pilTemplate), termsId, 1, address(this), "", 0);
        
        //Alice attaches the license term 
        vm.prank(alice);
            licensingModule.attachLicenseTerms(groupId, address(pilTemplate), termsId);
        
        // Attacker front-runs Alice's transaction, minting a new license token for the group IP
        vm.prank(attacker);
            licensingModule.mintLicenseTokens(groupId, address(pilTemplate), termsId, 1, address(attacker), "", 0);

        // Alice tries to add the intended IPs to the group IP
        vm.startPrank(alice);
            address[] memory ipIds = new address[](2);
            ipIds[0] = ipId1;
            ipIds[1] = ipId2;

            //Transaction reverts 
            vm.expectRevert();
            groupingModule.addIp(groupId, ipIds);
        vm.stopPrank();
    }

The result of the test is the following:

It is recommended to implement strict access control in the mintLicenseTokens function to ensure that only the IP owner or authorized operators can mint license tokens for their IPs. This can be achieved by adding a verifyPermission modifier to the function.

SOLVED: The Story team solved the issue in the specified commit id. The disabled field has been added to the LicensingConfig struct. When set to true, both the mintLicenseTokens function and _executeLicensingHookAndPayMintingFee (executed during the derivative registration process) will revert. This effectively provides the Group IP owner with the ability to manage their own group and revoke the disabled status when it is ready to operate.

The registerGroup function in the GroupingModule allows users to register a new Group IP account. After registering the Group IP, the group owner is intended to attach license terms to it using the attachLicenseTerms function in the LicensingModule. The owner then can attempt to add individual IPs to their group using the addIp function in the GroupingModule. Before adding IPs, the _checkIfGroupMembersLocked function checks if the group is locked due to previous actions, such as having derivative IPs or minted license tokens:

function _checkIfGroupMembersLocked(address groupIpId) internal view {
    if (LICENSE_REGISTRY.hasDerivativeIps(groupIpId)) {
        revert Errors.GroupingModule__GroupFrozenDueToHasDerivativeIps(groupIpId);
    }
    if (LICENSE_TOKEN.getTotalTokensByLicensor(groupIpId) > 0) {
        revert Errors.GroupingModule__GroupFrozenDueToAlreadyMintLicenseTokens(groupIpId);
    }
}

The hasDerivativeIps function in the LICENSE_REGISTRY checks if there are any derivative IPs registered under the specified IP.

function hasDerivativeIps(address parentIpId) external view returns (bool) {
    return _getLicenseRegistryStorage().childIps[parentIpId].length() > 0;
}

However, any user can front-run the owner's transaction intended to add the specified IP assets to the group IP, by invoking the registerDerivative function. By registering an unauthorized derivative IP, the attacker effectively locks the Group IP, preventing the legitimate owner from managing it. This creates a denial-of-service condition, disrupting the core functionality of the Group IP management system.

Note that the similar scenario applies also to removing IP from the Group IP by using the removeIp function, which also uses the internal _checkIfGroupMembersLocked.

When Alice calls the addIp function, the _checkIfGroupMembersLocked function checks if any derivative IPs have been registered under groupId. Since the attacker has front-run Alice's transaction and registered a derivative IP, hasDerivativeIps(groupId) returns true, causing the function to revert and preventing Alice from adding IPs to her group:

function test_GroupingModule_revertOnFrontRunning_registerDerivative() public {
    //Alice registers a new group IP
    vm.prank(alice);
        address groupId = groupingModule.registerGroup(address(rewardPool));
        
    uint256 termsId = pilTemplate.registerLicenseTerms(
        PILFlavors.commercialRemix({
            mintingFee: 0,
            commercialRevShare: 10_000_000,
            currencyToken: address(erc20),
            royaltyPolicy: address(royaltyPolicyLAP)
        })
    );

    vm.prank(ipOwner1);
        licensingModule.attachLicenseTerms(ipId1, address(pilTemplate), termsId);
        licensingModule.mintLicenseTokens(ipId1, address(pilTemplate), termsId, 1, address(this), "", 0);
    vm.prank(ipOwner2);
        licensingModule.attachLicenseTerms(ipId2, address(pilTemplate), termsId);
        licensingModule.mintLicenseTokens(ipId2, address(pilTemplate), termsId, 1, address(this), "", 0);

    //Alice attaches the license term 
    vm.prank(alice);
        licensingModule.attachLicenseTerms(groupId, address(pilTemplate), termsId);

    //Attacker front-runs Alice's transaction, registering a new derivative for the group IP
    vm.startPrank(ipOwner3);
        address[] memory parentIpIds = new address[](1);
        uint256[] memory licenseTermsIds = new uint256[](1);
        parentIpIds[0] = groupId;
        licenseTermsIds[0] = termsId;
        licensingModule.registerDerivative(ipId3, parentIpIds, licenseTermsIds, address(pilTemplate), "", 0);
    vm.stopPrank();

    // Alice tries to add the intended IPs to the group IP
    vm.startPrank(alice);
        address[] memory ipIds = new address[](2);
        ipIds[0] = ipId1;
        ipIds[1] = ipId2;
            
        //Transaction reverts 
        vm.expectRevert();
        groupingModule.addIp(groupId, ipIds);
    vm.stopPrank();
}

The result of the test is the following:

It is recommended to consider introducing a short time delay during which the Group IP owner can still add new IPs without the risk of being front-run by an attacker registering a derivative. This grace period would help mitigate the front-running vector, allowing the owner to add the intended IP assets even if the derivative was registered beforehand.

SOLVED: The Story team solved the issue in the specified commit id. The disabled field has been added to the LicensingConfig struct. When set to true, both the mintLicenseTokens function and _executeLicensingHookAndPayMintingFee (executed during the derivative registration process) will revert. This effectively provides the Group IP owner with the ability to manage their own group and revoke the disabled status when it is ready to operate.

The addIp function, defined in the GroupingModule contract, is responsible for adding IP Assets to a group. It performs several checks to ensure that both the group and the IP Assets meet specific criteria before the addition is allowed:

function addIp(address groupIpId, address[] calldata ipIds) external whenNotPaused verifyPermission(groupIpId) {
    _checkIfGroupMembersLocked(groupIpId);
    // the group IP must has license terms and minting fee is 0 to be able to add IP to group
    if (LICENSE_REGISTRY.getAttachedLicenseTermsCount(groupIpId) == 0) {
    revert Errors.GroupingModule__GroupIPHasNoLicenseTerms(groupIpId);
    }
    (address groupLicenseTemplate, uint256 groupLicenseTermsId) = LICENSE_REGISTRY.getAttachedLicenseTerms(
        groupIpId,
        0
    );
    PILTerms memory groupLicenseTerms = IPILicenseTemplate(groupLicenseTemplate).getLicenseTerms(
        groupLicenseTermsId
    );
    if (groupLicenseTerms.defaultMintingFee != 0) {
        revert Errors.GroupingModule__GroupIPHasMintingFee(groupIpId, groupLicenseTemplate, groupLicenseTermsId);
    }

    GROUP_IP_ASSET_REGISTRY.addGroupMember(groupIpId, ipIds); 
    IGroupRewardPool pool = IGroupRewardPool(GROUP_IP_ASSET_REGISTRY.getGroupRewardPool(groupIpId));
    for (uint256 i = 0; i < ipIds.length; i++) {
        if (GROUP_IP_ASSET_REGISTRY.isRegisteredGroup(ipIds[i])) {
            revert Errors.GroupingModule__CannotAddGroupToGroup(groupIpId, ipIds[i]);
        }
        // check if the IP has the same license terms as the group
        if (!LICENSE_REGISTRY.hasIpAttachedLicenseTerms(ipIds[i], groupLicenseTemplate, groupLicenseTermsId)) {
            revert Errors.GroupingModule__IpHasNoGroupLicenseTerms(
                ipIds[i],
                groupLicenseTemplate,
                groupLicenseTermsId
            );
        }
        pool.addIp(groupIpId, ipIds[i]);
    }

    emit AddedIpToGroup(groupIpId, ipIds);
}

However, it does not check whether the license terms attached to the IP Assets or the group have expired. As a result, IP Assets with expired license terms can be added to a group, potentially bypassing licensing rules and unfairly participate in the reward distribution.

The following test demonstrates how an IP Asset with expired license terms can be added to a group:

function test_registerExpired() public {
    vm.warp(block.timestamp + 10 days);

    //Alice registers a new group IP
    vm.prank(alice);
        address groupId = groupingModule.registerGroup(address(rewardPool));

    //Create outdated term 
    PILTerms memory terms = PILFlavors.commercialRemix({
        mintingFee: 0,
        commercialRevShare: 10,
        currencyToken: address(erc20),
        royaltyPolicy: address(royaltyPolicyLAP)
    });
    //Set expiration to 5 days ago
    terms.expiration = block.timestamp - 5 days;

    //Register the expired license terms
    uint256 termsId = pilTemplate.registerLicenseTerms(terms);

    // IP Owner 1 has attached the expired license terms to their IP Asset
    vm.prank(ipOwner1);
        licensingModule.attachLicenseTerms(ipId1, address(pilTemplate), termsId);
        licensingModule.mintLicenseTokens(ipId1, address(pilTemplate), termsId, 1, address(this), "", 0);

    //Alice attaches the same expired license terms to the group IP
    vm.startPrank(alice);
        licensingModule.attachLicenseTerms(groupId, address(pilTemplate), termsId);
        vm.stopPrank();
        
    //Alice adds the IP Asset with expired license terms to the group
    vm.startPrank(alice);
        address[] memory ipIds = new address[](1);
        ipIds[0] = ipId1;
        groupingModule.addIp(groupId, ipIds);
    vm.stopPrank();
}

It is recommended to add checks in the addIp function to ensure that the license terms attached to both the group and the IP Assets are not expired.

SOLVED: The Story team solved the issue in the specified commit id. The check to ensure whether the license term has expired was added.

The purpose of the DisputeModule contract is to allow users to raise and resolve disputes related to IP assets. Disputes are handled through arbitration policies, which define the rules and procedures for dispute resolution. The arbitration policies can be set by IP owners and are subject to a cooldown period before becoming active. The IP owners can set a new arbitration policy for their IP assets by calling the setArbitrationPolicy function.

function setArbitrationPolicy(address ipId, address nextArbitrationPolicy) external verifyPermission(ipId) {
    DisputeModuleStorage storage $ = _getDisputeModuleStorage();
    if (!$.isWhitelistedArbitrationPolicy[nextArbitrationPolicy])
        revert Errors.DisputeModule__NotWhitelistedArbitrationPolicy();

    $.nextArbitrationPolicies[ipId] = nextArbitrationPolicy;

    uint256 nextArbitrationUpdateTimestamp = block.timestamp + $.arbitrationPolicyCooldown;
    $.nextArbitrationUpdateTimestamps[ipId] = nextArbitrationUpdateTimestamp;

    emit ArbitrationPolicySet(ipId, nextArbitrationPolicy, nextArbitrationUpdateTimestamp);
}

Users can raise disputes against an IP asset by calling raiseDispute function. This function updates the active arbitration policy for the IP asset before proceeding. It checks if the cooldown period has passed and updates the active arbitration policy accordingly. Then the dispute is processed according to the active arbitration policy.

function raiseDispute(
    address targetIpId,
    bytes32 disputeEvidenceHash,
    bytes32 targetTag,
    bytes calldata data
) external nonReentrant whenNotPaused returns (uint256) {
    if (!IP_ASSET_REGISTRY.isRegistered(targetIpId)) revert Errors.DisputeModule__NotRegisteredIpId();
    DisputeModuleStorage storage $ = _getDisputeModuleStorage();
    if (!$.isWhitelistedDisputeTag[targetTag]) revert Errors.DisputeModule__NotWhitelistedDisputeTag();
    if (disputeEvidenceHash == bytes32(0)) revert Errors.DisputeModule__ZeroDisputeEvidenceHash();

    address arbitrationPolicy = _updateActiveArbitrationPolicy(targetIpId);
    uint256 disputeId = ++$.disputeCounter;
    uint256 disputeTimestamp = block.timestamp;

    $.disputes[disputeId] = Dispute({
        targetIpId: targetIpId,
        disputeInitiator: msg.sender,
        disputeTimestamp: disputeTimestamp,
        arbitrationPolicy: arbitrationPolicy,
        disputeEvidenceHash: disputeEvidenceHash,
        targetTag: targetTag,
        currentTag: IN_DISPUTE,
        parentDisputeId: 0
    });

    IArbitrationPolicy(arbitrationPolicy).onRaiseDispute(msg.sender, data);

    emit DisputeRaised(
        disputeId,
        targetIpId,
        msg.sender,
        disputeTimestamp,
        arbitrationPolicy,
        disputeEvidenceHash,
        targetTag,
        data
    );

    return disputeId;
}

Also, if a parent IP asset has been tagged with an infringement (e.g., plagiarism), derivative IP assets can be automatically tagged by calling the tagDerivativeIfParentInfringed function. It tags the derivative IP with the same infringement tag as the parent.

function tagDerivativeIfParentInfringed(
    address parentIpId,
    address derivativeIpId,
    uint256 parentDisputeId
) external whenNotPaused {
    DisputeModuleStorage storage $ = _getDisputeModuleStorage();

    Dispute memory parentDispute = $.disputes[parentDisputeId];
    if (parentDispute.targetIpId != parentIpId) revert Errors.DisputeModule__ParentIpIdMismatch();

    // a dispute current tag prior to being resolved can be in 3 states - IN_DISPUTE, 0, or a tag (ie. "PLAGIARISM)
    // by restricting IN_DISPUTE and 0 - it is ensire the parent has been tagged before resolving dispute
    if (parentDispute.currentTag == IN_DISPUTE || parentDispute.currentTag == bytes32(0))
        revert Errors.DisputeModule__ParentNotTagged();

    if (!LICENSE_REGISTRY.isParentIp(parentIpId, derivativeIpId)) revert Errors.DisputeModule__NotDerivative();

    address arbitrationPolicy = $.arbitrationPolicies[derivativeIpId];
    if (!$.isWhitelistedArbitrationPolicy[arbitrationPolicy]) arbitrationPolicy = $.baseArbitrationPolicy;

    uint256 disputeId = ++$.disputeCounter;
    uint256 disputeTimestamp = block.timestamp;

    $.disputes[disputeId] = Dispute({
        targetIpId: derivativeIpId,
        disputeInitiator: msg.sender,
        disputeTimestamp: disputeTimestamp,
        arbitrationPolicy: arbitrationPolicy,
        disputeEvidenceHash: "",
        targetTag: parentDispute.currentTag,
        currentTag: parentDispute.currentTag,
        parentDisputeId: parentDisputeId
    });

    $.successfulDisputesPerIp[derivativeIpId]++;

    emit DerivativeTaggedOnParentInfringement(
        parentIpId,
        derivativeIpId,
        parentDisputeId,
        parentDispute.currentTag,
        disputeTimestamp
    );
}

However, in the current implementation of tagDerivativeIfParentInfringed, the function fetches the arbitration policy for the derivativeIpId directly from the storage ($.arbitrationPolicies[derivativeIpId]) without updating it beforehand. The function does not call _updateActiveArbitrationPolicy, so it does not check if the cooldown period has passed and whether the arbitration policy should be updated. As a result, if the arbitration policy was changed and the cooldown period has passed, the tagDerivativeIfParentInfringed will use an outdated arbitration policy, which is not intended by the IP owner. This will lead to disputes being handled incorrectly, violating the expectations of the IP owner.

It is recommended to modify the tagDerivativeIfParentInfringed function by adding a call to _updateActiveArbitrationPolicy before fetching the arbitration policy. This ensures that the arbitration policy is current and reflects any changes that may occur after the cooldown period.

SOLVED: The Story team solved the issue in the specified PR. Instead of calling _updateActiveArbitrationPolicy, the parent dispute was used. According to the team:

This is because - in our roadmap - we are considering replacing the need for tagDerivativeIfParentInfringed() with a precompile that checks if any ancestor is tagged. The closest behaviour we can get to - without the precompile - would be to propagate the dispute to the derivative with the same arbitration policy it had in the parent.

The RoyaltyModule is designed to manage the distribution of royalty tokens among IP owners and royalty policies when a derivative IP is linked to one or more parent IPs. External Royalty Policies enable IP owners to define custom royalty distribution rules by implementing the 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);
}

IP Owners can deploy and register an External Royalty Policy contract that implements IExternalRoyaltyPolicy and mint license tokens specifying the intended policy. Other users can then create derivative IPs by linking to parent IPs using the specified licenses and royalty policies. During the linking process, the RoyaltyModule calculates and distributes royalty tokens, associated with the user's IPRoyaltyVault, to royalty policies and IP owners based on the outputs of getPolicyRtsRequiredToLink.

The vulnerability lies in the _distributeRoyaltyTokensToPolicies function within the RoyaltyModule contract. This function relies on the getPolicyRtsRequiredToLink method from External Royalty Policies to determine the number of RTs to distribute. However, there is no validation on the value returned by this method.

function _distributeRoyaltyTokensToPolicies(
    address ipId,
    address[] calldata parentIpIds,
    address[] calldata licenseRoyaltyPolicies,
    uint32[] calldata licensesPercent,
    address ipRoyaltyVault
) internal {
    RoyaltyModuleStorage storage $ = _getRoyaltyModuleStorage();

    uint32 totalRtsRequiredToLink;
    // this loop is limited to maxParents
    for (uint256 i = 0; i < parentIpIds.length; i++) {
        if (parentIpIds[i] == address(0)) revert Errors.RoyaltyModule__ZeroParentIpId();
        if (licenseRoyaltyPolicies[i] == address(0)) revert Errors.RoyaltyModule__ZeroRoyaltyPolicy();
        _addToAccumulatedRoyaltyPolicies(parentIpIds[i], licenseRoyaltyPolicies[i]);
        address[] memory accParentRoyaltyPolicies = $.accumulatedRoyaltyPolicies[parentIpIds[i]].values();

        // this loop is limited to accumulatedRoyaltyPoliciesLimit
        for (uint256 j = 0; j < accParentRoyaltyPolicies.length; j++) {
            // add the parent ancestor royalty policies to the child
            _addToAccumulatedRoyaltyPolicies(ipId, accParentRoyaltyPolicies[j]);
            // transfer the required royalty tokens to each policy
            uint32 licensePercent = accParentRoyaltyPolicies[j] == licenseRoyaltyPolicies[i]
                ? licensesPercent[i]
                : 0;
            uint32 rtsRequiredToLink = IRoyaltyPolicy(accParentRoyaltyPolicies[j]).getPolicyRtsRequiredToLink(
                parentIpIds[i],
                licensePercent
            );
            totalRtsRequiredToLink += rtsRequiredToLink;
            if (totalRtsRequiredToLink > MAX_PERCENT) revert Errors.RoyaltyModule__AboveMaxPercent();
            IERC20(ipRoyaltyVault).safeTransfer(accParentRoyaltyPolicies[j], rtsRequiredToLink);
        }
    }

    if ($.accumulatedRoyaltyPolicies[ipId].length() > $.maxAccumulatedRoyaltyPolicies)
        revert Errors.RoyaltyModule__AboveAccumulatedRoyaltyPoliciesLimit();

    // sends remaining royalty tokens to the ipId address or
    // in the case the ipId is a group then send to the group reward pool
    address receiver = IP_ASSET_REGISTRY.isRegisteredGroup(ipId)
        ? IP_ASSET_REGISTRY.getGroupRewardPool(ipId)
        : ipId;
    IERC20(ipRoyaltyVault).safeTransfer(receiver, MAX_PERCENT - totalRtsRequiredToLink);
}

A malicious IP owner can observe a pending derivative registration transaction and front-run it by increasing the RT requirements in their royalty policy. This forces the user to transfer more Royalty Tokens than anticipated, potentially draining the user's RTs.

It is recommended to enforce the maximum Royalty Tokens limit specified by the user during derivative registration. If the required RTs exceed the maximum limit, the transaction should revert, preventing the user from overpaying.

SOLVED: The Story team solved the issue in the specified PR. A new variable, maxRts, defined by the user when registering a derivative, was implemented to ensure that the royalty tokens distributed to royalty policies do not exceed the specified limit.

The getGroupMembers function, implemented in the GroupIPAssetRegistry contract, is intended to retrieve a subset of IP addresses that are members of a specific group:

function getGroupMembers(
    address groupId,
    uint256 startIndex,
    uint256 size
) external view returns (address[] memory results) {
    EnumerableSet.AddressSet storage allMemberIpIds = _getGroupIPAssetRegistryStorage().groups[groupId];
    uint256 totalSize = allMemberIpIds.length();
    if (startIndex >= totalSize) return results;

    uint256 resultsSize = (startIndex + size) > totalSize ? size - ((startIndex + size) - totalSize) : size;
    for (uint256 i = 0; i < resultsSize; i++) {
        results[i] = allMemberIpIds.at(startIndex + i);
    }
    return results;
}

The function declares a dynamic array results to hold the returned addresses but does not initialize it with a specific length before accessing its elements. In Solidity, when working with dynamic arrays in memory, it must be allocated with a defined length before accessing or assigning values to their elements. Attempting to assign values to results[i] without initializing results leads to a runtime error (Panic: Index out of bounds), causing the transaction to revert and making the getGroupMembers function broken.

The test will fail because the contract reverts when trying to assign a value to results[0] without initializing results.

function test_GroupingModule_getGroupMembers() public {
    vm.warp(100);

    //Alice registers a new group IP
    vm.prank(alice);
    address groupId = groupingModule.registerGroup(address(rewardPool));

    uint256 termsId = pilTemplate.registerLicenseTerms(
        PILFlavors.commercialRemix({
            mintingFee: 0,
            commercialRevShare: 10_000_000,
            currencyToken: address(erc20),
            royaltyPolicy: address(royaltyPolicyLAP)
        })
    );

    //IP owners attach license terms and mint license tokens
    vm.prank(ipOwner1);
        licensingModule.attachLicenseTerms(ipId1, address(pilTemplate), termsId);
        licensingModule.mintLicenseTokens(ipId1, address(pilTemplate), termsId, 1, address(this), "", 0);
    vm.prank(ipOwner2);
        licensingModule.attachLicenseTerms(ipId2, address(pilTemplate), termsId);
        licensingModule.mintLicenseTokens(ipId2, address(pilTemplate), termsId, 1, address(this), "", 0);
        
    //Alice attaches the license term 
    vm.prank(alice);
        licensingModule.attachLicenseTerms(groupId, address(pilTemplate), termsId);

    //Alice adds the intended IPs to the group IP
    vm.startPrank(alice);
        address[] memory ipIds = new address[](2);
        ipIds[0] = ipId1;
        ipIds[1] = ipId2;
        groupingModule.addIp(groupId, ipIds);
    vm.stopPrank();

    //Expect a revert due to uninitialized results array in getGroupMembers function
    vm.expectRevert();
    ipAssetRegistry.getGroupMembers(groupId, 0, 1); 
}

It is recommended to initialize the results array properly with the correct length before accessing or assigning values to it. This can be achieved by allocating memory for results: 

results = new address[](resultsSize);

SOLVED: The Story team solved the issue in the specified PR. The getGroupMembers function was updated to initialize the results array dynamically based on the size of the group members.

The distributeRewards function in the EvenSplitGroupPool contract calls the approve method on tokens intended for reward distribution:

function distributeRewards( 
    address groupId,
    address token,
     address[] calldata ipIds
) external whenNotPaused onlyGroupingModule returns (uint256[] memory rewards) {
    rewards = _getAvailableReward(groupId, token, ipIds);
    uint256 totalRewards = 0;
    for (uint256 i = 0; i < ipIds.length; i++) {
        totalRewards += rewards[i];
    }
    if (totalRewards == 0) return rewards;
    IERC20(token).approve(address(ROYALTY_MODULE), totalRewards); 
    EvenSplitGroupPoolStorage storage $ = _getEvenSplitGroupPoolStorage();
    for (uint256 i = 0; i < ipIds.length; i++) {
        if (rewards[i] == 0) continue;
        // calculate pending reward for each IP
        $.ipRewardDebt[groupId][token][ipIds[i]] += rewards[i];
        // call royalty module to transfer reward to IP's vault as royalty
        ROYALTY_MODULE.payRoyaltyOnBehalf(ipIds[i], groupId, token, rewards[i]);
    }
}

However, certain tokens like USDT do not strictly adhere to the ERC-20 standard, leading to potential failures in the contract's operation. Specifically:

• Approval Race Conditions: Some tokens, to protect against race conditions, do not allow approving an amount M > 0 when an existing amount N > 0 is already approved. This restriction can cause the approve call to revert if not handled correctly.

• Non-Standard Return Values: The approve call does not return a boolean in some tokens, which can cause compatibility issues with contracts expecting a standard ERC-20 boolean response.

• Approval Value Limits: The function reverts if the approval value exceeds uint96, which can happen if large token amounts are involved.

  
  

These issues can cause the distributeRewards function to revert, preventing the distribution of rewards when using affected tokens. This not only disrupts the reward mechanism but also affects user trust and the protocol's functionality, especially if popular tokens like USDT are intended for use.

It is recommended to use SafeERC20's forceApprove method instead of direct approve, to ensure compatibility with all ERC20 tokens, including those with non-standard implementations, This will provide a more robust solution for managing token allowances across various ERC20 token contracts.

SOLVED: The Story team solved the issue in the specified PR. The distributeRewards function was updated to use SafeERC20's forceApprove() method.

In ECDSA, a signature is composed of two values, r and s, derived from the message hash and the signer's private key. The malleability issue stems from the fact that for any valid signature (r, s), the signature (r, n - s) is also valid, where n is the order of the elliptic curve group.

In Go, the crypto/ecdsa package provides functions for ECDSA signature generation and verification. The ecdsa.Verify function checks the validity of a signature by recalculating the signature values using the public key and message hash:

func Verify(hash []byte, r, s, x, y *big.Int) bool {
	// Create the public key format
	publicKey := newPublicKey(x, y)

	// Check if they are invalid public key coordinates
	if publicKey == nil {
		return false
	}

	// Verify the signature with the public key,
	// then return true if it's valid, false otherwise
	return ecdsa.Verify(publicKey, hash, r, s)
}

However, the ecdsa.Verify function verifies signatures without normalizing the s value. Specifically, it accepts signatures where s is in the range (0,n) without normalizing s to ensure s ≤ n/2.

As a result, both s and n - s are considered valid, making the signature verification process susceptible to malleability attacks.

The Verify function is susceptible to signature malleability, showing that both the original signature (r, s) and the altered signature (r, n - s) are valid for the same message and public key:

package main

import (
	"crypto/ecdsa"
	"crypto/elliptic"
	"crypto/rand"
	"crypto/sha256"
	"fmt"
	"math/big"
)

// Generates appropriate public key format from given coordinates
func newPublicKey(x, y *big.Int) *ecdsa.PublicKey {
	//Check if the given coordinates are valid
	if x == nil || y == nil || !elliptic.P256().IsOnCurve(x, y) {
		return nil
	}

	return &ecdsa.PublicKey{
		Curve: elliptic.P256(),
		X:     x,
		Y:     y,
	}
}

// Verify verifies the given signature (r, s) for the given hash and public key (x, y).
// It returns true if the signature is valid, false otherwise.
func Verify(hash []byte, r, s, x, y *big.Int) bool {
	//Create the public key format
	publicKey := newPublicKey(x, y)

	//Check if they are invalid public key coordinates
	if publicKey == nil {
		return false
	}

	//Verify the signature with the public key,
	return ecdsa.Verify(publicKey, hash, r, s)
}

func main() {
	//Generate a new private key using P-256 curve
	privateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader)
	if err != nil {
		fmt.Println("Error generating private key:", err)
		return
	}

	//Get public key components
	x := privateKey.PublicKey.X
	y := privateKey.PublicKey.Y

	//Message to be signed
	message := []byte("xyz")

	//Hash the message using SHA-256
	hash := sha256.Sum256(message)

	//Sign the hash using the private key
	r, s, err := ecdsa.Sign(rand.Reader, privateKey, hash[:])
	if err != nil {
		fmt.Println("Error signing message:", err)
		return
	}

	//Verify the original signature
	validOriginal := Verify(hash[:], r, s, x, y)
	fmt.Printf("Original signature valid: %v\n", validOriginal)

	//Get the curve order n
	nCurve := elliptic.P256().Params().N

	//Compute s' = n - s
	sPrime := new(big.Int).Sub(nCurve, s)

	//Verify the malleable signature
	validMalleable := Verify(hash[:], r, sPrime, x, y)
	fmt.Printf("Malleable signature valid: %v\n", validMalleable)

	if validMalleable {
		fmt.Println("Malleability demonstrated in Verify function.")
	} else {
		fmt.Println("Malleability not demonstrated.")
	}
}

The result of the test is the following:

To mitigate the vulnerability, modify the signature verification process to enforce the canonical form of s by normalizing it to s ≤ n / 2.

func Verify(hash []byte, r, s, x, y *big.Int) ([]byte, error) {
	// Create the public key format
	publicKey := newPublicKey(x, y)
	if publicKey == nil {
		return nil, errors.New("invalid public key coordinates")
	}

	if checkMalleability(s) {
		return nil, errors.New("malleability issue")
	}

	// Verify the signature with the public key and return 1 if it's valid, 0 otherwise
	if ok := ecdsa.Verify(publicKey, hash, r, s); ok {
		return common.LeftPadBytes(common.Big1.Bytes(), 32), nil
	}

	return common.LeftPadBytes(common.Big0.Bytes(), 32), nil
}

// Check the malleability issue
func checkMalleability(s *big.Int) bool {
	return s.Cmp(secp256k1halfN) <= 0
}

RISK ACCEPTED: The Story team accepted the risk of this issue and states the following:

To ensure compatibility with the NIST P-256 specification, the malleability check was not implemented. The necessary protection will be applied at the contract layer whenever the precompile is invoked.

In the IPAccountImpl, RoyaltyModule, RoyaltyPolicyLAP RoyaltyPolicyLRP and LicenseRegistry contract, require statements are used. In Solidity development, replacing hard-coded revert message strings with the Error() syntax is an optimization strategy that can significantly reduce gas costs. Hardcoded strings, stored on the blockchain, increase the size and cost of deploying and executing contracts. The Error() syntax allows for the definition of reusable, parameterized custom errors, leading to a more efficient use of storage and reduced gas consumption.

This approach not only optimizes gas usage during deployment and interaction with the contract but also enhances code maintainability and readability by providing clearer, context-specific error information.

It is recommend to replace all revert strings with custom errors.

SOLVED: The Story team solved the issue in the specified PR.

During the audit, it was identified that DisputeModule, IPAccountStorage, AccessController, LicensorApprovalChecker and IPGraphACL contracts do not use named mappings, unlike the other contracts in the protocol.

The project is using Solidity version greater than 0.8.18, which supports named mappings. Using named mappings can improve the readability and maintainability of the code by making the purpose of each mapping clearer. This practice will enhance code readability and make the purpose of each mapping more explicit.

Consider refactoring all the mappings to use named arguments.

PARTIALLY SOLVED: The Story team partially solved the issue in the specified PR. The mappings in DisputeModule contract have been refactored.

To mitigate the risk of reentrancy attacks, a modifier named nonReentrant is commonly used. This modifier acts as a lock, ensuring that a function cannot be called recursively while it is still in execution. A typical implementation of the nonReentrant modifier locks the function at the beginning and unlocks it at the end. However, it is vital to place the nonReentrant modifier before all other modifiers in a function. Placing it first ensures that all other modifiers cannot bypass the reentrancy protection. In the current implementation, some functions use other modifiers before nonReentrant, which may compromise the protection it provides.

During the audit it was identified that the following functions does not use nonReentrant as a first modifier:

• ArbitrationPolicyUMA.sol

  • onRaiseDispute

  
  

• LicensingModule.sol

  • mintLicenseTokens

  • registerDerivative

  
  

• RoyaltyPolicyLAP.sol

  • onLicenseMinting

  • onLinkToParents

  
  

• RoyaltyPolicyLRP.sol

  • onLicenseMinting

  • onLinkToParents

It is recommended to follow the best practice of placing the nonReentrant modifier before all other modifiers. By doing so, one can reduce the risk of reentrancy-related vulnerabilities. This simple yet effective approach can help enhance the security posture of any Solidity smart contract.

SOLVED: The Story team solved the issue in the specified PR.

The inconsistent use of open-source licenses across the files in the protocol was identified. The VaultController and IpRoyaltyVault contracts specifying the MIT license, the IPAccountStorageOps is unlicensed and others using the more restrictive BUSL-1.1 license. Specifically:

• Certain files are marked with // SPDX-License-Identifier: MIT, which is a permissive open-source license that allows anyone to use, copy, modify, and distribute the code, provided the original license is included.

• Other files use // SPDX-License-Identifier: BUSL-1.1, a more restrictive license that limits commercial use for a period of time, making it incompatible with MIT in terms of redistributing or combining the code.

This discrepancy could lead to license incompatibility issues when attempting to combine or redistribute the project's code as a whole, potentially violating the terms of one or both licenses. This is particularly problematic in open-source projects, where license compliance is crucial for legal and community standards.

Ensure consistent licensing across the project by reviewing and aligning all file headers to use the same license, depending on the intended usage and distribution model.

SOLVED: The Story team solved the issue in the specified PRs. All contracts use the BUSL-1.1 license.

Throughout the code in scope, there are several custom errors that are declared but never used.

In Errors.sol:

• error GroupingModule__InvalidGroupRewardPool(address rewardPool);

• error GroupingModule__CallerIsNotLicensingModule(address caller);

• error GroupIPAssetRegistry__GroupFrozen(address groupId);

• error GroupIPAssetRegistry__GroupNotFrozen(address groupId);

• error LicenseRegistry__NotTransferable();

• error LicenseRegistry__NoParentIp();

• error LicenseToken__ZeroLicensingModule();

• error LicenseToken__ZeroDisputeModule();

• error LicensingModule__ReceiverCheckFailed(address receiver);

• error LicensingModule__ZeroGroupingModule();

• error RoyaltyPolicyLAP__IpTagged();

• error RoyaltyPolicyLAP__AlreadyClaimed();

• error RoyaltyPolicyLAP__ClaimerNotAnAncestor();

• error RoyaltyPolicyLAP__NotAllRevenueTokensHaveBeenClaimed();

• error RoyaltyPolicyLAP__InvalidTargetIpId();

• error EvenSplitGroupPool__UnregisteredCurrencyToken(address currencyToken);

• error EvenSplitGroupPool__UnregisteredGroupIP(address groupId);

It is recommended to remove all unused errors.

SOLVED: The Story team solved the issue in the specified PR. The mentioned custom errors were removed.