The Standard
The Standard
DeFiHardhat
20,000 USDC
View results
Previous Next
Submission Details
Severity: high
Valid

Possible DOS in `LiquidationPool` due to unbounded `pendingStakes` array

NentoR

Summary

LiquidationPool can get intentionally or unintentionally DOSed by there existing too many pending positions. This will lead to time periods where stakers might not have any incentive to interact with this contract.

Vulnerability Details

Anyone can start staking in the LiquidationPool contract by invoking LiquidationPool::increasePosition(). Every time a user creates a new position, it gets pushed into the pendingStakes array. This is an externally-controllable unbounded array and introduces a risk of a potential DOS through gas griefing.

The entry point of this exploit would be the increasePosition() function, which is invoked every time a user wants to increase their existing position or start staking for the first time:

function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
require(_tstVal > 0 || _eurosVal > 0);
consolidatePendingStakes();
ILiquidationPoolManager(manager).distributeFees();
if (_tstVal > 0) IERC20(TST).safeTransferFrom(msg.sender, address(this), _tstVal);
if (_eurosVal > 0) IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _eurosVal);
pendingStakes.push(PendingStake(msg.sender, block.timestamp, _tstVal, _eurosVal));
addUniqueHolder(msg.sender);
}

This would in turn call consolidatePendingStakes(), which iterates through all pendingStakes entries and checks which ones can be turned into consolidated positions:

function consolidatePendingStakes() private {
uint256 deadline = block.timestamp - 1 days;
for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {
PendingStake memory _stake = pendingStakes[uint256(i)];
if (_stake.createdAt < deadline) {
positions[_stake.holder].holder = _stake.holder;
positions[_stake.holder].TST += _stake.TST;
positions[_stake.holder].EUROs += _stake.EUROs;
deletePendingStake(uint256(i));
// pause iterating on loop because there has been a deletion. "next" item has same index
i--;
}
}
}

The following functions from the contract are directly or indirectly affected:

  • (public) position()

  • (public) increasePosition()

  • (public) decreasePosition()

  • (public) distributeFees()

  • (public) distributeAssets()

  • (private) getTstTotal()

  • (private) holderPendingStakes()

  • (private) deletePendingStake()

  • (private) consolidatePendingStakes()

Impact

This will result in varying gas costs for users interacting with the pool. Some of the stakers might lose incentive to use it because the gas costs could become far higher than their staked amounts and newcomers might not want to stake at all.

This could also be maliciously exploited by actors who want to DOS the protocol. They would create as many pending positions as their resources allow them to which would lead to most of the stakers being locked out of the contract or even pushing the transaction gas required beyond the block gas limit.

Tools Used

Manual Analysis

Recommendations

I would recommend converting pendingStakes to a mapping, similar to how it's done with positions.

Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

pendingstake-dos

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Validated
Assigned finding tags:

pendingstake-high

Support

FAQs

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