Lines of code

https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/WellUpgradeable.sol#L93-L96 https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/WellUpgradeable.sol#L104-L107 https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/WellUpgradeable.sol#L33

Vulnerability details

Impact

Unauthorized Upgrades: Without ownership restrictions on upgrade functions, anyone could potentially initiate an upgrade, leading to possible unauthorized modifications of the contract logic.
False Sense of Security: The presence of OwnableUpgradeable might create a false sense of security, misleading developers and users into thinking the contract has proper access controls when it does not.

Proof of Concept

The WellUpgradeable contract inherits from OwnableUpgradeable, but fails to utilize the ownership management functionalities provided by the OwnableUpgradeable contract. Specifically, there are no functions restricted to the owner using the onlyOwner modifier, which leaves critical functions, including the upgrade functionality, vulnerable to unauthorized access.

Code Snippet:

@> contract WellUpgradeable is Well, UUPSUpgradeable, OwnableUpgradeable {

      ...

 @>   function init(string memory _name, string memory _symbol) external override reinitializer(2) {
        __ERC20Permit_init(_name);
        __ERC20_init(_name, _symbol);
        __ReentrancyGuard_init();
        __UUPSUpgradeable_init();
        __Ownable_init();

        IERC20[] memory _tokens = tokens();
        uint256 tokensLength = _tokens.length;
        for (uint256 i; i < tokensLength - 1; ++i) {
            for (uint256 j = i + 1; j < tokensLength; ++j) {
                if (_tokens[i] == _tokens[j]) {
                    revert DuplicateTokens(_tokens[i]);
                }
            }
        }
    }

    ...
    function _authorizeUpgrade(address newImplmentation) internal view override {
        // verify the function is called through a delegatecall.
        require(address(this) != ___self, "Function must be called through delegatecall");

        // verify the function is called through an active proxy bored by an aquifer.
        address aquifer = aquifer();
        address activeProxy = IAquifer(aquifer).wellImplementation(_getImplementation());
        require(activeProxy == ___self, "Function must be called through active proxy bored by an aquifer");

        // verify the new implmentation is a well bored by an aquifier.
        require(
            IAquifer(aquifer).wellImplementation(newImplmentation) != address(0),
            "New implementation must be a well implmentation"
        );

        // verify the new implmentation is a valid ERC-1967 implmentation.
        require(
            UUPSUpgradeable(newImplmentation).proxiableUUID() == _IMPLEMENTATION_SLOT,
            "New implementation must be a valid ERC-1967 implmentation"
        );
    }

@>  function upgradeTo(address newImplementation) public override {
        _authorizeUpgrade(newImplementation);
        _upgradeToAndCallUUPS(newImplementation, new bytes(0), false);
    }

@>  function upgradeToAndCall(address newImplementation, bytes memory data) public payable override {
        _authorizeUpgrade(newImplementation);
        _upgradeToAndCallUUPS(newImplementation, data, true);
    }

   ...
}

The _authorizeUpgrade function does include some access controls, but these controls are based on proxy patterns and implementation checks, not on ownership.

Tools Used

VSCode

Recommended Mitigation Steps

Restrict access to critical functions like upgradeTo and upgradeToAndCall, etc. to the contract owner by applying the onlyOwner modifier. This will ensure that only authorized personnel can perform sensitive operations.

Assessed type

Access Control

code-423n4 / 2024-07-basin-findings

WellUpgradeable inherits from OwnableUpgradeable but misses restricting critical functions to onlyOwner #68