sherlock-audit / 2024-04-titles-judging

1 stars 1 forks source link

trachev - TitlesGraph.sol is not initialized #342

Closed sherlock-admin3 closed 2 months ago

sherlock-admin3 commented 2 months ago

trachev

high

TitlesGraph.sol is not initialized

Summary

The TitlesGraph.sol contract is intended to be an upgradeable contract, as it inherits the UUPSUpgradeable library. Therefore, it should not be initialized in its constructor, but inside of an initialization function.

Vulnerability Detail

constructor(address owner_, address admin_) {
        _initializeOwner(owner_);
        _grantRoles(admin_, ADMIN_ROLE);
    }

In the constructor of TitlesGraph.sol the owner is initialized, and the admin roles are granted. The issue is that as the TitlesGraph.sol will only be the implementation contract (as it is upgradeable), the code within its constructor will never be executed in the context of the proxy's state, therefore the contract's owner will not be initialized.

This causes the following issue: the functions grantRoles, revokeRoles and _authorizeUpgrade which use the onlyOwnerOrRoles modifier, will always fail, as there will be no owner, and the ADMIN_ROLE will not have been granted, rendering the TitlesGraph.sol contract non-functional.

Furthermore, it is important to add that in the TitlesCore.sol contract, a TitlesGraph contract is created the following way:


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

However, as the contract is upgradable the Proxy contract should be created and initialized instead.

Impact

Many functions in TitlesGraph.sol contract will be broken and the contract cannot be upgraded.

Code Snippet

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

https://github.com/sherlock-audit/2024-04-titles/blob/main/wallflower-contract-v2/src/graph/TitlesGraph.sol#L52-L55

Tool used

Manual Review

Recommendation

The constructor code should be moved inside of an initialize function.

Duplicate of #445