0x73696d616f - SplitV2 wallets created by splitFactory ownership are set to FeeManager instead of the protocol

[sherlock-admin3]

0x73696d616f

medium

SplitV2 wallets created by splitFactory ownership are set to FeeManager instead of the protocol

Summary

SplitV2 wallets created by splitFactory are supposed to be owned by the protocol, as mentioned by this comment, but it is not the case.

Vulnerability Detail

SplitV2 wallets created by splitFactory have an owner which is set in the SplitFactory::createSplit() call in FeeManager::createRoute(). The comment in the code indicates that the ownership is intended to be retained by the protocol

// Create the split. The protocol retains "ownership" to enable future use cases.

However, FeeManager is not upgradeable and does not have the functionality to call any onlyOwner functions of the created wallet.

Impact

Ownership of Split wallets is not guaranteed which could comprise future functionality or even funds, as it goes against what the protocol expected.

Code Snippet

FeeManager::createRoute()

...
// Create the split. The protocol retains "ownership" to enable future use cases.
receiver = Target({
    target: splitFactory.createSplit(
        SplitV2Lib.Split({
            recipients: targets,
            allocations: revshares,
            totalAllocation: 1e6,
            distributionIncentive: 0
        }),
        address(this),
        creator.target
        ),
    chainId: creator.chainId
});
...

Tool used

Manual Review

Vscode

Recommendation

The owner of the FeeManager is TitlesCore so the owner of the Split wallet can not be set to owner. Thus, consider making the owner of FeeManager the owner of TitlesCore (as explained in another issue, this would be done in the constructor of TitlesCore first. And then, set the owner of the Split wallet to owner().

Duplicate of #148

[0x73696d616f]

Escalate

This issue should be valid as it shows how the ownership of the Split Wallets is compromised as their ownership is not retained to the protocol, but to the FeeManager. FeeManager is not even upgradeable so there is nothing it can do.

[sherlock-admin3]

Escalate

This issue should be valid as it shows how the ownership of the Split Wallets is compromised as their ownership is not retained to the protocol, but to the FeeManager. FeeManager is not even upgradeable so there is nothing it can do.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[WangSecurity]

As I understand the only onlyOwner function in Split Wallet is to update the split and even if it's the case and we cannot update the split, then we can make a new one? Please correct me if I'm wrong

[0x73696d616f]

Making a new split will not help as the attributions from works will be linked to the past split (wallet).

[WangSecurity]

Thank you for that response, but I believe report fails to show the impact:

could comprise future functionality or even funds

In the current state, I believe the report is only a recommendation. Hence, planning to reject the escalation and leave the issue as it is.

[0x73696d616f]

The wallet can:

• update the split
• support EIP1271
• execute transactions.

All of this will be DoSed as the owner was not set. In any case, not setting the owner of a wallet is a severe vulnerability.

[0x73696d616f]

It's exactly like this valid issue but in a different smart contract (the Split Wallet).

[0x73696d616f]

Just highlighting that the code clearly intends to retain ownership, as show in the comment.

// Create the split. The protocol retains "ownership" to enable future use cases.

But this is not the case, as explained in the issue.

[0x73696d616f]

docs:

If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules.

[WangSecurity]

Be careful with the rules, there were changes to the Hierarchy of truth, but they're not applied to this contest cause it started earlier than the changes were made. The docs version for this contest are here And now, all contests have their own Rules Version right above the Total SLOC of the contest here.

About the escalation, I agree with it and believe it should be duplicated with #148 with the core issue "roles are set incorrectly or not set at all". Planning to accept the escalation and duplicate with #148.

[Evert0x]

Result:
Medium
Duplicate of #148

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• 0x73696d616f: accepted