Closed sherlock-admin3 closed 2 months ago
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.
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.
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
Making a new split will not help as the attributions from works will be linked to the past split (wallet).
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.
It's exactly like this valid issue but in a different smart contract (the Split Wallet).
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.
docs:
If the protocol team provides specific information in the README or CODE COMMENTS, that information stands above all judging rules.
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.
Result: Medium Duplicate of #148
0x73696d616f
medium
SplitV2
wallets created bysplitFactory
ownership are set toFeeManager
instead of the protocolSummary
SplitV2
wallets created bysplitFactory
are supposed to be owned by the protocol, as mentioned by this comment, but it is not the case.Vulnerability Detail
SplitV2
wallets created bysplitFactory
have anowner
which is set in theSplitFactory::createSplit()
call inFeeManager::createRoute()
. The comment in the code indicates that the ownership is intended to be retained by the protocolHowever,
FeeManager
is not upgradeable and does not have the functionality to call anyonlyOwner
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()
Tool used
Manual Review
Vscode
Recommendation
The
owner
of theFeeManager
isTitlesCore
so theowner
of the Split wallet can not be set toowner
. Thus, consider making theowner
ofFeeManager
theowner
ofTitlesCore
(as explained in another issue, this would be done in the constructor ofTitlesCore
first. And then, set theowner
of the Split wallet toowner()
.Duplicate of #148