Anyone can upgrade Well's implementation

[howlbot-integration]

Lines of code

https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/WellUpgradeable.sol#L64-L85
https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/WellUpgradeable.sol#L92-L95

Vulnerability details

Impact

Anyone can upgrade the contract to a malicious implementation that they control, allowing them to exploit the contract, and its users.

Proof of Concept

1. Context

WellUpgradeable.sol is designed to be an upgradeable version of Well.sol, for which a new implementation can be easily and seamlessly upgraded to. As thus, the contract inherits UUPSUprageable and other upgradeable counterparts and also exposes the _authorizeUpgrade, upgradeTo and upgradeToAndCall functions. This ensures that the contracts can be updated.

2. Bug Location

As would be expected from a sensitive contract's implementation, these functions should be limited to the admins alone. This however isn't the case as the functions lacks an onlyOwner modifier (despite inheriting openzeppelin's OwnableUpgradeable.sol).

Looking at the functions, It can be observed that there's no form of access control.

    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);
    }

    function _authorizeUpgrade(address newImplmentation) internal view override {
//...
    }

Particularly the _authorizeUpgrade function, which according to OpenZeppelin's implementation will normally be protected by an access control.

    /**
     * @dev Function that should revert when `msg.sender` is not authorized to upgrade the contract. Called by
     * {upgradeTo} and {upgradeToAndCall}.
     *
     * Normally, this function will use an xref:access.adoc[access control] modifier such as {Ownable-onlyOwner}.
     *
     * ```solidity
     * function _authorizeUpgrade(address) internal override onlyOwner {}
     * ```
     */
    function _authorizeUpgrade(address newImplementation) internal virtual;

3. Final Effect

The possibilities are endless as anyone can easily upgrade the contract to an implementation of their choice, tailoring the new upgrade to their desires.

4. Runnable POC

The gist link below holds a test case that shows a user can upgrade the contract to any implementation of his choice.

https://gist.github.com/ZanyBonzy/c982fad6a5a83759e98cf3f9860224c3

The test can be run using forge test --mt testAnyoneCanUpgradeToNewImplementation -vvvv and should pass.

    ├─ [1143] proxyAddress::getImplementation() [staticcall]
    │   ├─ [760] customWell::getImplementation() [delegatecall]
    │   │   ├─ [481] CustomMockWell::getImplementation() [delegatecall]
    │   │   │   └─ ← [Return] customWell: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]
    │   │   └─ ← [Return] customWell: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]
    │   └─ ← [Return] customWell: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]
    ├─ [0] VM::assertEq(customWell: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20], customWell: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]) [staticcall]
    │   └─ ← [Return] 
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    └─ ← [Stop] 

Suite result: ok. 1 passed; 0 failed; 0 skipped; finished in 2.41ms (598.46µs CPU time)

Ran 1 test suite in 1.05s (2.41ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual code review

Recommended Mitigation Steps

Recommend adding the onlyOwner modifier to the _authorizeUpgrade function.

Assessed type

Access Control

[nickkatsios]

The modifier was mistakenly omitted here and should definitely be added.

[c4-judge]

alex-ppg marked issue #52 as primary and marked this issue as a duplicate of 52

[c4-judge]

alex-ppg marked the issue as satisfactory