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

DOS Exposure due to Growing `LiquidationPool.holders` Array

tripathi

Summary

The increasePosition function enables users to join the liquidation pool, expanding the holders array with each new participation, such that all functions that depends on iteration over holders will run out of gas error and resulting into DOS

Vulnerability Details

The increasePosition() function adds users to the holders array:

File: contracts/LiquidationPool.sol
​
function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
//... (existing code)
​
pendingStakes.push(
PendingStake(msg.sender, block.timestamp, _tstVal, _eurosVal)
);
addUniqueHolder(msg.sender);
}
​

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

Any user can increase position by staking and address of user will be added to holder's array

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

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L112
As the number of stakers increases , holder's array will increase. There is only one instance where this length decreases when user's unstake completely

function decreasePosition(uint256 _tstVal, uint256 _eurosVal) external {
consolidatePendingStakes();
ILiquidationPoolManager(manager).distributeFees();
require(
_tstVal <= positions[msg.sender].TST &&
_eurosVal <= positions[msg.sender].EUROs,
"invalid-decr-amount"
);
if (_tstVal > 0) {
IERC20(TST).safeTransfer(msg.sender, _tstVal);
positions[msg.sender].TST -= _tstVal;
}
if (_eurosVal > 0) {
IERC20(EUROs).safeTransfer(msg.sender, _eurosVal);
positions[msg.sender].EUROs -= _eurosVal;
}
if (empty(positions[msg.sender])) deletePosition(positions[msg.sender]); //@audit a user always left 1wei in the position
}

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L149

function deletePosition(Position memory _position) private {
deleteHolder(_position.holder);
delete positions[_position.holder];
}

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L144

As Malicious user can always left 1wei stakes and their position won't be deleted i.e length won't decrease.

Further During liquidation distributeAssets() iterates through all the holders and doing state change for every holders wand hence a long list of holder's will run out of gas, breaking liquidations completely

Even distributeFees() iterates through all the holders and does write to storage variables leading to very high gas cost function

Impact

Critical functions like distributeAssets() and distributeFees() are susceptible to running out of gas due to prolonged holders array lengths, causing the breakdown of essential protocol functionalities reliant on these functions

same problem arises in the case of pendingStakes, if all the current active holder's staked within 1 day of window period consolidatePendingStakes() will revert due to gas error
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L119C14-L119C38

Tools Used

Manual

Recommendations

Seeing the current implementation I don't see any feasible solution.

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.