Summary

Because pending stakes positions are allocated a proportion of distributed fees; user can add a large pending stake to earn a high amount of fees. Pending Stakes should need to wait a minimum periuod of time before they can earn fees; otherwise opportunists will front-run fee paying functions simply to earn fees, causing instability and volatility to the protocol.

Vulnerability Details

In LiquidationPool::distributeFees() fees are distributed to both holders and pendingStakes positions. Pending Stakes become holders after 24 hours but while they are still pending stakes they are entitled to any fees distributed during runLiquidation() or when positions are increased / decreased in LiquidationPool.

This creates an incentive for malicious users to front-run pending transactions in the mempool for LiquidationPoolManager::runLiquidation(), LiquidationPool::increasePosition() & LiquidationPool::decreasePosition().
It would take just a few 100 holders on top of the users already in the protocol for these functions to begin running out of gas or to become prohibitively expensive to be used.

POC

To test, we need to update the code in 2 places:

1. In hardhat.config.js update the code to config to increase the timeout:

   Config

   module.exports = {

   solidity: "0.8.17",

   gasReporter: {

   enabled: true,

   // currency: "USD",

   noColors: true,

   showTimeSpent: false,

   outputFile: "gas-report.txt",

   // coinmarketcap: process.env.COINMARKETCAP_API_KEY,

   },

   + mocha: {

   + timeout: 1000000,

   + },

   + networks: {

   + hardhat: {

   + accounts: {

   + count: 800,

   + },

   + },

   + },

   };

2. Add this test to liquidationPoolManager.js where we assume 600 holders and 200 and run:

   Users stake to get fees OOG

   it.only("users stake to get fees OOG", async () => {

   const ethCollateral = ethers.utils.parseEther("10");

   await holder1.sendTransaction({

   to: SmartVaultManager.address,

   value: ethCollateral,

   });

   ​

   // malicious user mints EUROs and increases small positions in a loop

   const eurosStake = 100000;

   const approveAmount = 100000;

   ​

   const signers = await ethers.getSigners();

   ​

   const holders = [];

   ​

   for (let i = 0; i < 800; i++) {

   holders.push(signers[i]);

   }

   ​

   // Assuming holders array is already populated with 800 holders

   for (let i = 0; i < 800; i++) {

   holders.push(signers[i]);

   }

   ​

   // First holders create position

   for (let i = 0; i < 550; i++) {

   await EUROs.mint(holders[i].address, eurosStake);

   await EUROs.connect(holders[i]).approve(

   LiquidationPool.address,

   approveAmount

   );

   await LiquidationPool.connect(holders[i]).increasePosition(

   0,

   eurosStake

   );

   }

   ​

   // Wait for a day for pending stakes to become holders

   await fastForward(DAY);

   ​

   // Next holders increase position

   for (let i = 550; i < 800; i++) {

   await EUROs.mint(holders[i].address, eurosStake);

   await EUROs.connect(holders[i]).approve(

   LiquidationPool.address,

   approveAmount

   );

   await LiquidationPool.connect(holders[i]).increasePosition(

   0,

   eurosStake

   );

   }

   ​

   // runLiquidation runs out of gas

   await LiquidationPoolManager.runLiquidation(TOKEN_ID);

   });

   ​

Impact

Malicious users simply enter the system to gain fees and likely leave again. This will cause large swings in the amount of staked TST and EUROs in the protocol making other functionality behave inconsistently such as the rewards which only holders are entitled to; their returns will be very volatile based on inflows and outflows of capital.

Another more serious issue is where many malicious users enter the system to take fees; their pending stakes can be added to their position after 24 hours. This is likely to significantly increase the size of the holders storage array. In multiple places throughout the protocol this array is looped through so the larger it gets the higher the likelihood of OOG issues and DOSing of certain protocol functionalities.
Even if functions don't run out of gas, the gas cost of carrying them out can become prohibitively expensive to be used.

Given their looping of the holders array; functionality which would be seriously affected include runliquidation(), increasePosition(), decreasePosition(), distributeFees(), distributeAssets().

Tools Used

Manual Review
Hardhat Testing

Recommendations

Remove the ability of pendingStakes() to earn fees completely or add a few hours delay so they can't front-run:

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