The Standard

The Standard
DeFiHardhat
20,000 USDC
View results
Submission Details
Severity: low
Invalid

Storing unique holders in an array structure de-incentivizes participation to the protocol

Summary

In order for a staker to participate to the protocol, they have to call LiquidationPool::increasePosition. By doing so, they are going to be registered to the list of unique holders via the addUniqueHolder function.
However, the list of holders is stored in an array structure, so checking for uniqueness requires to examine all elements in the array (worst case scenario).
This means that, if the staker is late to join the protocol, they will have to loop through a potentially very big array, meaning gas costs can be very expensive.
This can effectively de-incentivize them from ever participating to the protocol, representing a missed opportunity for additional liquidity for the protocol.
The same issue is met when a staker wishes to delete their staking position, as to remove a staker it is necessary to loop through the entire holders' array.
In an extreme scenario (huge amount of different stakers), it could become effectively impossible to retrieve their stake, due to gas exhaustion when looping through the array.

Vulnerability Details

When joining the protocol as a staker, users call LiquidationPool::increasePosition, whose very last operation is to call the following function:

function addUniqueHolder(address _holder) private {
for (uint256 i = 0; i < holders.length; i++) {
if (holders[i] == _holder) return;
}
holders.push(_holder);
}

The longer the array, the higher the gas costs to face for a new staker to join. If the length of the array is big enough, the costs to face can be expensive enough to justify a lack of interest from new potential stakers.
This can effectively cause the protocol to miss out on new liquidity.

Additionally, the LiquidationPool::decreasePosition function triggers the execution of deletePosition when a staker wishes to remove their entire stake.
Function deletePosition is shown here:

function deletePosition(Position memory _position) private {
deleteHolder(_position.holder); exhaustion
delete positions[_position.holder];
}
function deleteHolder(address _holder) private {
for (uint256 i = 0; i < holders.length; i++) {
if (holders[i] == _holder) {
holders[i] = holders[holders.length - 1];
holders.pop();
}
}
}

To remove a holder, it is necessary to loop through the entire array (worst case scenario). If this array is big, this can be a very costly operation, in terms of gas fees. Even worse, if the array reaches a significant volume of entries, the attempt to loop through it can cause a gas exhaustion, meaning it would then be impossible for a staker to remove their existing stake from the protocol, as the transaction would always revert due to gas exhaustion.

Impact

If joining the protocol as a staker gets progressively more and more expensive, users coming to the protocol in a later phase will be de-incentivized to join. This represents a lost opportunity for the protocol, as the liquidity brought by new stakers will be lost.
If removing their stake from the protocol is extremely expensive or, worse, impossible, stakers will not want to participate in the protocol, causing a scarcity of liquidity for anyone wishing to borrow.

Tools Used

Manual review, VSCode

Recommendations

Using a mapping instead of an array would be a much more efficient solution.
For the first scenario, it would suffice to check whether the key holder contains a non-default value, in order to know whether the user has already registered on the platform.
For the second scenario, removing a user would mean to reset their address in the mapping to a default value, with constant execution cost.

Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

informational/invalid

Support

FAQs

Can't find an answer? Chat with us on Discord, Twitter or Linkedin.