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

Malicious user can permanently DOS core protocol functionality leading to funds stuck forever

amarfares

Vulnerability Details

When a user calls LiquidationPool.sol#increasePosition() their request to increase is pushed in the PendingStake[] pendingStakes array which is an unbounded array. The issue is that if a user calls this multiple times, his position is not added onto his existing pending stake, but instead, a new pendingStake is pushed into the array.

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L134-L142

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 opens the door for a user to grief by opening n amount of requests with amount 1 wei since there are no requirements for a minimum amount, and maliciously increase the size of the array to so large that when consolidatePendingStakes() loops through pendingStakes.length it reverts due to out of gas error. There is also no fee for creating a pendingStake so the only thing a user needs to pay is gas and since the protocol will be deployed on L2's, doing this griefing attack would have a negligible cost and high impact.

The consolidatePendingStakes() function is one of the most critical in the contract since it is called at the start of increasePosition(), decreasePosition(), distributeAssets(), and what it does is loop through the pendingStakes.length, thus, all 3 of these critical functions would revert due to an out of gas error and this completely DOSes the functionality of the protocol. pendingStakes.length is also looped through in distributeFees() so that would also not work.

Since reducing the length of the array is only done through deletePendingStake() which is ALSO called in consolidatePendingStakes(), once the the malicious user sets this scenario up there is no going back and the only fix is re-deployment of the liquidation pool. If there have been any previous pending stakes up until that point of innocent users, their funds will be forever stuck in the contract.

https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L119-L132

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

Impact

Malicious user forces permanent DOS of core protocol functionality, also possibly stuck funds

Tools Used

Manual Review

Recommendations

Few things to note:

  1. If a limit is placed on how many pendingStakes a user can have, they can just always submit from multiple addresses and achieve the same so that is kind of pointless

  2. A sanity check definitely needs to be added for some minAmount

  3. Addition new pending stake amounts to existing pending stake instead of pushing new elements for every new function call

2nd point:

function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
- require(_tstVal > 0 || _eurosVal > 0);
+ require(_tstVal > minAmount || _eurosVal > minAmount);
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);
}

3rd point would probably need a mapping for address => pendingStake or storing pendingStakeId to know which element to increment and I'm not a developer to refactor so much code lol..

Updates

Lead Judging Commences

hrishibhat Lead Judge almost 2 years ago
Submission Judgement Published
Validated
Assigned finding tags:

pendingstake-dos

hrishibhat Lead Judge almost 2 years 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.

Give us feedback!