The Standard

Summary

When the number of holders and/or pending stakes exceeds a certain limit, the iteration over the holders and/or pendingStakes array will cause an out of gas exception and block the entire protocol.

In order to distribute fees to all eligible stakers, the iteration over the holders array should be batched over several iterations if a specified gas limit is exceeded.

Also, updating the array length in a loop is very risky and should be avoided. In consolidatePendingStakes(), while iterating over the pendingStakes array, the deletePendingStake() function is called to delete a specific element in the pendingStakes array. This kind of practice should always be avoided.

Remark:

The unbounded loops problem affects various different functions in the current contract and needs to be addressed. At the launch of the project and for a certain time after the launch this probably won't cause any problems, however, at a certain stage, when the number of protocol users exceeds a certain limit, this contract will become unusable.

If it is not possible to use a mapping for certain data structures (like the holders array) and if there is no way to restrict the size of the array to a specific limit, the iteration of such an array needs to be batched over several transactions depending on the result of a call to the Solidity gasleft() function.

A detailed coding example is provided for the distributeFees() and distrubuteAssets() functions that needs to iterate over all stakers in order to distribute protocol fees.

This problem affects various parts in the LiquidationPool contract and I will briefly address each one of individually.

Vulnerability Details

Unbounded loops are loops without a defined endpoint, meaning they can theoretically run indefinitely. In Solidity smart contracts, these loops can be particularly problematic, as they consume an unpredictable amount of gas. At a certain stage, the block.gasLimit will be reached, the transaction will fail and the entire protocol is at risk of becoming unusable.

Therefore, if necessary, the distributeFees() and distributeAssets() function calls should be batched over several transactions. Whenever a specific gas limit is reached, the current transaction should be finalized and a new function call should be required to continue the loop over the holders array.

There is an example below in the Recommendations section that shows how this can be achieved for the distributeFees() and distributeAssets() functions.

Impact

When the number of holders and/or pending stakes exceeds a certain limit, the iteration over the holders and/or pendingStakes array will cause an out of gas exception, prevent the retrieval of position details for a specific holder and potentially block the entire protocol.

Tools Used

Manual Review

Recommendations

1. distributeFees()

Batch the distributeFees() function call over several transactions if a specific gas limit is exceeded.

The following code could be used for the distributeFees() function:

nextHolderIndexForFees: state variable to hold the current holder index

MIN_GAS_AMOUNT: a constant that holds the minimum amount of gas that needs to be available in order to continue the loop iteration.

continueFeeDistribution: a Boolean that indicates if a specific fee distribution event has been finished or if we need to keep calling the distributeFees() function in order to distribute fees to all stakers.

2. distributeAssets()

Batch the distributeAssets() function call over several transactions if a specific gas limit is exceeded.

The following code can be used for the distributeAssets() function:

nextHolderIndexForAssets: state variable to hold the current holder index for the asset distribution

MIN_GAS_AMOUNT: a constant that holds the minimum amount of gas that needs to be available in order to continue the loop iteration.

continueAssetDistribution: a Boolean that indicates if a specific asset distribution event has been finished or if we need to keep calling the distributeAssets() function in order to distribute assets to all stakers.

3. holderPendingStakes()

Don't use arrays for data structures that can't be contained to a limited size (like the Token array in the tokenManager).

The pendingStakes array as well as the PendingStake structure should be deleted and the TST, EUROs and createdAt variables from the PendingStake structure should be added as pendingTST, pendingEUROs and pendingStakeCreatedAt variables to the Position structure, so they can easily be accessed via the positions mapping for a specific user.

Whenever a user opens a new staking position or adds to an existing one, those values (pendingTST, pendingEUROs and pendingStakeCreatedAt) will be updated in the Position struct for the corresponding user.

Updated Position struct:

Pending stakes can then be consolidated for each individual user on specific actions, like: increasePosition, decreasePosition, distributeFees, distributeAssets...

After checking the timestamp and the value of pendingStakeCreatedAt, the variables can be updated in the Position structure for the corresponding user if the required amount of time has passed => reset the pendingTST, pendingEUROs and pendingStakeCreatedAt variables and update to the TST and EUROs variables in the Position structure accordingly.

4. deleteHolder()

The holders array is only required for the functions distributeFees() and distributeAssets() in order to distribute fees and assets to all eligible stakers. For those 2 functions, the iteration over the holders array should be batched over several iterations if a specified gas limit is exceeded.

Stakers can be added to the holders array in order to perform those fee and asset distributions, but they don't necessarily need to be deleted. So, the deleteHolder() function is not really required. When we iterate over the users in the holders array, we can easily verify the eligibility for fee and asset distribution of each staker by checking the corresponding position in the positions mapping.

Delete the deleteHolder() function and the corresponding function call in the deletePosition() function from the contract code and batch the distributeFees() and distributeAssets() function calls over several transactions if a specific gas limit is exceeded.

5. consolidatePendingStakes()

• The pendingStakes array as well as the PendingStake structure should be deleted and the TST, EUROs and createdAt variables from the PendingStake structure should be added as pendingTST, pendingEUROs and pendingStakeCreatedAt variables to the Position structure, so they can easily be accessed via the positions mapping for a specific user.

• Whenever a user opens a new staking position or adds to an existing one, those values (pendingTST, pendingEUROs and pendingStakeCreatedAt) will be updated in the Position struct for the corresponding user.

• Pending stakes can then be consolidated for each individual user on specific actions, like: increasePosition, decreasePosition, distributeFees, distributeAssets...

• After checking the timestamp and the value of pendingStakeCreatedAt, the variables can be updated in the Position structure for the corresponding user if the required amount of time has passed => reset the pendingTST, pendingEUROs and pendingStakeCreatedAt variables and update to the TST and EUROs variables in the Position structure accordingly.

Replace the existing consolidatePendingStakes() function with the following function:

And, in all functions that require a pending stake consolidation (increasePosition() and decreasePosition() ), call the new consolidatePendingStakesUser() function.

In the increasePosition() function, instead of adding a new entry to the pendingStakes array, simply update the values for pendingTST, pendingEUROs and pendingStakeCreatedAt in the corresponding positions mapping.

Here is a modified version of the increasePosition() function:

6. addUniqueHolder()

The addUniqueHolder() function is called in the increasePosition() function to add a new user to the holders array if that user is not already present. Before calling the addUniqueHolder() function, this function updates the pendingStakes mapping.

Again, the pendingStakes mapping is not required and the corresponding pending values should be added directly to the positions mapping.

By verifying the values in the positions mapping for the calling user, we can easily identify if that user is already present in the holders array and if that is not the case, we simply call:

holders.push(msg.sender);

So, the addUniqueHolder() function is not really required and can be deleted.

7. deletePendingStake()

• Again, the pendingStakes array is not required and can be deleted together with the PendingStake structure and the deletePendingStake() function.

• Instead, the TST, EUROs and createdAt variables from the PendingStake structure should be added as pendingTST, pendingEUROs and pendingStakeCreatedAt variables to the Position structure, so they can easily be accessed via the positions mapping for a specific user.

• Whenever a user opens a new staking position or adds to an existing one, those values (pendingTST, pendingEUROs and pendingStakeCreatedAt) will be updated in the Position struct for the corresponding user.

• Pending stakes can then be consolidated for each individual user on specific actions, like: increasePosition, decreasePosition, distributeFees, distributeAssets...

• After checking the timestamp and the value of pendingStakeCreatedAt, the variables can be updated in the Position structure for the corresponding user if the required amount of time has passed => reset the pendingTST, pendingEUROs and pendingStakeCreatedAt variables and update to the TST and EUROs variables in the Position structure accordingly.

8. getStakeTotal() and getTstTotal()

• Delete the functions getStakeTotal() and getTstTotal().

• Create 2 storage variables: stakeTotal and tstTotal and update them on all function calls that modify those values: increasePosition, decreasePosition, distributeFees, distributeAssets, deletePosition, deleteHolder, consolidatePendingStakes...