Polynomial Protocol contest - bytes032's results

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L367-L374 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L141 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/test/utils/TestSystem.sol#L202

Vulnerability details

Impact

This issue can lead to traders losing all their collateral due to excessive fees when opening and closing larger trades.

Proof of Concept

When the exchange opens/closes trades, it defines the order fees to be paid by the user using the orderFee function. The function takes into account both the hedging fee, which covers the cost of managing the risk associated with holding the position, and the trading fee, which accounts for the costs incurred when executing the trade.

The function works as follows:

1. It first calculates the delta, a factor representing the rate at which the position size changes.
2. Then, it computes the futuresSizeDelta by multiplying the input sizeDelta with the calculated delta.
3. It calls the perpMarket.orderFee function to get the hedgingFee and a boolean flag, isInvalid, which indicates if the trade is invalid.
4. It checks if the trade is valid by ensuring isInvalid is not set to true. If the trade is invalid, it will throw an exception.
5. It retrieves the markPrice from the exchange.getMarkPrice function, which represents the current market price of the asset.
6. The function calculates the valueExchanged by multiplying the markPrice with the absolute value of sizeDelta. This represents the total value of the trade.
7. It computes the tradeFee using the getSlippageFee function, which calculates the fee based on the size of the trade.
8. The tradingFee is calculated by multiplying the tradeFee with valueExchanged.
9. Finally, the function returns the sum of the hedgingFee and tradingFee as the total fee for the trade.

By combining both the hedging fee and trading fee, the orderFee function comprehensively calculates the costs associated for the trade.

The problem here lies in the getSlippageFee function. The purpose of the getSlippageFee function is to calculate the slippage fee for a trade based on the change in position size, represented by the input sizeDelta. The slippage fee is a cost associated with executing a trade that accounts for the potential price impact and liquidity changes due to the size of the order. It helps ensure that the trading platform can effectively manage the impact of large trades on the market.

    function getSlippageFee(int256 sizeDelta) public view returns (uint256) {
        // ceil(s/100) * baseFee

        uint256 size = sizeDelta.abs();
        uint256 region = size / standardSize;

        if (size % standardSize != 0) region += 1;
        return region * baseTradingFee;
    }

The function works as follows:

1. It first calculates the absolute value of sizeDelta to ensure the size is a positive value, regardless of whether the trade is a buy or sell.
2. It then divides the size by standardSize (set by default to 1e20), to determine the number of regions the trade occupies. This constant value represents the size of a "standard" trade, which is used to categorize trades into different regions based on their size.
3. If there is a remainder after dividing size by the constant value, it increments the region by 1. This ensures that even partial regions are taken into account when calculating the fee.
4. Finally, the function calculates the slippage fee by multiplying the region with baseTradingFee (currently set to 6e15 in the test suite). This constant value represents the base fee for each region.

By calculating the slippage fee based on the size of the trade, the getSlippageFee function helps account for the potential impact of the trade on the market. The larger the trade, the more regions it occupies, resulting in a higher slippage fee. This approach incentivizes traders to be mindful of the size of their trades and the potential impact they may have on market liquidity and pricing.

The issue is that region can get really big (depending on the trade) so that the openTrade/closeTrades orderFee could >= 100% meaning all the collateral of the user will be used to pay taxes.

While I completely understand the intent of higher taxes for bigger trades, I think there should be a limit where the trade won't get opened if a certain threshold is passed.

I've built a PoC with Foundry using the protocol test suite with a few comments here and there to represent the following cases:

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/test/Exchange.Simple.t.sol#L100-L145

    function depositToPool(address user, uint256 sum) internal {
        susd.mint(user, sum);
        vm.startPrank(user);
        susd.approve(address(pool), sum);
        pool.deposit(sum, user);
        vm.stopPrank();
    }

    function openShort(uint256 amount, uint256 collateral, address user)
        internal
        returns (uint256 positionId, Exchange.TradeParams memory tradeParams)
    {
        tradeParams.amount = amount;
        tradeParams.collateral = address(susd);
        tradeParams.collateralAmount = collateral;
        tradeParams.minCost = 0;

        vm.startPrank(user);
        susd.approve(address(exchange), collateral);
        (positionId,) = exchange.openTrade(tradeParams);
        vm.stopPrank();
    }

     function testSimpleShortCloseTrade() public {
        depositToPool(user_2, 1000e18 * 25000);
        uint256 pricingConstant = exchange.PRICING_CONSTANT();
        uint256 expectedPrice = initialPrice.mulDivDown(initialPrice, pricingConstant);
        uint256 multiplier = 165;
        uint256 collateralAmount = (expectedPrice * 2) * multiplier; // 200% Collateral ratio
        uint256 shortAmount = multiplier * 1e18;
        susd.mint(user_1, collateralAmount);
        console.log("CollateralAmount", collateralAmount / 1e18);
        console.log("ShortAmount", shortAmount / 1e18);
        console.log("User_1 sUSD balance", susd.balanceOf(user_1) / 1e18);


        console.log("*** OPEN TAX ***");
        (uint256 positionId,) = openShort(shortAmount, collateralAmount, user_1);
        console.log("*** OPEN TAX ***\n");

        console.log("*** CLOSE TAX ***");
        closeShort(positionId, shortAmount, type(uint256).max, collateralAmount, user_1);
        console.log("*** CLOSE TAX ***");

        console.log("User_1 sUSD balance", susd.balanceOf(user_1) / 1e18);
    }

I'm going to use the formula (orderFee * 2) / collateralAmount to calculate the tax percentage. For each case, I'll just modify the multiplier variable.

Case 1: (multiplier 165) CollateralAmount 237600 ShortAmount 165 The tax % is: 1,7996633

Running the tests, yields the following results.

Case 2: (multiplier 1650) CollateralAmount 2376000 ShortAmount 1650 The tax % is: 10,8

Running the tests yields the following results.

Case 3: (multiplier 16500) CollateralAmount 23760000 ShortAmount 16500 The tax % is: 99,6

Case 3 proves the point that a user can potentially lose all his collateral just by paying taxes when opening/closing a trade.

Tools Used

Manual review, Foundry

Recommended Mitigation Steps

This invariant is heavily influenced by the calculation right here. In case 1 the region is 1, in case 2 is 16 and in case 3 is 165.

To address this issue, a protocol-wide maximum amount of taxes to be paid in a trade (total of open + close) should be established. If the calculated fee exceeds this threshold, the transaction should be reverted instead of opening the trade for the user. This will help protect traders from losing all their collateral due to excessive trading fees.

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/KangarooVault.sol#L185-L334 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L181-L335

Vulnerability details

Impact

Direct loss of money for users who have deposited funds and wish to withdraw them, as they would be required to pay extremely high gas fees due to the queuing mechanism exploited by the attacker.

Proof of Concept

The affected contracts, KangarooVault and LiquidityPool, provide the functionality for instant deposits and withdrawals of tokens. However, in certain scenarios, deposits and withdrawals can be queued.

In both cases, the deposit transfers the sUSD from your address immediately. In the instant deposit scenario,mints you Vault or Liquidity token - depending on the contract. On the other hand, queuing it adds you to the deposit queue, by creating a "QueuedDeposit" object and sets its id to the current value of nextQueuedDepositId (mentioning that, because its going to be important later)

        QueuedDeposit storage newDeposit = depositQueue[nextQueuedDepositId];
        newDeposit.id = nextQueuedDepositId++;
        newDeposit.user = user;
        newDeposit.depositedAmount = amount;
        newDeposit.requestedTime = block.timestamp;
        totalQueuedDeposits += amount;

In KangarooVault, the deposits are queued ONLY If there are currently active positions.

    function initiateDeposit(address user, uint256 amount) external nonReentrant {
        require(user != address(0x0));
        require(amount >= minDepositAmount);

        // Instant processing
        if (positionData.positionId == 0) {
            uint256 tokenPrice = getTokenPrice();
            uint256 tokensToMint = amount.divWadDown(tokenPrice);
            VAULT_TOKEN.mint(user, tokensToMint);
            totalFunds += amount;
            emit ProcessDeposit(0, user, amount, tokensToMint, block.timestamp);
        } else {
            // Queueing the deposit request
            QueuedDeposit storage newDeposit = depositQueue[nextQueuedDepositId];

            newDeposit.id = nextQueuedDepositId++;
            newDeposit.user = user;
            newDeposit.depositedAmount = amount;
            newDeposit.requestedTime = block.timestamp;

            totalQueuedDeposits += amount;
            emit InitiateDeposit(newDeposit.id, msg.sender, user, amount);
        }

        // SUSD checks for zero address
        SUSD.safeTransferFrom(msg.sender, address(this), amount);
    }

On the other hand, in LiquidityPool instant deposits have a fee whereas the regular deposits don't have a fee. After discussing with the protocol team, queue deposits its primarily for the regular traders, because the price won't fluctate that much and instant is mostly for other protocols to build on top.

    function queueDeposit(uint256 amount, address user)
        external
        override
        nonReentrant
        whenNotPaused("POOL_QUEUE_DEPOSIT")
    {
        QueuedDeposit storage newDeposit = depositQueue[nextQueuedDepositId];
        newDeposit.id = nextQueuedDepositId++;
        newDeposit.user = user;
        newDeposit.depositedAmount = amount;
        newDeposit.requestedTime = block.timestamp;
        totalQueuedDeposits += amount;
        SUSD.safeTransferFrom(msg.sender, address(this), amount);

        emit InitiateDeposit(newDeposit.id, msg.sender, user, amount);
    }

Finally, the deposits are processed in a nearly identical way (LP, Kangaroo). Below, I've only extracted the vulnerable code which contains a for loop that iterates through a specified number of deposits, denoted by the variable count. The main purpose of this loop is to process each deposit in the queue and update the overall state of the contract.

At the beginning of each iteration, the function accesses the deposit at the current queuedDepositHead position in the depositQueue. The deposit's requestedTime is then evaluated to ensure that it is not equal to 0 and that the current block timestamp exceeds the deposit's requestedTime plus the minDepositDelay. If either of these conditions is true, the function terminates early, halting further deposit processing.

Upon passing the aforementioned check, it mints the tokens for the user and updates the contract accounting balance. Finally, the queuedDepositHead is incremented, advancing to the next deposit in the queue.

We can conclude that:

• queuedDepositHead = the next in line deposit to be executed
• nextQueuedDepositId = currently, the last deposit id in the queue

As a result, this means all the deposits are executed in sequential order and if there are currently 10 deposits and yours is the 11th, if you want to process your own, you have to process the 10 before that or wait for somebody else to process them.

      for (uint256 i = 0; i < count; i++) {
            QueuedDeposit storage current = depositQueue[queuedDepositHead];

            if (current.requestedTime == 0 || block.timestamp < current.requestedTime + minDepositDelay) {
                return;
            }

            uint256 tokensToMint = current.depositedAmount.divWadDown(tokenPrice);

            current.mintedTokens = tokensToMint;
            totalQueuedDeposits -= current.depositedAmount;
            totalFunds += current.depositedAmount;
            liquidityToken.mint(current.user, tokensToMint);

            emit ProcessDeposit(current.id, current.user, current.depositedAmount, tokensToMint, current.requestedTime);

            current.depositedAmount = 0;
            queuedDepositHead++;
        }

The queuing mechanism can be exploited by a malicious actor, who can queue a large number of deposits or withdrawals for a very low cost. This essentially locks all the deposited funds in the contract and forces the users to pay extremely high gas fees to process their transactions. This vulnerability can be exploited in a similar way for both contracts.

This can be mitigated to extent by the minDepositAmount variable, but that will just make the attack vector a bit more expensive for the attacker and the vulnerability would still be there.

To apply that to real world, assume the following scenario:

1. Alice queues 10000 deposits for 1 wei
2. Bob queues 1 deposit for 1e18

Since there's no way to force Alice to process her deposits, Bob can either wait for somebody else to process them or process them himself. However, whoever does that will have to pay enormous amount of gas fees.

Here's a PoC using Foundry, making use of LiquidityPoolTest test suite. However, it would work pretty much in the same way for KangarooVault, where the only prerequisite would be that there have to be currently active positions.

  function testDepositFee() public {
        address alice = makeAddr("alice");
        address bob = makeAddr("bob");
        susd.mint(alice, 1e18);
        susd.mint(bob, 1e18);
        
        vm.startPrank(alice);
        susd.approve(address(pool), 1e18);

        for(uint256 i = 0; i < 100000; i++) {
            pool.queueDeposit(1 wei, alice);
        }
        vm.stopPrank();

        vm.startPrank(bob);
        susd.approve(address(pool), 1e18);
        pool.queueDeposit(1e18, address(bob));

        vm.warp(block.timestamp + pool.minDepositDelay());

        pool.processDeposits(100001);

        vm.stopPrank();
  }

Running the test using the following command forge t --match-test testDepositFee -vv --gas-report yiels the following result.

The same scenario can happen when processing withdrawals. If we expand the example to extreme values (100,000,000 deposits), this would mean that the approximate gas to be paid for users that want to withdraw their funds equal approximately 2448749420000, which converted to today's ETH:USD ratio is around 4070335.77 USD.

Tools Used

Manual review, foundry.

Recommended Mitigation Steps

My recommendations for this vulnerability are the following:

1. Replace the sequential processing of deposits and withdrawals with functionality where users can execute their own deposits and withdrawals without having to process all the deposits and withdrawals before theirs.
2. If you insist keeping the sequential processing, add a mechanism to cancel deposits but still implement the second part of point 1.

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortCollateral.sol#L253-L270 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortCollateral.sol#L200-L203 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortCollateral.sol#L224-L226 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortCollateral.sol#L184-L186

Vulnerability details

Impact

The impact of this vulnerability is that once a collateral type is approved, it cannot be removed from the ShortCollateral contract, even if it is no longer needed or supported. This could potentially cause issues and lead to a need for redeploying the contract, which can be costly and time-consuming.

Proof of Concept

On a high level, the ShortCollateral contract is designed to store and manage collateral information for each user's short position. It facilitates the liquidation of collateral when required and awards a liquidation bonus to the liquidator. The contract also allows for the approval of new collateral types, each with specific collateral ratios, liquidation ratios, and liquidation bonuses. Additionally, it calculates the minimum required collateral for a short position and determines if a specific position can be liquidated and the maximum liquidatable debt of a position.

After discussing with mubaris, he noted that for the moment the pool won't have multiple collateral types, but additional collateral types can be added later by using approveCollateral

    function approveCollateral(bytes32 key, uint256 collateralRatio, uint256 liqRatio, uint256 liqBonus)
        external
        requiresAuth
    {
        Collateral storage collateral = collaterals[key];
        address synth = synthetixAdapter.getSynth(key);
        require(synth != address(0x0));
        require(liqRatio < collateralRatio);

        collateral.currencyKey = key;
        collateral.synth = synth;
        collateral.isApproved = true;
        collateral.collateralRatio = collateralRatio;
        collateral.liqRatio = liqRatio;
        collateral.liqBonus = liqBonus;

        emit ApproveCollateral(key, collateralRatio, liqRatio, liqBonus);
    }

The vulnerability here is that once a collateral is approved, there's no way to reverse that and the only way to remove the now unwanted type of collateral is redeploying the pool, which can be troublesome and costly.

Consider the following scenario:

1. Protocol adds and approves collateral sCLTR
2. One month later the team decides they want to drop support for sCLTR, but that's not possible, because there's no way isApproved can be set to false.

Given the check is used in various places,

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortCollateral.sol#L200-L203

        bytes32 collateralKey = synthetixAdapter.getCurrencyKey(position.collateral);
        Collateral memory collateral = collaterals[collateralKey];
        require(collateral.isApproved);

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortCollateral.sol#L224-L226

        bytes32 collateralKey = synthetixAdapter.getCurrencyKey(position.collateral);
        Collateral memory collateral = collaterals[collateralKey];
        require(collateral.isApproved);

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortCollateral.sol#L161-L162

        Collateral memory coll = collaterals[currencyKey];
        require(coll.isApproved);

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortCollateral.sol#L184-L186

        Collateral memory coll = collaterals[currencyKey];
        require(coll.isApproved);

I kinda feel that's a low/medium, but im rating it as Medium because I'm not certain of the amount of trouble that will be introduced by having to redeploy the ShortCollateral contract. Still, ultimately it's up to the protocol team and the judge to decide.

Tools Used

Manual review

Recommended Mitigation Steps

Add the possibility to unapprove collaterals;