Yu3H0 - wrong usage of UUPSUpgradeable leads to non-upgradeable

[sherlock-admin4]

Yu3H0

high

wrong usage of UUPSUpgradeable leads to non-upgradeable

Summary

wrong usage of UUPSUpgradeable

Vulnerability Detail

TitlesGraph is an UUPSUpgradeable contract, and the admin said it will be an upgradeable contract. However, when using TitlesGraph, it directly use new rather than proxy, which will leads to non-upgradeable.

The upgrade function upgradeToAndCall can only be called by proxy. https://github.com/Vectorized/solady/blob/main/src/utils/UUPSUpgradeable.sol#L81

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

contract TitlesGraph is IOpenGraph, IEdgeManager, OwnableRoles, EIP712, UUPSUpgradeable {

function upgradeToAndCall(address newImplementation, bytes calldata data)
        public
        payable
        virtual
        onlyProxy

/// @dev Requires that the execution is performed through a proxy.
    modifier onlyProxy() {
        uint256 s = __self;
        /// @solidity memory-safe-assembly
        assembly {
            // To enable use cases with an immutable default implementation in the bytecode,
            // (see: ERC6551Proxy), we don't require that the proxy address must match the
            // value stored in the implementation slot, which may not be initialized.
            if eq(s, address()) {
                mstore(0x00, 0x9f03a026) // `UnauthorizedCallContext()`.
                revert(0x1c, 0x04)
            }
        }
        _;
    }

Impact

non-upgradeable contract

Code Snippet

https://github.com/Vectorized/solady/blob/main/src/utils/UUPSUpgradeable.sol#L81

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

contract TitlesGraph is IOpenGraph, IEdgeManager, OwnableRoles, EIP712, UUPSUpgradeable {

function upgradeToAndCall(address newImplementation, bytes calldata data)
        public
        payable
        virtual
        onlyProxy

/// @dev Requires that the execution is performed through a proxy.
    modifier onlyProxy() {
        uint256 s = __self;
        /// @solidity memory-safe-assembly
        assembly {
            // To enable use cases with an immutable default implementation in the bytecode,
            // (see: ERC6551Proxy), we don't require that the proxy address must match the
            // value stored in the implementation slot, which may not be initialized.
            if eq(s, address()) {
                mstore(0x00, 0x9f03a026) // `UnauthorizedCallContext()`.
                revert(0x1c, 0x04)
            }
        }
        _;
    }

Tool used

Manual Review

Recommendation

use uups correctly

Duplicate of #445

[MageIntern]

Escalation

dup of https://github.com/sherlock-audit/2024-04-titles-judging/issues/445

[Hash01011122]

Responded in #272 and #445.

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