Polynomial Protocol contest - Bauer's results

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L288-L330

Vulnerability details

Impact

The user's remaining collateral assets will be locked in the contract

Proof of Concept

The protocol allows user to close short trade. Inside the _closeTrade() function, the totalShortAmount is calculated according to the formula uint256 totalShortAmount = shortPosition.shortAmount - params.amount;, the params.amount comes from the params structure in the parameter.The remaining collateral amount totalCollateralAmount is uint256 totalCollateralAmount = shortPosition.collateralAmount - params.collateralAmount;.Before returning user collateral ,the protocol will get minCollateral based on totalShortAmount and make sure require(totalCollateralAmount >= minCollateral, "Not enough collateral");. After sending collaterals to user ,the protocol will adjust the position,if the position.shortAmount is 0 ,the ShortToken contract will burn the positionId and user will be not able to close the position. However ,if params.collateralAmount < shortPosition.collateralAmount and shortPosition.shortAmount == params.amount ,the totalShortAmount and minCollateral will be 0, the totalCollateralAmount >= minCollateral will also meet, and the protocol burn the positionId, but the remaining collateral assets of the user will be locked in the contract.

function _closeTrade(TradeParams memory params) internal returns (uint256) {
        if (params.isLong) {
            uint256 shortPositions = shortToken.balanceOf(msg.sender);
            require(shortPositions == 0, "Shouldn't have short positions to close long postions");
            uint256 totalCost = pool.closeLong(params.amount, msg.sender, params.referralCode);
            uint256 minCost = params.minCost;
            require(totalCost >= minCost);
            powerPerp.burn(msg.sender, params.amount);

            emit CloseLong(msg.sender, params.amount, totalCost);

            return totalCost;
        } else {
            uint256 holdings = powerPerp.balanceOf(msg.sender);
            require(holdings == 0, "Shouldn't have long positions to close short postions");

            require(shortToken.ownerOf(params.positionId) == msg.sender);

            IShortToken.ShortPosition memory shortPosition = shortToken.shortPositions(params.positionId);

            require(shortPosition.collateral == params.collateral);

            uint256 totalShortAmount = shortPosition.shortAmount - params.amount;
            uint256 totalCollateralAmount = shortPosition.collateralAmount - params.collateralAmount;

            uint256 minCollateral = shortCollateral.getMinCollateral(totalShortAmount, params.collateral);
            require(totalCollateralAmount >= minCollateral, "Not enough collateral");

            shortCollateral.sendCollateral(params.positionId, params.collateralAmount);
            shortToken.adjustPosition(
                params.positionId, msg.sender, params.collateral, totalShortAmount, totalCollateralAmount
            );
            uint256 totalCost = pool.closeShort(params.amount, msg.sender, params.referralCode);
            uint256 maxCost = params.maxCost;
            require(totalCost <= maxCost);

            emit CloseShort(
                msg.sender, params.positionId, params.amount, params.collateral, params.collateralAmount, totalCost
                );

            return totalCost;
        }
    }

Tools Used

Vscode

Recommended Mitigation Steps

Inside the _closeTrade() function ,if totalShortAmount == 0 , make sure the shortPosition.collateralAmount == params.collateralAmount.

Lines of code

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

Vulnerability details

Impact

When the removeCollateral() is executed, the protocol updates the usedFunds and positionData.totalCollateral without calling the EXCHANGE.removeCollateral function, it could lead to process withdraw transaction to fail and collateral be locked in the contract

Proof of Concept

The KangarooVault.removeCollateral() is used to remove collateral from position. Inside the function, the protocol updates the usedFunds and positionData.totalCollateral without calling the EXCHANGE.removeCollateral, the ShortCollateral contract does not return vault collateral.It may lead to the transaction to fail. e.g. When the protocol processes withdrawal queue, the available funds is calculated based on the formula uint256 availableFunds = totalFunds - usedFunds;, if the removeCollateral() has already been executed ,it causes the availableFunds to be incorrect and the transaction may fail at line SUSD.safeTransfer(current.user, availableFunds);

    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);
    }

uint256 availableFunds = totalFunds - usedFunds;

            if (availableFunds == 0) {
                return;
            }

            uint256 susdToReturn = current.withdrawnTokens.mulWadDown(tokenPrice);

            // Partial withdrawals if not enough available funds in the vault
            // Queue head is not increased
            if (susdToReturn > availableFunds) {
                current.returnedAmount = availableFunds;
                uint256 tokensBurned = availableFunds.divWadUp(tokenPrice);

                totalQueuedWithdrawals -= tokensBurned;
                current.withdrawnTokens -= tokensBurned;

                totalFunds -= availableFunds;

                if (withdrawalFee > 0) {
                    uint256 withdrawFees = availableFunds.mulWadDown(withdrawalFee);
                    SUSD.safeTransfer(feeReceipient, withdrawFees);
                    availableFunds -= withdrawFees;
                }

                SUSD.safeTransfer(current.user, availableFunds);

                emit ProcessWithdrawalPartially(
                    current.id, current.user, tokensBurned, availableFunds, current.requestedTime
                    );
                return;

And when the closePosition() function is called, as the code below the protocol will assign the positionData.totalCollateral value to the tradeParams.collateralAmount, since the positionData.totalCollateral is smaller than the actual value,the shortCollateral protocol will send less collateral to the vault, vault's remaining collateral assets will be locked in the shortCollateral contract.

function _closePosition(uint256 amt, uint256 maxCost) internal {
        require(positionData.positionId != 0 && positionData.pendingLongPerp == 0 && positionData.pendingShortPerp == 0);

        IPerpsV2MarketBaseTypes.DelayedOrder memory delayedOrder = PERP_MARKET.delayedOrders(address(this));
        require(delayedOrder.sizeDelta == 0);

        IPerpsV2MarketBaseTypes.Position memory position = PERP_MARKET.positions(address(this));
        require(position.size >= 0);

        uint256 longPositionToClose;
        IExchange.TradeParams memory tradeParams;

        tradeParams.positionId = positionData.positionId;
        tradeParams.maxCost = maxCost;
        tradeParams.referralCode = referralCode;
        tradeParams.collateral = address(SUSD);

        if (amt >= positionData.shortAmount) {
            longPositionToClose = positionData.longPerp;

            tradeParams.amount = positionData.shortAmount;
            tradeParams.collateralAmount = positionData.totalCollateral;
        } else {
            longPositionToClose = amt.mulDivDown(positionData.longPerp, positionData.shortAmount);
            uint256 collateralToRemove = amt.mulDivDown(positionData.totalCollateral, positionData.shortAmount);

            tradeParams.amount = amt;
            tradeParams.collateralAmount = collateralToRemove;
        }

Tools Used

Vscode

Recommended Mitigation Steps

add code: EXCHANGE.removeCollateral(positionData.positionId, collateralToRemove);

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L236 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/PowerPerp.sol#L41 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/PowerPerp.sol#L46

Vulnerability details

Impact

Due to the missing check for amount 0 for the Exchange.openTrade() function, users can create many short positions and then use the minted NFT to prevent other users from interacting with the protocol.

Proof of Concept

The Exchange.openTrade() is used to open long or short trade. When users create a short position, there is no check for amount 0, users can create a lot of short posionts. When users create a long position, there is a check that user shouldn't have short positions for users. Inside the PowerPerp.transfer() function ,there is also a check that receiver should have not short positions. Here is the problem. A bad actor can create a lot of short positions ,once s/he finds someone want to open,close a position or transfer PowerPerp tokens to another user, s/he can front-run the PowerPerp.transfer() function to transfer NFT to _to address,then user's open ,close long postion and transfer PowerPerp tokens transactions will fail.

    function _openTrade(TradeParams memory params) internal returns (uint256, uint256) {
        if (params.isLong) {
            uint256 shortPositions = shortToken.balanceOf(msg.sender);
            require(shortPositions == 0, "Short position must be closed before opening");

            uint256 totalCost = pool.openLong(params.amount, msg.sender, params.referralCode);
            uint256 maxCost = params.maxCost;
            require(totalCost <= maxCost);
            powerPerp.mint(msg.sender, params.amount);

            emit OpenLong(msg.sender, params.amount, totalCost);

            return (0, totalCost);

   function transfer(address _to, uint256 _amount) public override returns (bool) {
        require(shortToken.balanceOf(_to) == 0, "Receiver has short positions");
        return ERC20.transfer(_to, _amount);
    }

    function transferFrom(address _from, address _to, uint256 _amount) public override returns (bool) {
        require(shortToken.balanceOf(_to) == 0, "Receiver has short positions");
        return ERC20.transferFrom(_from, _to, _amount);
    }

Tools Used

Vscode

Recommended Mitigation Steps

Set min deposit amount for opening short position