unix515 - Add default constructor that calls `_disableInitializers()` to StrategyPassiveManagerVelodrome

[sherlock-admin3]

unix515

medium

Add default constructor that calls _disableInitializers() to StrategyPassiveManagerVelodrome

Summary

StrategyPassiveManagerVelodrome contract uses upgradable contracts. Calling the initialize() function directly on the implementation contract behind a proxy is dangerous. In such case, if the implementation calls self-destruct or performs delegate calls it’s possible to delete the implementation leaving the contract bricked.

Vulnerability Detail

Upgradable contracts are an essential protocol feature, allowing for flexible updates and maintenance. Contracts should include a default constructor calling _disableInitializers() function: (https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/master/contracts/proxy/utils/Initializable.sol#L192) of Initializable.sol.

Impact

When implementing upgradable contracts, ensuring that the initialize() function is not accidentally called directly on the implementation contract behind the proxy is crucial.

If this occurs and the implementation contract contains self-destruct or delegate calls, it can result in the unintended deletion of the implementation contract.

Code Snippet

https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/tree/main/cowcentrated-contracts/contracts/strategies/velodrome/StrategyPassiveManagerVelodrome.sol#L155-L189

Tool used

Manual Review

Recommendation

Please append StrategyPassiveManagerVelodrome#constructor() same as StrategyPassiveManagerUniswap#constructor().

contract StrategyPassiveManagerVelodrome is 
    StratFeeManagerInitializable, IStrategyConcLiq, IStrategyVelodrome {
    ...
    event ClaimedFees(uint256 fees);

++  constructor() {
++      _disableInitializers();
++  }

    modifier onlyCalmPeriods() {
        _onlyCalmPeriods();
        _;
    }
    ...
}

Include a default constructor in the contract that calls the _disableInitializers() function from Initializable.sol. This ensures that initializers cannot be accidentally invoked on the implementation contract.

[sherlock-admin3]

2 comment(s) were left on this issue during the judging contest.

z3s commented:

Invalid; It has modifier initializer, It can not be called more than once.

DHTNS commented:

Low -> will only be valid if attack path to selfDestruct is provided