Liquid Staking

Github

https://github.com/Cyfrin/2024-09-stakelink/blob/f5824f9ad67058b24a2c08494e51ddd7efdbb90b/contracts/linkStaking/base/VaultControllerStrategy.sol#L199

Summary

The _depositToVaults function in the VaultControllerStrategy contract contains a potential data corruption vulnerability due to unsafe downcasting from uint256 to uint64, which could lead to incorrect vault indexing and fund distribution.

Vulnerability details

In the _depositToVaults function, there's an unsafe downcasting operation:

Here, vaultIndex is a uint256, but it's being cast to a uint64. If vaultIndex is larger than the maximum uint64 value (18446744073709551615), this operation will silently truncate the value, potentially corrupting the groupDepositIndex.

Impact

• Corrupted vault indexing: The contract may lose track of which vaults have been processed, leading to skipped vaults or repeated processing of the same vault.

• Uneven fund distribution: Funds may not be distributed correctly across vaults, potentially favoring some vaults over others.

• Stuck funds: In extreme cases, funds might become trapped in the contract due to incorrect indexing.

• Silent failures: The issue doesn't cause a revert, making it hard to detect and potentially allowing the contract to operate with corrupted state.

The severity is high due to the potential for fund mismanagement and the silent nature of the failure.

What causes it and where?

The issue occurs in the _depositToVaults function of the VaultControllerStrategy contract. It's caused by the implicit assumption that the vaultIndex will always fit within a uint64, which may not hold true as the number of vaults grows.

Proof of concept

In these examples, any input larger than 18446744073709551615 results in corrupted data without any error or revert.

Recommendation

Below are some suggestions which should be implemented for fixing this bug:

1. You can use SafeCast library from OZ:

   import "@openzeppelin/contracts/utils/math/SafeCast.sol";

   ​

   globalState.groupDepositIndex = SafeCast.toUint64(vaultIndex);

   This will revert if the value doesn't fit, preventing silent failures.

2. or you can also add explicit bounds checking:

   require(vaultIndex <= type(uint64).max, "Vault index too large");

   globalState.groupDepositIndex = uint64(vaultIndex);

3. or maybe consider using uint256 for groupDepositIndex if larger values are expected in the future.