The Standard

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

Potential DoS vulnerability in the LiquidationPool contract due to the unbounded growth of the `pendingStakes` array.

Summary

The issue at hand is a potential denial of service (DoS) vulnerability in the LiquidationPool contract due to the unbounded growth of the pendingStakes array. An attacker could exploit this by repeatedly calling the increasePosition() function to add a large number of entries to the pendingStakes array within a short period, such as 24 hours. This issue arises due to the fact that the pendingStakes array is only reduced when the stakes reach a 1-day cooling period. This vulnerability can be exploited by an attacker in combination with another vulnerability: users can stake any amount without restrictions. Consequently, an attacker can create a large number of pending stakes with negligible amounts, making the system to become brick.

The key points of this issue are:

  1. Unbounded Array: The pendingStakes array can grow indefinitely, which can lead to high gas costs as the array gets larger.

  2. Gas Costs: Functions that iterate over the pendingStakes array, like consolidatePendingStakes(), will require more gas as the number of entries increases.

  3. DoS Attack: An attacker can cause a DoS by bloating the pendingStakes array to the point where the gas required to process it exceeds the block gas limit, making it impossible to call functions that interact with the array.

  4. Contract Functionality: If the attack is successful, legitimate users would be unable to perform operations that rely on the pendingStakes array, effectively "bricking" the contract.

Vulnerability Details

File: contracts/LiquidationPool.sol
119: function consolidatePendingStakes() private {
120: @> uint256 deadline = block.timestamp - 1 days;
121: @> for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {
122: PendingStake memory _stake = pendingStakes[uint256(i)];
123: @> if (_stake.createdAt < deadline) {
124: positions[_stake.holder].holder = _stake.holder;
125: positions[_stake.holder].TST += _stake.TST;
126: positions[_stake.holder].EUROs += _stake.EUROs;
127: deletePendingStake(uint256(i));
128: // pause iterating on loop because there has been a deletion. "next" item has same index
129 i--;
130: }
131: }
132: }
133:
134: function increasePosition(uint256 _tstVal, uint256 _eurosVal) external {
135: @> require(_tstVal > 0 || _eurosVal > 0);
136: consolidatePendingStakes();
137: ILiquidationPoolManager(manager).distributeFees();
138: if (_tstVal > 0) IERC20(TST).safeTransferFrom(msg.sender, address(this), _tstVal);
139: if (_eurosVal > 0) IERC20(EUROs).safeTransferFrom(msg.sender, address(this), _eurosVal);
140: pendingStakes.push(PendingStake(msg.sender, block.timestamp, _tstVal, _eurosVal));
141: addUniqueHolder(msg.sender);
142: }

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

The increasePosition() function is utilized for participating in staking with either TST or EUROs tokens. Line number 135 permits users to stake any amount greater than zero.

135: require(_tstVal > 0 || _eurosVal > 0);

This function adds the stake to a waiting list called PendingStake, and the added stake is only confirmed and added to the confirmed position after 1 day (see line number 120 and 123).

120: uint256 deadline = block.timestamp - 1 days;
121: for (int256 i = 0; uint256(i) < pendingStakes.length; i++) {
123: if (_stake.createdAt < deadline) {

Consequently, an attacker can add a large number of stakes to the pendingStakes array with negligible amounts. Take note of line number 121; if the gas costs exceed the block gas limit due to the size of the pendingStakes array, it can potentially brick the contract.

As a result, functions utilizing the pendingStakes array, such as consolidatePendingStakes(), increasePosition(), and distributeAssets(), will be affected.

Impact

The impact of the unbounded growth of the pendingStakes array due to an attacker's actions can have several significant consequences on the LiquidationPool contract and its functionalities:

  1. Increased Gas Costs: As the pendingStakes array grows, the gas costs for executing functions that iterate over the array, such as consolidatePendingStakes(), increasePosition(), and distributeAssets(), will increase. This can make it prohibitively expensive for users to interact with the contract.

  2. Denial of Service (DoS): If the gas costs exceed the block gas limit, it will become impossible to execute functions that need to iterate over the pendingStakes array. This effectively prevents the contract from operating as intended, denying service to legitimate users.

  3. Contract Bricking: In a severe case, if the array grows too large, the contract could become bricked. This could happen if the gas required for operations consistently exceeds the block gas limit, leading to transactions that cannot be processed.

  4. Inability to Consolidate Stakes: Legitimate users would be unable to consolidate their pending stakes into confirmed positions, which could affect their ability to participate in fee distributions, claim rewards, or adjust their stakes.

  5. Inaccurate Accounting: The inability to consolidate stakes could lead to inaccurate accounting of total stakes, affecting the distribution of fees and rewards, as well as the overall financial logic of the contract.

Tools Used

Manual Review

Recommendations

To address this risk, the contract could implement measures to limit the number of pending stakes. One approach is to introduce a cap on the maximum number of pending stakes that the contract can have at any given time. This would prevent an attacker from overwhelming the contract with an excessive number of stakes.

Sample

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

Additionally, the contract could keep track of the number of pending stakes for each user using mappings. Users would be restricted from increasing their position if they reach a certain threshold of pending stakes. Furthermore, the number of pending stakes should be reduced when consolidating a user's stakes.

Implementing a combination of these methods would effectively enhance the security of the protocol.

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.