Griefer can deny holders of their fair share of fees

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPoolManager.sol#L35-L40

Summary

LiquidationPoolManager::distributeFees() can be called by anyone to distribute the _feesForPool to the holders. However, this distribution happens only when \_feesForPool > 0. With the current poolFeePercentage at 50% (50000) and HUNDRED_PC at 100000, a Griefer can call distributeFees() whenever eurosToken.balanceOf(address(this)) = 1, not allowing it to accumulate it to a higher value:

LiquidationPoolManager.sol#L35-L40

    function distributeFees() public {
        IERC20 eurosToken = IERC20(EUROs);
@--->   uint256 _feesForPool = eurosToken.balanceOf(address(this)) * poolFeePercentage / HUNDRED_PC;
@--->   if (_feesForPool > 0) {
            eurosToken.approve(pool, _feesForPool);
            LiquidationPool(pool).distributeFees(_feesForPool);
        }
@--->   eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
    }

This causes the eurosToken.balanceOf(address(this)) of 1 wei to be sent to the protocol, effectively resetting the counting.
Note that this attack can cause further harm if in future poolFeePercentage is set to something like 30% (30000), where the griefer can call the function as long as the eurosToken.balanceOf(address(this)) is less than 4.

Note that this issue may arise also without a griefer, under the normal flow of operations. Users calling LiquidationPool::increasePosition() and LiquidationPool::decreasePosition() functions also internally call ILiquidationPoolManager(manager).distributeFees() which could give rise to an identical issue.

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

Vulnerability Details

Under ideal conditions, a 'fair' user will let the eurosToken.balanceOf(address(this)) accumulate to a higher value and then eventually call the LiquidationPoolManager::distributeFees() function, benefitting the holders and sweeping any leftover amount into the protocol.
This is thwarted by any griefer who sees an opportunity to deny the holders of their rightful fees.

PoC

Apply the following patch inside file test/liquidationPool.js and run via npx hardhat test --grep "pays no fee due to rounding-down" to see the test pass:

diff --git a/test/liquidationPool.js b/test/liquidationPool2.js
index 8dd8580..c9e68d6 100644
--- a/test/liquidationPool.js
+++ b/test/liquidationPool2.js
@@ -146,6 +146,37 @@ describe('LiquidationPool', async () => {
       ({_position} = await LiquidationPool.position(user3.address));
       expect(_position.EUROs).to.equal(ethers.utils.parseEther('25'));
     });
+
+    it('pays no fee due to rounding-down', async () => {
+      expect(await EUROs.balanceOf(Protocol.address)).to.equal(0);
+
+      let tstStakeValue = ethers.utils.parseEther('100');
+      await TST.mint(user1.address, tstStakeValue.mul(2));
+      await TST.connect(user1).approve(LiquidationPool.address, tstStakeValue.mul(2));
+
+      await LiquidationPool.connect(user1).increasePosition(tstStakeValue, 0);
+      
+      let fees = 1;
+      await EUROs.mint(LiquidationPoolManager.address, fees);
+      
+      await LiquidationPoolManager.distributeFees();
+      
+      // @audit-info : no fee received by user1, all sweeped up by protocol
+      let { _position } = await LiquidationPool.position(user1.address);
+      expect(_position.EUROs).to.equal(0);
+      expect(await EUROs.balanceOf(Protocol.address)).to.equal(fees);
+      
+      // second round
+      await LiquidationPool.connect(user1).increasePosition(tstStakeValue, 0);
+      await EUROs.mint(LiquidationPoolManager.address, fees);
+      await LiquidationPoolManager.distributeFees();
+
+      // @audit-info : again, no fee received by user1, as the fee did not get a chance to accumulate
+      // @audit : had the fee accumulated correctly, user1 would have received 50% of 2 = 1 wei
+      ({ _position } = await LiquidationPool.position(user1.address));
+      expect(_position.EUROs).to.equal(0);
+      expect(await EUROs.balanceOf(Protocol.address)).to.equal(fees * 2);
+    });
   });
 
   describe('decrease position', async () => {

Impact

• Loss of rightful fee for the holders.

Tools Used

Manual inspection, Hardhat.

Recommendations

Do not immediately transfer the dust amount if _feesForPool is 0. Leave it there so that it can accumulate over time. Instead, add a separate access-controlled function sweepDust() which enables sweeping of any leftover dust EUROs to the protocol:

LiquidationPoolManager.sol#L33

    function distributeFees() public {
        IERC20 eurosToken = IERC20(EUROs);
        uint256 _feesForPool = eurosToken.balanceOf(address(this)) * poolFeePercentage / HUNDRED_PC;
        if (_feesForPool > 0) {
            eurosToken.approve(pool, _feesForPool);
            LiquidationPool(pool).distributeFees(_feesForPool);
+           eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
        }
-       eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
    }

+   // @audit : introduce a separate function with access control enabling recovery of any dust amount
+   function sweepDust() external onlyOwner {
+       IERC20 eurosToken = IERC20(EUROs);    
+       distributeFees();
+       eurosToken.transfer(protocol, eurosToken.balanceOf(address(this)));
+   }