GitHub
https://github.com/Cyfrin/2024-07-zaros/blob/d687fe96bb7ace8652778797052a38763fbcbb1b/src/perpetuals/branches/LiquidationBranch.sol#L167

Summary

The liquidateAccounts function in the LiquidationBranch contract relies on the values function from EnumerableSet.UintSet to copy the entire activeMarketsIds set from storage to memory. This operation is gas-intensive and designed primarily for view accessors. If the activeMarketsIds set contains a large number of entries, the function may fail due to out-of-gas errors, causing the entire transaction to revert.

Even though the documentation mentions that Arbitrum will be the deployment chain for this protocol, where gas is less of an issue, the values function is still not recommended for state-changing functions. Accessing these values without proper limitations can cause issues during liquidations.

Impact

Using the values function within a state-changing function poses a significant risk of transaction failure due to high gas consumption. The values function copies the entire storage to memory, which can be expensive and is typically used for view accessors to avoid gas costs. As stated in the natspac docs:

WARNING: This operation will copy the entire storage to memory, which can be quite expensive. This is designed to mostly be used by view accessors that are queried without any gas fees. Developers should keep in mind that this function has an unbounded cost, and using it as part of a state-changing function may render the function uncallable if the set grows to a point where copying to memory consumes too much gas to fit in a block.

Given there are no restrictions on adding entries to activeMarketsIds, if a large number of entries of activeMarketsIds are liquidated, the liquidateAccounts function might fail due to out-of-gas errors caused by copying the set to memory and iterating over it. This can disrupt the protocol's operation and user experience by causing transaction failures.

Proof of Concept

The liquidateAccounts function calls the values function from EnumerableSet.UintSet to copy activeMarketsIds to memory:

The values function copies all the entries of activeMarketsIds from storage to memory, potentially consuming a large amount of gas if the set is large.

Recommendation

Consider adding a limit on the number of activeMarketsIds to be processed in a single call. This way, the gas limit can never be reached, preventing potential denial-of-service attacks due to out-of-gas errors.