Summary

https://github.com/Cyfrin/2025-01-zaros-part-2/blob/39e33b2f6b3890573bb1affc41a7e520277ceb2c/src/zlp/ZlpVault.sol#L39

The ZlpVault contract has a potential storage collision risk due to the way it handles custom storage. The contract utilizes the ERC4626Upgradeable standard and integrates custom storage to manage the marketMakingEngine, decimalsOffset, and vaultId. These values are stored in a specific location that may conflict with other storage variables in the future, especially in the context of proxy upgrades or when additional contract features are added. This could lead to unexpected behavior or loss of critical state information.

Vulnerability Details

Custom Storage Location:

The contract manually defines a custom storage location using a specific bytes32 slot (ZLP_VAULT_STORAGE_LOCATION) to store its internal state (marketMakingEngine, decimalsOffset, and vaultId).

This is done via inline assembly, where the storage pointer is manually set to the computed location. This approach works well when the storage structure is well-defined and the contract is isolated, but it introduces a potential risk when new variables or features are added to the contract, or if there is a mismatch in storage locations between contract versions.

Storage Collision Risk:

The bytes32 slot calculation is based on a combination of keccak256 hashes, which can potentially collide with other contract storage locations, especially as new features are introduced in the future. This can result in accidental overwriting of critical contract data or unintentional state changes.

If another contract (or an upgrade) uses the same slot for different storage, the contract's state could be corrupted, leading to incorrect or lost information.

Upgradeability Complications:

In the context of proxy upgradeable contracts, if the storage layout isn't properly managed, upgrades could result in collisions. If the slot for ZLP_VAULT_STORAGE_LOCATION is changed during an upgrade, the contract could lose access to its previous state, causing unexpected failures or vulnerabilities in the contract’s behavior.

Exploitation Scenario:

State Corruption: A storage collision could result in a mismatch of contract data, causing unexpected behavior. For example, overwriting the marketMakingEngine could result in the vault becoming unusable, or overwriting the decimalsOffset could cause incorrect asset-to-share conversion rates.

Upgrade Failures: If the contract is upgraded and the storage layout changes, previously stored data may not align with the new contract’s expectations. This could result in loss of critical information such as marketMakingEngine or vaultId, making the contract unusable or causing financial losses.

Interference from Other Contracts: If the same storage slot is unintentionally used by other contracts interacting with ZlpVault, the data might be corrupted, leading to potential exploits or loss of funds.

Impact

Loss of Contract State: The contract could lose its state, such as the marketMakingEngine address or vaultId, if storage locations collide. This could result in a complete failure of the contract’s operations.

Upgrade Issues: During upgrades, a mismatch in storage layout can lead to broken contract functionality, loss of data, or errors in the contract logic.

Corrupted Interactions: Other contracts interacting with ZlpVault might experience data corruption or incorrect functionality due to storage collisions, leading to potential loss of funds or failure to execute intended operations.

Tools Used

Manual Review

Recommendations

Use a Standardized Storage Approach

Avoid Custom Storage Locations: Instead of manually computing a custom storage location, consider using standard Solidity storage practices. If a custom storage structure is needed, ensure it is well-documented and follow a strict convention for managing storage across contract upgrades.

Storage Layout Management: To ensure a proper upgrade path, manage storage locations with caution. You can avoid using raw assembly for storage slot assignments unless absolutely necessary. If custom storage is used, ensure the contract is tested with different versions to confirm that upgrades do not result in storage collisions.

Example of Standard Storage Definition:

Upgrade and Proxy Management Best Practices

Use Transparent Proxy Pattern: If using a proxy pattern (e.g., UUPS), make sure the proxy and implementation contracts are well-aligned in terms of storage layout. This ensures that state is maintained across upgrades.

Storage Layout Versioning: Implement versioning for storage layouts, ensuring that changes to storage variables are backward-compatible with previous contract versions.

Test Contract Upgrades: Before upgrading the contract, ensure that the storage layout remains compatible with previous versions. Use testnets to perform upgrade tests to avoid state corruption.