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

Looping over unbounded `pendingStakes` array can lead to permanent DoS and frozen funds

cosine

Summary

The increasePosition function inside the LiquidationPool contract can be used by stakers to increase their stake. The stake is not increased instantly, but instead the request to do so is pushed into a pendingStakes array, which acts as a queue to increase the stake and waits there for at least 24 hours before the stake is increased. This is done to reduce MEV opportunities. The system loops over this array a lot, and there are no mechanisms implemented to reduce the chances of running into the block gas limit. This can lead to a DoS and frozen funds.

Vulnerability Details

There are no minimum checks implemented for the increasePosition function, therefore the pendingStakes array can be spammed with dust amounts:

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 decreasePosition function is used to reduce the stake and get the funds out of the contract. It loops up to three times over the pendingStakes array:

  1. By calling consolidatePendingStakes

  2. As consolidatePendingStakes calls deletePendingStake

  3. By calling distributeFees

It will also loop up to two times over the holders array, which is also from arbitrary length and can be spammed, but this is a known issue.

Therefore, if the pendingStakes array is very big, the block gas limit will be reached and users are not able to get their funds out of the contract.

Here we can see the decreasePosition function flow:

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]);
}
​
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--;
}
}
}
​
function deletePendingStake(uint256 _i) private {
for (uint256 i = _i; i < pendingStakes.length - 1; i++) {
pendingStakes[i] = pendingStakes[i+1];
}
pendingStakes.pop();
}
​
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 < holders.length; i++) {
address _holder = holders[i];
positions[_holder].EUROs += _amount * positions[_holder].TST / tstTotal;
}
for (uint256 i = 0; i < pendingStakes.length; i++) {
pendingStakes[i].EUROs += _amount * pendingStakes[i].TST / tstTotal;
}
}
}

The following POC can be implemented in the liquidationPool test file and showcases that the pendingStakes array can be filled up with dust amounts and therefore a loop over it can hit the block gas limit:

describe("pendingStakes_DoS", async () => {
it("pendingStakes_DoS", async () => {
const balance = ethers.utils.parseEther("5000");
​
await EUROs.mint(user1.address, balance);
await EUROs.approve(LiquidationPool.address, balance);
​
// pendingStakes array can be spammed with 1 wei of EUROs
for (let i = 0; i < 100; i++) {
let increase = LiquidationPool.increasePosition(0, 1);
await expect(increase).not.to.be.reverted;
}
});
});

As already mentioned (but known) it also loops over the holders array and therefore this griefing attack can be improved by pushing dust amounts into the pendingStakes array with a different EOA on every call.

As we can see, funds are stuck inside this array and there is only one way to reduce the size of this array, again the consolidatePendingStakes function, showcased above. This function also loops over the pendingStakes array and therefore if the size of that array grows too big it will also hit the block gas limit. This makes a permanent DoS and stuck funds possible. The griefer just needs to push into dust amounts into the pendingStakes array by calling increasePosition till it reverts because of the block gas limit, as we can see here:

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

Permanent DoS and frozen funds.

Recommendations

The best practice would be to not use arrays and instead handle it with mapping and math. But there are a few things that can be done to reduce the chances of running into the block gas limit, with the current design:

  • Add a minimum check for the increasePosition function

  • Make the consolidatePendingStakes function externally callable as currently it is private and only callable by calling other functions which waste further gas

  • allow calling specific pendingStakes indices

  • Try to decrease the amount of loops over the pendingStakes array

Updates

Lead Judging Commences

hrishibhat Lead Judge over 1 year ago
Submission Judgement Published
Invalidated
Reason: Non-acceptable severity
Assigned finding tags:

pendingstake-dos

informational/invalid

pontifex Auditor
over 1 year ago
hrishibhat Lead Judge
over 1 year ago
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.