Lines of code

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

Impact

Anyone can change implementation of a Well, which could result in funds theft of all liquidity providers. Diagram

Currently the original flow, which is demostrated by the team to deploy a WellUpgradeable is to pass a WellUpgradeable implementation. to Aquifer#boreWell function, which will create new WellUpgradeable with set immutable data. After that owner of the well should create a ERC1967Proxy and as implementation set the newly cloned contract.

The result will be that users will call functions on the ERC1967Proxy, which forwards to creators WellUpgradeable for the logic and write to ERC1967Proxy storage.

The problem is that the team has forgotten to put onlyOwner modifier to WellUpgradeable#upgradeTo and WellUpgradeable#upgradeToAndCall functions. Actually currently owner role is useless.

This is a critical bug, because proxy contract will be holding token liquidity and any malicious party can change the implementation, if he has first registered the malicious implementation in Aquifer, which is easily done. Hacker can update to implementation, which has function transferAllTokensToMe(). Because implementation is the address that ERC1967Proxy delegatecall to, transferAllTokensToMe will be executed in the context of the ERC1967Proxy, which is the address holding all the liquidity. transferAllTokensToMe function will then call ERC20#transfer({hackerAddress}, {allBalance}) and drain all user's funds.

Proof of Concept

I have commented vm.startPrank(initialOwner); in /test/WellUpgradeable.t.soltestUpgradeToNewImplementation test to demonstrate that anyone can upgrade the implementation:

    function testUpgradeToNewImplementation() 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(initialOwner);
        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();
    }

Tools Used

Manual Review

Recommended Mitigation Steps

Add onlyOwner to to the critical functions.

Assessed type

Invalid Validation

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

`WellUpgradeable::upgradeTo/upgradeToAndCall` lack access control #75