alexzoid - Incompatibility of Upgradeability Pattern in TitlesGraph Contract

[sherlock-admin4]

alexzoid

medium

Incompatibility of Upgradeability Pattern in TitlesGraph Contract

Summary

The TitlesGraph contract is designed to be upgradeable, utilizing the UUPSUpgradeable pattern. However, it's instantiated via a constructor in the TitlesCore contract setup.

Vulnerability Detail

In the TitlesCore contract, TitlesGraph is instantiated directly using a constructor rather than being set up as a proxy. This could lead to unexpected behavior when attempting to upgrade the contract, as the proxy would not have access to the initialized state variables or might interact incorrectly with uninitialized storage.

Impact

Inability to leverage the upgradeability.

Code Snippet

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/TitlesCore.sol#L44-L49

graph = new TitlesGraph(address(this), msg.sender);

Tool used

Manual Review

Recommendation

Deploy the TitlesGraph contract without initializing state in the constructor. Deploy a proxy that points to the deployed TitlesGraph implementation. Correct approach using a proxy pattern for upgradeable contracts:

address graphImplementation = address(new TitlesGraph());
graph = TitlesGraph(payable(LibClone.deployERC1967(graphImplementation)));
graph.initialize(address(this), msg.sender);

[alexzoid-eth]

Escalate

This issue is not a duplicate of #272. This is a valid medium issue uncovering the inability of TitlesGraph contract upgrade.

Possible duplicates are #87 #142 #170 #180 #209 #281 #319 #342

[sherlock-admin3]

Escalate

This issue is not a duplicate of #272. This is a valid medium issue uncovering the inability of TitlesGraph contract upgrade.

Possible duplicates are #87 #142 #170 #180 #209 #281 #319 #342

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.

[realfugazzi]

Agree with escalation. Also documented the issue in #170 which has been incorrectly duped as #272 too

[Hash01011122]

Responded in #272.

Borderline low/medium. Tending towards low because in earlier contest issues like this were considered low.

[realfugazzi]

Responded in #272.

Borderline low/medium. Tending towards low because in earlier contest issues like this were considered low.

I think you are confusing this issue with #281, which is an entirely different problem. There are at least 3 different issues being grouped here, see #272 (comment)

[alexzoid-eth]

Responded in #272.

Borderline low/medium. Tending towards low because in earlier contest issues like this were considered low.

Medium due to rules (https://docs.sherlock.xyz/audits/judging/judging#v.-how-to-identify-a-medium-issue): Breaks core contract functionality.

[WangSecurity]

Agree with the escalation, planning to accept it and make a new issue family of medium severity with the following duplicates:

#142, #87, #170, #180, #209, #281, #319, #342, #182

[Evert0x]

Result:
Medium
Has Duplicates

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• alexzoid-eth: accepted

[sherlock-admin2]

The protocol team fixed this issue in the following PRs/commits:
https://github.com/titlesnyc/wallflower-contract-v2/pull/1

[sherlock-admin2]

The Lead Senior Watson signed off on the fix.