Polynomial Protocol contest - chaduke's results

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/Exchange.sol#L100-L109 https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortToken.sol#L83

Vulnerability details

Impact

Detailed description of the impact of this finding. A user might lose his collateral after calling closeTrade() to adjust a short position. This happens after a position positionId is adjusted, if shortPositions[positionId].shortAmount == 0, the shortToken for the position will be burned, even in the case of shortPositions[positionId].collateralAmount != 0. As a result, the user cannot retrieve all the collateral in the amount of shortPositions[positionId].collateralAmount.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

I illustrate the scenario in which Bob can lose his collateral, and then present the code POC to confirm my finding.

1. Bob calls openTrade() with 2000e18 collateral (SUSD) for a short position of 1e18.

2. Bob then calls closeTrade() to close the short position of 1e18 but claiming ZERO collateral. As a result, shortToken.adjustPosition() will be called to adjust the position, since position.shortAmount == 0 the short position token will be burned at L83, even though position.collateralAmount = 2000e18.

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/ShortToken.sol#L48-L90

3. Since now positionId is burned, calling closeTrade() and exchange.removeCollateral() will both fail. Therefore the user will not be able to retrieve his collateral in the amount of position.collateralAmount = 2000e18 (loss of funds).

The following POC confirms my finding. The code can be inserted into Exchange.Simple.t.sol for the testing.

 function testLosingCollateralAfterCloseShort() public {
        // open a short position
        Exchange.TradeParams memory tradeParams; 
        uint256 collateralAmount = 2000e18;
        susd.mint(user_1, 2*collateralAmount);
        tradeParams.positionId = 0; 
        tradeParams.isLong = false;
        tradeParams.amount = 1e18;
        tradeParams.collateral = address(susd);
        tradeParams.collateralAmount = collateralAmount;
        tradeParams.minCost = 0;

        vm.startPrank(user_1);
        susd.approve(address(exchange), collateralAmount);
        (uint256 positionId, ) = exchange.openTrade(tradeParams);
        vm.stopPrank();

        (, uint256 _shortAmount, uint256 _collateralAmount,) = shortToken.shortPositions(positionId);
        assertEq(_collateralAmount, collateralAmount);
        assertEq(_shortAmount, tradeParams.amount);

        // close the short position, but not taking out all collateral
        tradeParams.positionId = positionId; 
        tradeParams.isLong = false;
        tradeParams.amount = 1e18;
        tradeParams.collateral = address(susd);
        tradeParams.collateralAmount = 0;           // no collateral is retrieved
        tradeParams.maxCost = 2000e18;

        vm.startPrank(user_1);
        susd.approve(address(getPool()), tradeParams.maxCost);
        exchange.closeTrade(tradeParams);
        vm.stopPrank();

        (, _shortAmount, _collateralAmount,) = shortToken.shortPositions(positionId);
        assertEq(_collateralAmount, collateralAmount);
        assertEq(_shortAmount, 0);

        // try to retrieve the collateral with another trade but will fail since positionId has been burned
         tradeParams.collateralAmount = collateralAmount; 
        vm.startPrank(user_1);
        susd.approve(address(getPool()), tradeParams.maxCost);
        vm.expectRevert();
        exchange.closeTrade(tradeParams);
        vm.stopPrank();

        // Calling exchange.removeCollateral() fails as well, so the collateral is lost!
        vm.startPrank(user_1);
        vm.expectRevert();
        exchange.removeCollateral(positionId, collateralAmount);
        vm.stopPrank();
     }

Tools Used

VScode, foundry

Recommended Mitigation Steps

We make sure to burn a shortToken only when shortPositions[positionId].shortAmount == 0 and shortPositions[positionId].collateralAmount == 0.

 function adjustPosition(
        uint256 positionId,
        address trader,
        address collateral,
        uint256 shortAmount,
        uint256 collateralAmount
    ) external onlyExchange returns (uint256) {
        if (positionId == 0) {
            positionId = nextId++;

            ShortPosition storage position = shortPositions[positionId];

            position.positionId = positionId;
            position.collateral = collateral;
            position.collateralAmount = collateralAmount;
            position.shortAmount = shortAmount;

            totalShorts += shortAmount;

            _mint(trader, positionId);
        } else {
            require(trader == ownerOf(positionId));

            ShortPosition storage position = shortPositions[positionId];

            if (shortAmount >= position.shortAmount) {
                totalShorts += shortAmount - position.shortAmount;
            } else {
                totalShorts -= position.shortAmount - shortAmount;
            }

            position.collateralAmount = collateralAmount;
            position.shortAmount = shortAmount;

-            if (position.shortAmount == 0) {
+            if (position.shortAmount == 0 && position.collateralAmount == 0) {
                _burn(positionId);
            }
        }

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/KangarooVault.sol#L436-L447

Vulnerability details

Impact

Detailed description of the impact of this finding. KangarooVault.removeCollateral() fails to call EXCHANGE.addCollateral(), as result, the collateral is not removed from the position.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The function KangarooVault.removeCollateral() allows the contract owner or an athorized user to remove collateral from Power Perp position.

 function removeCollateral(uint256 collateralToRemove) external requiresAuth nonReentrant {
        (uint256 markPrice,) = LIQUIDITY_POOL.getMarkPrice();
        uint256 minColl = positionData.shortAmount.mulWadDown(markPrice);
        minColl = minColl.mulWadDown(collRatio);

        require(positionData.totalCollateral >= minColl + collateralToRemove);

        usedFunds -= collateralToRemove;
        positionData.totalCollateral -= collateralToRemove;

        emit RemoveCollateral(positionData.positionId, collateralToRemove);
    }

However, the function does not call EXCHANGE.removeCollateral() to do the real job. As a result, although some accounting has been done in this contract, the collateral will not be removed from the shortCollateral contract. So, the user of the position will not receive any collateral either (lose of funds).

We need to add the line EXCHANGE.removeCollateral() so that collateral will be indeed removed from the position.

Tools Used

VScode

Recommended Mitigation Steps

We can fix it as follows:

 function removeCollateral(uint256 collateralToRemove) external requiresAuth nonReentrant {
        (uint256 markPrice,) = LIQUIDITY_POOL.getMarkPrice();
        uint256 minColl = positionData.shortAmount.mulWadDown(markPrice);
        minColl = minColl.mulWadDown(collRatio);

        require(positionData.totalCollateral >= minColl + collateralToRemove);
+       EXCHANGE.removeCollateral(positionData.positionId, collateralToRemove);

        usedFunds -= collateralToRemove;
        positionData.totalCollateral -= collateralToRemove;

        emit RemoveCollateral(positionData.positionId, collateralToRemove);
    }

Lines of code

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

Vulnerability details

Impact

Detailed description of the impact of this finding. A liquidator might lose some funds due to calling liquidation. This is because the liquidator might pay more debt than the collateral value that he can claim from the shortCollateral contract.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

A liquidator might call exchange.liquidate()->exchange._liquidate()->shortCollateral.liquidate() to claim collateral at the cost of paying the debt.

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

However at the following line, we see that the liquidator can claim at most all the collateral of the debtor:

 if (totalCollateralReturned > userCollateral.amount) totalCollateralReturned = userCollateral.amount;
        userCollateral.amount -= totalCollateralReturned;

It is possible that totalCollateralReturned < collateralClaim where collateralClaim is the amount of collateral a liquidator can purchase from the market if he uses the same amount of debt value.

In other words, if totalCollateralReturned < collateralClaim, the liquidator is actually losing funds after liquidation since he paid more than what he got from the collateral. In this case, the liquidator should pay less debt - the input debt amount should be adjusted, but the current implementation still needs the liquidator to pay the debt/debtRepaying amount specified from the input.

The code POC below confirms my finding that user_2 lost 732.24e18 susd as a result of liquidating 1e18 short position of user_1.

1. user_1 opens a short position of 1e18 with collateral amount 907.2e18 (126% collateral ratio).

2. Initial price increases by 150% and time forwards by 1000;

3. Now user_1's position becomes fully liquitable;

4. user_2 opens a long position of 1e18 by spending 1639.44e18 susd in order to liquidate for user_1;

5. user_2 liquidates user_1 for the maxDebt of 1e18, fully liquidating it.

6. However, user_2 can at most claim all its collateral 732.24e18. The final susd balance of user_2 is 2267.76e18. As a result of liquidation, user_2 lost 732.24e18!

The following POC can be inserted into exchange.simple.t.sol for the testing.

function testLosingFundsAfterLiquidate() public {
        uint256 currentTime = block.timestamp;
        uint256 amount = 1e18;
        uint256 pricingConstant = exchange.PRICING_CONSTANT();
        uint256 expectedPrice = initialPrice.mulDivDown(initialPrice, pricingConstant);
        uint256 collateralAmount = expectedPrice.mulWadDown(amount) * 126 / 100; // 126% Collateral ratio
        assertEq(collateralAmount, 907.2e18);
        susd.mint(user_1, 3000e18);

        // open a short position
        Exchange.TradeParams memory tradeParams;         
        tradeParams.positionId = 0; 
        tradeParams.isLong = false;
        tradeParams.amount = amount;
        tradeParams.collateral = address(susd);
        tradeParams.collateralAmount = collateralAmount;
        tradeParams.minCost = 0;

        vm.startPrank(user_1);
        susd.approve(address(exchange), collateralAmount);
        (uint256 positionId, ) = exchange.openTrade(tradeParams);
        vm.stopPrank();

        currentTime += 100;
        vm.warp(currentTime);
        clearPerpTrades();
        currentTime += 900;
        vm.warp(currentTime);
        setAssetPrice(initialPrice * 150 / 100); // 50% increase

        bool isUnsafe = shortCollateral.canLiquidate(positionId);
        uint256 maxDebt = shortCollateral.maxLiquidatableDebt(positionId);
        console2.log("maxDebt: %s", maxDebt);
        assertEq(isUnsafe, true);

        // the liquidator open a long position for 1e18
        uint beforeBalance = 3000e18;
        susd.mint(user_2, beforeBalance);
        tradeParams.isLong = true;
        tradeParams.amount = 1e18;
        tradeParams.maxCost = 2000e18;
        vm.startPrank(user_2);
        susd.approve(address(getPool()), tradeParams.maxCost);
        exchange.openTrade(tradeParams);
        vm.stopPrank();
        assertEq(powerPerp.balanceOf(user_2), 1e18);
        console2.log("balance of user2: %s", susd.balanceOf(user_2));
        assertEq(susd.balanceOf(user_2), 1360.56e18); // spent 1639.44e18 susd for the 1e18 perp

        // user_2 liquidates the 1e18 position for user_1
        vm.startPrank(user_2);
        powerPerp.approve(address(exchange), 1e18); 
        exchange.liquidate(positionId, maxDebt);
        vm.stopPrank();
        console2.log("balance of user2: %s", susd.balanceOf(user_2));  
        (, uint256 _shortAmount, uint256 _collateralAmount,) = shortToken.shortPositions(positionId);
        console2.log("remaining collateral: %s", _collateralAmount); 
        assertEq(_collateralAmount, 0);
        uint256 afterBalance = susd.balanceOf(user_2); 
        console2.log("afterBalance", afterBalance); 
        assertEq(beforeBalance-afterBalance, 732.24e18); // user_2 lost 732.24e18 as a liquidator
     }

Tools Used

VScode

Recommended Mitigation Steps

Mitigation: we should make sure totalCollateralReturned >= collateralClaim and the liquidator will not lose funding if not gaining for doing liquidation. Another fix is to recalculate/reduce the debt that a liquidator need to pay when the remaining collateral is not sufficient to cover the original debt value.

function liquidate(uint256 positionId, uint256 debt, address user)
        external
        override
        onlyExchange
        nonReentrant
        returns (uint256 totalCollateralReturned)
    {
        UserCollateral storage userCollateral = userCollaterals[positionId];

        bytes32 currencyKey = synthetixAdapter.getCurrencyKey(userCollateral.collateral);
        Collateral memory coll = collaterals[currencyKey];

        (uint256 markPrice,) = exchange.getMarkPrice();
        (uint256 collateralPrice,) = synthetixAdapter.getAssetPrice(currencyKey);
        uint256 collateralClaim = debt.mulDivDown(markPrice, collateralPrice);
        uint256 liqBonus = collateralClaim.mulWadDown(coll.liqBonus);
        totalCollateralReturned = liqBonus + collateralClaim;
        if (totalCollateralReturned > userCollateral.amount) totalCollateralReturned = userCollateral.amount;
        userCollateral.amount -= totalCollateralReturned;

+       if(totalCollateralReturned < collateralClaim) revert LiquidateFailureDueToLowCollateral();

        ERC20(userCollateral.collateral).safeTransfer(user, totalCollateralReturned);

        emit LiquidateCollateral(positionId, user, userCollateral.collateral, debt, totalCollateralReturned);
    }

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L727-L735

Vulnerability details

Impact

Detailed description of the impact of this finding.

_getDelta() might return invalid delta. This is because its calculation is based on spotPrice, which is not ensured to be valid in _getDelta() in its current implementation.

Proof of Concept

Provide direct links to all referenced code in GitHub. Add screenshots, logs, or any other relevant proof that illustrates the concept.

The _getDelta() function calculates delta based on spotPrice, which is retrieved by function baseAssetPrice(). baseAssetPrice() might return invalid spotPrice, which is indicated by the isInvalid flag.

https://github.com/code-423n4/2023-03-polynomial/blob/aeecafc8aaceab1ebeb94117459946032ccdff1e/src/LiquidityPool.sol#L727-L735

Unfortunately, there is a logical error in the following code snippet:

require(!isInvalid || spotPrice > 0);

An invalid positive spotPrice will bypass this check. As a result, the invalid spotPrice will be used to calculate delta. Thus, invalid dealta will be returned by _getDelta() in this case.

Tools Used

VScode

Recommended Mitigation Steps

We can fix the logical error as this:

function _getDelta() internal view returns (uint256 delta) {
        (uint256 spotPrice, bool isInvalid) = baseAssetPrice();
        uint256 pricingConstant = exchange.PRICING_CONSTANT();

-        require(!isInvalid || spotPrice > 0);
+        require(!isInvalid && spotPrice > 0);


        delta = spotPrice.mulDivDown(2e18, pricingConstant);
        delta = delta.mulWadDown(exchange.normalizationFactor());
    }