xiaoming90 - Uninitialized `TitlesCore` implementation contract can be taken over by an attacker

[sherlock-admin3]

xiaoming90

medium

Uninitialized TitlesCore implementation contract can be taken over by an attacker

Summary

An attacker could take over the implementation/logic contract of TitlesCore, which might impact the proxy.

Vulnerability Detail

Per Line 21, the TitlesCore contract inherits from the UUPSUpgradeable contract. The TitlesCore contract will contain the logic/implementation, and the UUPS proxy will point its implementation to the TitlesCore contract address.

It was observed that the TitlesCore implementation/logic contract is left uninitialized. As a result, an attacker could take over the implementation/logic contract of TitlesCore by calling the TitlesCore.initialize function directly on the TitlesCore implementation/logic contract, which might impact the proxy.

When that happens, the attackers will become the owner of the implementation/logic contract per Line 45 below.

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

File: TitlesCore.sol
32: contract TitlesCore is OwnableRoles, Initializable, UUPSUpgradeable, Receiver {
..SNIP..
44:     function initialize(address feeReceiver_, address splitFactory_) external initializer {
45:         _initializeOwner(msg.sender);
46: 
47:         feeManager = new FeeManager(msg.sender, feeReceiver_, splitFactory_);
48:         graph = new TitlesGraph(address(this), msg.sender);
49:     }

Impact

An attacker could take over the implementation/logic contract of TitlesCore, which might impact the proxy.

Code Snippet

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

Tool used

Manual Review

Recommendation

To prevent the implementation contract from being used or taken over, invoke the initializer in the constructor to automatically lock the initializer on the implementation contract when it is deployed. This is also the recommendation from OpenZeppelin when handling upgradable contracts (https://docs.openzeppelin.com/upgrades-plugins/1.x/writing-upgradeable#initializing_the_implementation_contract).

contract TitlesCore is OwnableRoles, Initializable, UUPSUpgradeable, Receiver {
    using LibClone for address;
    using LibZip for bytes;
    using SafeTransferLib for address;

    address public editionImplementation = address(new Edition());
    FeeManager public feeManager;
    TitlesGraph public graph;

+       constructor() initializer {}

    /// @notice Initializes the protocol.
    /// @param feeReceiver_ The address to receive fees.
    /// @param splitFactory_ The address of the split factory.
    function initialize(address feeReceiver_, address splitFactory_) external initializer {
        _initializeOwner(msg.sender);

        feeManager = new FeeManager(msg.sender, feeReceiver_, splitFactory_);
        graph = new TitlesGraph(address(this), msg.sender);
    }

Ensure that this change is also applied to the TitlesGraph contract, as it is also an upgradable contract.

[pqseags]

Will address

[0x73696d616f]

Escalate

This issue shows no impact. The vulnerability described happens and would be valid when the upgrade functions are exposed, which is not the case as UUPSUpgradeable proxies use the onlyProxy to protect upgradeable functions.

[sherlock-admin3]

Escalate

This issue shows no impact. The vulnerability described happens and would be valid when the upgrade functions are exposed, which is not the case as UUPSUpgradeable proxies use the onlyProxy to protect upgradeable functions.

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 on the lack of impact.

[realfugazzi]

There's been a lot of confusion regarding this set of issues, which I believe have been incorrectly reduced to a unique set of duplicates.

• Issue #272 (this issue) refers to initializing the implementation. I don't see how this qualifies as a med severity, there is no impact.
• Issue #281 refers to the incorrect initialization of the TitlesGraph contract. This has a severe impact since it damages the upgradeability of this contract. There should be other duplicates that fall into this category, such as #170.
• Issue #162 refers to the invalid initialization of the Edition reference in the context of TitlesCore being an upgradeable contract (using a field declaration initialization is incompatible with the contract upgradeable pattern).

Let's hope all these could get addressed.

[WangSecurity]

Agree that this report has no impact and falls down into "Front-running initialisers rule". Planning to accept the escalation and invalidate the report.

Regarding different issue families in the report, will look closer into it a bit later, thank you for noticing.

[Hash01011122]

Agreed with the reasoning of @realfugazzi, this issue can be invalidated.

For Issue #281 and #162 it's borderline low/medium severity issue, I am tending towards low because issues related to breakage of initialize was considered low in earlier contests.

[Evert0x]

Result: Invalid Has Duplicates

[sherlock-admin4]

Escalations have been resolved successfully!

Escalation status:

• 0x73696d616f: accepted

[sherlock-admin2]

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