Incorrect value returned by position() function

Severity

Medium Risk

Relevant GitHub Links

https://github.com/Cyfrin/2023-12-the-standard/blob/main/contracts/LiquidationPool.sol#L88

Summary

The position() function returns incorrect value for a user's EUROs holdings when balance of LiquidationPoolManager.sol contract is greater than zero. It forgets to apply the _poolFeePercentage.

Vulnerability Details

The function is designed to let the caller know of their current EUROs position, including the yet-to-come fee still sitting in the LiquidationPoolManager contract. However, it incorrectly considers the full balance of LiquidationPoolManager instead of considering only 50% of it (the current _poolFeePercentage is 50%). This gives a wrong view to the caller.

    function position(address _holder) external view returns(Position memory _position, Reward[] memory _rewards) {
        _position = positions[_holder];
        (uint256 _pendingTST, uint256 _pendingEUROs) = holderPendingStakes(_holder);
        _position.EUROs += _pendingEUROs;
        _position.TST += _pendingTST;
@---->  if (_position.TST > 0) _position.EUROs += IERC20(EUROs).balanceOf(manager) * _position.TST / getTstTotal();
        _rewards = findRewards(_holder);
    }

PoC

Apply following patch to update test/liquidationPoolManager.js and run via npx hardhat test --grep 'incorrect position function'. The test will fail since it returns 201 instead of the correct value of 101:

diff --git a/test/liquidationPoolManager.js b/test/liquidationPoolManager_position.js
index 7e0f5c2..03ef4f1 100644
--- a/test/liquidationPoolManager.js
+++ b/test/liquidationPoolManager_position.js
@@ -426,5 +426,26 @@ describe('LiquidationPoolManager', async () => {
       expect(rewardAmountForAsset(_rewards, 'WBTC')).to.equal(wbtcCollateral);
       expect(rewardAmountForAsset(_rewards, 'USDC')).to.equal(usdcCollateral);
     });
+    
+    it('incorrect position function', async () => {
+      const tstStake1 = 1;
+      const eurosStake1 = 1;
+      await TST.mint(holder1.address, tstStake1);
+      await EUROs.mint(holder1.address, eurosStake1);
+      await TST.connect(holder1).approve(LiquidationPool.address, tstStake1);
+      await EUROs.connect(holder1).approve(LiquidationPool.address, eurosStake1);
+      let { _rewards, _position } = await LiquidationPool.position(holder1.address);
+      expect(_position.EUROs).to.equal(0);
+      await LiquidationPool.connect(holder1).increasePosition(tstStake1, eurosStake1);
+      ({ _rewards, _position } = await LiquidationPool.position(holder1.address));
+      expect(_position.EUROs).to.equal(eurosStake1);
+      
+      // "mint some fee"
+      await EUROs.mint(LiquidationPoolManager.address, 200);
+      
+      ({ _rewards, _position } = await LiquidationPool.position(holder1.address));
+      // @audit : expected = existing_EUROs + 50% of fee = 1 + 50% of 200 = 101
+      expect(_position.EUROs).to.equal(101, "Incorrect position calculation");
+    });
   });
 });

Impact

This is a view function which has so far been only used in the unit tests. However, its design seems to suggest it is supposed to help holders understand their expected holdings, including prospective fees coming their way. "Prospective" because this is still in the LiquidationPoolManager.sol contract and distributeFees() is yet to be called. As such, this misleads holders about their positions based on which they may be inclined to make further incorrect decisions about withdrawal, staking etc.

Tools Used

Hardhat

Recommendations

The LiquidationPool.sol contract needs to know the values of _poolFeePercentage and HUNDRED_PC to calculate this correctly. These can either be initialized in the constructor at deployment time, or need to be passed to the position() function:

-   function position(address _holder) external view returns(Position memory _position, Reward[] memory _rewards) {
+   function position(address _holder, uint256 _poolFeePercentage, uint256 _hundred_pc) external view returns(Position memory _position, Reward[] memory _rewards) {
        _position = positions[_holder];
        (uint256 _pendingTST, uint256 _pendingEUROs) = holderPendingStakes(_holder);
        _position.EUROs += _pendingEUROs;
        _position.TST += _pendingTST;
-       if (_position.TST > 0) _position.EUROs += IERC20(EUROs).balanceOf(manager) * _position.TST / getTstTotal();
+       if (_position.TST > 0) _position.EUROs += IERC20(EUROs).balanceOf(manager) * _position.TST * _poolFeePercentage / (_hundred_pc * getTstTotal());
        _rewards = findRewards(_holder);
    }