Open howlbot-integration[bot] opened 1 month ago
alex-ppg marked the issue as selected for report
alex-ppg marked the issue as satisfactory
The Warden and its duplicates have properly identified that the upgrade methodology of a WellUpgradeable
is insecure, permitting any contract to be upgraded to an arbitrary implementation. Specifically, the upgrade authorization mechanism will ensure that:
Given that well registration on the Aquifier is unrestricted as seen here, it is possible to practically upgrade any well to any implementation.
I believe a high-risk assessment is valid as this represents a critical security issue that can directly lead to total fund loss for all wells deployed in the system.
Lines of code
https://github.com/code-423n4/2024-07-basin/blob/7d5aacbb144d0ba0bc358dfde6e0cc913d25310e/src/WellUpgradeable.sol#L65
Vulnerability details
Impact
WellUpgradeable
is an upgradeable version of theWell
contract, inheriting from OpenZeppelin'sUUPSUpgradeable
andOwnableUpgradeable
contracts. According to OpenZeppelin’s documentation forUUPSUpgradeable.sol
(here and here), the internal_authorizeUpgrade
function must be overridden to include access restriction, typically using theonlyOwner
modifier. This must be done to prevent unauthorized users from upgrading the contract to a potentially malicious implementation.However, in the current implementation the
_authorizeUpgrade
function is overridden with custom logic but lacks theonlyOwner
modifier. As a result, theupgradeTo
andupgradeToAndCall
methods can be invoked by any address, allowing anyone to upgrade the contract, leading to the deployment of malicious code and compromise the integrity of the contract.Proof of concept
The following test demonstrates that
WellUpgradeable
can be upgraded by any address, not just the owner. It is based ontestUpgradeToNewImplementation
with the difference that a new user address is created and used to call theupgradeTo
function, successfully upgrading the contract and exposing the lack of access control.Paste the following test into
WellUpgradeable.t.sol
:Recommended mitigation steps
Add the
onlyOwner
modifier to the_authorizeUpgrade
function inWellUpgradeable.sol
to restrict upgrade permissions:Assessed type
Access Control