The Standard

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

A malicious actor can DOS the functions in the LiquidationPool by adding so many number of the pending stake data (`PendingStake`) to the `pendingStakes` array storage in the short period

Summary

A malicious actor can DOS by adding so many number of the pending stake data (PendingStake) to the pendingStakes array storage at low-cost by repeatedly calling the LiquidationPool#increasePosition() with 1 wei of $TST or $EUROs in the short period (i.e. within 10-30 minutes).

Since each pending stake data would be kept being held in the pendingStakes array storage for at least 1 day from it would be created via the LiquidationPool#increasePosition(), every TXs of the following functions in the LiquidationPool, which is called by the subsequent users after the malicious actor did this attack, would be reverted (DOSed) for at least 1 day - due to reaching a gas limit in the for-loop.

  • LiquidationPool#increasePosition()/decreasePosition()

  • LiquidationPoolManager#runLiquidation()

Vulnerability Details

Within the LiquidationPool contract, the pendingStakes array storage would be defined like this:
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L23

PendingStake[] private pendingStakes;

When a user would stake their $TST or/and $EUROs into the LiquidationPool, the user would call the LiquidationPool#increasePosition().

Within the LiquidationPool#increasePosition(), the LiquidationPool#consolidatePendingStakes() and the LiquidationPool#distributeFees() would be called.
And then, the user's staking data (PendingStake) would be added to the pendingStakes array storage with the current timestamp (block.timestamp) like this:
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L136-L137
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L140

function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
...
consolidatePendingStakes(); ///<------------------------- @audit
ILiquidationPoolManager(manager).distributeFees(); ///<------------------------- @audit
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)); ///<---------- @audit
addUniqueHolder(msg.sender);
}

Within the LiquidationPool#consolidatePendingStakes(), a for-loop processing would be started based on the number of elements in the pendingStakes (pendingStakes.length).
Within the for-loop, if this function would be called after the deadline (_stake.createdAt < deadline), the given pending stake data (PendingStake) would be consolidated to the given staker's staked-balance of $TST and $EUROs (positions[_stake.holder].TST and positions[_stake.holder].EUROs) and then the pending stake data would be deleted via the LiquidationPool#deletePendingStake() like this:
(NOTE:Each pending stake data's deadline would be defined as block.timestamp - 1 days)
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L120-L121
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L123
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L125-L127

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

Within the LiquidationPool#distributeFees(), a for-loop processing would be started based on the number of the elements in the pendingStakes (pendingStakes.length). Within the for-loop, the amount (_amount * pendingStakes[i].TST / tstTotal) of $EUROs would be added to the each pending stake data of $EUROs (pendingStakes[i].EUROs) as a fee-collected like this:
https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L190-L192

function distributeFees(uint256 _amount) external onlyManager {
uint256 tstTotal = getTstTotal();
if (tstTotal > 0) {
IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _amount);
...
for (uint256 i = 0; i < pendingStakes.length; i++) { ///<-------- @audit
pendingStakes[i].EUROs += _amount * pendingStakes[i].TST / tstTotal; ///<-------- @audit
}
}
}

As we can see above, within the LiquidationPool contract, each staker's pending stake data would be stored into the (same) pendingStakes array storage and they would be kept being held in the (same) pendingStakes array storage until elapsing the deadline of each pending stake data (_stake.createdAt < deadline).
Since each pending stake data's deadline would be defined as block.timestamp - 1 days, each pending stake data would be kept being held for at least 1 day from it would be created via the LiquidationPool#increasePosition().

If a malicious actor would repeatedly call the LiquidationPool#increasePosition() with 1 wei of $TST (_tstVal) or $EUROs (_eurosVal) in the short period (i.e. within 10-30 minutes), the malicious actor can add so many number of the pending stake's data (i.e. 10000 PendingStake) to the pendingStakes array storage at low-cost.

After that, if the LiquidationPool#consolidatePendingStakes() or the LiquidationPool#distributeFees(), which includes the for-loop processing based on the number of elements in the pendingStakes (pendingStakes.length), would be called via the following functions by any subsequent user, the TX would be reverted at least for 1 day - due to reaching a gas limit.

The LiquidationPool#consolidatePendingStakes() would be called via (inside) the following functions:

The LiquidationPool#distributeFees() would be called via (inside) the following functions:

As a result, every TXs of the LiquidationPool#increasePosition()/decreasePosition() and the LiquidationPoolManager#runLiquidation(), which is called by the subsequent users after the malicious actor did this attack, would be reverted for 1 day at least.

But, the malicious actor can frequently launch this attack again and again - because this attack can be done at low cost. So, this DOSed situation for the functions in the LiquidationPool may be longer than 1 day.

Based on that, I marked this vulnerability as a High severity.

Impact

The main functions (the LiquidationPool#increasePosition()/decreasePosition() and the LiquidationPoolManager#runLiquidation()) could be reverted (DOSed) for more than 1 day.

Tools Used

  • Manual review

Recommendations

Within the LiquidationPool#increasePosition(), consider adding a requirement of the minimum staking amount.

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.