Liquidity providers can collude and increase their fees received by the borrowers

[code423n4]

Lines of code

https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/core/Lendgine.sol#L71-L102 https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/core/Lendgine.sol#L152-L180 https://github.com/code-423n4/2023-01-numoen/blob/2ad9a73d793ea23a25a381faadc86ae0c8cb5913/src/core/Lendgine.sol#L238-L261

Vulnerability details

Impact

How it works

Liquidity providers are supposed to provide the protocol with some funds and receive fees based on the percentage of the fund borrowed and its size. There is a jumprate of kink=0.8 hard coded into the project which means when the percentage of the borrowed liquidity increases over 80%, the fees suddenly go up by each increasing percentage until 100%.

The attack

By understanding the fee method of the protocol, we see that the protocol expects the liquidity providers to leave their funds in the pool and receive its fees. However, liquidity providers can possibly collude and increase the percentage of totalLiquidityBorrowed/totalLiquidity in two ways:

• Pulling out the free funds from the totalLiquidity
• Borrowing some money themselves to increase the amount of totalLiquidityBorrowed

This attack would become more dangerous if the following conditions are true:

• Liquidity providing market for a certain upperbound and pair of tokens is highly decentralized
• Already a high percentage of the liquidity is borrowed

When does this attack does not work?

When there is a decent percentage of liquidity providers that are acting honest and not joining in the collude. If the 100 percent of the liquidity providers agree to join together, there is absolutely no down-side to the Liquidity providers at least in short-term. In the case of the collude, if they decide to cut down the liquidity each will take the same percentage off, And if they decide to borrow the remaining money and pay themselves, they will be doing this in proportion of their fees. Even if an honest/unknown liquidity provider joins the pool, all of the dishonest ones can easily revert back to the previous state and continue working like honest nodes.

Therefore, protocol should somehow de-incentivize this behaviour where it can highly profit the dishonest liquidity providers and can cause the borrowers to lose money much faster.

Proof of Concept

In the PoC below, you can see the victim is losing more fees on the same collateral when the liquidity providers collude.

// SPDX-License-Identifier: GPL-3.0-only
pragma solidity ^0.8.0;

import { Lendgine } from "../src/core/Lendgine.sol";
import { Pair } from "../src/core/Pair.sol";
import { TestHelper } from "./utils/TestHelper.sol";
import { ERC20 } from "../src/core/ERC20.sol";
import "../src/core/Lendgine.sol";

import "forge-std/console.sol";

contract PoC is TestHelper{
    address public liquidityProvider0;
    address public liquidityProvider1;
    address public victim;

    function setUp() public {
        _setUp();

        liquidityProvider0 = mkaddr("evilDoer0");
        liquidityProvider1 = mkaddr("evilDoer1");
        victim = mkaddr("poorVictim");
    }

    /**
     * A scenario where the attacker liquidity providers collude and increase
     * the fees that the victim needs to pay, therefore, his collateral will 
     * dry up faster.
     * Note: In the follownig contract, the test contract is running all the minting
     * and borrowig for the actors, the actors will do it for themselves in the real
     * case but the outcome would be the same.
     */
    function testColludeLiquidity() public {

        uint256 time = 0;
        uint256 timeInteraval = 1 days;

        time += timeInteraval;
        vm.warp(time); //sets timestamp

        _deposit( // Helper function provided by the project itself
            liquidityProvider0, // to
            liquidityProvider0, // from
            1 ether, // amount0
            8 ether, // amount1
            1 ether // liquidity
            );

        _deposit( // Helper function provided by the project itself
            liquidityProvider1, // to
            liquidityProvider1, // from
            1 ether, // amount0
            8 ether, // amount1
            1 ether // liquidity
            );

        assertEq(lendgine.totalLiquidity(), 2 ether);

        token1.mint(address(this), 20 ether); // Enough amount to borrow all of the amount

        uint256 collateral = 1 ether;

        uint256 victimShare = lendgine.mint(
            victim,
            collateral,
            ""
        );

        assertEq(convertShareToCollateral(victimShare), collateral);

        time += timeInteraval;
        vm.warp(time);

        lendgine.accrueInterest();
        uint256 afterFirstCollateral = collateral - convertShareToCollateral(victimShare);
        // console.log(afterFirstCollateral); 188356164383560

        lendgine.mint( // They can either borrow tokens from their own pools, or they can just leave with the liquidity
            liquidityProvider0,
            0.45 ether,
            ""
        );

        lendgine.mint(
            liquidityProvider0,
            0.45 ether,
            ""
        );

        time += timeInteraval;
        vm.warp(time);

        lendgine.accrueInterest();
        uint256 afterSecondCollateral = collateral - convertShareToCollateral(victimShare);
        // console.log(afterSecondCollateral); 546133366542390

        // console.log((afterSecondCollateral - afterFirstCollateral) * 1e18 /afterFirstCollateral); 1899471691461441304

        assertGe((afterSecondCollateral - afterFirstCollateral) * 1e18 /afterFirstCollateral, 1.8e18); // fees for the victim for the second round almost doubled!
    }

    function convertShareToCollateral(uint256 share) public view returns(uint256 collateral){
        collateral = lendgine.convertLiquidityToCollateral(
            lendgine.convertShareToLiquidity(
                share
            )
        );
    }

    function mintCallback( // Need to change the mintCallBack in the testHelper to virtual
        uint256 _collateral,
        uint256,
        uint256,
        uint256,
        bytes calldata
    )
    external
    override {
        token1.transfer(msg.sender, _collateral);
    }
}

Tools Used

Foundry

Recommended Mitigation Steps

There should be some modifications to the Jumprate contract, where it does not escalate the fees for the previously taken positions. It should be possible for the earlier loans to have some sort of advantage over the newly taken positions.

[kyscott18]

I don't think this is exactly feasible but would like some more thought about it. My idea when designing this is that if the rate is above market rate, either the borrowers could burn their power tokens, which they can do at any time, or new lenders would see the mispricing and deposit liquidity, moving the price down.

For me this test doesn't account for the fact that someone new might do one of these two actions in the time period when interest is accruing.

I analogize this to UniswapV3, why haven't we seen liquidity providers collude to only provide liquidity to the 1% fee contract?

[c4-sponsor]

kyscott18 requested judge review

[berndartmueller]

I agree with the sponsor. This finding relies on the fact that the majority of LPs have to collude and act in a similar, dishonest way. This does not warrant the High severity chosen by the warden and is overly inflated. I consider QA (Low) to be the appropriate severity given the improbability and heavy reliance on external factors.

[c4-judge]

berndartmueller changed the severity to QA (Quality Assurance)

[c4-judge]

berndartmueller marked the issue as grade-c