pendingStakes
array can lead to permanent DoS and frozen fundsHigh Risk
https://github.com/Cyfrin/2023-12-the-standard/blob/91132936cb09ef9bf82f38ab1106346e2ad60f91/contracts/LiquidationPool.sol#L140
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.
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:
consolidatePendingStakes
deletePendingStake
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--;
}
}
}
Permanent DoS and frozen funds.
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:
increasePosition
functionconsolidatePendingStakes
function externally callable as currently it is private and only callable by calling other functions which waste further gaspendingStakes
array