code423n4 commented 1 year ago

Lines of code

https://github.com/code-423n4/2023-03-polynomial/blob/main/src/ShortToken.sol#L82-L84 https://github.com/code-423n4/2023-03-polynomial/blob/main/src/Exchange.sol#L304

Vulnerability details

Impact

When completely closing a short trade, a user is supposed to input TradeParams such that:

shortPosition.shortAmount == params.shortAmount shortPosition.collateralAmount == params.collateralAmount

However, if cares have not been given particularly when inputting params.collateralAmount via a non-frontend method such as https://optimistic.etherscan.io/, a zero or a value smaller than shortPosition.collateralAmount could be accidentally entered. After the transaction has succeeded, the user's collateral would be permanently locked in contract ShortCollateral.

Proof of Concept

As can be seen from the code block pertaining to _closeTrade() below, totalShortAmount == 0 will make the require statement pass easily because minCollateral == 0.

File: Exchange.sol#L310-L319

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

The inadequate params.collateralAmount accidentally inputted is then sent to the user:

File: ShortCollateral.sol#L106-L116

    function sendCollateral(uint256 positionId, uint256 amount) external override onlyExchange nonReentrant {
        UserCollateral storage userCollateral = userCollaterals[positionId];

        userCollateral.amount -= amount;

        address user = shortToken.ownerOf(positionId);

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

        emit SendCollateral(positionId, userCollateral.collateral, amount);
    }

Next, the user's position is adjusted such that its position is burned because of a complete position close. Note that position.shortAmount is assigned 0 whereas position.collateralAmount is assigned a non-zero value.

File: ShortToken.sol#L79-L84

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

            if (position.shortAmount == 0) {
                _burn(positionId);
            }

Because the user's ERC-721 short token is now burned, removing the forgotten/remaining collateral from the short position is going to revert on the ownership check:

File: Exchange.sol#L388

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

Tools Used

Manual inspection

Recommended Mitigation Steps

Consider adding a check in _closeTrade() that will fully send the collateral to the user when the position is intended to be fully closed as follows:

File: Exchange.sol#L310-L316

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

+            if (totalShortAmount == 0) {
+                params.collateralAmount = shortPosition.collateralAmount;
+            }
            shortCollateral.sendCollateral(params.positionId, params.collateralAmount);

c4-sponsor commented 1 year ago

mubaris marked the issue as sponsor confirmed

c4-judge commented 1 year ago

JustDravee marked the issue as selected for report

code-423n4 / 2023-03-polynomial-findings