Lines of code

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

Vulnerability details

Impcat

The WellUpgradeable::_authorizeUpgrade function is internal but it used in WellUpgradeable::upgradeTo and WellUpgradeable::upgradeToAndCall and all of them lacks proper access control, allowing unauthorized parties to perform upgrades. This can compromise the security and integrity of the contract by enabling malicious actors to deploy potentially harmful contract upgrades.

Proof of Concept

Unauthorized parties can call the _authorizeUpgrade function, leading to unauthorized upgrades of the contract.

function _authorizeUpgrade(address newImplmentation) internal view override {
    // No access control check => anyone can call this 
    require(address(this) != ___self, "Function must be called through delegatecall");

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

    require(
        IAquifer(aquifer).wellImplementation(newImplmentation) != address(0),
        "New implementation must be a well implementation"
    );

    require(
        UUPSUpgradeable(newImplmentation).proxiableUUID() == _IMPLEMENTATION_SLOT,
        "New implementation must be a valid ERC-1967 implementation"
    );
}

Tools Used

Manual code review

Recommended Mitigations

Add an access control check to ensure that only authorized entities (e.g., the contract owner, dao ...) can call the _authorizeUpgrade function:

function _authorizeUpgrade(address newImplementation) internal view override {
    // Add access control check
++  require(msg.sender == owner(), "Only the owner can authorize upgrades");

    require(address(this) != ___self, "Function must be called through delegatecall");

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

    require(
        IAquifer(aquifer).wellImplementation(newImplementation) != address(0),
        "New implementation must be a well implementation"
    );

    require(
        UUPSUpgradeable(newImplementation).proxiableUUID() == _IMPLEMENTATION_SLOT,
        "New implementation must be a valid ERC-1967 implementation"
    );
}

Assessed type

Upgradable

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

Incorrect Access Control in _authorizeUpgrade #58