Due to no access control on `WellUpgradeable::_authorizeUpgrade()` anyone can change the implementation contract and can destroy the main Proxy contract.

[c4-bot-10]

Lines of code

https://github.com/code-423n4/2024-07-basin/blob/main/src/WellUpgradeable.sol#L65-L84

Vulnerability details

Vulnerability Details :

WellUpgradeable contract inheriting UUPSUpgradeable so overriding _authorizeUpgrade() function. But adding no access control to this function can lead to setting implementation contract by anyone since this function is called authorization. By setting any malicious contract as implementation attacker can selfdestruct() the proxy contract. Since implementation is called using delegatecall from proxy.

Code Snippet :

src/WellUpgradeable.sol#L65-L84

65: 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"
        );

Since WellUpgradeable contract is inheriting UUPSUpgradeable from openzeppelin and WellUpgradeable overrides upgradeTo ,we can see in upgrading implementation address _authorizeUpgrade() is called.

https://github.com/code-423n4/2024-07-basin/blob/main/src/WellUpgradeable.sol#L93C5-L96C6

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

Malicious WellUpgradeable implementation contract

contract WellUpgradeable is UUPSUpgradeable {

   function destroy() public override {
        selfdestruct(address(0));//@audit this code will be implanted here
    }

    function _authorizeUpgrade(address) internal view override {}
}

When destroy() will be called from proxy contract using delegatecall it will destroy the main proxy contract using selfdestruct(). And erase all the storage from main proxy contract of WellUpgradeable.

Impact :

The main proxy contract will be destroyed and erase all the storage from main proxy contract of WellUpgradeable.

Tools Used

Manual Review

Recommended Mitigation Steps :

Add proper access control to _authorizeUpgrade() function since it is used for authorization at the time of changing implementation contract address. Add onlyOwner modifier to the _authorizeUpgrade() function so. Only owner can be authorized to upgrade the implementation address.

File : src/WellUpgradeable.sol
  ...

- function _authorizeUpgrade(address newImplementation) internal view override {}
+ function _authorizeUpgrade(address newImplementation) internal view override onlyOwner {}

Assessed type

Access Control