xiaoming90 - Constructor is used during initialization when a proxy pattern is used

[sherlock-admin4]

xiaoming90

medium

Constructor is used during initialization when a proxy pattern is used

Summary

There is no initialize function on the logic/implementation contract of TitlesGraph. As a result, the core functionalities of TitlesGraph contract will be broken. Any functions within the TitlesGraph that are restricted to only the owner or admin will not be executable since the owner and admin are not initialized. This includes the following functions:

• createEdges
• grantRoles
• revokeRoles
• _authorizeUpgrade (Contract no longer upgradable)

Vulnerability Detail

Per Line 17, the TitlesGraph contract inherits from the UUPSUpgradeable contract. This shows that the TitlesGraph is expected to be upgradable and that the UUPS proxy pattern is adopted.

The TitlesGraph contract will contain the logic/implementation, and the UUPS proxy will point its implementation to the TitlesGraph contract address.

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

File: TitlesGraph.sol
14: /// @title TitlesGraph
15: /// @notice Titles.xyz implementation of the OpenGraph standard
16: /// @dev The TitlesGraph contract implements the OpenGraph standard and is responsible for managing the creation and acknowledgment of {Node}s and {Edge}s in the graph.
17: contract TitlesGraph is IOpenGraph, IEdgeManager, OwnableRoles, EIP712, UUPSUpgradeable {
..SNIP..
52:     constructor(address owner_, address admin_) {
53:         _initializeOwner(owner_);
54:         _grantRoles(admin_, ADMIN_ROLE);
55:     }
..SNIP..

Note that the constructor cannot be used in the proxy pattern. In Line 52 above, the constructor attempts to set the owner to the deployer address when the contract is deployed.

However, because the proxy pattern is being used, the constructor does not have any effect on the proxy. As such, the proxy's owner is still empty.

Impact

Core functionalities of TitlesGraph contract will be broken. Any functions within the TitlesGraph that are restricted to only the owner or admin will not be executable since the owner and admin are not initialized. This includes the following functions:

• createEdges
• grantRoles
• revokeRoles
• _authorizeUpgrade (Contract no longer upgradable)

Code Snippet

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

Tool used

Manual Review

Recommendation

Consider implementing an initialize function so that the state of the proxy can be initialized accordingly.

contract TitlesGraph is IOpenGraph, IEdgeManager, OwnableRoles, EIP712, UUPSUpgradeable {
..SNIP..
+    function initialize(address owner_, address admin_) external initializer {
+        _initializeOwner(owner_);
+               _grantRoles(admin_, ADMIN_ROLE);
+    }
+
+   constructor() initializer {}

The last line of code will ensure that no one can execute the initialize function on the logic/implementation contract after deployment to guard against attacks in which attackers attempt to take over the logic/implementation by making themselves the owner or admin.

Duplicate of #445