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

User can create a large number of entries in pendingStakes causing core functions to DoS

georgishishkov

Summary

The pendingStakes array is unbounded and a malicios user can deposit arbitrarily small amounts, creating numerous PendingStake entries via increasePosition(). If the array is large enough it will cause the consolidatePendingStakes() function to DoS due to it running out of gas and hitting the block gas limit. This will block numerous core functions of the protocol and lock funds.

Vulnerability Details

If a user wants to deposit funds into the LiquidityPool and increase their stake, they call the increasePosition() function, having pre-approved an amount of TST or EUROs to deposit.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L134

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);
}

As we can see the function checks if either _tstVal or _eurosVal is 0, and then pulls the amount from the user. There is no lower limit on the this amount, meaning it can be as low as 1. Then a new entry into the pendingStakes array is made.

This opens the opportunity for a malicios user to create as many entries as they want. Let's now take a look at the consolidatePendingStakes() function.
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L119

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--;
}
}
}

As we can see there is a deadline. This seems to be a mesure to prevent sandwich attacks on liquidations. This deadline functionality checks if a stake has been created at least 1 day ago. If 1 day has passed since the stake is created, then it can be consolidated, meaning the user will have their pending stake added to their actual stake and they can start receiving rewards for it.

Also, the function iterates over the pendings stakes array, adding each of it's stakes one by one to the positions[] storage array. This is very gas intensive and if the pendingStakes array is large enough, this function will cause the transaction to run out of gas and revert.

There are calls to consolidatePendingStakes() from:

  • increasePosition()

  • decreasePosition()

  • distributeAssets()

An attacker must: create a large number of entries into the pendingStakes array via increasePosition() and then wait 1 day. After that calls to the above listed functions will revert due to transactions running out of gas.

Impact

After this attack it will not be possible:

  • For a user to add stake

  • For a user to withdraw stake

  • To liquidate a vault

Tools Used

Manual Review

Recommendations

Adding a minimum deposit amount will mitigate this.

Updates

Lead Judging Commences

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

pendingstake-dos

georgishishkov Submitter
over 1 year ago
hrishibhat Lead Judge
over 1 year ago
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.