Submitted on Mar 6th 2024 at 19:13:58 UTC by @SAAJ for Boost | Puffer Finance
Report ID: #29081
Report type: Smart Contract
Report severity: Insight
Target: https://etherscan.io/address/0x7276925e42f9c4054afa2fad80fa79520c453d6a
Impacts:
- wrong usage of OpenZeppelin library in contract
As per OZ docs, values set in constructor in upgradeable is not safe as constructors are only called during the contract deployment, proxies can't access that information as it's stored on the original contract not on the proxy, meaning, proxies are completely oblivious to the existence of constructors.
All the initialization will be lost since it was run in the context of the logic implementation contract and not the Proxy contract.
The exception is given for immutable variable but it is also not effective as both the PufferDepositor and PufferVault are upgradeable, meaning the implementation stores the value passed in constructor and not in proxy storage, refer to OZ’S documentation which clearly states this issue.
Since implementation address can be changed meaning when upgrade will be carried out the values stored through constructor will be lost.
The addresses stored in implementation contract of PufferDepositor and PufferVault through constructor will be lost when upgrade will be processed.
This can cause unexpected behavior like loss of funds if depositing or reverting when withdrawal is made as call made to these contract will be actually made to address(0).
The recommendation is made to avoid using constructor in upgradeable contract and move the immutable variables passed in constructor for both contracts of PufferDepositor and PufferVault to the initialize function.
The POC for the issue is visible in context of not following OZ guideline in terms of not using constructor in upgradeable and using immutable with caution.
File: PufferVault.sol
constructor(PufferVault pufferVault, IStETH stETH) payable {
PUFFER_VAULT = pufferVault;
_ST_ETH = stETH;
_disableInitializers();
}
File: PufferDepositor.sol
constructor(
IStETH stETH,
ILidoWithdrawalQueue lidoWithdrawalQueue,
IStrategy stETHStrategy,
IEigenLayer eigenStrategyManager
) payable {
_ST_ETH = stETH;
_LIDO_WITHDRAWAL_QUEUE = lidoWithdrawalQueue;
_EIGEN_STETH_STRATEGY = stETHStrategy;
_EIGEN_STRATEGY_MANAGER = eigenStrategyManager;
_disableInitializers();
}