No disableInitializers in StrategyPassiveManagerVelodrome which sits behind a proxy , also lacks gaps array & StratFeeManagerInitializable has incorrect gap size
Vulnerability Detail
disableInitializers call is important in constructor of implementation contract as it prevents any possibility of a selfDestruct. StrategyPassiveManagerVelodrome lacks this call . It also lack gaps array which is useful in cases of implementation changes. StratFeeManagerInitializable has incorrect gap array size, 'The size of the __gap array is calculated so that the amount of storage used by a contract always adds up to the same number (in this case 50 storage slots).' for OZ docs . Contracts inheriting should follow the convention to reduce storage corruption chances in highly unlikely cases. So the __gap array size should be 42(not 49) in StratFeeManagerInitializable. As it uses 8 slots above to store addresses.
Impact
Increased likelyhood of storage corruption
Though successful storage corruption attacks are unlikely and require admin negligence, they can occur. See recent Pike finance incident.
Incorrect usage of Beacon Proxy pattern
Low/Info issue submitted by Kalyan-Singh
Summary
No disableInitializers in StrategyPassiveManagerVelodrome which sits behind a proxy , also lacks gaps array & StratFeeManagerInitializable has incorrect gap size
Vulnerability Detail
disableInitializers call is important in constructor of implementation contract as it prevents any possibility of a selfDestruct. StrategyPassiveManagerVelodrome lacks this call . It also lack gaps array which is useful in cases of implementation changes. StratFeeManagerInitializable has incorrect gap array size, 'The size of the
__gaparray is calculated so that the amount of storage used by a contract always adds up to the same number (in this case 50 storage slots).' for OZ docs . Contracts inheriting should follow the convention to reduce storage corruption chances in highly unlikely cases. So the__gaparray size should be 42(not 49) in StratFeeManagerInitializable. As it uses 8 slots above to store addresses.Impact
Increased likelyhood of storage corruption Though successful storage corruption attacks are unlikely and require admin negligence, they can occur. See recent Pike finance incident.
Code Snippet
https://github.com/sherlock-audit/2024-05-beefy-cowcentrated-liquidity-manager/blob/42ef5f0eac1bc954e888cf5bfb85cbf24c08ec76/cowcentrated-contracts/contracts/strategies/StratFeeManagerInitializable.sol#L195
Tool used
Manual Review
Recommendation
Add disable initializers and
__gapsin StrategyPassiveManagerVelodrome & ensure correct gap size in StratFeeManagerInitializable